Re: [PATCH 5/5] pathspec: record labels

From: Junio C Hamano <gitster@pobox.com>
Date: May 16, 2016, 17:38 UTC
Message-ID: <xmqqlh393nuv.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kZsVH3mE0zXu9wSWgit3QZ_CiKZqg-TQdyWnZw6D9=Cgw@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 9 quoted lines

>> Let's avoid "are meant to", which is merely to give you an excuse to
>> say "it was meant to ... but the implementation did not quite achieve
>> that goal".
>>
>>     The "label" attribute can be attached to paths, and the pathspec
>>     mechanism is extended via the new ":(label=X)pattern/to/match"
>
> I wasn't sure about whether we want to have '=' or ':' as the separator
> of "label" and its values. ...

Oh, sorry, the above wasn't a suggestion to change : to = at all. I was merely being sloppy in the details that is irrelevant to my main point (which is, it is better spend the bits you spent for "meant to" instead for saying clearly what it does).

> ... But as that is only internal, we can be inconsistent to a new
> public feature, so '=' seems better for the labeling system.

I can buy that, too. Good that my sloppy wording helped you think about it further ;-).

> But still we do not enforce it early enough. Some crazy upstream may
> add some labels which do not follow the requirements and then
> tell downstream to run a command like `git foo "(label=!@#$)".
> and then downstream curses at upstream as well as Git.

That is why it is "warn and ignore", not "die".

Show 16 quoted lines

>>> +     if (ATTR_TRUE(check.value))
>>> +             ret = 1; /* has all the labels */
>>
>> So this is "pretend that I have all the labels under the sun".
>>
>>> +     else if (ATTR_FALSE(check.value))
>>> +             ret = 0; /* has no labels */
>>
>> I do not see a value in this.  What difference does it have compared
>> to having a "label=" that explicitly says "I do not have one"?  What
>> is your answer to an end-user question "When should I say 'label='
>> and when should I say '!label'?"
>>
>> Just forbid it for now; we may find a better use for it later.
>
> I don't think we want to die in that code as it is in the data already.

Is it? I think this is coming from the command line pathspec magic, not from .gitattributes file recorded in the tree.

> We need to allow the UNSET case, as otherwise you'd need to label
> any path if using labels?

You do need UNSET (roughly, "no label is mentioned for the path").

If I want to say "Everything under Documentation/ and also things with label=doc", I'd say

	git cmd "Documentation/" ":(label=doc)"

and no path in Documentation/ need any label, i.e. for them, the labels are unset. They will not be caught with ":(label=doc)" because they are not set, but that is OK.

FALSE is different from UNSET.  It needs an explicit mention, i.e.

	*.doc	doc
        false.doc	-doc

What's the difference between saying "-doc" and not saying anything? If you really want to explicitly say "doc attribute is unset for this path (perhaps because you may have other patterns that set the doc elsewhere), you would say "!doc", and you already have code for that.

>         if (ATTR_TRUE(check.value))
>                 ret = 1; /* has all the labels */
>         else if (ATTR_FALSE(check.value))
>                 die(_("Label attributes must not be unset"));

The message is wrong. You are dying because the user explicitly set it to false, not because the user made it unset.

>         else if (ATTR_UNSET(check.value))
>                 ret = 0; /* has no labels */

Show 10 quoted lines

>>> +             struct string_list_item *si;
>>> +             struct string_list attr_labels = STRING_LIST_INIT_DUP;
>>> +             string_list_split(&attr_labels, check.value, ',', -1);
>>> +             string_list_sort(&attr_labels);
>>
>> Filter out a non-compliant label values here, so that they are
>> ignored from day one.  That way you would not have to deal with "I
>> know I got the warning, but it used to work and you broke it" later.
>
> So you're saying we should not die(...) but just ignore those labels?

Do not die() but warn-and-ignore when you see funnies in .gitattributes; do die() if you see funnies in pathspec magic.

Show 10 quoted lines

>> I am NOT suggesting to make this enhancement in the prototype to
>> allow us experiment with submodule selection use case, but this is
>> an obvious place to allow
>>
>>         :(label=A B):(label=C D)
>>
>> to mean ((A & B) | (C & D)) by making item->labels an array of set
>> of labels.
>>
>> And no, I do not think arbitrary boolean expression is too complex

s/do not/do/

Show 6 quoted lines

>> to understand to the end-users, especially if we have to stay within
>> the pathspec magic syntax.  And my gut feeling is that it is not
>> worth it to support anything more complex than "OR of these ANDed
>> ones".
>
> That makes sense.

Thanks.