git bisect fails to handle annotated tags

git bisect fails to handle annotated tags

[Andreas Schwab]

$ git --version
git version 2.31.0
$ git bisect start
$ git bisect good v2.30.0
$ git bisect bad v2.31.0
3e90d4b58f3819cfd58ac61cb8668e83d3ea0563 was both good and bad
$ git bisect log
git bisect start
# good: [71ca53e8125e36efbda17293c50027d31681a41f] Git 2.30
git bisect good 2d9685d47a7e516281aa093bf0cddc8aafa72448
# bad: [a5828ae6b52137b913b978e16cd2334482eb4c1f] Git 2.31
git bisect bad 3e90d4b58f3819cfd58ac61cb8668e83d3ea0563
$ git bisect start
Already on 'master'
Your branch is up to date with 'origin/master'.
$ git bisect good v2.30.0^{}
$ git bisect bad v2.31.0^{}
Bisecting: 453 revisions left to test after this (roughly 9 steps)
[41abfe15d95ede4c2a047180a6062eac23d8f7d6] maintenance: add pack-refs task
$ git bisect log
git bisect start
# good: [71ca53e8125e36efbda17293c50027d31681a41f] Git 2.30
git bisect good 71ca53e8125e36efbda17293c50027d31681a41f
# bad: [a5828ae6b52137b913b978e16cd2334482eb4c1f] Git 2.31
git bisect bad a5828ae6b52137b913b978e16cd2334482eb4c1f

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: git bisect fails to handle annotated tags

[Jeff King]

On Tue, Mar 16, 2021 at 02:05:51PM +0100, Andreas Schwab wrote:

> $ git --version
> git version 2.31.0
> $ git bisect start
> $ git bisect good v2.30.0
> $ git bisect bad v2.31.0
> 3e90d4b58f3819cfd58ac61cb8668e83d3ea0563 was both good and bad

Looks like it bisects to 27257bc466 (bisect--helper: reimplement
`bisect_state` & `bisect_head` shell functions in C, 2020-10-15), which
isn't too surprising. So it broke in v2.30, but nobody seems to have
noticed during the last cycle.

I'd guess it's just missing a call to peel the input oid.

-Peff

[PATCH] bisect: peel annotated tags to commits

[Jeff King]

On Tue, Mar 16, 2021 at 10:53:19AM -0400, Jeff King wrote:

> On Tue, Mar 16, 2021 at 02:05:51PM +0100, Andreas Schwab wrote:
> 
> > $ git --version
> > git version 2.31.0
> > $ git bisect start
> > $ git bisect good v2.30.0
> > $ git bisect bad v2.31.0
> > 3e90d4b58f3819cfd58ac61cb8668e83d3ea0563 was both good and bad
> 
> Looks like it bisects to 27257bc466 (bisect--helper: reimplement
> `bisect_state` & `bisect_head` shell functions in C, 2020-10-15), which
> isn't too surprising. So it broke in v2.30, but nobody seems to have
> noticed during the last cycle.
> 
> I'd guess it's just missing a call to peel the input oid.

Yep. Here's a fix. Again, not new in v2.31, so we don't have to worry
about a brown-bag fix for yesterday's release. But I do think it's worth
trying to get onto a maint release. I prepared this patch on top of
mr/bisect-in-c-3.

-- >8 --
Subject: [PATCH] bisect: peel annotated tags to commits

This patch fixes a bug where git-bisect doesn't handle receiving
annotated tags as "git bisect good <tag>", etc. It's a regression in
27257bc466 (bisect--helper: reimplement `bisect_state` & `bisect_head`
shell functions in C, 2020-10-15).

The original shell code called:

  sha=$(git rev-parse --verify "$rev^{commit}") ||
          die "$(eval_gettext "Bad rev input: \$rev")"

which will peel the input to a commit (or complain if that's not
possible). But the C code just calls get_oid(), which will yield the oid
of the tag.

The fix is to peel to a commit. The error message here is a little
non-idiomatic for Git (since it starts with a capital). I've mostly left
it, as it matches the other converted messages (like the "Bad rev input"
we print when get_oid() fails), though I did add an indication that it
was the peeling that was the problem. It might be worth taking a pass
through this converted code to modernize some of the error messages.

Note also that the test does a bare "grep" (not i18ngrep) on the
expected "X is the first bad commit" output message. This matches the
rest of the test script.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/bisect--helper.c    |  9 ++++++++-
 t/t6030-bisect-porcelain.sh | 12 ++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index fc6ca257a4..f0eeb4a2f0 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -876,12 +876,19 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a
 	 */
 
 	for (; argc; argc--, argv++) {
+		struct commit *commit;
+
 		if (get_oid(*argv, &oid)){
 			error(_("Bad rev input: %s"), *argv);
 			oid_array_clear(&revs);
 			return BISECT_FAILED;
 		}
-		oid_array_append(&revs, &oid);
+
+		commit = lookup_commit_reference(the_repository, &oid);
+		if (!commit)
+			die(_("Bad rev input (not a commit): %s"), *argv);
+
+		oid_array_append(&revs, &commit->object.oid);
 	}
 
 	if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index b886529e59..9c389553a7 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -929,4 +929,16 @@ test_expect_success 'git bisect reset cleans bisection state properly' '
 	test_path_is_missing "$GIT_DIR/BISECT_START"
 '
 
+test_expect_success 'bisect handles annotated tags' '
+	test_commit commit-one &&
+	git tag -m foo tag-one &&
+	test_commit commit-two &&
+	git tag -m foo tag-two &&
+	git bisect start &&
+	git bisect good tag-one &&
+	git bisect bad tag-two >output &&
+	bad=$(git rev-parse --verify tag-two^{commit}) &&
+	grep "$bad is the first bad commit" output
+'
+
 test_done
-- 
2.31.0.559.g509d4a088b

Re: [PATCH] bisect: peel annotated tags to commits

[Bagas Sanjaya]

I can reproduce this issue with v2.31.0 (as you mentioned).

Applying the patch, bisecting between annotated tags now worked
just before git bisect is rewritten in C.

Thanks.

Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>

On 16/03/21 22.15, Jeff King wrote:
> On Tue, Mar 16, 2021 at 10:53:19AM -0400, Jeff King wrote:
> 
>> On Tue, Mar 16, 2021 at 02:05:51PM +0100, Andreas Schwab wrote:
>>
>>> $ git --version
>>> git version 2.31.0
>>> $ git bisect start
>>> $ git bisect good v2.30.0
>>> $ git bisect bad v2.31.0
>>> 3e90d4b58f3819cfd58ac61cb8668e83d3ea0563 was both good and bad
>>
>> Looks like it bisects to 27257bc466 (bisect--helper: reimplement
>> `bisect_state` & `bisect_head` shell functions in C, 2020-10-15), which
>> isn't too surprising. So it broke in v2.30, but nobody seems to have
>> noticed during the last cycle.
>>
>> I'd guess it's just missing a call to peel the input oid.
> 
> Yep. Here's a fix. Again, not new in v2.31, so we don't have to worry
> about a brown-bag fix for yesterday's release. But I do think it's worth
> trying to get onto a maint release. I prepared this patch on top of
> mr/bisect-in-c-3.
> 
> -- >8 --
> Subject: [PATCH] bisect: peel annotated tags to commits
> 
> This patch fixes a bug where git-bisect doesn't handle receiving
> annotated tags as "git bisect good <tag>", etc. It's a regression in
> 27257bc466 (bisect--helper: reimplement `bisect_state` & `bisect_head`
> shell functions in C, 2020-10-15).
> 
> The original shell code called:
> 
>    sha=$(git rev-parse --verify "$rev^{commit}") ||
>            die "$(eval_gettext "Bad rev input: \$rev")"
> 
> which will peel the input to a commit (or complain if that's not
> possible). But the C code just calls get_oid(), which will yield the oid
> of the tag.
> 
> The fix is to peel to a commit. The error message here is a little
> non-idiomatic for Git (since it starts with a capital). I've mostly left
> it, as it matches the other converted messages (like the "Bad rev input"
> we print when get_oid() fails), though I did add an indication that it
> was the peeling that was the problem. It might be worth taking a pass
> through this converted code to modernize some of the error messages.
> 
> Note also that the test does a bare "grep" (not i18ngrep) on the
> expected "X is the first bad commit" output message. This matches the
> rest of the test script.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   builtin/bisect--helper.c    |  9 ++++++++-
>   t/t6030-bisect-porcelain.sh | 12 ++++++++++++
>   2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index fc6ca257a4..f0eeb4a2f0 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -876,12 +876,19 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a
>   	 */
>   
>   	for (; argc; argc--, argv++) {
> +		struct commit *commit;
> +
>   		if (get_oid(*argv, &oid)){
>   			error(_("Bad rev input: %s"), *argv);
>   			oid_array_clear(&revs);
>   			return BISECT_FAILED;
>   		}
> -		oid_array_append(&revs, &oid);
> +
> +		commit = lookup_commit_reference(the_repository, &oid);
> +		if (!commit)
> +			die(_("Bad rev input (not a commit): %s"), *argv);
> +
> +		oid_array_append(&revs, &commit->object.oid);
>   	}
>   
>   	if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index b886529e59..9c389553a7 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -929,4 +929,16 @@ test_expect_success 'git bisect reset cleans bisection state properly' '
>   	test_path_is_missing "$GIT_DIR/BISECT_START"
>   '
>   
> +test_expect_success 'bisect handles annotated tags' '
> +	test_commit commit-one &&
> +	git tag -m foo tag-one &&
> +	test_commit commit-two &&
> +	git tag -m foo tag-two &&
> +	git bisect start &&
> +	git bisect good tag-one &&
> +	git bisect bad tag-two >output &&
> +	bad=$(git rev-parse --verify tag-two^{commit}) &&
> +	grep "$bad is the first bad commit" output
> +'
> +
>   test_done
> 

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH] bisect: peel annotated tags to commits

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Looks like it bisects to 27257bc466 (bisect--helper: reimplement
>> `bisect_state` & `bisect_head` shell functions in C, 2020-10-15), which
>> isn't too surprising. So it broke in v2.30, but nobody seems to have
>> noticed during the last cycle.
>> 
>> I'd guess it's just missing a call to peel the input oid.
>
> Yep. Here's a fix. Again, not new in v2.31, so we don't have to worry
> about a brown-bag fix for yesterday's release. But I do think it's worth
> trying to get onto a maint release. I prepared this patch on top of
> mr/bisect-in-c-3.

Thanks.  Yes, if we ever do another update to 2.30.x, this probably
should be in it, as it is expected that people will start with tags
and not with individual commits.