Re: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C

[Pranit Bauva]

Hey Junio,

On Thu, Aug 4, 2016 at 10:20 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> Hey Junio,
>>
>> On Thu, Aug 4, 2016 at 9:15 PM, Junio C Hamano  wrote:
>>> Pranit Bauva  writes:
>>>
> Also you do not seem to check the error from the function to smudge
> the "result" you are returning from this function.

 Yes I should combine the results from every removal.

> Isn't unlink_or_warn() more correct helper to use here?

 The shell code uses rm -f which is silent and it removes only if
 present.
>>>
>>> Isn't that what unlink_or_warn() do?  Call unlink() and happily
>>> return if unlink() succeeds or errors with ENOENT (i.e. path didn't
>>> exist in the first place), but otherwise reports an error (imagine:
>>> EPERM).
>>
>> Umm, I am confused. I tried "rm -f" with a non-existing file and it
>> does not show any warning or error.
>
> You are, or you were?  I hope it is the latter, iow, you are no
> longer confused and now understand why unlink_or_warn() was
> suggested.

I meant to use past tense. Did not re-check before sending it.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> Hey Junio,
>
> On Thu, Aug 4, 2016 at 9:15 PM, Junio C Hamano  wrote:
>> Pranit Bauva  writes:
>>
 Also you do not seem to check the error from the function to smudge
 the "result" you are returning from this function.
>>>
>>> Yes I should combine the results from every removal.
>>>
 Isn't unlink_or_warn() more correct helper to use here?
>>>
>>> The shell code uses rm -f which is silent and it removes only if
>>> present.
>>
>> Isn't that what unlink_or_warn() do?  Call unlink() and happily
>> return if unlink() succeeds or errors with ENOENT (i.e. path didn't
>> exist in the first place), but otherwise reports an error (imagine:
>> EPERM).
>
> Umm, I am confused. I tried "rm -f" with a non-existing file and it
> does not show any warning or error.

You are, or you were?  I hope it is the latter, iow, you are no
longer confused and now understand why unlink_or_warn() was
suggested.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C

[Pranit Bauva]

Hey Junio,

On Thu, Aug 4, 2016 at 9:15 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>>> Also you do not seem to check the error from the function to smudge
>>> the "result" you are returning from this function.
>>
>> Yes I should combine the results from every removal.
>>
>>> Isn't unlink_or_warn() more correct helper to use here?
>>
>> The shell code uses rm -f which is silent and it removes only if
>> present.
>
> Isn't that what unlink_or_warn() do?  Call unlink() and happily
> return if unlink() succeeds or errors with ENOENT (i.e. path didn't
> exist in the first place), but otherwise reports an error (imagine:
> EPERM).

Umm, I am confused. I tried "rm -f" with a non-existing file and it
does not show any warning or error.

Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

>> Also you do not seem to check the error from the function to smudge
>> the "result" you are returning from this function.
>
> Yes I should combine the results from every removal.
>
>> Isn't unlink_or_warn() more correct helper to use here?
>
> The shell code uses rm -f which is silent and it removes only if
> present.

Isn't that what unlink_or_warn() do?  Call unlink() and happily
return if unlink() succeeds or errors with ENOENT (i.e. path didn't
exist in the first place), but otherwise reports an error (imagine:
EPERM).

> So it makes me wonder which would be more appropriate
> unlink_or_warn() or remove_or_warn() or remove_path(). Is
> remove_path() different from its shell equivalent "rm -f"?

Read it again.

>>> + remove_path(git_path_bisect_start());
>>
>> I can see that refs/files-backend.c misuses it already, but
>> remove_path() helper is about removing a path in the working tree,
>> together with any parent directory that becomes empty due to the
>> removal.  You do not expect $GIT_DIR/ to become an empty directory
>> after removing $GIT_DIR/BISECT_LOG nor want to rmdir $GIT_DIR even
>> if it becomes empty.  It is a wrong helper function to use here.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C

[Pranit Bauva]

Hey Junio,

On Tue, Aug 2, 2016 at 11:16 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> +static int bisect_clean_state(void)
>> +{
>> + int result = 0;
>> +
>> + /* There may be some refs packed during bisection */
>> + struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
>> + for_each_ref_in("refs/bisect/", mark_for_removal, (void *) 
>> _for_removal);
>> + string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
>> + result = delete_refs(_for_removal);
>> + refs_for_removal.strdup_strings = 1;
>> + string_list_clear(_for_removal, 0);
>> + remove_path(git_path_bisect_expected_rev());
>> + remove_path(git_path_bisect_ancestors_ok());
>> + remove_path(git_path_bisect_log());
>> + remove_path(git_path_bisect_names());
>> + remove_path(git_path_bisect_run());
>> + remove_path(git_path_bisect_terms());
>> + /* Cleanup head-name if it got left by an old version of git-bisect */
>> + remove_path(git_path_head_name());
>> +  * Cleanup BISECT_START last to support the --no-checkout option
>> +  * introduced in the commit 4796e823a.
>> +  */
>> + remove_path(git_path_bisect_start());
>
> I can see that refs/files-backend.c misuses it already, but
> remove_path() helper is about removing a path in the working tree,
> together with any parent directory that becomes empty due to the
> removal.  You do not expect $GIT_DIR/ to become an empty directory
> after removing $GIT_DIR/BISECT_LOG nor want to rmdir $GIT_DIR even
> if it becomes empty.  It is a wrong helper function to use here.
>
> Also you do not seem to check the error from the function to smudge
> the "result" you are returning from this function.

Yes I should combine the results from every removal.

> Isn't unlink_or_warn() more correct helper to use here?

The shell code uses rm -f which is silent and it removes only if
present. So it makes me wonder which would be more appropriate
unlink_or_warn() or remove_or_warn() or remove_path(). Is
remove_path() different from its shell equivalent "rm -f"?

>> + return result;
>> +}

Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> +static int bisect_clean_state(void)
> +{
> + int result = 0;
> +
> + /* There may be some refs packed during bisection */
> + struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
> + for_each_ref_in("refs/bisect/", mark_for_removal, (void *) 
> _for_removal);
> + string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
> + result = delete_refs(_for_removal);
> + refs_for_removal.strdup_strings = 1;
> + string_list_clear(_for_removal, 0);
> + remove_path(git_path_bisect_expected_rev());
> + remove_path(git_path_bisect_ancestors_ok());
> + remove_path(git_path_bisect_log());
> + remove_path(git_path_bisect_names());
> + remove_path(git_path_bisect_run());
> + remove_path(git_path_bisect_terms());
> + /* Cleanup head-name if it got left by an old version of git-bisect */
> + remove_path(git_path_head_name());
> +  * Cleanup BISECT_START last to support the --no-checkout option
> +  * introduced in the commit 4796e823a.
> +  */
> + remove_path(git_path_bisect_start());

I can see that refs/files-backend.c misuses it already, but
remove_path() helper is about removing a path in the working tree,
together with any parent directory that becomes empty due to the
removal.  You do not expect $GIT_DIR/ to become an empty directory
after removing $GIT_DIR/BISECT_LOG nor want to rmdir $GIT_DIR even
if it becomes empty.  It is a wrong helper function to use here.

Also you do not seem to check the error from the function to smudge
the "result" you are returning from this function.

Isn't unlink_or_warn() more correct helper to use here?

> + return result;
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C

[Pranit Bauva]

Reimplement `bisect_clean_state` shell function in C and add a
`bisect-clean-state` subcommand to `git bisect--helper` to call it from
git-bisect.sh .

Using `--bisect-clean-state` subcommand is a measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired and will be called by
bisect_reset() and bisect_start().

Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 55 +++-
 git-bisect.sh| 26 +++
 2 files changed, 57 insertions(+), 24 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 965bcc1..10dfb68 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -3,12 +3,21 @@
 #include "parse-options.h"
 #include "bisect.h"
 #include "refs.h"
+#include "dir.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
+static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
+static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
+static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
+static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
+static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
+static GIT_PATH_FUNC(git_path_head_name, "head-name")
+static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms  "),
+   N_("git bisect--helper --bisect-clean-state"),
NULL
 };
 
@@ -78,11 +87,49 @@ static int write_terms(const char *bad, const char *good)
return (res < 0) ? -1 : 0;
 }
 
+static int mark_for_removal(const char *refname, const struct object_id *oid,
+   int flag, void *cb_data)
+{
+   struct string_list *refs = cb_data;
+   char *ref = xstrfmt("refs/bisect/%s", refname);
+   string_list_append(refs, ref);
+   return 0;
+}
+
+static int bisect_clean_state(void)
+{
+   int result = 0;
+
+   /* There may be some refs packed during bisection */
+   struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
+   for_each_ref_in("refs/bisect/", mark_for_removal, (void *) 
_for_removal);
+   string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
+   result = delete_refs(_for_removal);
+   refs_for_removal.strdup_strings = 1;
+   string_list_clear(_for_removal, 0);
+   remove_path(git_path_bisect_expected_rev());
+   remove_path(git_path_bisect_ancestors_ok());
+   remove_path(git_path_bisect_log());
+   remove_path(git_path_bisect_names());
+   remove_path(git_path_bisect_run());
+   remove_path(git_path_bisect_terms());
+   /* Cleanup head-name if it got left by an old version of git-bisect */
+   remove_path(git_path_head_name());
+   /*
+* Cleanup BISECT_START last to support the --no-checkout option
+* introduced in the commit 4796e823a.
+*/
+   remove_path(git_path_bisect_start());
+
+   return result;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
NEXT_ALL = 1,
-   WRITE_TERMS
+   WRITE_TERMS,
+   BISECT_CLEAN_STATE
} cmdmode = 0;
int no_checkout = 0;
struct option options[] = {
@@ -90,6 +137,8 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
 N_("perform 'git bisect next'"), NEXT_ALL),
OPT_CMDMODE(0, "write-terms", ,
 N_("write the terms to .git/BISECT_TERMS"), 
WRITE_TERMS),
+   OPT_CMDMODE(0, "bisect-clean-state", ,
+N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
OPT_BOOL(0, "no-checkout", _checkout,
 N_("update BISECT_HEAD instead of checking out the 
current commit")),
OPT_END()
@@ -108,6 +157,10 @@ int cmd_bisect__helper(int argc, const char **argv, const 
char *prefix)
if (argc != 2)
die(_("--write-terms requires two arguments"));
return write_terms(argv[0], argv[1]);
+   case BISECT_CLEAN_STATE:
+   if (argc != 0)
+   die(_("--bisect-clean-state requires no arguments"));
+   return bisect_clean_state();
default:
die("BUG: unknown subcommand '%d'", cmdmode);
}
diff --git a/git-bisect.sh b/git-bisect.sh
index cd39bd0..bbc57d2 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -187,7 +187,7 @@ bisect_start() {
#
# Get rid of any old bisect state.
#
-   bisect_clean_state || exit
+   git bisect--helper