Re: [PATCHv2] git bisect old/new

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

On Wed, Jun 13, 2012 at 8:06 PM,  <duperrav@minatec.inpg.fr> wrote:
>
> Junio C Hamano <gitster@pobox.com> a écrit :
>
>
>>> Some commands are still not available for old/new:
>>>
>>>     * git bisect start [<new> [<old>...]] is not possible: the
>>>       commits will be treated as bad and good.
>>>     * git rev-list --bisect does not treat the revs/bisect/new and
>>>       revs/bisect/old-SHA1 files.
>>>     * thus, git bisect run <cmd> is not available for new/old.
>>>     * git bisect visualize seem to work partially: the tags are
>>>       displayed correctly but the tree is not limited to the bisect
>>>       section.
>>
>>
>> Would be easier to review if the subject is marked as RFC while
>> these todo items are still there.
>>
>> Also before going too far into the implementation, I think it is a
>> good idea to think how you are going to address the above issues. I
>> suspect the changes to bisect.c will have to be vastly different
>> depending on that plan.
>
>
>        * git bisect start [<new> [<old>...]]:
>
> The idea would be to add a "--new" option to start in new/old mode.

I am ok with that.

>        * git rev-list --bisect:
>
> I see two solutions for this:
>
>        - read revisions from both refs/bisect/bad and refs/bisect/new
>          (resp. refs/bisect/good and refs/bisect/old).
>
>        - read revisions only from refs/bisect/bad and refs/bisect/good
>          when the BISECT_TERMS doesn't exist or contains bad/good
>          and
>          read revisions only from refs/bisect/new and refs/bisect/old
>          when the BISECT_TERMS exists and contains new/old.
>
> I prefer the latter because I don't really know how reading all files
> will affect the calls of "git rev-list" outside of a bisect session and
> the two types of files should not be present simultaneously anyway.

Why didn't you consider adding another option: "--bisect-terms-new" or
"--bisect-terms=new,old" or "--bisect-refs=new,old"?

By the way, I just looked at the doc for "--bisect-vars". This outputs
text ready to be eval'ed by the shell, and, among the variables it
outputs, there are "bisect_bad" and "bisect_good".
So maybe we should avoid using BISECT_BAD and BISECT_GOOD shell
variables in git-bisect.sh to avoid confusion.

>> While this is not wrong per-se, I am not sure if storing and reading
>> two lines from this file is really worth the trouble.
>>
>> Wouldn't it be easier to change the convention so that the presense
>> of BISECT_OLDNEW file signals that the program is working in the
>> old/new mode as opposed to the traditional good/bad mode, or perhaps
>> a single line "true" or "false" in the file tells us if we are in
>> OLDNEW mode, or something?
>
>
> If there is consensus around the fact that no other terms will be added
> after old/new, only checking if the file is present would be easier
> indeed.

Here is the end of the previous thread where old/new was originaly discussed:

http://thread.gmane.org/gmane.comp.version-control.git/182398/focus=183410

Peff said:

-------
Hmm. I think this is not quite as nice, but it is way simpler. It may be
worth trying for a bit to see how people like it. If they don't, the
cost of failure is that we have to maintain "old/new" forever, even
after we implement a yes/no reversible scheme. But maintaining the
old/new mapping from yes/no would not be any harder than the good/bad
mapping, which we would need to do anyway.

So it sounds like a reasonable first step.
-------

So in my opinion, there was a consensus that if old/new is not enough
it should not be a big deal to maintain anyway.
And I agree with this provided that we indeed implement old/new so
that it's not a big deal to maintain if we change our mind later.
(For example we might later want "yes/no", or "good/bad" with a
meaning reversed, or perhaps something else.)

So I'd rather have a file with a generic name like "BISECT_TERMS", but
it may contain just one line like for example "new/old".
We could just check that the content of the line is "new/old" and
die("Only 'new/old' is supported in $GIT_DIR/BISECT_TERMS") if it is
something else.

Thanks,
Christian.