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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, May 16, 2016 at 9:23 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> 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)

>
>> +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?

>
>> +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.

>
> 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.
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]