[PATCH v2] describe: limit default ref iteration to tags

[PATCH v2] describe: limit default ref iteration to tags

[Tamir Duberstein]

Unless --all is given, get_name() rejects every ref outside refs/tags/.
The rejection happens only after the ref backend has enumerated the ref,
so repositories with many other refs spend most of a simple describe
invocation visiting refs which cannot affect its result.

Commit 8a5a1884e9 (Avoid accessing non-tag refs in git-describe unless
--all is requested, 2008-02-24) moved this rejection before object
lookup, but left iteration unscoped. Pass the existing refs/tags/
restriction to the iterator unless --all is given so the backend can
avoid unrelated refs.

The benchmark checkout had 120,532 refs, of which 330 were tags. With
`$repo` naming the checkout, `$commit` an exactly tagged commit, and
`$parent` and `$this` the two binaries, I ran:

    hyperfine --warmup 3 --runs 15 \
        --command-name parent \
        '$parent -C $repo describe --exact-match $commit' \
        --command-name 'this commit' \
        '$this -C $repo describe --exact-match $commit'

The results were:

    Benchmark 1: parent
      Time (mean ± σ):     171.7 ms ±  18.5 ms    [User: 23.9 ms, System: 133.6 ms]
      Range (min … max):   142.3 ms … 198.3 ms    15 runs

    Benchmark 2: this commit
      Time (mean ± σ):       9.9 ms ±   1.1 ms    [User: 3.3 ms, System: 4.7 ms]
      Range (min … max):     8.8 ms …  13.1 ms    15 runs

    Summary
      this commit ran
       17.35 ± 2.63 times faster than parent

Both revisions were built with -O3, -mcpu=native, and ThinLTO using
Apple clang 21.0.0 on macOS 26.5. The machine was a MacBook Pro
(Mac16,6) with a 16-core Apple M4 Max (12 performance and four
efficiency cores) and 128 GB RAM.

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
Changes in v2:
- Exercise the performance test with both ref backends.
- Keep the ref count local to its setup test.
- Report native hyperfine output for an exact-tag lookup.
- Link to v1: https://patch.msgid.link/20260607-describe-tag-ref-scope-v1-1-653d232b86b5@gmail.com
---
 builtin/describe.c       |  3 +++
 t/perf/p6100-describe.sh | 15 +++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/builtin/describe.c b/builtin/describe.c
index 1c47d7c0b7..3532c8ff22 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -740,6 +740,9 @@ int cmd_describe(int argc,
 		return ret;
 	}
 
+	if (!all)
+		for_each_ref_opts.prefix = "refs/tags/";
+
 	hashmap_init(&names, commit_name_neq, NULL, 0);
 	refs_for_each_ref_ext(get_main_ref_store(the_repository),
 			      get_name, NULL, &for_each_ref_opts);
diff --git a/t/perf/p6100-describe.sh b/t/perf/p6100-describe.sh
index 069f91ce49..ed9f1abe18 100755
--- a/t/perf/p6100-describe.sh
+++ b/t/perf/p6100-describe.sh
@@ -27,4 +27,19 @@ test_perf 'describe HEAD with one tag' '
 	git describe --match=new HEAD
 '
 
+test_expect_success 'set up many unrelated refs' '
+	ref_count=10000 &&
+	git tag -m tip tip HEAD &&
+	for i in $(test_seq $ref_count)
+	do
+		printf "create refs/heads/describe-perf/%05d HEAD\n" $i ||
+		return 1
+	done >instructions &&
+	git update-ref --stdin <instructions
+'
+
+test_perf 'describe exact tag with many unrelated refs' '
+	git describe --exact-match HEAD
+'
+
 test_done

---
base-commit: 9ac3f193c05c2237e2b14ebaa1149e9fc8a1abe0
change-id: 20260607-describe-tag-ref-scope-7d00ae140a58

Best regards,
--  
Tamir Duberstein <tamird@gmail.com>

Re: [PATCH v2] describe: limit default ref iteration to tags

[Jeff King]

On Mon, Jun 08, 2026 at 07:32:14PM -0700, Tamir Duberstein wrote:

> The benchmark checkout had 120,532 refs, of which 330 were tags. With
> `$repo` naming the checkout, `$commit` an exactly tagged commit, and
> `$parent` and `$this` the two binaries, I ran:
> 
>     hyperfine --warmup 3 --runs 15 \
>         --command-name parent \
>         '$parent -C $repo describe --exact-match $commit' \
>         --command-name 'this commit' \
>         '$this -C $repo describe --exact-match $commit'
> 
> The results were:
> 
>     Benchmark 1: parent
>       Time (mean ± σ):     171.7 ms ±  18.5 ms    [User: 23.9 ms, System: 133.6 ms]
>       Range (min … max):   142.3 ms … 198.3 ms    15 runs
> 
>     Benchmark 2: this commit
>       Time (mean ± σ):       9.9 ms ±   1.1 ms    [User: 3.3 ms, System: 4.7 ms]
>       Range (min … max):     8.8 ms …  13.1 ms    15 runs
> 
>     Summary
>       this commit ran
>        17.35 ± 2.63 times faster than parent
> 
> Both revisions were built with -O3, -mcpu=native, and ThinLTO using
> Apple clang 21.0.0 on macOS 26.5. The machine was a MacBook Pro
> (Mac16,6) with a 16-core Apple M4 Max (12 performance and four
> efficiency cores) and 128 GB RAM.

This patch looks fine to me, but let me pick a nit for a minute, because
I think there is a broader conversation to be had.

Given the discussion in earlier rounds and sibling topics, I assume the
commit message here was AI-generated. And it's OK in the sense that it
is describing what happened and I assume is entirely accurate. But as a
human reader, it feels so much more verbose than what I'd expect, as it
is full of semi-irrelevant details. Why set --warmup and --runs? Why
bother with --command-name, which just means you have to show the
commands separately anyway? Is the amount of RAM in the machine
important for this test? Surely it could be if it was absurdly tiny, but
in general, no, I would not expect it to be.

So while it is perhaps reasonable to document every detail in case
somebody later wants to verify or reproduce timings, it is a little
overwhelming when trying to tell a story, the core of which is:

  In a repo with ~120k refs, ~300 of which were tags, running:

    git describe --exact-match $some_tag

  went from ~170ms to ~10ms, since we no longer needed to iterate all of
  those other refs.

That has _way_ less detail, but makes the point succinctly.

I dunno. I am not trying to pick apart your commit in particular, but am
more interested in the broader use of AI commit messages going forward.
This kind of verbosity is quite common in the output (from my limited
experience), and I think creates more work for reviewers. Should we be
expecting contributors to make things more concise before submitting
(either manually or through prompting)? Or do people even agree that the
shorter version is preferable? I could be the only one.

I have a few other comments on the patch itself below.

> diff --git a/builtin/describe.c b/builtin/describe.c
> index 1c47d7c0b7..3532c8ff22 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -740,6 +740,9 @@ int cmd_describe(int argc,
>  		return ret;
>  	}
>  
> +	if (!all)
> +		for_each_ref_opts.prefix = "refs/tags/";
> +
>  	hashmap_init(&names, commit_name_neq, NULL, 0);
>  	refs_for_each_ref_ext(get_main_ref_store(the_repository),
>  			      get_name, NULL, &for_each_ref_opts);

The code change looks fine. It creates a bit of a subtle dependency
between what's happening here, and the filtering inside get_name(). But
I think that's OK for the scope of a single command. It _might_ be
possible to simplify the top of get_name(), since we'd no longer see
non-tag refs in the input. But it also may not, since we have to strip
out the prefix anyway. It can certainly come on top as a cleanup later
if we want.

> diff --git a/t/perf/p6100-describe.sh b/t/perf/p6100-describe.sh

It is a little curious that we add a perf test here, but the commit
message does not even show it off. ;)

I ran it myself here and had trouble showing improvement, simply because
it is already quite fast! I guess that's because I'm on Linux, where
warm-cache filesystem operations are pretty fast. Bumping $ref_count by
a factor of 10 made the "before" case 30ms, and after is still sub-1ms.

> +test_expect_success 'set up many unrelated refs' '
> +	ref_count=10000 &&
> +	git tag -m tip tip HEAD &&
> +	for i in $(test_seq $ref_count)
> +	do
> +		printf "create refs/heads/describe-perf/%05d HEAD\n" $i ||
> +		return 1
> +	done >instructions &&
> +	git update-ref --stdin <instructions
> +'

A few things come to mind on reading this.

I have mixed feelings on sticking synthetic constructions in the t/perf
suite. Part of the original point was that we'd run it against real
repos to see how they perform. But that implies that people running it
have some clue about which tests may be interesting on which repos,
which is hopeful at best. So we've turned to this kind of synthetic
construction at times (and this is certainly not the first). It's
probably a reasonable tactic here.

I suspect the resulting state is not all that realistic, though. If you
have 10,000 refs, you probably didn't make them all at once. And so in
practice the majority of them would be packed. Sticking "git pack-refs
--all" at the end might give more realistic numbers.

Bumping to a larger number of refs shows the effect more clearly, but at
the cost of making the setup take a long time (since we have to take a
lockfile on each ref!). We could sneak around it by generating a
packed-refs file directly, but now the test really would be
backend-specific. Probably better not to go there.

And finally, the loop can be written a bit more succinctly these days
as:

diff --git a/t/perf/p6100-describe.sh b/t/perf/p6100-describe.sh
index ed9f1abe18..b365dc67ee 100755
--- a/t/perf/p6100-describe.sh
+++ b/t/perf/p6100-describe.sh
@@ -30,12 +30,8 @@ test_perf 'describe HEAD with one tag' '
 test_expect_success 'set up many unrelated refs' '
 	ref_count=10000 &&
 	git tag -m tip tip HEAD &&
-	for i in $(test_seq $ref_count)
-	do
-		printf "create refs/heads/describe-perf/%05d HEAD\n" $i ||
-		return 1
-	done >instructions &&
-	git update-ref --stdin <instructions
+	test_seq -f "create refs/heads/describe-perf/%05d HEAD" $ref_count |
+	git update-ref --stdin
 '
 
 test_perf 'describe exact tag with many unrelated refs' '


Probably not worth re-rolling on its own, though.

-Peff

Re: [PATCH v2] describe: limit default ref iteration to tags

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So while it is perhaps reasonable to document every detail in case
> somebody later wants to verify or reproduce timings, it is a little
> overwhelming when trying to tell a story, the core of which is:
>
>   In a repo with ~120k refs, ~300 of which were tags, running:
>
>     git describe --exact-match $some_tag
>
>   went from ~170ms to ~10ms, since we no longer needed to iterate all of
>   those other refs.
>
> That has _way_ less detail, but makes the point succinctly.
>
> I dunno. I am not trying to pick apart your commit in particular, but am
> more interested in the broader use of AI commit messages going forward.
> This kind of verbosity is quite common in the output (from my limited
> experience), and I think creates more work for reviewers. Should we be
> expecting contributors to make things more concise before submitting
> (either manually or through prompting)? Or do people even agree that the
> shorter version is preferable? I could be the only one.

Count me in.  You are the one who often gives us a patch with 60
lines that explains a single line change, but I haven't found these
60 lines are _overly verbose_ in the same way as AI generated log
messages.

Re: [PATCH v2] describe: limit default ref iteration to tags

[D. Ben Knoble]

On Tue, Jun 9, 2026 at 7:10 AM Jeff King <peff@peff.net> wrote:
>
> On Mon, Jun 08, 2026 at 07:32:14PM -0700, Tamir Duberstein wrote:
>
> > The benchmark checkout had 120,532 refs, of which 330 were tags. With
> > `$repo` naming the checkout, `$commit` an exactly tagged commit, and
> > `$parent` and `$this` the two binaries, I ran:
> >
> >     hyperfine --warmup 3 --runs 15 \
> >         --command-name parent \
> >         '$parent -C $repo describe --exact-match $commit' \
> >         --command-name 'this commit' \
> >         '$this -C $repo describe --exact-match $commit'
> >
> > The results were:
> >
> >     Benchmark 1: parent
> >       Time (mean ± σ):     171.7 ms ±  18.5 ms    [User: 23.9 ms, System: 133.6 ms]
> >       Range (min … max):   142.3 ms … 198.3 ms    15 runs
> >
> >     Benchmark 2: this commit
> >       Time (mean ± σ):       9.9 ms ±   1.1 ms    [User: 3.3 ms, System: 4.7 ms]
> >       Range (min … max):     8.8 ms …  13.1 ms    15 runs
> >
> >     Summary
> >       this commit ran
> >        17.35 ± 2.63 times faster than parent
> >
> > Both revisions were built with -O3, -mcpu=native, and ThinLTO using
> > Apple clang 21.0.0 on macOS 26.5. The machine was a MacBook Pro
> > (Mac16,6) with a 16-core Apple M4 Max (12 performance and four
> > efficiency cores) and 128 GB RAM.
>
> This patch looks fine to me, but let me pick a nit for a minute, because
> I think there is a broader conversation to be had.
>
> Given the discussion in earlier rounds and sibling topics, I assume the
> commit message here was AI-generated. And it's OK in the sense that it
> is describing what happened and I assume is entirely accurate. But as a
> human reader, it feels so much more verbose than what I'd expect, as it
> is full of semi-irrelevant details. Why set --warmup and --runs? Why
> bother with --command-name, which just means you have to show the
> commands separately anyway? Is the amount of RAM in the machine
> important for this test? Surely it could be if it was absurdly tiny, but
> in general, no, I would not expect it to be.

[You probably know this] It is common in academic papers to report
benchmarks with details about the hardware and how they were run to
contextualize the results and help with reproducibility.

Of course, Git's commits do not form an academic paper… so I have no
real opinion on what to see here. But I've seen a few other mails
where having perf test outputs or similar was suggested (maybe that
was to be reserved for the cover letter? idk).

_If_ we show all the hyperfine details, I think it's reasonable to use
--command-name to make distinguishing the versions easy, unless it's
obvious from the path/to/git in each benchmark (which I think I've
seen from Peff's benchmark reports before?).

Someone with better lore skills can probably dig up a few exemplars of
how to write about performance in a commit message?

-- 
D. Ben Knoble

Re: [PATCH v2] describe: limit default ref iteration to tags

[Tamir Duberstein]

On Tue, Jun 9, 2026 at 4:09 AM Jeff King <peff@peff.net> wrote:
>
> On Mon, Jun 08, 2026 at 07:32:14PM -0700, Tamir Duberstein wrote:
>
> > The benchmark checkout had 120,532 refs, of which 330 were tags. With
> > `$repo` naming the checkout, `$commit` an exactly tagged commit, and
> > `$parent` and `$this` the two binaries, I ran:
> >
> >     hyperfine --warmup 3 --runs 15 \
> >         --command-name parent \
> >         '$parent -C $repo describe --exact-match $commit' \
> >         --command-name 'this commit' \
> >         '$this -C $repo describe --exact-match $commit'
> >
> > The results were:
> >
> >     Benchmark 1: parent
> >       Time (mean ± σ):     171.7 ms ±  18.5 ms    [User: 23.9 ms, System: 133.6 ms]
> >       Range (min … max):   142.3 ms … 198.3 ms    15 runs
> >
> >     Benchmark 2: this commit
> >       Time (mean ± σ):       9.9 ms ±   1.1 ms    [User: 3.3 ms, System: 4.7 ms]
> >       Range (min … max):     8.8 ms …  13.1 ms    15 runs
> >
> >     Summary
> >       this commit ran
> >        17.35 ± 2.63 times faster than parent
> >
> > Both revisions were built with -O3, -mcpu=native, and ThinLTO using
> > Apple clang 21.0.0 on macOS 26.5. The machine was a MacBook Pro
> > (Mac16,6) with a 16-core Apple M4 Max (12 performance and four
> > efficiency cores) and 128 GB RAM.
>
> This patch looks fine to me, but let me pick a nit for a minute, because
> I think there is a broader conversation to be had.

Just to say from the start: I appreciate you taking the time to discuss this.

>
> Given the discussion in earlier rounds and sibling topics, I assume the
> commit message here was AI-generated. And it's OK in the sense that it
> is describing what happened and I assume is entirely accurate. But as a
> human reader, it feels so much more verbose than what I'd expect, as it
> is full of semi-irrelevant details. Why set --warmup and --runs? Why
> bother with --command-name, which just means you have to show the
> commands separately anyway? Is the amount of RAM in the machine
> important for this test? Surely it could be if it was absurdly tiny, but
> in general, no, I would not expect it to be.

Well, the details matter in case some human reader knows something I
don't, or wants to reproduce the findings and observes something
completely different - they aught to be able to reconstruct my
environment.

The command-name flag is needed; without it the output of the
hyperfine would include local paths and would require post-processing
to include in the message.

>
> So while it is perhaps reasonable to document every detail in case
> somebody later wants to verify or reproduce timings, it is a little
> overwhelming when trying to tell a story, the core of which is:
>
>   In a repo with ~120k refs, ~300 of which were tags, running:
>
>     git describe --exact-match $some_tag
>
>   went from ~170ms to ~10ms, since we no longer needed to iterate all of
>   those other refs.
>
> That has _way_ less detail, but makes the point succinctly.

I don't disagree. Ultimately, it is a matter of maintainer preference,
and I'm happy to follow (and instruct the AI to follow) the
preferences described in this thread.

>
> I dunno. I am not trying to pick apart your commit in particular, but am
> more interested in the broader use of AI commit messages going forward.
> This kind of verbosity is quite common in the output (from my limited
> experience), and I think creates more work for reviewers. Should we be
> expecting contributors to make things more concise before submitting
> (either manually or through prompting)? Or do people even agree that the
> shorter version is preferable? I could be the only one.

The AI does what you tell it; in this case I was telling it to follow
the precedent in the repo and to ensure its claims are always cited.
I'll tune it for succinct output going forward.

>
> I have a few other comments on the patch itself below.
>
> > diff --git a/builtin/describe.c b/builtin/describe.c
> > index 1c47d7c0b7..3532c8ff22 100644
> > --- a/builtin/describe.c
> > +++ b/builtin/describe.c
> > @@ -740,6 +740,9 @@ int cmd_describe(int argc,
> >               return ret;
> >       }
> >
> > +     if (!all)
> > +             for_each_ref_opts.prefix = "refs/tags/";
> > +
> >       hashmap_init(&names, commit_name_neq, NULL, 0);
> >       refs_for_each_ref_ext(get_main_ref_store(the_repository),
> >                             get_name, NULL, &for_each_ref_opts);
>
> The code change looks fine. It creates a bit of a subtle dependency
> between what's happening here, and the filtering inside get_name(). But
> I think that's OK for the scope of a single command. It _might_ be
> possible to simplify the top of get_name(), since we'd no longer see
> non-tag refs in the input. But it also may not, since we have to strip
> out the prefix anyway. It can certainly come on top as a cleanup later
> if we want.
>
> > diff --git a/t/perf/p6100-describe.sh b/t/perf/p6100-describe.sh
>
> It is a little curious that we add a perf test here, but the commit
> message does not even show it off. ;)
>
> I ran it myself here and had trouble showing improvement, simply because
> it is already quite fast! I guess that's because I'm on Linux, where
> warm-cache filesystem operations are pretty fast. Bumping $ref_count by
> a factor of 10 made the "before" case 30ms, and after is still sub-1ms.
>
> > +test_expect_success 'set up many unrelated refs' '
> > +     ref_count=10000 &&
> > +     git tag -m tip tip HEAD &&
> > +     for i in $(test_seq $ref_count)
> > +     do
> > +             printf "create refs/heads/describe-perf/%05d HEAD\n" $i ||
> > +             return 1
> > +     done >instructions &&
> > +     git update-ref --stdin <instructions
> > +'
>
> A few things come to mind on reading this.
>
> I have mixed feelings on sticking synthetic constructions in the t/perf
> suite. Part of the original point was that we'd run it against real
> repos to see how they perform. But that implies that people running it
> have some clue about which tests may be interesting on which repos,
> which is hopeful at best. So we've turned to this kind of synthetic
> construction at times (and this is certainly not the first). It's
> probably a reasonable tactic here.
>
> I suspect the resulting state is not all that realistic, though. If you
> have 10,000 refs, you probably didn't make them all at once. And so in
> practice the majority of them would be packed. Sticking "git pack-refs
> --all" at the end might give more realistic numbers.
>
> Bumping to a larger number of refs shows the effect more clearly, but at
> the cost of making the setup take a long time (since we have to take a
> lockfile on each ref!). We could sneak around it by generating a
> packed-refs file directly, but now the test really would be
> backend-specific. Probably better not to go there.
>
> And finally, the loop can be written a bit more succinctly these days
> as:
>
> diff --git a/t/perf/p6100-describe.sh b/t/perf/p6100-describe.sh
> index ed9f1abe18..b365dc67ee 100755
> --- a/t/perf/p6100-describe.sh
> +++ b/t/perf/p6100-describe.sh
> @@ -30,12 +30,8 @@ test_perf 'describe HEAD with one tag' '
>  test_expect_success 'set up many unrelated refs' '
>         ref_count=10000 &&
>         git tag -m tip tip HEAD &&
> -       for i in $(test_seq $ref_count)
> -       do
> -               printf "create refs/heads/describe-perf/%05d HEAD\n" $i ||
> -               return 1
> -       done >instructions &&
> -       git update-ref --stdin <instructions
> +       test_seq -f "create refs/heads/describe-perf/%05d HEAD" $ref_count |
> +       git update-ref --stdin
>  '
>
>  test_perf 'describe exact tag with many unrelated refs' '
>
>
> Probably not worth re-rolling on its own, though.

The suggested changes seem reasonable to me. Certainly I am happy to
make them, and re-rolls are cheap. Do let me know explicitly if you'd
like that done.

>
> -Peff

Thanks for your time! I really appreciate it.