Christian Couder [off-list ref] writes:
On Tue, Mar 17, 2015 at 7:33 PM, Junio C Hamano [off-list ref] wrote:
quoted
Christian Couder [off-list ref] writes:
quoted
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".
quoted
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.
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.
Simply put, bisect_state function can become more careful and
intelligent to help users.
I view this "user goes out of way to tell us a commit that is known
to be bad as bad, even though it is not what we offered to test and
is not an ancestor of the commit that currently marked as bad" case
the same way. We by now hopefully understand that blindly replacing
the current 'bad' is suboptimal. By teaching bisect_state to do the
"merge-base thing", we would be fixing that.
Why is it a regression?
On Tue, Mar 17, 2015 at 9:46 PM, Junio C Hamano [off-list ref] wrote:
Christian Couder [off-list ref] writes:
quoted
On Tue, Mar 17, 2015 at 7:33 PM, Junio C Hamano [off-list ref] wrote:
quoted
Christian Couder [off-list ref] writes:
quoted
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.
quoted
quoted
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.