Re: [PATCH] describe: fix matching to actually match all patterns

From: Jacob Keller <jacob.keller@gmail.com>
Date: Sep 16, 2017, 07:48 UTC
Message-ID: <CA+P7+xqQQAd4X9PjjbRy7RTZVO6BqomQ2uiRXZ6pCSb-z+RC6Q@mail.gmail.com>
In-Reply-To: <20170916055344.31866-1-max@max630.net>

On Fri, Sep 15, 2017 at 10:53 PM, Max Kirillov <max@max630.net> wrote:

Show 31 quoted lines

> `git describe --match` with multiple patterns matches only first pattern.
> If it fails, next patterns are not tried.
>
> Fix it, add test cases and update existing test which has wrong
> expectation.
>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
>  builtin/describe.c  | 9 ++++++---
>  t/t6120-describe.sh | 6 +++++-
>  2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/describe.c b/builtin/describe.c
> index 89ea1cdd60..94ff2fba0b 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -155,18 +155,21 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi
>          * pattern.
>          */
>         if (patterns.nr) {
> +               int found = 0;
>                 struct string_list_item *item;
>
>                 if (!is_tag)
>                         return 0;
>
>                 for_each_string_list_item(item, &patterns) {
> -                       if (!wildmatch(item->string, path + 10, 0))
> +                       if (!wildmatch(item->string, path + 10, 0)) {
> +                               found = 1;
>                                 break;

I see what was wrong. The "if we got here" check is inside the loop, so after the first wildmatch we never loop again. The fix is to add an additional variable to store when we found something and exit the loop, and ensure that we actually did the whole loop without finding a match.

Thanks for the fix and proper tests!

Regards, Jake

Show 33 quoted lines

> +                       }
> +               }
>
> -                       /* If we get here, no pattern matched. */
> +               if (!found)
>                         return 0;
> -               }
>         }
>
>         /* Is it annotated? */
> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> index aa74eb8f0d..25110ea55d 100755
> --- a/t/t6120-describe.sh
> +++ b/t/t6120-describe.sh
> @@ -182,10 +182,14 @@ check_describe "test2-lightweight-*" --tags --match="test2-*"
>
>  check_describe "test2-lightweight-*" --long --tags --match="test2-*" HEAD^
>
> -check_describe "test1-lightweight-*" --long --tags --match="test1-*" --match="test2-*" HEAD^
> +check_describe "test2-lightweight-*" --long --tags --match="test1-*" --match="test2-*" HEAD^
>
>  check_describe "test2-lightweight-*" --long --tags --match="test1-*" --no-match --match="test2-*" HEAD^
>
> +check_describe "test1-lightweight-*" --long --tags --match="test1-*" --match="test3-*" HEAD
> +
> +check_describe "test1-lightweight-*" --long --tags --match="test3-*" --match="test1-*" HEAD
> +
>  test_expect_success 'name-rev with exact tags' '
>         echo A >expect &&
>         tag_object=$(git rev-parse refs/tags/A) &&
> --
> 2.11.0.1122.gc3fec58.dirty
>