Re: [PATCHv2] git bisect old/new

Valentin Duperray [off-list ref] writes:

Please make sure that you have Christian who took the time to review
the previous round on Cc:, and have addressed the issues raised in
the review.

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.

Huh?

+If you are not at ease with the terms "bad" and "good", perhaps
+because you are looking for the commit that introduced a fix, you can
+alternatively use "new" and "old" instead.
+But note that you cannot mix "bad" and good" with "new" and "old".
+
+------------------------------------------------
+git bisect new [<rev>]
+------------------------------------------------
+
+Same as "git bisect bad [<rev>]".

It somewhat makes me feel uneasy to see a sentence without any verb.

+------------------------------------------------
+git bisect old [<rev>...]
+------------------------------------------------
+
+Same as "git bisect good [<rev>...]".

Likewise.

+You must run `git bisect start` without commits as argument and run
+`git bisect new <rev>`/`git bisect old <rev>...` after to add the
+commits.

What happens when you do:

	git bisect start
        git bisect new HEAD
        git bisect old v1.0.0

and then

        git bisect bad v1.2.0

Does it error out?  For that matter, what happens if you do this?

	git bisect start
        git bisect new HEAD
	git bisect good v1.0.0

Note that I am not suggesting to document the behaviour here.

We do not usually use "bug" as a transitive verb that means "to
break".  Perhaps "buggy_version" is easier to read and gramatically
more correct.

++
+The "new" commits are the fixed ones and the "old" commits are the unfixed ones.
+At the end of the commit session, you will have the first fixed commit.
+

commit session?  Is it a bisect session?

It feels kind of strange that only one of "bad" or "new" is allowed,
while "good-X" and "old-Y" can coexist and be used interchangeably.

s/,/, /;

But see below.  It feels wrong to always running string comparison
when we know there are either good/bad mode or old/new mode.

-	fprintf(stderr, "Some good revs are not ancestor of the bad rev.\n"
+	fprintf(stderr, "Some %s revs are not ancestor of the %s rev.\n"
 		"git bisect cannot work properly in this case.\n"
-		"Maybe you mistake good and bad revs?\n");
+		"Maybe you mistake %s and %s revs?\n",
+		bisect_good, bisect_bad, bisect_good,
+		bisect_bad);

You merely inherited this from the original, but shouldn't it be
"mistook" in the past tense?

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?

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

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.

         * 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.

What happens when you do:

	git bisect start
        git bisect new HEAD
        git bisect old v1.0.0

and then

        git bisect bad v1.2.0

Does it error out?  For that matter, what happens if you do this?

	git bisect start
        git bisect new HEAD
	git bisect good v1.0.0

In both cases, the `git bisect good`/`git bisect bad`command is
considered invalid. The message
"Invalid command : you're currently in a new/old bisect."
is displayed and the bisect section is not reseted.
Same thing happens if you try to use new/old in a bad/good
bisect session.

quoted

@@ -731,18 +735,25 @@ static void handle_bad_merge_base(void)
 	if (is_expected_rev(current_bad_sha1)) {
 		char *bad_hex = sha1_to_hex(current_bad_sha1);
 		char *good_hex = join_sha1_array_hex(&good_revs, ' ');
+		if (!strcmp(bisect_bad,"bad")) {

s/,/, /;

But see below.  It feels wrong to always running string comparison
when we know there are either good/bad mode or old/new mode.

quoted

@@ -889,6 +900,31 @@ static void show_diff_tree(const char *prefix,  

struct commit *commit)
 }

 /*
+ * The terms used for this bisect session are stocked in
+ * BISECT_TERMS: it can be bad/good or new/old.
+ * We read them and stock them to adapt the messages
+ * accordingly. Default is bad/good.
+ */
+void read_bisect_terms(void)
+{
+	struct strbuf str = STRBUF_INIT;
+	const char *filename = git_path("BISECT_TERMS");
+	FILE *fp = fopen(filename, "r");
+
+	if (!fp) {
+		bisect_bad = "bad";
+		bisect_good = "good";
+	} else {
+	strbuf_getline(&str, fp, '\n');
+	bisect_bad = strbuf_detach(&str, NULL);
+	strbuf_getline(&str, fp, '\n');
+	bisect_good = strbuf_detach(&str, NULL);
+	}
+	strbuf_release(&str);
+	fclose(fp);
+}

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.

Thanks,

Valentin

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.