{"thread":{"id":"46763","subject":"[PATCH] describe: fix matching to actually match all patterns","startedAt":"2017-09-16T06:02:23Z","lastAt":"2017-09-16T07:49:07Z","messageCount":2,"participants":["Max Kirillov","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"328183","messageId":"20170916055344.31866-1-max@max630.net","threadId":"46763","inReplyTo":null,"subject":"[PATCH] describe: fix matching to actually match all patterns","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2017-09-16T05:53:44Z","receivedAt":"2017-09-16T06:02:23Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"`git describe --match` with multiple patterns matches only first pattern.\nIf it fails, next patterns are not tried.\n\nFix it, add test cases and update existing test which has wrong\nexpectation.\n\nSigned-off-by: Max Kirillov \n---\n builtin/describe.c | 9 ++++++---\n t/t6120-describe.sh | 6 +++++-\n 2 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 89ea1cdd60..94ff2fba0b 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -155,18 +155,21 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi\n \t * pattern.\n \t */\n \tif (patterns.nr) {\n+\t\tint found = 0;\n \t\tstruct string_list_item *item;\n \n \t\tif (!is_tag)\n \t\t\treturn 0;\n \n \t\tfor_each_string_list_item(item, &patterns) {\n-\t\t\tif (!wildmatch(item->string, path + 10, 0))\n+\t\t\tif (!wildmatch(item->string, path + 10, 0)) {\n+\t\t\t\tfound = 1;\n \t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n \n-\t\t\t/* If we get here, no pattern matched. */\n+\t\tif (!found)\n \t\t\treturn 0;\n-\t\t}\n \t}\n \n \t/* Is it annotated? */\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex aa74eb8f0d..25110ea55d 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -182,10 +182,14 @@ check_describe \"test2-lightweight-*\" --tags --match=\"test2-*\"\n \n check_describe \"test2-lightweight-*\" --long --tags --match=\"test2-*\" HEAD^\n \n-check_describe \"test1-lightweight-*\" --long --tags --match=\"test1-*\" --match=\"test2-*\" HEAD^\n+check_describe \"test2-lightweight-*\" --long --tags --match=\"test1-*\" --match=\"test2-*\" HEAD^\n \n check_describe \"test2-lightweight-*\" --long --tags --match=\"test1-*\" --no-match --match=\"test2-*\" HEAD^\n \n+check_describe \"test1-lightweight-*\" --long --tags --match=\"test1-*\" --match=\"test3-*\" HEAD\n+\n+check_describe \"test1-lightweight-*\" --long --tags --match=\"test3-*\" --match=\"test1-*\" HEAD\n+\n test_expect_success 'name-rev with exact tags' '\n \techo A >expect &&\n \ttag_object=$(git rev-parse refs/tags/A) &&\n-- \n2.11.0.1122.gc3fec58.dirty\n\n"},{"id":"328186","messageId":"CA+P7+xqQQAd4X9PjjbRy7RTZVO6BqomQ2uiRXZ6pCSb-z+RC6Q@mail.gmail.com","threadId":"46763","inReplyTo":"20170916055344.31866-1-max@max630.net","subject":"Re: [PATCH] describe: fix matching to actually match all patterns","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-09-16T07:48:24Z","receivedAt":"2017-09-16T07:49:07Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Sep 15, 2017 at 10:53 PM, Max Kirillov wrote:\n> `git describe --match` with multiple patterns matches only first pattern.\n> If it fails, next patterns are not tried.\n>\n> Fix it, add test cases and update existing test which has wrong\n> expectation.\n>\n> Signed-off-by: Max Kirillov \n> ---\n> builtin/describe.c | 9 ++++++---\n> t/t6120-describe.sh | 6 +++++-\n> 2 files changed, 11 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index 89ea1cdd60..94ff2fba0b 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -155,18 +155,21 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi\n> * pattern.\n> */\n> if (patterns.nr) {\n> + int found = 0;\n> struct string_list_item *item;\n>\n> if (!is_tag)\n> return 0;\n>\n> for_each_string_list_item(item, &patterns) {\n> - if (!wildmatch(item->string, path + 10, 0))\n> + if (!wildmatch(item->string, path + 10, 0)) {\n> + found = 1;\n> break;\n\nI see what was wrong. The \"if we got here\" check is inside the loop,\nso after the first wildmatch we never loop again. The fix is to add an\nadditional variable to store when we found something and exit the\nloop, and ensure that we actually did the whole loop without finding a\nmatch.\n\nThanks for the fix and proper tests!\n\nRegards,\nJake\n\n\n> + }\n> + }\n>\n> - /* If we get here, no pattern matched. */\n> + if (!found)\n> return 0;\n> - }\n> }\n>\n> /* Is it annotated? */\n> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> index aa74eb8f0d..25110ea55d 100755\n> --- a/t/t6120-describe.sh\n> +++ b/t/t6120-describe.sh\n> @@ -182,10 +182,14 @@ check_describe \"test2-lightweight-*\" --tags --match=\"test2-*\"\n>\n> check_describe \"test2-lightweight-*\" --long --tags --match=\"test2-*\" HEAD^\n>\n> -check_describe \"test1-lightweight-*\" --long --tags --match=\"test1-*\" --match=\"test2-*\" HEAD^\n> +check_describe \"test2-lightweight-*\" --long --tags --match=\"test1-*\" --match=\"test2-*\" HEAD^\n>\n> check_describe \"test2-lightweight-*\" --long --tags --match=\"test1-*\" --no-match --match=\"test2-*\" HEAD^\n>\n> +check_describe \"test1-lightweight-*\" --long --tags --match=\"test1-*\" --match=\"test3-*\" HEAD\n> +\n> +check_describe \"test1-lightweight-*\" --long --tags --match=\"test3-*\" --match=\"test1-*\" HEAD\n> +\n> test_expect_success 'name-rev with exact tags' '\n> echo A >expect &&\n> tag_object=$(git rev-parse refs/tags/A) &&\n> --\n> 2.11.0.1122.gc3fec58.dirty\n>\n"}]}