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

On Mon, May 16, 2016 at 9:23 PM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

+ * 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)

quoted

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

quoted

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

quoted

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