Re: [PATCHv2] pathspec: allow escaped query values

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jun 2, 2016 at 2:54 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> writes:
>
>> In our own .gitattributes file we have attributes such as:
>>
>>     *.[ch] whitespace=indent,trail,space
>>
>> When querying for attributes we want to be able to ask for the exact
>> value, i.e.
>>
>>     git ls-files :(attr:whitespace=indent,trail,space)
>>
>> should work, but the commas are used in the attr magic to introduce
>> the next attr, ...
>> ...
>> So here is the "escaping only, but escaping done right" version.
>> (It goes on top of sb/pathspec-label)
>
> The phrase "should work" is probably a bit too strong (I'd have said
> "it would be nice if this worked"), as we do not have to even
> support comma for our immediately expected use cases.  Allowing it
> merely makes a casual test using our own .gitattributes easier.
>
>> +static size_t strcspn_escaped(const char *s, const char *reject)
>
> Perhaps s/reject/stop/?

I took the signature from the strcspn man page, and my version of the
man page has `reject` there, `stop` certainly works too

>
>> +{
>> +     const char *i, *j;
>> +
>> +     for (i = s; *i; i++) {
>> +             /* skip escaped the character */
>> +             if (i[0] == '\\' && i[1]) {
>> +                     i++;
>> +                     continue;
>> +             }
>> +             /* see if any of the chars matches the current character */
>> +             for (j = reject; *j; j++)
>> +                     if (!*i || *i == *j)
>> +                             return i - s;
>
> I somehow doubt that *i can be NUL here.  In any case, this looks
> more like
>
>         /* is *i is one of the stop codon? */
>         if (strchr(stop, *i))
>                 break;

right, that seems easier.


>
>> +     }
>> +     return i - s;
>> +}
>
>> +static char *attr_value_unescape(const char *value)
>> +{
>> +     char *i, *ret = xstrdup(value);
>> +
>> +     for (i = ret; *i; i++) {
>> +             if (i[0] == '\\') {
>> +                     if (!i[1])
>> +                             die(_("Escape character '\\' not allowed as "
>> +                                   "last character in attr value"));
>> +
>> +                     /* remove the backslash */
>> +                     memmove(i, i + 1, strlen(i));
>> +                     /* and ignore the character after that */
>> +                     i++;
>> +             }
>> +     }
>> +     return ret;
>> +}
>> +
>
> Repeated memmove() and strlen() somehow bothers me.  Would there be
> a more efficient and straight-forward way to do this, perhaps along
> the lines of this instead?
>
>         const char *src;
>         char *dst, *ret;
>
>         ret = xmalloc(strlen(value));
>         for (src = value, dst = ret; *src; src++, dst++) {
>                 if (*src == '\\') {
>                         if (!src[1])
>                                 die();
>                         src++;
>                 }
>                 if (*src && invalid_value_char(*src))
>                         die("cannot use '%c' for value matching", *src)
>                 *dst = *src;
>         }
>         *dst = '\0'
>         return ret;
>
> Even though I earlier said "Now we have an unquote mechanism, we can
> open the floodgate by lifting the "no backslash in value" check, I
> now realize that we do want to keep some escape hatch for us to
> extend the quoting syntax even more later, so perhaps with something
> like this:
>
>         static inline int invalid_value_char(const char ch)
>         {
>                 if (isalnum(ch) || strchr(",-_", ch))
>                         return 0;
>                 return -1;
>         }

Makes sense.

Later on we could have : or ; for an and/or of the values and
parens and such, so the invalid_value_char makes sense.

>
> Thanks.

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]