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

[Christian Couder <christian.couder@gmail.com>]

On Tue, Mar 17, 2015 at 7:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> 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.

So if there is already a bad commit and the user gives another
bad commit, that means that the user knows that it will replace the
existing bad commit with the new one and that it's done for this
purpose.

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

What I wanted to say is that if we change "git bisect bad <commitish>",
so that now it means "add a new bad commit" instead of the previous
"replace the current bad commit, if any, with this one", then experienced
users might see that change as a regression in the user interface and
it might even break scripts.

That's why I suggested to use a new option to mean
"add a new bad commit", though --alternate might not be the best
name for this option.

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

What I find sensible is to not break the semantics of the current
interface.

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

Yes, I agree that it could be an improvement to make it possible for the
user to specify another bad commit. I just think it should be done with
a new option if there is already a bad commit...

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

... or by allowing any number of bad commits after "git bisect bad".

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

Yeah, I agree that we can do better in this case.

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

Yeah, it is not as simple as it might look like, neither for the interface
nor for the behavior.