Re: [PATCH 4/4] pathspec: record labels
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 13, 2016, 05:26 UTC
- Message-ID
- <xmqqy47ea5ql.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20160513001936.7623-5-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 6 quoted lines
> + > +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?
Show 19 quoted lines
> +{
> + 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.
Show 13 quoted lines
> @@ -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"?
Show 12 quoted lines
> /* 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.
Show 8 quoted lines
> /* 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?