Re: ds/maintenance-part-3 (was Re: What's cooking in git.git (Nov 2020, #02; Mon, 9))

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 17, 2020, 19:34 UTC
Message-ID: <xmqqlfez6alb.fsf@gitster.c.googlers.com>
In-Reply-To: <29212864-ab96-5757-cbfb-f5621a43f8d8@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 12 quoted lines

>>> Good find.  And it is even worse that value_regex uses ERE, not BRE,
>>> which means even an otherwise innocuous letter like '+' cannot be
>>> used without quoting.
>> 
>> I should have mentioned in the first letter than Jonathan Nieder was the
>> one who made the jump from "this is breaking in the buildbot but not
>> locally" to regular expression metachars. Credit where it's due.
>
> Thank you for finding and reporting this bug.
>
> Can I at least have a short moment of griping about anyone putting
> regex characters into their directory names? ;)

Sure, but the blame mostly lies in the one who thought using ERE was a good idea ('+' is very often used).

Show 16 quoted lines

>>> 0. Quote the value_regex properly, instead of blindly using a value
>>>    that comes from the environment.
>
> Pulling the subcommand from my test enfironment using GIT_TRACE2_PERF=1
> I see the following quotes being used:
>
> git config --global --unset maintenance.repo "/repos/new+repo*test"
>
> I'm guessing that what we really want is to _escape_ the regex glob
> characters? This command works:
>
> git config --global --unset maintenance.repo "/repos/new\+repo\*test"
>
> The only place I see where we do that currently is in
> builtin/sparse-checkout.c:escaped_pattern(). Please let me know if
> you know of a more suitable way to escape regex characters.

If we wanted to go that route, yes, we need to prevent random input taken from the end user or the environment to be regexes, when they are literal strings. But I think we should just bite the bullet and say "git config --unset --literal-value-pattern vari.able va+l+ue", etc.

This is not a suggestion for the option name, but a suggestion to do this with a new option and not with a special value-pattern syntax.

	Side note.  It is tempting to declare that something like

	    git config --unset vari.able "!!$end_user_value"

	is the syntax to use literal/fixed pattern, and that way we
	do not have to touch the callchain from builtin/config.c
	leading down to git_config_set_multivar_in_file_gently().
	It is backward incompatible change that is unlikely hurt
	real people.  If a script is feeding "$end_user_value"
	without cleansing as the value_regex already, it is already
	broken (e.g. if $end_user_value happens to being with '!',
	this will unset everything that does not match the regexp)
	anyway.  And users already know to say '[!]some-pattern'
	when they mean the pattern begins with a literal '!' and not
	"does not match some-pattern", so reserving '!!' prefix does
	not sound too bad.

Show 25 quoted lines

>>>> 1. Teach 'git config' to learn either which regex parser to use
>>>> (including fixed), or at least to learn "value isn't a regex", or
>>>>
>>>> 2. Don't spin a child process in 'git maintenance [un]register' and
>>>> instead just call the config API.
>>> ...
>>> My short-to-mid-term preference is to do #1 to allow a value to be
>>> spelled literally (i.e. remove entry with _this_ value, and add this
>>> one instead), and optionally do #2 as an optimization that is not
>>> essential.  I do not offhand know how you can make #2 alone fly
>>> without doing some form of #1, as I think the same value_regex that
>>> ought to be ERE to specify entries to be replaced needs to be used
>>> under the cover even if you use "config API" anyway.
>> 
>> Ah, right you are - I had figured the regex parsing was done earlier,
>> but it indeed looks to happen in
>> config.c:git_config_set_multivar_in_file_gently. Thanks.
>
> So the "real fix" is to allow a command-line option to 'git config'
> that makes the "value_regex" parameter a literal string? Of course,
> this would either require wiring an option down into
> git_config_set_multivar_in_file_gently() to treat the string as a
> literal _or_ to escape the input string in builtin/config.c.
>
> Am I understanding the intended plan here?

Yup, if people cannot poke holes with the wishful thinking that the breaking of backward compatibility by using the "!!" prefix would not cause practical issues, then I am also fine with that, but inventing a flags word with a VALUE_PATTERN_FIXED bit in it and updating the callchain to pass it down from the command line option parser would be much less risky, I would think.