Re: Segfault in the attr stack

From: Stefan Beller <sbeller@google.com>
Date: Jun 1, 2016, 22:29 UTC
Message-ID: <CAGZ79kZ6dh7AP5b5N_2RRHQmq=OKpr05JEwyOCbfOj6KWjFC2A@mail.gmail.com>
In-Reply-To: <xmqqpos0wodp.fsf@gitster.mtv.corp.google.com>

On Wed, Jun 1, 2016 at 3:11 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

>
> By the way, I just noticed that the <specification> part of the
> ':(attr:<specification>)' syntax would need to be rethought.  In the
> .gitattributes file everybody has, we see these lines:
>
>         *.[ch] whitespace=indent,trail,space
>         *.sh whitespace=indent,trail,space
>
> but because comma is a special separator in the pathspec magic
> system, we cannot do
>
>         $ git status ':(attr:whitespace=indent,trail,space)'
>

Right. In [1] I wrote:

Show 17 quoted lines

>>
>>> +     if (!item->attr_check)
>>> +             item->attr_check = git_attr_check_alloc();
>>
>> Given more than one "attr" magic, e.g. ":(attr:A=a,attr:B)/path",
>> the check may not be empty when we process the second one; we just
>> extend it without losing the existing contents.
>
> 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.
> This "wastes" the white space as a possible convenient separator
> character, e.g. for multiple values. On the other hand it will be easier
> to type, specially for many attrs (read submodule groups).

So at that time I thought I had communicated the issue enough and we'd be fine ignoring it for now. I propose to not escape commas, but use white spaces instead, i.e.

    git status ':(attr:whitespace=indent trail space,attr:label=with
more values)' ':attr(attr:foo:bar)'

would match
* all files that have the whitespace AND the label setting (matching
exactly the values)
* OR foo=bar attribute

This syntax would require to repeat ",attr:" for multiple ANDed attributes, but would save us from escaped commas, which may be a pain both in parsing as well as doing the input on a shell?

[1] http://thread.gmane.org/gmane.comp.version-control.git/294989/focus=295016

Show 21 quoted lines

> I think we should introduce a quoting mechanism to hide these commas
> from the pathspec magic splitter, e.g.
>
> where attr_value_unquote() would copy string while unquoting some
> special characters (i.e. at least ' ' and ',' because they are used
> as syntactic elements in the higher level; there might be others).
>
> diff --git a/pathspec.c b/pathspec.c
> index 0a02255..fb22f28 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -132,7 +132,7 @@ static void parse_pathspec_attr_match(struct pathspec_item *item, const char *va
>                                 am->match_mode = MATCH_SET;
>                         else {
>                                 am->match_mode = MATCH_VALUE;
> -                               am->value = xstrdup(&attr[attr_len + 1]);
> +                               am->value = attr_value_unquote(&attr[attr_len + 1]);
>                                 if (strchr(am->value, '\\'))
>                                         die(_("attr spec values must not contain backslashes"));
>                         }
>

If we go by whitespaces, we can implement attr_value_unquote as a `tr " " ","` conceptually, which seems easy.

Thanks, Stefan