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

From: Junio C Hamano <[email-masked]>
Date: Tue, 17 Mar 2015 11:33:24 -0700

Christian Couder <[email-masked]> writes:

> On Mon, Mar 16, 2015 at 10:05 PM, Junio C Hamano <[email-masked]> 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
;-).

>> 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.