Re: [PATCH] describe: teach --match to handle branches and remotes

[Jacob Keller <jacob.keller@gmail.com>]

On Mon, Sep 18, 2017 at 4:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Max Kirillov <max@max630.net> writes:
>
>>  --match <pattern>::
>>       Only consider tags matching the given `glob(7)` pattern,
>> -     excluding the "refs/tags/" prefix.  This can be used to avoid
>> -     leaking private tags from the repository. If given multiple times, a
>> -     list of patterns will be accumulated, and tags matching any of the
>> -     patterns will be considered. Use `--no-match` to clear and reset the
>> -     list of patterns.
>> +     excluding the "refs/tags/" prefix. If used with `--all`, it also
>> +     considers local branches and remote-tracking references matching the
>> +     pattern, excluding respectively "refs/heads/" and "refs/remotes/"
>> +     prefix; references of other types are never considered. If given
>> +     multiple times, a list of patterns will be accumulated, and tags
>> +     matching any of the patterns will be considered.  Use `--no-match` to
>> +     clear and reset the list of patterns.
>>
>>  --exclude <pattern>::
>>       Do not consider tags matching the given `glob(7)` pattern, excluding
>> -     the "refs/tags/" prefix. This can be used to narrow the tag space and
>> -     find only tags matching some meaningful criteria. If given multiple
>> -     times, a list of patterns will be accumulated and tags matching any
>> -     of the patterns will be excluded. When combined with --match a tag will
>> -     be considered when it matches at least one --match pattern and does not
>> +     the "refs/tags/" prefix. If used with `--all`, it also does not consider
>> +     local branches and remote-tracking references matching the pattern,
>> +     excluding respectively "refs/heads/" and "refs/remotes/" prefix;
>> +     references of other types are never considered. If given multiple times,
>> +     a list of patterns will be accumulated and tags matching any of the
>> +     patterns will be excluded. When combined with --match a tag will be
>> +     considered when it matches at least one --match pattern and does not
>>       match any of the --exclude patterns. Use `--no-exclude` to clear and
>>       reset the list of patterns.
>
> OK, I find this written clearly enough.
>
>> diff --git a/builtin/describe.c b/builtin/describe.c
>> index 94ff2fba0b..2a2e998063 100644
>> --- a/builtin/describe.c
>> +++ b/builtin/describe.c
>> @@ -124,6 +124,22 @@ static void add_to_known_names(const char *path,
>>       }
>>  }
>>
>> +/* Drops prefix. Returns NULL if the path is not expected with current settings. */
>> +static const char *get_path_to_match(int is_tag, int all, const char *path)
>> +{
>> +     if (is_tag)
>> +             return path + 10;
>
> This is a faithful conversion of the existing code that wants to
> behave the same as original, but a bit more on this later.
>
>> +     else if (all) {
>> +             if (starts_with(path, "refs/heads/"))
>> +                     return path + 11; /* "refs/heads/..." */
>> +             else if (starts_with(path, "refs/remotes/"))
>> +                     return path + 13; /* "refs/remotes/..." */
>> +             else
>> +                     return 0;
>
> I think you can use skip_prefix() to avoid counting the length of
> the prefix yourself, i.e.
>
>         else if all {
>                 const char *body;
>
>                 if (skip_prefix(path, "refs/heads/", &body))
>                         return body;
>                 else if (skip_prefix(path, "refs/remotes/", &body))
>                         ...
>         }
>
> Whether you do the above or not, the last one that returns 0 should
> return NULL (to the language it is the same thing, but to humans, we
> write NULL when it is the null pointer, not the number 0).
>
>> +     } else
>> +             return NULL;
>> +}
>
> Perhaps the whole thing may want to be a bit more simplified, like:
>
>         static const *skip_ref_prefix(const char *path, int all)
>         {
>                 const char *prefix[] = {
>                         "refs/tags/", "refs/heads/", "refs/remotes/"
>                 };
>                 const char *body;
>                 int cnt;
>                 int bound = all ? ARRAY_SIZE(prefix) : 1;
>

I found the implicit use of "bound = 1" means "we only care about
tags" to be a bit weird here. I guess it's not really that big a deal
overall, and this is definitely cleaner than the original
implementation.

>                 for (cnt = 0; cnt < bound; cnt++)
>                         if (skip_prefix(path, prefix[cnt], &body);
>                                 return body;
>                 return NULL;
>         }
>
> The hardcoded +10 for "is_tag" case assumes that anything other than
> "refs/tags/something" would ever be used to call into this function
> when is_tag is true, and that may well be true in the current code
> and have been so ever since the original code was written, but it
> still smells like an invitation for future bugs.
>
> I dunno.
>
>> +
>>  static int get_name(const char *path, const struct object_id *oid, int flag, void *cb_data)
>>  {
>>       int is_tag = starts_with(path, "refs/tags/");
>> @@ -140,12 +156,13 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi
>>        */
>>       if (exclude_patterns.nr) {
>>               struct string_list_item *item;
>> +             const char *path_to_match = get_path_to_match(is_tag, all, path);
>
>> +test_expect_success 'set-up branches' '
>> +     git branch branch_A A &&
>> +     git branch branch_c c &&
>
> Was there a reason why A and c are in different cases?  Are we
> worried about case insensitive filesystems or something?
>
>> +     git update-ref refs/remotes/origin/remote_branch_A "A^{commit}" &&
>> +     git update-ref refs/remotes/origin/remote_branch_c "c^{commit}" &&
>> +     git update-ref refs/original/original_branch_A test-annotated~2
>> +'
>
> Thanks.