{"thread":{"id":"65710","subject":"[PATCH] describe: fix --exclude, --match with --contains and --all","startedAt":"2026-05-28T23:31:18Z","lastAt":"2026-06-02T00:11:02Z","messageCount":6,"participants":["Jacob Keller","Junio C Hamano","Tuomas Ahola"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"544246","messageId":"20260528232950.187002-2-jacob.e.keller@intel.com","threadId":"65710","inReplyTo":null,"subject":"[PATCH] describe: fix --exclude, --match with --contains and --all","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2026-05-28T23:29:51Z","receivedAt":"2026-05-28T23:31:18Z","isPatch":true,"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\ngit describe --contains acts as a wrapper around git name-rev. When\noperating with --contains and --all, the --match and --exclude patterns\nare not properly forwarded to name-rev as --exclude and --refs options.\n\nThis results in the command silently discarding match and exclude\nrequests from the user when operating in --all mode.\n\nWe could check and die() if the user provides --contains, --all, and\n--match/--exclude. However, its also straight forward to just pass the\nfilters down to git name-rev.\n\nNotice that the documentation for --match and --exclude mention the\n--all mode. It explains that they operate on refs with the prefix\nrefs/tags, and additionally refs/heads and refs/remotes when using\n--all.\n\nFix the describe logic to pass the patterns down with the appropriate\nprefixes when --all is provided. This fixes the support to match the\ndocumented behavior.\n\nAdd tests to check that this works as expected.\n\nReported-by: Tuomas Ahola <taahol@utu.fi>\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n\nI was looking into reviving the patch that just added a simple die() and\nrealized that its actually pretty straight forward to just fix the support\ninstead. I'm open to either route, if we think this support isn't\nnecessary... I'm not sure if there are any gotchas or other issues with how\nI implemented this.\n\n builtin/describe.c  | 18 +++++++++++++++---\n t/t6120-describe.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 1c47d7c0b7c3..faaf44cec573 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -712,13 +712,25 @@ int cmd_describe(int argc,\n \t\t\t     NULL);\n \t\tif (always)\n \t\t\tstrvec_push(&args, \"--always\");\n-\t\tif (!all) {\n+\t\tif (!all)\n \t\t\tstrvec_push(&args, \"--tags\");\n+\n+\t\tfor_each_string_list_item(item, &patterns)\n+\t\t\tstrvec_pushf(&args, \"--refs=refs/tags/%s\", item->string);\n+\t\tfor_each_string_list_item(item, &exclude_patterns)\n+\t\t\tstrvec_pushf(&args, \"--exclude=refs/tags/%s\", item->string);\n+\n+\t\tif (all) {\n \t\t\tfor_each_string_list_item(item, &patterns)\n-\t\t\t\tstrvec_pushf(&args, \"--refs=refs/tags/%s\", item->string);\n+\t\t\t\tstrvec_pushf(&args, \"--refs=refs/heads/%s\", item->string);\n \t\t\tfor_each_string_list_item(item, &exclude_patterns)\n-\t\t\t\tstrvec_pushf(&args, \"--exclude=refs/tags/%s\", item->string);\n+\t\t\t\tstrvec_pushf(&args, \"--exclude=refs/heads/%s\", item->string);\n+\t\t\tfor_each_string_list_item(item, &patterns)\n+\t\t\t\tstrvec_pushf(&args, \"--refs=refs/remotes/%s\", item->string);\n+\t\t\tfor_each_string_list_item(item, &exclude_patterns)\n+\t\t\t\tstrvec_pushf(&args, \"--exclude=refs/remotes/%s\", item->string);\n \t\t}\n+\n \t\tif (argc)\n \t\t\tstrvec_pushv(&args, argv);\n \t\telse\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 8ee3d2c37d02..f46e628d6a1a 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -359,6 +359,35 @@ test_expect_success 'describe --contains and --no-match' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'describe --contains --all --match' '\n+\techo \"tags/A^0\" >expect &&\n+\ttagged_commit=$(git rev-parse \"refs/tags/A^0\") &&\n+\ttest_must_fail git describe --contains --all --match=\"B\" $tagged_commit >actual &&\n+\tgit describe --contains --all --match=\"A\" $tagged_commit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'describe --contains --all --match branch' '\n+\techo \"branch_A\" >expect &&\n+\ttagged_commit=$(git rev-parse \"refs/tags/A^0\") &&\n+\tgit describe --contains --all --match=\"branch*\" $tagged_commit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'describe --contains --all --match and --exclude' '\n+\techo \"branch_C~1\" >expect &&\n+\ttagged_commit=$(git rev-parse \"refs/tags/A^0\") &&\n+\tgit describe --contains --all --match=\"branch*\" --exclude=\"branch_A\" $tagged_commit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'describe --contains --all --exclude' '\n+\techo \"branch_A\" >expect &&\n+\ttagged_commit=$(git rev-parse \"refs/tags/A^0\") &&\n+\tgit describe --contains --all --exclude=\"A\" --exclude=\"c\" --exclude=\"test*\" $tagged_commit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'setup and absorb a submodule' '\n \ttest_create_repo sub1 &&\n \ttest_commit -C sub1 initial &&\n-- \n2.54.0.633.g0ded84c31b89\n\n"},{"id":"544312","messageId":"xmqqo6hwcves.fsf@gitster.g","threadId":"65710","inReplyTo":"20260528232950.187002-2-jacob.e.keller@intel.com","subject":"Re: [PATCH] describe: fix --exclude, --match with --contains and --all","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-30T23:47:07Z","receivedAt":"2026-05-30T23:47:10Z","isPatch":true,"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> From: Jacob Keller <jacob.keller@gmail.com>\n>\n> git describe --contains acts as a wrapper around git name-rev. When\n> operating with --contains and --all, the --match and --exclude patterns\n> are not properly forwarded to name-rev as --exclude and --refs options.\n>\n> This results in the command silently discarding match and exclude\n> requests from the user when operating in --all mode.\n>\n> We could check and die() if the user provides --contains, --all, and\n> --match/--exclude. However, its also straight forward to just pass the\n> filters down to git name-rev.\n>\n> Notice that the documentation for --match and --exclude mention the\n> --all mode. It explains that they operate on refs with the prefix\n> refs/tags, and additionally refs/heads and refs/remotes when using\n> --all.\n>\n> Fix the describe logic to pass the patterns down with the appropriate\n> prefixes when --all is provided. This fixes the support to match the\n> documented behavior.\n>\n> Add tests to check that this works as expected.\n>\n> Reported-by: Tuomas Ahola <taahol@utu.fi>\n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> ---\n>\n> I was looking into reviving the patch that just added a simple die() and\n> realized that its actually pretty straight forward to just fix the support\n> instead. I'm open to either route, if we think this support isn't\n> necessary... I'm not sure if there are any gotchas or other issues with how\n> I implemented this.\n\nIt is curious that this fails in some but not all CI jobs, and even\nmore curious that these failures look the same.\n\ne.g., https://github.com/git/git/actions/runs/26671595367/job/78615760984#step:4:1984\n\n  +++ diff -u expect actual\n  --- expect\t2026-05-30 02:21:23\n  +++ actual\t2026-05-30 02:21:23\n  @@ -1 +1 @@\n  -branch_A\n  +remotes/origin/remote_branch_A\n  error: last command exited with $?=1\n  not ok 70 - describe --contains --all --exclude\n  #\t\n  #\t\techo \"branch_A\" >expect &&\n  #\t\ttagged_commit=$(git rev-parse \"refs/tags/A^0\") &&\n  #\t\tgit describe --contains --all --exclude=\"A\" --exclude=\"c\" --exclude=\"test*\" $tagged_commit >actual &&\n  #\t\ttest_cmp expect actual\n\nRings any bell?\n"},{"id":"544334","messageId":"20260531234644.97LRl%taahol@utu.fi","threadId":"65710","inReplyTo":"xmqqo6hwcves.fsf@gitster.g","subject":"Re: [PATCH] describe: fix --exclude, --match with --contains and --all","fromName":"Tuomas Ahola","fromEmail":"taahol@utu.fi","sentAt":"2026-05-31T23:46:44Z","receivedAt":"2026-06-01T00:02:09Z","isPatch":true,"body":"Junio C Hamano <gitster@pobox.com> wrote:\n\n> It is curious that this fails in some but not all CI jobs, and even\n> more curious that these failures look the same.\n> \n> e.g., https://github.com/git/git/actions/runs/26671595367/job/78615760984#step:4:1984\n> \n>   +++ diff -u expect actual\n>   --- expect\t2026-05-30 02:21:23\n>   +++ actual\t2026-05-30 02:21:23\n>   @@ -1 +1 @@\n>   -branch_A\n>   +remotes/origin/remote_branch_A\n>   error: last command exited with $?=1\n>   not ok 70 - describe --contains --all --exclude\n>   #\t\n>   #\t\techo \"branch_A\" >expect &&\n>   #\t\ttagged_commit=$(git rev-parse \"refs/tags/A^0\") &&\n>   #\t\tgit describe --contains --all --exclude=\"A\" --exclude=\"c\" --exclude=\"test*\" $tagged_commit >actual &&\n>   #\t\ttest_cmp expect actual\n> \n> Rings any bell?\n\nThat's way out of my wheelhouse but this seems to fix the failure\nfor Alpine at least:\n\n-----8<-----\n\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex d6594ada53..1776ffab46 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -416,7 +416,7 @@ static void name_tips(struct mem_pool *string_pool)\n \t * Try to set better names first, so that worse ones spread\n \t * less.\n \t */\n-\tQSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age);\n+\tSTABLE_QSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age);\n \tfor (i = 0; i < tip_table.nr; i++) {\n \t\tstruct tip_table_entry *e = &tip_table.table[i];\n \t\tif (e->commit) {\n"},{"id":"544336","messageId":"xmqq33z7ay9e.fsf@gitster.g","threadId":"65710","inReplyTo":"20260531234644.97LRl%taahol@utu.fi","subject":"Re: [PATCH] describe: fix --exclude, --match with --contains and --all","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-01T00:40:45Z","receivedAt":"2026-06-01T00:40:48Z","isPatch":true,"body":"Tuomas Ahola <taahol@utu.fi> writes:\n\n> Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> It is curious that this fails in some but not all CI jobs, and even\n>> more curious that these failures look the same.\n>> \n>> e.g., https://github.com/git/git/actions/runs/26671595367/job/78615760984#step:4:1984\n>> \n>>   +++ diff -u expect actual\n>>   --- expect\t2026-05-30 02:21:23\n>>   +++ actual\t2026-05-30 02:21:23\n>>   @@ -1 +1 @@\n>>   -branch_A\n>>   +remotes/origin/remote_branch_A\n>>   error: last command exited with $?=1\n>>   not ok 70 - describe --contains --all --exclude\n>>   #\t\n>>   #\t\techo \"branch_A\" >expect &&\n>>   #\t\ttagged_commit=$(git rev-parse \"refs/tags/A^0\") &&\n>>   #\t\tgit describe --contains --all --exclude=\"A\" --exclude=\"c\" --exclude=\"test*\" $tagged_commit >actual &&\n>>   #\t\ttest_cmp expect actual\n>> \n>> Rings any bell?\n>\n> That's way out of my wheelhouse but this seems to fix the failure\n> for Alpine at least:\n>\n> -----8<-----\n>\n> diff --git a/builtin/name-rev.c b/builtin/name-rev.c\n> index d6594ada53..1776ffab46 100644\n> --- a/builtin/name-rev.c\n> +++ b/builtin/name-rev.c\n> @@ -416,7 +416,7 @@ static void name_tips(struct mem_pool *string_pool)\n>  \t * Try to set better names first, so that worse ones spread\n>  \t * less.\n>  \t */\n> -\tQSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age);\n> +\tSTABLE_QSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age);\n>  \tfor (i = 0; i < tip_table.nr; i++) {\n>  \t\tstruct tip_table_entry *e = &tip_table.table[i];\n>  \t\tif (e->commit) {\n\nAh, OK, when the test has multiple candidates with the same score,\nof course emitting any one of them as the answer is a valid and\ncorrectly working program.\n\nSo switching to stable-qsort here may \"fix\" the test breakage, but\nit makes the real-world use cases worse, doesn't it?  When any one\nof the solutions with the same \"goodness\" is acceptable, the change\nmakes the code behave as if the elements in the table before they\nare sorted have an \"if same score, earlier the better\" kind of\nrelationship between them.\n\nI would have preferred to see a tweak on the test side to avoid\nhaving more than one answer of the same goodness, or perhaps list\nall the possible acceptable answers and instead of using test_cmp to\ncheck for the exact answer, take any of the acceptable ones, or\nsomething like that.\n\nThanks.\n\n"},{"id":"544439","messageId":"3ad3a7ad-14de-4972-acbd-433ad4ced7f8@intel.com","threadId":"65710","inReplyTo":"xmqq33z7ay9e.fsf@gitster.g","subject":"Re: [PATCH] describe: fix --exclude, --match with --contains and --all","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2026-06-01T22:35:08Z","receivedAt":"2026-06-01T22:35:15Z","isPatch":true,"body":"On 5/31/2026 5:40 PM, Junio C Hamano wrote:\n> Tuomas Ahola <taahol@utu.fi> writes:\n> \n>> Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>>> It is curious that this fails in some but not all CI jobs, and even\n>>> more curious that these failures look the same.\n>>>\n>>> e.g., https://github.com/git/git/actions/runs/26671595367/job/78615760984#step:4:1984\n>>>\n>>>   +++ diff -u expect actual\n>>>   --- expect\t2026-05-30 02:21:23\n>>>   +++ actual\t2026-05-30 02:21:23\n>>>   @@ -1 +1 @@\n>>>   -branch_A\n>>>   +remotes/origin/remote_branch_A\n>>>   error: last command exited with $?=1\n>>>   not ok 70 - describe --contains --all --exclude\n>>>   #\t\n>>>   #\t\techo \"branch_A\" >expect &&\n>>>   #\t\ttagged_commit=$(git rev-parse \"refs/tags/A^0\") &&\n>>>   #\t\tgit describe --contains --all --exclude=\"A\" --exclude=\"c\" --exclude=\"test*\" $tagged_commit >actual &&\n>>>   #\t\ttest_cmp expect actual\n>>>\n>>> Rings any bell?\n>>\n>> That's way out of my wheelhouse but this seems to fix the failure\n>> for Alpine at least:\n>>\n>> -----8<-----\n>>\n>> diff --git a/builtin/name-rev.c b/builtin/name-rev.c\n>> index d6594ada53..1776ffab46 100644\n>> --- a/builtin/name-rev.c\n>> +++ b/builtin/name-rev.c\n>> @@ -416,7 +416,7 @@ static void name_tips(struct mem_pool *string_pool)\n>>  \t * Try to set better names first, so that worse ones spread\n>>  \t * less.\n>>  \t */\n>> -\tQSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age);\n>> +\tSTABLE_QSORT(tip_table.table, tip_table.nr, cmp_by_tag_and_age);\n>>  \tfor (i = 0; i < tip_table.nr; i++) {\n>>  \t\tstruct tip_table_entry *e = &tip_table.table[i];\n>>  \t\tif (e->commit) {\n> \n> Ah, OK, when the test has multiple candidates with the same score,\n> of course emitting any one of them as the answer is a valid and\n> correctly working program.\n> \n> So switching to stable-qsort here may \"fix\" the test breakage, but\n> it makes the real-world use cases worse, doesn't it?  When any one\n> of the solutions with the same \"goodness\" is acceptable, the change\n> makes the code behave as if the elements in the table before they\n> are sorted have an \"if same score, earlier the better\" kind of\n> relationship between them.\n> \n> I would have preferred to see a tweak on the test side to avoid\n> having more than one answer of the same goodness, or perhaps list\n> all the possible acceptable answers and instead of using test_cmp to\n> check for the exact answer, take any of the acceptable ones, or\n> something like that.\n> \n> Thanks.\n> \n\nYa something like that is probably better. I'll look at cooking up a v2\nwhich improves the test here. I think part of the issue is that the\nprevious tests setup a bunch of tags and branches, so figuring out what\nall the possible outputs are is tricky. Probably I can just add\nadditional excludes until there is only one answer.\n"},{"id":"544451","messageId":"xmqqldcxztrg.fsf@gitster.g","threadId":"65710","inReplyTo":"3ad3a7ad-14de-4972-acbd-433ad4ced7f8@intel.com","subject":"Re: [PATCH] describe: fix --exclude, --match with --contains and --all","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-02T00:10:59Z","receivedAt":"2026-06-02T00:11:02Z","isPatch":true,"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> Ya something like that is probably better. I'll look at cooking up a v2\n> which improves the test here. I think part of the issue is that the\n> previous tests setup a bunch of tags and branches, so figuring out what\n> all the possible outputs are is tricky. Probably I can just add\n> additional excludes until there is only one answer.\n\nThat sounds workable.  Thanks.\n"}]}