Hey Junio,
On Tue, Aug 30, 2016 at 11:55 PM, Pranit Bauva <pranit.bauva@xxxxxxxxx> 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
Attachment:
out3
Description: Binary data