Volume XXII, number 280Wednesday, October 7, 2026Latest message 53 minutes ago

The Git List

News and archive of git@vger.kernel.org, since April 2005

patchdescribe: fix --exclude, --match with --contains and --all

6 messages between May 28, 2026 and Jun 2, 2026, from Jacob Keller, Junio C Hamano, Tuomas Ahola.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Jacob KellerMay 28, 2026, 23:29 UTC on lore
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(-)
Show changes to 2 files +44 −3

builtin/describe.c, t/t6120-describe.sh

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
Junio C HamanoMay 30, 2026, 23:47 UTC in reply to Jacob Keller on lore

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

Jacob Keller <jacob.e.keller@intel.com> writes:
Show 33 quoted lines
> 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?
Tuomas AholaMay 31, 2026, 23:46 UTC in reply to Junio C Hamano on lore

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

Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> 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<-----
Show changes to builtin/name-rev.c +1 −1
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) {
Junio C HamanoJun 1, 2026, 00:40 UTC in reply to Tuomas Ahola on lore

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

Tuomas Ahola <taahol@utu.fi> writes:
Show 41 quoted lines
> 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.
Jacob KellerJun 1, 2026, 22:35 UTC in reply to Junio C Hamano on lore

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

On 5/31/2026 5:40 PM, Junio C Hamano wrote:
Show 63 quoted lines
> 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.

Junio C HamanoJun 2, 2026, 00:10 UTC in reply to Jacob Keller on lore

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

Jacob Keller <jacob.e.keller@intel.com> writes:
Show 5 quoted lines
> 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.

Back to recent threads

[PATCH] describe: fix --exclude, --match with --contains and --all | The Git List