Re: [PATCHv7 5/5] pathspec: allow querying for attributes

From: Junio C Hamano <gitster@pobox.com>
Date: May 18, 2016, 22:59 UTC
Message-ID: <xmqq4m9vq8fr.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79ka0Of6S3AGx24jrvso=AwMsxQOnWVFZA-XWy3590JbVgA@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

> and those are not yet codified, but for discussion then:
>
>  - "`?`" the attribute must not be unspecified, i.e. set, unset or has any value
>  - "`+`" the attribute must be set or has any value

I'd suggest not to support these until we see any concrete and convincing example use case to tell us why this is useful.

Show 6 quoted lines

> That is why I am not super happy with it though.
>
>     ":(attr:A=a,attr:B)/path",
>     ":(attr:A=a B)/path",
>
> are the same for the user as well as in the internal data structures.

You can interpret the former as ORed :(attr:A=a) and :(attr:B) if you really wanted to. You left the door open for such a future extension by explicitly rejecting multiple attributes added to a single pathspec element in earlier round, which was good. You could do the same here if you do not want to code that ORed ANDs, so that it (or some other semantics) can be introduced later without breaking the mental model the users would form with the initial implementation.

Show 6 quoted lines

>>> +             val_len = strcspn(val, "=,)");
>>
>> I understand "=", but can "," and ")" appear here?
>
> This was overly caution from some intermediate state, where the caller
> handed in more than required.

Is that being overly cautious?

It looks to me more like being sloppy and sweeping bugs in callers that you could have diagnosed here.

If you didn't have ",)" in the cspn set above, at least you would see the effect caused by a broken caller. E.g. ":(attr:FOO,icase)" would be given to you as "FOO,icase)" by a broken caller, you would split "FOO,icase" as one of the SP separated items, and try to parse it as an attribute name. You can notice the breakage of the caller at that time. With ",)" in cspn set, you silently pass the parameter given by a broken caller.