Re: [PATCH v14 14/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C

>
>>> @@ -729,7 +735,7 @@ static struct commit **get_bad_and_good_commits(int *rev_nr)
>>>       return rev;
>>>  }
>>>
>>> -static void handle_bad_merge_base(void)
>>> +static int handle_bad_merge_base(void)
>>>  {
>>>       if (is_expected_rev(current_bad_oid)) {
>>>               char *bad_hex = oid_to_hex(current_bad_oid);
>>> @@ -750,17 +756,18 @@ static void handle_bad_merge_base(void)
>>>                               "between %s and [%s].\n"),
>>>                               bad_hex, term_bad, term_good, bad_hex, good_hex);
>>>               }
>>> -             exit(3);
>>> +             return 3;
>>>       }
>>>
>>>       fprintf(stderr, _("Some %s revs are not ancestor of the %s rev.\n"
>>>               "git bisect cannot work properly in this case.\n"
>>>               "Maybe you mistook %s and %s revs?\n"),
>>>               term_good, term_bad, term_good, term_bad);
>>> -     exit(1);
>>> +     bisect_clean_state();
>>> +     return 1;
>>
>> What is the logic behind this function sometimes clean the state,
>> and some other times do not, when it makes an error-return?  We see
>> above that "return 3" codepath leaves the state behind.
>>
>> Either you forgot a necessary clean_state in "return 3" codepath,
>> or you forgot to document why the distinction exists in the in-code
>> comment for the function.  I cannot tell which, but I am leaning
>> towards guessing that it is the former.
>
> This is a very tricky one. I have purposely not included this after a
> lot of testing. I have hand tested with the original git and with this
> branch. The reason why anyone wouldn't be able to catch this is
> because its not covered in the test suite. I am including a patch with
> this as an attachment (because I am behind a proxy right now but don't
> worry I will include this as a commit in the next series). The
> original behaviour of git does not clean the bisect state when this
> situation occurs. On another note which you might have missed that
> bisect_clean_state() is purposely put before return 1 which is covered
> by the test suite. You can try removing it and see that there is a
> broken tes. tI was thinking of including the tests after the whole
> conversion but now I think including this before will make the
> conversion more easier for review.