Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes

From: Stefan Beller <sbeller@google.com>
Date: May 17, 2016, 16:45 UTC
Message-ID: <CAGZ79kZSZQwnatExNFKCiWz8zgJfnS+8VWLkZcMaiHS1ESqBaQ@mail.gmail.com>
In-Reply-To: <xmqqa8jpz51m.fsf@gitster.mtv.corp.google.com>

On Mon, May 16, 2016 at 9:23 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> + * attr:+val to find value set to true
>> + * attr:-val to find a value set to false
>> + * attr:!val to find a value that is not set
>> + *     (i.e. it is neither set as "val", "val=<empty>", nor unset as "-val")
>> + * attr:val=value: to find value that have at least a and b set.
>
> I would have expected that there won't be "attr:+val", but it is
> spelled as "attr:val" instead.

"val" matches if the attr is not unspecified, i.e. one of {true, false, value} "+val" matches {true} only.

Maybe we want to redo that to

"val" matches {true} only. "?val" matches {true, false, value}. (I can leave this case out in the first series, too)

Show 6 quoted lines

>
>> +static void parse_attr_item(struct attr_item *attr, const char *value)
>
> Please do not call something that is not part of the attribute
> infrastructure as "attr_item"; I wasted time looking for the
> structure definition for "attr_item" in <attr.h>.

So "parse_pathspec_attr_match" instead?

Show 20 quoted lines

>
>> +static int match_attrs(const char *name, int namelen,
>> +                    const struct pathspec_item *item)
>> +{
>> +     char *path;
>> +     int i;
>> +
>> +     if (!check) {
>> +             check = git_attr_check_alloc();
>> +             for (i = 0; i < item->attr_nr; i++)
>> +                     git_attr_check_append(check, item->attrs[i].attr);
>> +     }
>> +
>> +     path = xmemdupz(name, namelen);
>> +     git_all_attrs(path, check);
>
> PLEASE DON'T.  git_all_attrs() asks for all the attribute under the
> sun and has no hope to perform sensibly, especially at the very leaf
> level of the pathspec logic where one call to this function is made
> for each and every path in the tree.

This is executed only once, as check is static? From a users perception it doesn't matter if it is executed once just after parsing all pathspec items or at the first path to check for a match, no?

The mistake is using the API wrong. So inside the '!check', after the preparation loop of git_attr_check_append, we'd need to hand over the "check" to git_check_attr instead?

>
> Instead, have a pointer to "struct git_attr_check" in pathspec_item
> and make a call to git_check_attr(path, item->check) here.

I see, then we have multiple `check` structs. Makes sense.

Show 9 quoted lines

>
> Which means that you would need to prepare git_attr_check around ...
>
>> +             if (skip_prefix(copyfrom, "attr:", &body)) {
>> +                     ALLOC_GROW(item->attrs, item->attr_nr + 1, item->attr_alloc);
>> +                     parse_attr_item(&item->attrs[item->attr_nr++], body);
>
> ... HERE.
>