On Wed, Dec 04, 2024 at 06:27:50PM -0500, Jeff King wrote:
> > 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?
Actually, after thinking on this a bit more, I think the solution below
is a bit more elegant. This can go on top of jk/describe-perf.
-- >8 --
From: Josh Steadmon <steadmon@xxxxxxxxxx>
Subject: [PATCH] describe: drop early return for max_candidates == 0
Before we even start the describe algorithm, we check to see if
max_candidates is 0 and bail immediately if we did not find an exact
match. This comes from 2c33f75754 (Teach git-describe --exact-match to
avoid expensive tag searches, 2008-02-24), since the the --exact-match
option just sets max_candidates to 0.
But this interacts badly with the --always option (ironically added only
a week later in da2478dbb0 (describe --always: fall back to showing an
abbreviated object name, 2008-03-02)). With --always, we'd still want to
show the hash rather than calling die().
So this:
git describe --exact-match --always
and likewise:
git describe --exact-match --candidates=0
has always been broken. But nobody ever noticed, because using those
options together is rather unlikely. However, this bug became a lot
easier to trigger with a30154187a (describe: stop traversing when we run
out of names, 2024-10-31). There we reduce max_candidates automatically
based on the number of tags available. So in a repo with no tags (or one
where --match finds no tags), max_candidates becomes 0, and --always
will never show anything.
So that early check for --exact-match's zero candidates needs to be
adjusted. One way to do so is to have it check the "always" flag and
handle it specially, producing the expected hash. But that would require
duplicating the output code for "always".
Instead, we'd prefer to just fall through to the normal algorithm, which
should notice that we are not allowed to find any more candidates, stop
looking, and then hit the regular "always" output code. Back when
2c33f75754 was first done, this was a bad idea, since the normal
algorithm kept looking for the max+1 candidate. But since 082a4d90af
(describe: stop digging for max_candidates+1, 2024-10-31), we don't do
that anymore, and the algorithm is essentially a noop.
So we can drop the early return entirely, and the fact that
max_candidates is 0 will let us quit early without any special casing.
Reported-by: Josh Steadmon <steadmon@xxxxxxxxxx>
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
There is some small bit of setup work in the algorithm, like creating
the reverse index of commits->names in a slab. I don't think that's
worth worrying about. But if we did care, we could lazily initialize
that index, which would also benefit any other cases that bail before
needing it.
builtin/describe.c | 2 --
t/t6120-describe.sh | 6 ++++++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index 8ec3be87df..21e1c87c65 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -336,8 +336,6 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)
return;
}
- if (!max_candidates)
- 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"));
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 5633b11d01..009d84ff17 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -715,4 +715,10 @@ 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 rev-parse HEAD >expect &&
+ git describe --no-abbrev --always --match=no-such-tag >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.47.1.734.g721956425b
