git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: Emily Shaffer <emilyshaffer@google.com>, git@vger.kernel.org
Subject: Re: ds/maintenance-part-3 (was Re: What's cooking in git.git (Nov 2020, #02; Mon, 9))
Date: Tue, 17 Nov 2020 11:34:56 -0800
Message-ID: <xmqqlfez6alb.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <29212864-ab96-5757-cbfb-f5621a43f8d8@gmail.com> (Derrick Stolee's message of "Tue, 17 Nov 2020 08:56:16 -0500")

Derrick Stolee <stolee@gmail.com> writes:

>>> 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).

>>> 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.

>>>> 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.

  parent reply	other threads:[~2020-11-17 19:36 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-09 23:42 What's cooking in git.git (Nov 2020, #02; Mon, 9) Junio C Hamano
2020-11-10  0:44 ` Elijah Newren
2020-11-10  1:03   ` Jeff King
2020-11-10 13:31 ` ds/maintenance-part-3 (was Re: What's cooking in git.git (Nov 2020, #02; Mon, 9)) Derrick Stolee
2020-11-16 23:56   ` Emily Shaffer
2020-11-17  0:40     ` Junio C Hamano
2020-11-17  1:07       ` Emily Shaffer
2020-11-17 13:56         ` Derrick Stolee
2020-11-17 19:18           ` Emily Shaffer
2020-11-17 19:34           ` Junio C Hamano [this message]
2020-11-17 21:12             ` Derrick Stolee
2020-11-19  2:16               ` Junio C Hamano
2020-11-19 13:25                 ` Derrick Stolee
2020-11-19 15:53                   ` Derrick Stolee
2020-11-19 18:06                   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqlfez6alb.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=stolee@gmail.com \
    --subject='Re: ds/maintenance-part-3 (was Re: What'\''s cooking in git.git (Nov 2020, #02; Mon, 9))' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git