Re: [PATCH 4/4] pathspec: record labels
On Thu, May 12, 2016 at 10:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
> 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).
Reminder for myself: add a test for
":(label=a) :(exclude,label=b)"
>
>> +void load_labels(const char *name, int namelen, struct string_list *list)
>
> This must be file scope static, no?
Yes. It seems like I forget these statics often. :(
Show 24 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?Would it make sense to mark a file as
"follows the labeling system, but has no label" (TRUE)
"doesn't follow the labeling system at all" (FALSE)This may be a useful addition later on to say
"I want to talk only about labeled files, but not label 'a'"
I would think.
Show 5 quoted lines
>
> 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 25 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.This is how I understood your initial design idea.
:(label=C_code)contrib/
gives all the retired C programs.
> 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 16 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 12 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?I can do that. Though I did not want to clutter this patch with it.
I wonder that you focus on the details already, but not on the grand design of things. "Is it actually a sane thing I am proposing here?" Though you may be biased as the the high level idea came from you. :)
One of the things I switched last minute and tried to address in the cover letter is the semantics of ORing or ANDing the labels given within one pathspec item.
Thanks, Stefan
Stefan Beller
