Re: [PATCH 4/4] bisect: add the terms old/new

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 8, 2015, 21:21 UTC
Message-ID: <xmqqmw0999rl.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1433794930-5158-4-git-send-email-antoine.delaite@ensimag.grenoble-inp.fr>

Antoine Delaite <antoine.delaite@ensimag.grenoble-inp.fr> writes:

Show 18 quoted lines

> When not looking for a regression during a bisect but for a fix or a
> change in another given property, it can be confusing to use 'good'
> and 'bad'.
>
> This patch introduce `git bisect new` and `git bisect old` as an
> alternative to 'bad' and good': the commits which have a certain
> property must be marked as `new` and the ones which do not as `old`.
>
> The output will be the first commit after the change in the property.
> During a new/old bisect session you cannot use bad/good commands and
> vice-versa.
>
> `git bisect replay` works fine for old/new bisect sessions.
>
> 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.

Just throwing a suggestion. You could perhaps add a new verb to be used before starting to do anything, e.g.

	$ git bisect terms new old

(where the default mode is "git bisect terms bad good") to set up the "terms", and then after that

	$ git bisect start $new $old1 $old2...

would be treated as you would naturally expect, no?

>      * git rev-list --bisect does not treat the revs/bisect/new and
>        revs/bisect/old-SHA1 files.

That shouldn't be hard to add, I would imagine, either by

	git rev-list --bisect=new,old

or teaching "git rev-list --bisect" to read the "terms" itself, as a follow-up patch.

>      * git bisect visualize seem to work partially: the tags are
>        displayed correctly but the tree is not limited to the bisect
>        section.

This is directly related to the lack of "git rev-list --bisect" support, I would imagine. If you make the command read "terms", I suspect that it would solve itself.

Show 14 quoted lines

> @@ -379,6 +408,21 @@ In this case, when 'git bisect run' finishes, bisect/bad will refer to a commit
>  has at least one parent whose reachable graph is fully traversable in the sense
>  required by 'git pack objects'.
>  
> +* Look for a fix instead of a regression in the code
> ++
> +------------
> +$ git bisect start
> +$ git bisect new HEAD    # current commit is marked as new
> +$ git bisect old HEAD~10 # the tenth commit from now is marked as old
> +------------
> ++
> +If the last commit has a given property, and we are looking for the commit
> +which introduced this property.

Hmph, that is not a complete sentence and I cannot understand what it is trying to say.

> +For each commit the bisection guide us to we

s/guide us to we/guides us to, we/;

> +will test if the property is present, if it is we will mark the commit as new
> +with 'git bisect new', otherwise we will mark it as old.

It would be easier to read as separate sentences, i.e.

s/is present, if it is/is present. If it is/;

Show 24 quoted lines

> diff --git a/bisect.c b/bisect.c
> index 3b7df85..511b905 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -409,7 +409,8 @@ static int register_ref(const char *refname, const unsigned char *sha1,
>  	if (!strcmp(refname, name_bad)) {
>  		current_bad_oid = xmalloc(sizeof(*current_bad_oid));
>  		hashcpy(current_bad_oid->hash, sha1);
> -	} else if (starts_with(refname, "good-")) {
> +	} else if (starts_with(refname, "good-") ||
> +		   starts_with(refname, "old-")) {
>  		sha1_array_append(&good_revs, sha1);
>  	} else if (starts_with(refname, "skip-")) {
>  		sha1_array_append(&skipped_revs, sha1);
> @@ -741,6 +742,12 @@ static void handle_bad_merge_base(void)
>  				"between %s and [%s].\n",
>  				bad_hex, bad_hex, good_hex);
>  		}
> +		else if (!strcmp(name_bad, "new")) {
> +			fprintf(stderr, "The merge base %s is new.\n"
> +				"The property has changed "
> +				"between %s and [%s].\n",
> +				bad_hex, bad_hex, good_hex);
> +		}

After reading the previous patches in the series, I expected that this new code would know to read "terms" and does not limit us only to "new" and "old". Somewhat disappointing.

Show 6 quoted lines

> @@ -439,7 +441,7 @@ bisect_replay () {
>  		start)
>  			cmd="bisect_start $rev"
>  			eval "$cmd" ;;
> -		good|bad|skip)
> +		good|bad|skip|old|new)

Not NAME_GOOD / NAME_BAD?

To support replay, you may want to consider the "bisect terms" approach and when the bisection is using non-standard terms record that as the first entry to the bisect log.