On Mon, May 16, 2016 at 9:23 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Stefan Beller <sbel...@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)
>
>> +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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
