From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 13 May 2016 05:26:10 GMT
Subject: Re: [PATCH 4/4] pathspec: record labels
Message-ID: <xmqqy47ea5ql.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160513001936.7623-5-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> +
> +label:<white space separated list>;;
> +	Labels can be assigned to pathspecs in the .gitattributes file.
> +	By specifying a list of labels the pattern will match only
> +	files which have all of the listed labels.
>  +

Attributes are given to paths, not pathspecs.  You specify which paths
the entry applies to by matching the pattern (which is at the beginning
of the line) against each path, and take the matching entries.

In any case, what the first sentence tries to explain applies to
each and every attribute .gitattributes file can define, and
explaining it should be better left for the first sentence in the
DESCRIPTION section.

As to the second sentence, I would say "When specifying the paths to
work on to various Git commands, the :(label=<label>,...)  magic
pathspec can be used to select paths that have all the labels given
by the 'label' attribute.", or something like that, to clarify where
that "specifying" and "match" happens (they do not happen here, but
happen when using magic pathspec).

> +void load_labels(const char *name, int namelen, struct string_list *list)

This must be file scope static, no?

> +{
> +	static struct git_attr *attr;
> +	struct git_attr_check check;
> +	char *path = xmemdupz(name, namelen);
> +
> +	if (!attr)
> +		attr = git_attr("label");
> +	check.attr = attr;
> +
> +	if (git_check_attr(path, 1, &check))
> +		die("git_check_attr died");
> +
> +	if (ATTR_CUSTOM(check.value)) {
> +		string_list_split(list, check.value, ',', -1);
> +		string_list_sort(list);
> +	}
> +
> +	free(path);
> +}

I am wondering where we should sanity check (and die, pointing out
an error in .gitattributes file).  Is this function a good place to
reject TRUE and FALSE?

By the way, ATTR_CUSTOM() is probably a bad name.  gitattributes.txt
calls them Set, Unset, Set to a value and Unspecified, and this is
trying to single out "Set to a value" case.  ATTR_STRING() may be
more appropriate.

> @@ -263,6 +285,15 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
>  	    strncmp(item->match, name - prefix, item->prefix))
>  		return 0;
>  
> +	if (item->group) {
> +		struct string_list has_labels = STRING_LIST_INIT_DUP;
> +		struct string_list_item *si;
> +		load_labels(name, namelen, &has_labels);
> +		for_each_string_list_item(si, item->group)
> +			if (!string_list_has_string(&has_labels, si->string))
> +				return 0;
> +	}
> +

Is this the right place, before we even check if the prefix already
covered everything?

We are looking at one element in the pathspec here.  If that element
is known to be a "group", and the path has all the required labels,
is it correct to fall through?

    Ahh, you are making ":(label=...)makefile" to say "paths must
    match the string 'makefile' in some way, and further the paths
    must have all these labels?  Then falling through is correct.
    The placement before the "prefix covered" looks still a bit
    strange, but understandable.

Is this code leaking has_labels every time it is called?

By the way, we should pick one between label and group and stick to
it, no?  Perhaps call the new field "item->label"?

>  	/* If the match was just the prefix, we matched */
>  	if (!*match)
>  		return MATCHED_RECURSIVELY;
> diff --git a/pathspec.c b/pathspec.c
> index 4dff252..c227c25 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -94,6 +94,7 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,
>  {
>  	int i;
>  	const char *copyfrom = *copyfrom_;
> +	const char *out;

It probably is meant as "output", but it looks more like the "body" or
the "contents" of the named magic to me.

>  	/* longhand */
>  	const char *nextat;
>  	for (copyfrom = elt + 2;
> @@ -117,6 +118,20 @@ static void eat_long_magic(struct pathspec_item *item, const char *elt,
>  			continue;
>  		}
>  
> +		if (skip_prefix(copyfrom, "label:", &out)) {

This is good, but can you fix the "prefix:" one we see a few lines
above, too, using this to lose "copyfrom + 7" there?