mbox series

[0/7] config: add --literal-value option

Message ID pull.796.git.1605801143.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series config: add --literal-value option | expand

Message

Phillip Wood via GitGitGadget Nov. 19, 2020, 3:52 p.m. UTC
As reported [1], 'git maintenance unregister' fails when a repository is
located in a directory with regex glob characters.

[1] 
https://lore.kernel.org/git/2c2db228-069a-947d-8446-89f4d3f6181a@gmail.com/T/#mb96fa4187a0d6aeda097cd95804a8aafc0273022

The discussed solution was to modify 'git config' to specify that the
'value_regex' argument should be treated as an exact string match. This is
the primary change in this series, with an additional patch at the end to
make 'git maintenance [un]register' use this option, when necessary.

Thanks, -Stolee

Derrick Stolee (7):
  t1300: test "set all" mode with value_regex
  t1300: add test for --replace-all with value_regex
  config: convert multi_replace to flags
  config: add --literal-value option, un-implemented
  config: plumb --literal-value into config API
  config: implement --literal-value with --get*
  maintenance: use 'git config --literal-value'

 Documentation/git-config.txt |  20 ++++--
 builtin/branch.c             |   4 +-
 builtin/config.c             |  50 ++++++++++---
 builtin/gc.c                 |   5 +-
 builtin/remote.c             |   8 ++-
 config.c                     |  29 ++++----
 config.h                     |  24 +++++--
 t/t1300-config.sh            | 136 +++++++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh       |  12 ++++
 9 files changed, 248 insertions(+), 40 deletions(-)


base-commit: 0016b618182f642771dc589cf0090289f9fe1b4f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-796%2Fderrickstolee%2Fmaintenance%2Fconfig-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-796/derrickstolee/maintenance/config-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/796

Comments

Ævar Arnfjörð Bjarmason Nov. 20, 2020, 1:19 p.m. UTC | #1
On Thu, Nov 19 2020, Derrick Stolee via GitGitGadget wrote:

> As reported [1], 'git maintenance unregister' fails when a repository is
> located in a directory with regex glob characters.

Just as bikeshedding on the name: Did you consider something
thematically similar to the corresponding git-grep option,
i.e. --fixed-string[s]. I see -F is also free in git-config(1).
Derrick Stolee Nov. 20, 2020, 1:23 p.m. UTC | #2
On 11/20/2020 8:19 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Nov 19 2020, Derrick Stolee via GitGitGadget wrote:
> 
>> As reported [1], 'git maintenance unregister' fails when a repository is
>> located in a directory with regex glob characters.
> 
> Just as bikeshedding on the name: Did you consider something
> thematically similar to the corresponding git-grep option,
> i.e. --fixed-string[s]. I see -F is also free in git-config(1).

I definitely wanted to be specific about "value" in the name,
since some options include regexes on the key as well. I'm open
to new ideas, and combining your idea with mine would introduce
"--fixed-value". Thoughts?

Thanks,
-Stolee
Junio C Hamano Nov. 20, 2020, 6:30 p.m. UTC | #3
Derrick Stolee <stolee@gmail.com> writes:

> On 11/20/2020 8:19 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Thu, Nov 19 2020, Derrick Stolee via GitGitGadget wrote:
>> 
>>> As reported [1], 'git maintenance unregister' fails when a repository is
>>> located in a directory with regex glob characters.
>> 
>> Just as bikeshedding on the name: Did you consider something
>> thematically similar to the corresponding git-grep option,
>> i.e. --fixed-string[s]. I see -F is also free in git-config(1).
>
> I definitely wanted to be specific about "value" in the name,
> since some options include regexes on the key as well. I'm open
> to new ideas, and combining your idea with mine would introduce
> "--fixed-value". Thoughts?

I very much appreciate "value" is in the name, with the current
semantics that this only controls how the pattern matching is done
on the value side and not on the key side.  When making an obvious
addition of a separate option to control how the pattern matching is
done on keys in the future, we would regret if we called this option
"--fixed-strings" today.  And no, I do not think it is an acceptable
option to introduce "--fixed-strings" that only affects value side
and then later change its behaviour to affect also on the key side.

	Side note.  It _is_ possible to ship such a "--fixed-strings"
	option that does not work on the key side and document it as
	a known bug, later to be fixed.  I am not sure if I like it.

But stepping back a bit, is the extra flexibility that allows us to
control the matching on keys and values separately with such a
scheme really worth the complexity (at the end-user facing interface
level, not the implementation level)?

So an alternative may be to use a single option, whose name would
probably be one of "--(literal|fixed)-(match|strings)", but extend
the implementation in this series to make the single option affect
both the value and key matching the same way.

That would however be more work in the shorter term.  Offhand, I am
not sure if I like it (i.e. spending time and effort that is more
than the absolute minimum necessary to fix a breakage.  And the end
result of doing so is less powerful/flexible, even though it may be
easier to explain to users simply because the feature is less
powerful than it could be).  It would be easier, if I can convince
myself that the extra flexibility is not worth it, to just declare
that simpler is better here, but I am not quite ready to do so yet.

As to "-F", I do not think it is a good idea; in some context "-F"
means work on a <file> given via that option, i.e. "-F <file>".

Thanks.
Derrick Stolee Nov. 20, 2020, 6:51 p.m. UTC | #4
On 11/20/2020 1:30 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>> On 11/20/2020 8:19 AM, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> On Thu, Nov 19 2020, Derrick Stolee via GitGitGadget wrote:
>>>
>>>> As reported [1], 'git maintenance unregister' fails when a repository is
>>>> located in a directory with regex glob characters.
>>>
>>> Just as bikeshedding on the name: Did you consider something
>>> thematically similar to the corresponding git-grep option,
>>> i.e. --fixed-string[s]. I see -F is also free in git-config(1).
>>
>> I definitely wanted to be specific about "value" in the name,
>> since some options include regexes on the key as well. I'm open
>> to new ideas, and combining your idea with mine would introduce
>> "--fixed-value". Thoughts?
> 
> I very much appreciate "value" is in the name, with the current
> semantics that this only controls how the pattern matching is done
> on the value side and not on the key side.  When making an obvious
> addition of a separate option to control how the pattern matching is
> done on keys in the future, we would regret if we called this option
> "--fixed-strings" today.  And no, I do not think it is an acceptable
> option to introduce "--fixed-strings" that only affects value side
> and then later change its behaviour to affect also on the key side.
> 
> 	Side note.  It _is_ possible to ship such a "--fixed-strings"
> 	option that does not work on the key side and document it as
> 	a known bug, later to be fixed.  I am not sure if I like it.
> 
> But stepping back a bit, is the extra flexibility that allows us to
> control the matching on keys and values separately with such a
> scheme really worth the complexity (at the end-user facing interface
> level, not the implementation level)?
>
> So an alternative may be to use a single option, whose name would
> probably be one of "--(literal|fixed)-(match|strings)", but extend
> the implementation in this series to make the single option affect
> both the value and key matching the same way.
> 
> That would however be more work in the shorter term.  Offhand, I am
> not sure if I like it (i.e. spending time and effort that is more
> than the absolute minimum necessary to fix a breakage.  And the end
> result of doing so is less powerful/flexible, even though it may be
> easier to explain to users simply because the feature is less
> powerful than it could be).  It would be easier, if I can convince
> myself that the extra flexibility is not worth it, to just declare
> that simpler is better here, but I am not quite ready to do so yet.
I had not thought about making this option be related to the key
names at all. In particular, we already have the --get-regexp option:

--get-regexp::
	Like --get-all, but interprets the name as a regular expression and
	writes out the key names.  Regular expression matching is currently
	case-sensitive and done against a canonicalized version of the key
	in which section and variable names are lowercased, but subsection
	names are not.

I suppose that there could be reason to create a similar --unset-regexp
that is the same equivalent of --unset-all. Perhaps the --replace-all
mode could benefit, too? These are also more dangerous to use with regex
matches since they also _change_ the config, not just query it.

Since there is already asymmetry between the key and value (only one
mode, --get-regexp, has a "name_regex"), I would prefer to treat the
value_regex in isolation here.

Thanks,
-Stolee
Junio C Hamano Nov. 20, 2020, 9:52 p.m. UTC | #5
Derrick Stolee <stolee@gmail.com> writes:

> Since there is already asymmetry between the key and value (only one
> mode, --get-regexp, has a "name_regex"), I would prefer to treat the
> value_regex in isolation here.

Ah, I forgot about --get-regexp vs --get; so the ship has already
sailed---I agree with you that --(fixed|literal)-value is all we
need.

Thanks.
Ævar Arnfjörð Bjarmason Nov. 24, 2020, 12:35 p.m. UTC | #6
On Fri, Nov 20 2020, Derrick Stolee wrote:

> On 11/20/2020 8:19 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Thu, Nov 19 2020, Derrick Stolee via GitGitGadget wrote:
>> 
>>> As reported [1], 'git maintenance unregister' fails when a repository is
>>> located in a directory with regex glob characters.
>> 
>> Just as bikeshedding on the name: Did you consider something
>> thematically similar to the corresponding git-grep option,
>> i.e. --fixed-string[s]. I see -F is also free in git-config(1).
>
> I definitely wanted to be specific about "value" in the name,
> since some options include regexes on the key as well. I'm open
> to new ideas, and combining your idea with mine would introduce
> "--fixed-value". Thoughts?

Hi. I see you've already sent a v2 with that. Looks good to me. FWIW I
didn't feel strongly one way or the other about it, either would be a
fine choice by someone who's eyeballing this more than my cursory
glance.

I just wanted to point out the grep option in case you'd missed it,
since we've got a bazillion CLI options all over the place, and
sometimes there's an existing option that's thematically similar to a
proposed new one that's easy to miss. So we can re-use (or partially
re-use) the name, behavior, or not.