[PATCH] describe: drop early return for max_candidates == 0

On Wed, Dec 04, 2024 at 06:27:50PM -0500, Jeff King wrote:

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@google.com>
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@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
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(-)