Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 17, 2015, 18:33 UTC
Message-ID: <xmqqtwxjo4nf.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAP8UFD12UX+3psD2=9_RsGv8JA2C8N54qAYGydYgr7n5ta7dzw@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 28 quoted lines

> On Mon, Mar 16, 2015 at 10:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> However, you can say "git bisect bad <rev>" (and "git bisect good
>> <rev>" for that matter) on a rev that is unrelated to what the
>> current bisection state is.  E.g. after you mark the child of 8 as
>> "bad", the bisected graph would become
>>
>>    G...1---2---3---4---6---8---B
>>
>> and you would be offered to test somewhere in the middle, say, 4.
>> But it is perfectly OK for you to respond with "git bisect bad 7",
>> if you know 7 is bad.
>>
>> I _think_ the current code blindly overwrites the "bad" pointer,
>> making the bisection state into this graph if you do so.
>>
>>    G...1---2---3---4
>>                     \
>>                      5---B
>
> Yes, we keep only one "bad" pointer.
>
>> This is very suboptimal.  The side branch 4-to-7 could be much
>> longer than the original trunk 4-to-the-tip, in which case we would
>> have made the suspect space _larger_, not smaller.
>
> Yes, but the user is supposed to not change the "bad" pointer for no
> good reason.

That is irrelevant, no? Nobody is questioning that the user is supposed to judge if a commit is "good" or "bad" correctly.

And nobody sane is dreaming that "Git could do better and detect user's mistakes when the user says 'bad' for a commit that is actually 'good'"; if Git can do that, then it should be able to do the bisect without any user input (including "bisect run") at all ;-).

Show 9 quoted lines

>> We certainly should be able to take advantage of the fact that the
>> current "bad" commit (i.e. the child of 8) and the newly given "bad"
>> commit (i.e. 7) are both known to be bad and mark 4 as "bad" instead
>> when that happens, instead of doing the suboptimal thing the code
>> currently does.
>
> Yeah, we could do that, but we would have to allow it only if a
> special option is passed on the command line, for example:
> git bisect bad --alternate <commitish>

I am not quite sure if I am correctly getting what you meant to say, but if you meant "only when --alternate is given, we should do the merge-base thing; we should keep losing the current 'bad' and replace it with the new one without the --alternate option", I would see that as an exercise of a bad taste.

Because the merge-base thing is using both the current and the new one, such a use is not "alternate" in the first place.

If the proposal were "with a new option, the user can say 'oh, I made a mistake earlier and said that a commit that is not bad as 'bad'. Let me replace the commit currently marked as 'bad' with this one.", I would find it very sensible, actually.

I can see that such an operation can be called "alternate", but "--fix" might be shorter-and-sweeter-and-to-the-point.

In the "normal" case, the commit we offer the user to check (and respond with "git bisect bad" without any commit parameter) is always an ancestor of the current 'bad', so the merge-base with 'bad' and the commit that was just checked would always be the current commit. Using the merge-base thing will be transparent to the end users in the normal case, and when the user has off-line knowledge that some other commit that is not an ancestor of the current 'bad' commit is bad, the merge-base thing will give a better behaviour than the current implementation that blindly replaces.

> and/or we could make "git bisect bad" accept any number of bad
> commitishs.

Yes, that is exactly what I meant.

The way I understand the Philip's point is that the user may have a-priori knowledge that a breakage from the same cause appears in both tips of these branches. In such a case, we can start bisection after marking the merge-base of two 'bad' commits, e.g. 4 in the illustration in the message you are responding to, instead of including 5, 6, and 8 in the suspect set.

You need to be careful, though. An obvious pitfall is what you should do when there is a criss-cross merge.

Thanks.