From: Junio C Hamano <email@example.com> To: Derrick Stolee <firstname.lastname@example.org> Cc: Emily Shaffer <email@example.com>, firstname.lastname@example.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 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <firstname.lastname@example.org> (Derrick Stolee's message of "Tue, 17 Nov 2020 08:56:16 -0500") Derrick Stolee <email@example.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.
next prev 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 \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --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
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).