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

From: Junio C Hamano <gitster@pobox.com>
Date: May 17, 2016, 04:23 UTC
Message-ID: <xmqqa8jpz51m.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160517031353.23707-5-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 5 quoted lines

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

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

Show 14 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.

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

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.