Re: [PATCH 1/4] bisect--helper: `bisect_clean_state` shell function in C

[Pranit Bauva <pranit.bauva@gmail.com>]

Hey Eric,

On Wed, Jun 8, 2016 at 4:01 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> 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 .
>> [...]
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -78,11 +87,43 @@ static int write_terms(const char *bad, const char *good)
>> +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);
>
> Here you're allocating a string...
>
>> +       string_list_append(refs, ref);
>> +       return 0;
>> +}
>> +
>> +int bisect_clean_state(void)
>> +{
>> +       int result = 0;
>> +       struct string_list refs_for_removal = STRING_LIST_INIT_DUP;
>> +       for_each_ref_in("refs/bisect/", mark_for_removal, (void *) &refs_for_removal);
>
> ...and the allocated string gets inserted into a string_list which
> itself duplicates the string (STRING_LIST_INIT_DUP), so this is
> leaking the string you created with xstrfmt(), isn't it?

Yes nice catch. I would prefer using the string_list with
STRING_LIST_INIT_DUP and free the ref.

>> +       string_list_append(&refs_for_removal, "BISECT_HEAD");
>> +       result |= delete_refs(&refs_for_removal);
>
> Not sure I understand the point of using |= here rather than merely =
> since this is the only place 'result' is assigned in this function.

In some initial iterations I assigned value of result before so used
|=. Then I realized it isn't necessary, so removed it. Forgot to
remove |=. Will do it!

>> +       string_list_clear(&refs_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_write_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 */
>> +       remove_path(git_path_bisect_start());
>
> The BISECT_START comment merely repeats what the code itself already
> says. I realize that you merely copied this from the shell code, but
> it isn't helpful in its current form. Much more helpful would be to
> explain *why* this needs to be done last. Perhaps the commit message
> of the commit which introduced the comment originally would give a
> clue (I haven't checked).

The commit(4796e823a) by Jon Seymour (4 Aug, 2011) explains this. I am
not sure how I would frame the sentence as that commit introduces a
concept whose by-product is the comment. I do want to include it in
the commit message otherwise it would be passed on as "you have to do
it but no one knows why".

>> +       return result;
>> +}
>> diff --git a/git-bisect.sh b/git-bisect.sh
>> @@ -430,27 +430,7 @@ bisect_reset() {
>> -bisect_clean_state() {
>> -       # There may be some refs packed during bisection.
>
> This comment doesn't seem to be reproduced in the C version. Should it
> be? Is it no longer relevant in the C version? What does it mean
> exactly?

git-bisect itself never packs refs on its own. But there is a
possibility that the user might have packed the refs in the bisection
state. This was introduced by Christian ( 15 Nov, 2007) in commit
947a604b01.

>> -       git for-each-ref --format='%(refname) %(objectname)' refs/bisect/\* |
>> -       while read ref hash
>> -       do
>> -               git update-ref -d $ref $hash || exit
>> -       done
>> -       rm -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
>> -       rm -f "$GIT_DIR/BISECT_ANCESTORS_OK" &&
>> -       rm -f "$GIT_DIR/BISECT_LOG" &&
>> -       rm -f "$GIT_DIR/BISECT_NAMES" &&
>> -       rm -f "$GIT_DIR/BISECT_RUN" &&
>> -       rm -f "$GIT_DIR/BISECT_TERMS" &&
>> -       # Cleanup head-name if it got left by an old version of git-bisect
>> -       rm -f "$GIT_DIR/head-name" &&
>> -       git update-ref -d --no-deref BISECT_HEAD &&
>> -       # clean up BISECT_START last
>> -       rm -f "$GIT_DIR/BISECT_START"
>> +       git bisect--helper --bisect-clean-state || exit
>>  }

I will also include static in function declaration. Thanks!

Regards,
Pranit Bauva