Re: [BUG REPORT] Git does not correctly replay bisect log

From: Christian Couder <christian.couder@gmail.com>
Date: Dec 6, 2018, 16:31 UTC
Message-ID: <CAP8UFD3VCzEdtfRqi_d2ibPtTN0uocGW+CshY_5m16TW1_YUdw@mail.gmail.com>
In-Reply-To: <278123655fbdb565aed16bba804711774716c554.camel@gmail.com>

Hi,

On Thu, Dec 6, 2018 at 3:43 PM Lukáš Krejčí <lskrejci@gmail.com> wrote:

Show 10 quoted lines

>
> Hello again,
>
> after looking into this today, I'm not sure if this can be considered a
> bug - it's just that I expected Git to check out the exact commit to
> test that was there before resetting the bisect. That made me uncertain
> whether Git restored the correct state.
>
> When I looked at what Git actually does, it became clear that the
> behavior is not incorrect but perhaps a bit surprising.

Yeah, I agree. I suspect, but I am not sure, that the difference of behavior is because in one case we only check merge bases once at the beginning (maybe because the BISECT_ANCESTORS_OK file always exists) while in the other case we check them more than once during the bisection. I haven't had time to look closely at this, but I would like to.

Show 7 quoted lines

> When Git replays the bisect log, it only updates refs/bisect/bad,
> refs/bisect/good-*, refs/bisect/skip-* and reconstructs the log in
> .git/BISECT_LOG. After that check_good_are_ancestors_of_bad() verifies
> that all good commits are ancestors of the bad commit, and if not, the
> message "Bisecting: a merge base must be tested" is printed and the
> branch is switched to the merge base of the bad and all the good
> commits.

I am not sure if you are talking about running `git bisect replay` or sourcing the log in the above.

> Basically, some state is lost because Git "forgot" the first good
> commit from the log already was an ancestor of the first bad one.

The BISECT_ANCESTORS_OK file should be there to avoid forgetting that we already checked the merge bases.

Show 9 quoted lines

> In other words, it's as if I just started the bisect with the following
> commands and just pasted the whole bisect log to .git/BISECT_LOG:
>
> $ git bisect start
> $ git bisect good 94710cac0ef4ee177a63b5227664b38c95bbf703
> $ git bisect good 958f338e96f874a0d29442396d6adf9c1e17aa2d
> $ git bisect bad 1b0d274523df5ef1caedc834da055ff721e4d4f0
> Bisecting: a merge base must be tested
> [1e4b044d22517cae7047c99038abb444423243ca] Linux 4.18-rc4

Yeah, when we start a new bisection the BISECT_ANCESTORS_OK file should be erased if it exists, while it shouldn't be erased when we are already in the middle of an existing bisection.

[...]

Show 6 quoted lines

> And indeed, marking the merge base as good switches to the correct
> commit after the bisect. Marking it as bad will fail, so at least you
> can't make a mistake after replaying the bisect log:
> $ git bisect bad
> The merge base 1e4b044d22517cae7047c99038abb444423243ca is bad.
> This means the bug has been fixed between 1e4b044d22517cae7047c99038abb444423243ca and [94710cac0ef4ee177a63b5227664b38c95bbf703 958f338e96f874a0d29442396d6adf9c1e17aa2d].

Yeah, I think this works as expected.

> Once again, I'm sorry for the noise. I guess it wasn't clear from the
> man page that something like this could happen and that made me think
> that this was a bug.

No reason to be sorry, there might still be a bug related to the BISECT_ANCESTORS_OK file or something. I hope I can take a look at this more closely soon.

Thanks for your report and your work on this, Christian.