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

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

On Tue, Mar 17, 2015 at 9:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> On Tue, Mar 17, 2015 at 7:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Christian Couder <christian.couder@gmail.com> writes:
>>>
>>>> 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.
>
> ECANNOTQUITEPARSE.  The user may say "git bisect bad $that" and we
> do not question $that is bad. Git does not know better than the
> user.
>
> But that does not mean Git does not know better than the user how
> the current bad commit and $that commit are related.  The user is
> not interested in "replacing" at all.  The user is telling just one
> single fact, that is, "$that is bad".

The user may make mistakes and try to fix them, like for example:

$ git checkout master
$ git bisect bad
$ git log --oneline --decorate --graph --all
# Ooops I was not on the right branch
$ git checkout dev
$ git bisect bad
$ git log --oneline --decorate --graph --all
# Everything looks ok now; the "bad" commit is what I expect
# I can properly continue bisecting using "git bisect good"...

In this case the user, who knows how git bisect works, expected that
the second "git bisect bad" would fix the previous mistake made using
the first "git bisect bad".

If we make "git bisect bad" behave in another way we may break an
advanced user's expectation.

>>> 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.
>
> Huh?
>
> Step back a bit.  The place you need to start from is to admit the
> fact that what "git bisect bad <committish>" currently does is
> broken.
>
> Try creating this history yourself
>
>     a---b---c---d---e---f
>
> and start bisection this way:
>
>     $ git bisect start f c
>     $ git bisect bad a
>
> Immediately after the second command, "git bisect" moans
>
>     Some good revs are not ancestor of the bad rev.
>     git bisect cannot work properly in this case.
>     Maybe you mistake good and bad revs?
>
> when it notices that the good rev (i.e. 'c') is no longer an
> ancestor of the 'bad', which now points at 'a'.
>
> But that is because "git bisect bad" _blindly_ moved 'bad' that used
> to point at 'f' to 'a', making a good rev (i.e. 'c') an ancestor of
> the bad rev, without even bothering to check.

Yeah, "git bisect bad" currently does what it is asked and then
complains when it looks like a mistake has been made.

You might see that as a bug. I am seeing that more as "git bisect"
expecting users to know what they are doing.

For example an advanced user might have realized that the first "git
bisect start f c" was completely rubish for some reason, and the "git
bisect bad a" might be a first step to fix that. (The next step might
then be deleting the "good" pointer...)

> Now, if we fixed this bug and made the bisect_state function more
> careful (namely, when accepting "bad", make sure it is not beyond
> any existing "good", or barf like the above, _without_ moving the
> bad pointer), the user interface and behaviour would be changed.  Is
> that a regression?  No, it is a usability fix and a progress.

Yeah, you might see that as a usability fix and a progress.

> Simply put, bisect_state function can become more careful and
> intelligent to help users.

Yeah, we can try to help users more, but doing that we might annoy
some advanced users,  who are used to the current way "git bisect"
works, and perhaps break some scripts.