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

From: Stefan Beller <sbeller@google.com>
Date: May 16, 2016, 19:03 UTC
Message-ID: <CAGZ79kaV+hQRn6ZraZcG9ZRp1XwPGrEFibBEX5qFJdWDCg0uxQ@mail.gmail.com>
In-Reply-To: <xmqq8tz93kf9.fsf@gitster.mtv.corp.google.com>

On Mon, May 16, 2016 at 11:52 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> So "warn and ignore" for data from .gitattributes and die for
>> commandline arguments? That makes sense.
>
> Yes.
>
> On the "command line" front, because we may want to give different
> meanings to these two entries in the future:
>
>         :(label=-doc)Documentation/
>         :(label=!doc)Documentation/

Yes but both of these already errors out with:

    Label -doc': Label names must start with an
    alphabetic character and use only alphanumeric
    characters, dashes and underscores.
    fatal: Labels in the wrong syntax are prohibited.

The command line arguments are not parsed via the attr system. Only data from .gitattributes are parsed via the attr system.

I can add tests for those to fail though.

Show 19 quoted lines

>
> we should diagnose -doc (FALSE) as an error, not treating it as the
> same as !doc (UNSET).  And we should warn and ignore -doc (FALSE) in
> .gitattributes.  Yes, ignoring it would be more or less equivalent
> to treating it as UNSET, but because we may use -doc (FALSE) for a
> better purpose later, we should still warn.
>
>> Ok, so here is the warn-and-ignore code:
>>
>>
>>         if (ATTR_TRUE(check.value))
>>                 ret = 1; /* has all the labels */
>>         else if (ATTR_FALSE(check.value)) {
>>                 warning(_("Path '%s': Label must not be false. Treat
>> as if no label was set"), path);
>>                 ret = 0;
>
> s/Treat as if .../The -label may be used differently in future
> versions of Git, so do not use it/;

"... but for now Git treats it as if it is not set at all" is a valuable information to the user, still?

That code path is only used for data coming from .gitattributes, so we can bike shed the best warning message.

Show 5 quoted lines

>
> But if we are going in the direction of :(attr:crlf=auto), all this
> discussion is moot, isn't it?  I haven't formed a firm opinion on
> this, but it sure does sound tempting, doesn't it?
>

That direction sounds scary as people may use any sort of labels, so we can no longer add labels later on sanely.

    :(attr:random-attr=foo)

should also fall into the "warn and ignore" space. We only want to allow

    :(attr:known-attr)
    :(attr:label=..)

instead of label= we may want to allow a little bit more, such as abbreviated 'l=' or 'group=', but not any sort of labeling.