Re: [PATCHv2] git bisect old/new

On Wed, Jun 13, 2012 at 8:06 PM,  [off-list ref] wrote:

Junio C Hamano [off-list ref] a écrit :

quoted

quoted

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.

quoted

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.