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, 00:40 UTC
Message-ID: <xmqqh7po7r3w.fsf@gitster.c.googlers.com>
In-Reply-To: <20201116235642.GA15562@google.com>

Emily Shaffer <emilyshaffer@google.com> writes:

Show 5 quoted lines

> Because 'git maintenance unregister' spins a child process to call 'git
> config --unset maintenance.repo <cwd>', it actually fails if "cwd"
> contains a POSIX regular expression special character:
>
>   git config [<file-option>] --unset name [value_regex]

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.

Show 10 quoted lines

> You can demo it for yourself like so:
>
>   git init repro+for+maintenance
>   git maintenance register
>   git maintenance unregister
>   echo $?	# returns '5'
>   git config --list --global
>
> I see two paths forward:
>

0. Quote the value_regex properly, instead of blindly using a value
   that comes from the environment.

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

> I'd suggest #2. The config API is very nice, and seems to have a simple
> way to add or remove configs to your global file in just a couple of
> lines. If there's a reason why it's not simpler to do it that way, it's
> my fault for missing the review :)

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.

Thanks.