Re: [PATCHv2] git bisect old/new

[duperrav@minatec.inpg.fr]

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.

         * 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