[PATCH] describe: fix --exclude, --match with --contains and --all

[PATCH] describe: fix --exclude, --match with --contains and --all

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

git describe --contains acts as a wrapper around git name-rev. When
operating with --contains and --all, the --match and --exclude patterns
are not properly forwarded to name-rev as --exclude and --refs options.

This results in the command silently discarding match and exclude
requests from the user when operating in --all mode.

We could check and die() if the user provides --contains, --all, and
--match/--exclude. However, its also straight forward to just pass the
filters down to git name-rev.

Notice that the documentation for --match and --exclude mention the
--all mode. It explains that they operate on refs with the prefix
refs/tags, and additionally refs/heads and refs/remotes when using
--all.

Fix the describe logic to pass the patterns down with the appropriate
prefixes when --all is provided. This fixes the support to match the
documented behavior.

Add tests to check that this works as expected.

Reported-by: Tuomas Ahola <taahol@utu.fi>
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---

I was looking into reviving the patch that just added a simple die() and
realized that its actually pretty straight forward to just fix the support
instead. I'm open to either route, if we think this support isn't
necessary... I'm not sure if there are any gotchas or other issues with how
I implemented this.

 builtin/describe.c  | 18 +++++++++++++++---
 t/t6120-describe.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 1c47d7c0b7c3..faaf44cec573 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -712,13 +712,25 @@ int cmd_describe(int argc,
 			     NULL);
 		if (always)
 			strvec_push(&args, "--always");
-		if (!all) {
+		if (!all)
 			strvec_push(&args, "--tags");
+
+		for_each_string_list_item(item, &patterns)
+			strvec_pushf(&args, "--refs=refs/tags/%s", item->string);
+		for_each_string_list_item(item, &exclude_patterns)
+			strvec_pushf(&args, "--exclude=refs/tags/%s", item->string);
+
+		if (all) {
 			for_each_string_list_item(item, &patterns)
-				strvec_pushf(&args, "--refs=refs/tags/%s", item->string);
+				strvec_pushf(&args, "--refs=refs/heads/%s", item->string);
 			for_each_string_list_item(item, &exclude_patterns)
-				strvec_pushf(&args, "--exclude=refs/tags/%s", item->string);
+				strvec_pushf(&args, "--exclude=refs/heads/%s", item->string);
+			for_each_string_list_item(item, &patterns)
+				strvec_pushf(&args, "--refs=refs/remotes/%s", item->string);
+			for_each_string_list_item(item, &exclude_patterns)
+				strvec_pushf(&args, "--exclude=refs/remotes/%s", item->string);
 		}
+
 		if (argc)
 			strvec_pushv(&args, argv);
 		else
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 8ee3d2c37d02..f46e628d6a1a 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -359,6 +359,35 @@ test_expect_success 'describe --contains and --no-match' '
 	test_cmp expect actual
 '
 
+test_expect_success 'describe --contains --all --match' '
+	echo "tags/A^0" >expect &&
+	tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+	test_must_fail git describe --contains --all --match="B" $tagged_commit >actual &&
+	git describe --contains --all --match="A" $tagged_commit >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'describe --contains --all --match branch' '
+	echo "branch_A" >expect &&
+	tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+	git describe --contains --all --match="branch*" $tagged_commit >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'describe --contains --all --match and --exclude' '
+	echo "branch_C~1" >expect &&
+	tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+	git describe --contains --all --match="branch*" --exclude="branch_A" $tagged_commit >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'describe --contains --all --exclude' '
+	echo "branch_A" >expect &&
+	tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+	git describe --contains --all --exclude="A" --exclude="c" --exclude="test*" $tagged_commit >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'setup and absorb a submodule' '
 	test_create_repo sub1 &&
 	test_commit -C sub1 initial &&
-- 
2.54.0.633.g0ded84c31b89

Re: [PATCH] describe: fix --exclude, --match with --contains and --all

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> From: Jacob Keller <jacob.keller@gmail.com>
>
> git describe --contains acts as a wrapper around git name-rev. When
> operating with --contains and --all, the --match and --exclude patterns
> are not properly forwarded to name-rev as --exclude and --refs options.
>
> This results in the command silently discarding match and exclude
> requests from the user when operating in --all mode.
>
> We could check and die() if the user provides --contains, --all, and
> --match/--exclude. However, its also straight forward to just pass the
> filters down to git name-rev.
>
> Notice that the documentation for --match and --exclude mention the
> --all mode. It explains that they operate on refs with the prefix
> refs/tags, and additionally refs/heads and refs/remotes when using
> --all.
>
> Fix the describe logic to pass the patterns down with the appropriate
> prefixes when --all is provided. This fixes the support to match the
> documented behavior.
>
> Add tests to check that this works as expected.
>
> Reported-by: Tuomas Ahola <taahol@utu.fi>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>
> I was looking into reviving the patch that just added a simple die() and
> realized that its actually pretty straight forward to just fix the support
> instead. I'm open to either route, if we think this support isn't
> necessary... I'm not sure if there are any gotchas or other issues with how
> I implemented this.

It is curious that this fails in some but not all CI jobs, and even
more curious that these failures look the same.

e.g., https://github.com/git/git/actions/runs/26671595367/job/78615760984#step:4:1984

  +++ diff -u expect actual
  --- expect	2026-05-30 02:21:23
  +++ actual	2026-05-30 02:21:23
  @@ -1 +1 @@
  -branch_A
  +remotes/origin/remote_branch_A
  error: last command exited with $?=1
  not ok 70 - describe --contains --all --exclude
  #	
  #		echo "branch_A" >expect &&
  #		tagged_commit=$(git rev-parse "refs/tags/A^0") &&
  #		git describe --contains --all --exclude="A" --exclude="c" --exclude="test*" $tagged_commit >actual &&
  #		test_cmp expect actual

Rings any bell?

Re: [PATCH] describe: fix --exclude, --match with --contains and --all

[Tuomas Ahola]

Junio C Hamano <gitster@pobox.com> wrote:

> It is curious that this fails in some but not all CI jobs, and even
> more curious that these failures look the same.
> 
> e.g., https://github.com/git/git/actions/runs/26671595367/job/78615760984#step:4:1984
> 
>   +++ diff -u expect actual
>   --- expect	2026-05-30 02:21:23
>   +++ actual	2026-05-30 02:21:23
>   @@ -1 +1 @@
>   -branch_A
>   +remotes/origin/remote_branch_A
>   error: last command exited with $?=1
>   not ok 70 - describe --contains --all --exclude
>   #	
>   #		echo "branch_A" >expect &&
>   #		tagged_commit=$(git rev-parse "refs/tags/A^0") &&
>   #		git describe --contains --all --exclude="A" --exclude="c" --exclude="test*" $tagged_commit >actual &&
>   #		test_cmp expect actual
> 
> Rings any bell?

That's way out of my wheelhouse but this seems to fix the failure
for Alpine at least:

-----8<-----

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index d6594ada53..1776ffab46 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -416,7 +416,7 @@ static void name_tips(struct mem_pool *string_pool)
 	 * Try to set better names first, so that worse ones spread
 	 * less.
 	 */
-	QSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age);
+	STABLE_QSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age);
 	for (i = 0; i < tip_table.nr; i++) {
 		struct tip_table_entry *e = &tip_table.table[i];
 		if (e->commit) {

Re: [PATCH] describe: fix --exclude, --match with --contains and --all

[Junio C Hamano]

Tuomas Ahola <taahol@utu.fi> writes:

> Junio C Hamano <gitster@pobox.com> wrote:
>
>> It is curious that this fails in some but not all CI jobs, and even
>> more curious that these failures look the same.
>> 
>> e.g., https://github.com/git/git/actions/runs/26671595367/job/78615760984#step:4:1984
>> 
>>   +++ diff -u expect actual
>>   --- expect	2026-05-30 02:21:23
>>   +++ actual	2026-05-30 02:21:23
>>   @@ -1 +1 @@
>>   -branch_A
>>   +remotes/origin/remote_branch_A
>>   error: last command exited with $?=1
>>   not ok 70 - describe --contains --all --exclude
>>   #	
>>   #		echo "branch_A" >expect &&
>>   #		tagged_commit=$(git rev-parse "refs/tags/A^0") &&
>>   #		git describe --contains --all --exclude="A" --exclude="c" --exclude="test*" $tagged_commit >actual &&
>>   #		test_cmp expect actual
>> 
>> Rings any bell?
>
> That's way out of my wheelhouse but this seems to fix the failure
> for Alpine at least:
>
> -----8<-----
>
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index d6594ada53..1776ffab46 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -416,7 +416,7 @@ static void name_tips(struct mem_pool *string_pool)
>  	 * Try to set better names first, so that worse ones spread
>  	 * less.
>  	 */
> -	QSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age);
> +	STABLE_QSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age);
>  	for (i = 0; i < tip_table.nr; i++) {
>  		struct tip_table_entry *e = &tip_table.table[i];
>  		if (e->commit) {

Ah, OK, when the test has multiple candidates with the same score,
of course emitting any one of them as the answer is a valid and
correctly working program.

So switching to stable-qsort here may "fix" the test breakage, but
it makes the real-world use cases worse, doesn't it?  When any one
of the solutions with the same "goodness" is acceptable, the change
makes the code behave as if the elements in the table before they
are sorted have an "if same score, earlier the better" kind of
relationship between them.

I would have preferred to see a tweak on the test side to avoid
having more than one answer of the same goodness, or perhaps list
all the possible acceptable answers and instead of using test_cmp to
check for the exact answer, take any of the acceptable ones, or
something like that.

Thanks.

Re: [PATCH] describe: fix --exclude, --match with --contains and --all

[Jacob Keller]

On 5/31/2026 5:40 PM, Junio C Hamano wrote:
> Tuomas Ahola <taahol@utu.fi> writes:
> 
>> Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> It is curious that this fails in some but not all CI jobs, and even
>>> more curious that these failures look the same.
>>>
>>> e.g., https://github.com/git/git/actions/runs/26671595367/job/78615760984#step:4:1984
>>>
>>>   +++ diff -u expect actual
>>>   --- expect	2026-05-30 02:21:23
>>>   +++ actual	2026-05-30 02:21:23
>>>   @@ -1 +1 @@
>>>   -branch_A
>>>   +remotes/origin/remote_branch_A
>>>   error: last command exited with $?=1
>>>   not ok 70 - describe --contains --all --exclude
>>>   #	
>>>   #		echo "branch_A" >expect &&
>>>   #		tagged_commit=$(git rev-parse "refs/tags/A^0") &&
>>>   #		git describe --contains --all --exclude="A" --exclude="c" --exclude="test*" $tagged_commit >actual &&
>>>   #		test_cmp expect actual
>>>
>>> Rings any bell?
>>
>> That's way out of my wheelhouse but this seems to fix the failure
>> for Alpine at least:
>>
>> -----8<-----
>>
>> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
>> index d6594ada53..1776ffab46 100644
>> --- a/builtin/name-rev.c
>> +++ b/builtin/name-rev.c
>> @@ -416,7 +416,7 @@ static void name_tips(struct mem_pool *string_pool)
>>  	 * Try to set better names first, so that worse ones spread
>>  	 * less.
>>  	 */
>> -	QSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age);
>> +	STABLE_QSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age);
>>  	for (i = 0; i < tip_table.nr; i++) {
>>  		struct tip_table_entry *e = &tip_table.table[i];
>>  		if (e->commit) {
> 
> Ah, OK, when the test has multiple candidates with the same score,
> of course emitting any one of them as the answer is a valid and
> correctly working program.
> 
> So switching to stable-qsort here may "fix" the test breakage, but
> it makes the real-world use cases worse, doesn't it?  When any one
> of the solutions with the same "goodness" is acceptable, the change
> makes the code behave as if the elements in the table before they
> are sorted have an "if same score, earlier the better" kind of
> relationship between them.
> 
> I would have preferred to see a tweak on the test side to avoid
> having more than one answer of the same goodness, or perhaps list
> all the possible acceptable answers and instead of using test_cmp to
> check for the exact answer, take any of the acceptable ones, or
> something like that.
> 
> Thanks.
> 

Ya something like that is probably better. I'll look at cooking up a v2
which improves the test here. I think part of the issue is that the
previous tests setup a bunch of tags and branches, so figuring out what
all the possible outputs are is tricky. Probably I can just add
additional excludes until there is only one answer.

Re: [PATCH] describe: fix --exclude, --match with --contains and --all

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> Ya something like that is probably better. I'll look at cooking up a v2
> which improves the test here. I think part of the issue is that the
> previous tests setup a bunch of tags and branches, so figuring out what
> all the possible outputs are is tricky. Probably I can just add
> additional excludes until there is only one answer.

That sounds workable.  Thanks.