Re: [PATCH] fixup! describe: stop traversing when we run out of names

From: Jeff King <peff@peff.net>
Date: Dec 4, 2024, 23:27 UTC
Message-ID: <20241204232750.GA1460551@coredump.intra.peff.net>
In-Reply-To: <00270315b83b585f7d62ad1204ca1df93a668791.1733354035.git.steadmon@google.com>

On Wed, Dec 04, 2024 at 03:15:42PM -0800, Josh Steadmon wrote:

Show 13 quoted lines

> diff --git a/builtin/describe.c b/builtin/describe.c
> index 8ec3be87df..065c1bde6e 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -336,7 +336,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)
>  		return;
>  	}
>  
> -	if (!max_candidates)
> +	if (!max_candidates && !always)
>  		die(_("no tag exactly matches '%s'"), oid_to_hex(&cmit->object.oid));
>  	if (debug)
>  		fprintf(stderr, _("No exact match on refs or tags, searching to describe\n"));

Yep, this is the same spot I found. I think it's the right place to make the fix.

> Subject: Re: [PATCH] fixup! describe: stop traversing when we run out of names

This commit is already in 'next', so it's too late to squash in a change (though I'd have done this separately anyway, as it's already an issue for a manual --candidates=0 setting, as unlikely as that is).

Can you re-send with a full commit message?

Show 21 quoted lines

> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> index 5633b11d01..9aebf09d3d 100755
> --- a/t/t6120-describe.sh
> +++ b/t/t6120-describe.sh
> @@ -715,4 +715,18 @@ test_expect_success 'describe --broken --dirty with a file with changed stat' '
>  	)
>  '
>  
> +test_expect_success '--always with no refs falls back to commit hash' '
> +	git init always-no-refs &&
> +	(
> +		cd always-no-refs &&
> +		test_commit --no-tag A &&
> +		test_commit --no-tag B &&
> +		test_commit --no-tag C &&
> +		git describe --abbrev=12 --always HEAD^ >actual &&
> +		echo 13 >expected_size &&
> +		test_file_size actual >actual_size &&
> +		test_cmp expected_size actual_size
> +	)
> +'

I'm not sure if I'm missing anything subtle, but this seems more complicated than necessary to show the bug. I think just the exit code of:

  git describe --match=does-not-exist --always HEAD

is sufficient, even in a repo with tags. If you really want to check stdout, then probably comparing against:

  git rev-list -1 --abbrev-commit --abbrev=13 HEAD >expect &&
  test_cmp expect actual

is a little more obvious than the size check.

-Peff