Re: [PATCH v10.1 7/7] bisect: allow any terms set by user

From: Christian Couder <christian.couder@gmail.com>
Date: Jun 29, 2015, 08:08 UTC
Message-ID: <CAP8UFD1gWXOZHfiELZYgvGP1dA-aeaxbyaokKCut9pfWghy9uQ@mail.gmail.com>
In-Reply-To: <vpq1tgvnf2g.fsf@anie.imag.fr>

On Mon, Jun 29, 2015 at 9:34 AM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 17 quoted lines

> Christian Couder <christian.couder@gmail.com> writes:
>
>> On Sun, Jun 28, 2015 at 8:46 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>>> I understand that the user might make a mistake when marking the initial
>>> commits, but as soon as bisect says
>>>
>>>     Commit <sha1-abbrev> is an ancestor of <sha1-abbrev>, so I
>>>     will look for the commit that caused the transition from
>>>     "xyzzy" to "plugh".
>>>
>>> then I hope the user will notice and correct her/his mistake.
>>
>> This looks fragile to me. Unfortunately many users will probably not
>> read it and continue, and then spend a lot of time later trying to
>> understand what went wrong,
>
> I don't understand what you mean by "went wrong".

It happens that users mistake the "good" and the "bad" commits when giving them to git bisect.

Right now in the most common case, we can error out because we know that a "bad" commit cannot be an ancestor of a "good" commit.

Show 5 quoted lines

> As a user, when I
> discovered "git bisect", I was actually surprised that it expected one
> particular order between good and bad. I would have expected to be able
> to say "this is good, this is bad, tell me where it changed" without
> having an idea of who's good and who's bad.

Maybe, but it's not how it has been developed.

Show 6 quoted lines

> In particular when bisecting
> from two branches, the user knows that branch A is good, and branch B is
> bad, but does not necessarily know whether it's a regression in B or a
> fix in A. The fact that bisect can find out should be just "normal" from
> the user point of view. There's no mistake involved, nothing to fix, and
> nothing that went wrong.

Well in this case, it's possible that the merge base is bad and what the user is interested in is the first bad commit that was commited before the merge base. We just don't know, in the case the merge base is bad, what is more interesting for the user.

So I disagree with you and Michael that we should decide that the user is interested by the fix in this case. It's better to error out like we do now and let the user decide what he/she wants rather than decide for him/her that he/she is interested by the fix.

Show 9 quoted lines

>> By the way we could use "mark" or "term" instead of "name" in the
>> option name (like --mark-old or --term-old) and in the code too if it
>> looks clearer.
>
> I prefer "term" to "mark" because "mark" is both a verb and a noun, so
> --mark-old=foo could mean both "mark foo as old" or "the name of the
> marks for old commits is foo".
>
> I think I prefer "term" to "name".

Ok with that. I agree that it would be more consistent to have a "git bisect terms" and "--term-{old,new,bad,good}".

Thanks, Christian.