Re: [PATCHv2] git bisect old/new

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Junio C Hamano <gitster@xxxxxxxxx> 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.

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


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

@@ -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


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]