Re: [PATCHv3] pathspec: allow escaped query values

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> However if we add a value restriction here, we need to be as strict in the
> .gitattributes parsing as well and put a warning there (similar to
> invalid_attr_name_message) I would think.

Remember, the attribute system is used for many purposes other than
this new "further limit pathspec".  While the (hopefully temporary)
limitation hurts here, users will limit the attributes they use for
":(attr:VAR=VAL)" pathspec magic, and it is unlikely that either the
VAR part or its VALue are something the core Git currently uses
anyway.

So I do not think it is necessary or even beneficial to add such a
warning.

> +static char *attr_value_unescape(const char *value)
> +{
> +	const char *src;
> +	char *dst, *ret;
> +
> +	ret = xmallocz(strlen(value));
> +	for (src = value, dst = ret; *src; src++, dst++) {
> +		if (*src == '\\') {
> +			if (!src[1])
> +				die(_("Escape character '\\' not allowed as "
> +				      "last character in attr value"));
> +			src++;
> +		}
> +		if (*src && invalid_value_char(*src))
> +			die("cannot use '%c' for value matching", *src);
> +		*dst = *src;
> +	}
> +	*dst = '\0';
> +	return ret;
> +}

Please sanity-check me.  Just like I said to your original "I doubt
*i could be NUL here", I now doubt *src could be NUL there where
invalid_value_char() gets called.

If *src could be NUL there, then *dst gets NUL once, and then after
loop exits (presumably after incrementing dst), *dst gets another
NUL, which was the terminating NUL condition being iffy I mentioned,
but as you said, I do not think it would happen, so we can lose the
"*src && " before invalid_value_char() is called.

Thanks.