Hey Junio,

On Tue, Aug 30, 2016 at 11:55 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>
>>> @@ -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.

The patch which I included as an attachment before will fail for
different reasons if you apply it on master branch. To test it on
master branch use the current attachment. Again sorry I couldn't
include this in the email itself.

Regards,
Pranit Bauva