[PATCH] clone: handle empty config values in -c

[PATCH] clone: handle empty config values in -c

[Jonathan Nieder]

"git clone --config" uses the following incantation to add an item to
a config file, instead of replacing an existing value:

	git_config_set_multivar_gently(key, value, "^$", 0)

As long as no existing value matches the regex ^$, that works as
intended and adds to the config.  When a value is empty, though, it
replaces the existing value.

Noticed while trying to set credential.helper during a clone to use a
specific helper without inheriting from ~/.gitconfig and
/etc/gitconfig.  That is, I ran

	git clone -c credential.helper= \
		-c credential.helper=myhelper \
		https://example.com/repo

intending to produce the configuration

	[credential]
		helper =
		helper = myhelper

Without this patch, the 'helper =' line is not included and the
credential helper from /etc/gitconfig gets used.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thoughts?

Thanks,
Jonathan

 builtin/clone.c         | 4 +++-
 t/t5611-clone-config.sh | 8 ++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index de85b85254..a6ae7d6180 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -773,7 +773,9 @@ static int checkout(int submodule_progress)
 
 static int write_one_config(const char *key, const char *value, void *data)
 {
-	return git_config_set_multivar_gently(key, value ? value : "true", "^$", 0);
+	return git_config_set_multivar_gently(key,
+					      value ? value : "true",
+					      CONFIG_REGEX_NONE, 0);
 }
 
 static void write_config(struct string_list *config)
diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
index e4850b778c..39329eb7a8 100755
--- a/t/t5611-clone-config.sh
+++ b/t/t5611-clone-config.sh
@@ -19,6 +19,14 @@ test_expect_success 'clone -c can set multi-keys' '
 	test_cmp expect actual
 '
 
+test_expect_success 'clone -c can set multi-keys, including some empty' '
+	rm -rf child &&
+	git clone -c credential.helper= -c credential.helper=hi . child &&
+	printf "%s\n" "" hi >expect &&
+	git --git-dir=child/.git config --get-all credential.helper >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'clone -c without a value is boolean true' '
 	rm -rf child &&
 	git clone -c core.foo . child &&
-- 
2.13.0.rc1.294.g07d810a77f

Re: [PATCH] clone: handle empty config values in -c

[Brandon Williams]

On 05/01, Jonathan Nieder wrote:
> "git clone --config" uses the following incantation to add an item to
> a config file, instead of replacing an existing value:
> 
> 	git_config_set_multivar_gently(key, value, "^$", 0)
> 
> As long as no existing value matches the regex ^$, that works as
> intended and adds to the config.  When a value is empty, though, it
> replaces the existing value.
> 
> Noticed while trying to set credential.helper during a clone to use a
> specific helper without inheriting from ~/.gitconfig and
> /etc/gitconfig.  That is, I ran
> 
> 	git clone -c credential.helper= \
> 		-c credential.helper=myhelper \
> 		https://example.com/repo
> 
> intending to produce the configuration
> 
> 	[credential]
> 		helper =
> 		helper = myhelper
> 
> Without this patch, the 'helper =' line is not included and the
> credential helper from /etc/gitconfig gets used.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Thoughts?

After reading this I'm still a little fuzzy on why the empty helper line
is needed to avoid using the credential helper from /etc/gitconfig.

> 
> Thanks,
> Jonathan
> 
>  builtin/clone.c         | 4 +++-
>  t/t5611-clone-config.sh | 8 ++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index de85b85254..a6ae7d6180 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -773,7 +773,9 @@ static int checkout(int submodule_progress)
>  
>  static int write_one_config(const char *key, const char *value, void *data)
>  {
> -	return git_config_set_multivar_gently(key, value ? value : "true", "^$", 0);
> +	return git_config_set_multivar_gently(key,
> +					      value ? value : "true",
> +					      CONFIG_REGEX_NONE, 0);
>  }
>  
>  static void write_config(struct string_list *config)
> diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
> index e4850b778c..39329eb7a8 100755
> --- a/t/t5611-clone-config.sh
> +++ b/t/t5611-clone-config.sh
> @@ -19,6 +19,14 @@ test_expect_success 'clone -c can set multi-keys' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'clone -c can set multi-keys, including some empty' '
> +	rm -rf child &&
> +	git clone -c credential.helper= -c credential.helper=hi . child &&
> +	printf "%s\n" "" hi >expect &&
> +	git --git-dir=child/.git config --get-all credential.helper >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'clone -c without a value is boolean true' '
>  	rm -rf child &&
>  	git clone -c core.foo . child &&
> -- 
> 2.13.0.rc1.294.g07d810a77f
> 

-- 
Brandon Williams

[PATCH] credential doc: make multiple-helper behavior more prominent (Re: [PATCH] clone: handle empty config values in -c)

[Jonathan Nieder]

Subject: credential doc: make multiple-helper behavior more prominent

Git's configuration system works by reading multiple configuration
files in order, from general to specific:

 - first, the system configuration /etc/gitconfig
 - then the user's configuration (~/.gitconfig or ~/.config/git/config)
 - then the repository configuration (.git/config)

For single-valued configuration items, the latest value wins.  For
multi-valued configuration items, values accumulate in that order.

For example, this allows setting a credential helper globally in
~/.gitconfig that git will try to use in all repositories, regardless
of whether they additionally provide another helper.  This is usually
a nice thing --- e.g. I can install helpers to use my OS keychain and
to cache credentials for a short period of time globally.

Sometimes people want to be able to override an inherited setting.
For the credential.helper setting, this is done by setting the
configuration item to empty before giving it a new value.  This is
already documented by the documentation is hard to find ---
git-config(1) says to look at gitcredentials(7) and the config
reference in gitcredentials(7) doesn't mention this issue.

Move the documentation to the config reference to make it easier to
find.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Brandon Williams wrote:

>> Noticed while trying to set credential.helper during a clone to use a
>> specific helper without inheriting from ~/.gitconfig and
>> /etc/gitconfig.  That is, I ran
>>
>> 	git clone -c credential.helper= \
>> 		-c credential.helper=myhelper \
>> 		https://example.com/repo
>>
>> intending to produce the configuration
>>
>> 	[credential]
>> 		helper =
>> 		helper = myhelper
>>
>> Without this patch, the 'helper =' line is not included and the
>> credential helper from /etc/gitconfig gets used.
>>
>> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>> ---
>> Thoughts?
>
> After reading this I'm still a little fuzzy on why the empty helper line
> is needed to avoid using the credential helper from /etc/gitconfig.

See "git help credentials":

       If there are multiple instances of the credential.helper configuration
       variable, each helper will be tried in turn, and may provide a
       username, password, or nothing. Once Git has acquired both a username
       and a password, no more helpers will be tried.

       If credential.helper is configured to the empty string, this resets the
       helper list to empty (so you may override a helper set by a
       lower-priority config file by configuring the empty-string helper,
       followed by whatever set of helpers you would like).

That's a bit obscure, though --- I didn't find it when I looked in "git
help config".  How about this patch?

Tested using 'make -C Documentation gitcredentials.7'.

 Documentation/gitcredentials.txt | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt
index f3a75d1ce1..f970196bc1 100644
--- a/Documentation/gitcredentials.txt
+++ b/Documentation/gitcredentials.txt
@@ -101,16 +101,6 @@ $ git help credential-foo
 $ git config --global credential.helper foo
 -------------------------------------------
 
-If there are multiple instances of the `credential.helper` configuration
-variable, each helper will be tried in turn, and may provide a username,
-password, or nothing. Once Git has acquired both a username and a
-password, no more helpers will be tried.
-
-If `credential.helper` is configured to the empty string, this resets
-the helper list to empty (so you may override a helper set by a
-lower-priority config file by configuring the empty-string helper,
-followed by whatever set of helpers you would like).
-
 
 CREDENTIAL CONTEXTS
 -------------------
@@ -162,6 +152,16 @@ helper::
 	shell (so, for example, setting this to `foo --option=bar` will execute
 	`git credential-foo --option=bar` via the shell. See the manual of
 	specific helpers for examples of their use.
++
+If there are multiple instances of the `credential.helper` configuration
+variable, each helper will be tried in turn, and may provide a username,
+password, or nothing. Once Git has acquired both a username and a
+password, no more helpers will be tried.
++
+If `credential.helper` is configured to the empty string, this resets
+the helper list to empty (so you may override a helper set by a
+lower-priority config file by configuring the empty-string helper,
+followed by whatever set of helpers you would like).
 
 username::
 
-- 
2.13.0.rc1.294.g07d810a77f

Re: [PATCH] credential doc: make multiple-helper behavior more prominent (Re: [PATCH] clone: handle empty config values in -c)

[Brandon Williams]

On 05/01, Jonathan Nieder wrote:
> Subject: credential doc: make multiple-helper behavior more prominent
> 
> Git's configuration system works by reading multiple configuration
> files in order, from general to specific:
> 
>  - first, the system configuration /etc/gitconfig
>  - then the user's configuration (~/.gitconfig or ~/.config/git/config)
>  - then the repository configuration (.git/config)
> 
> For single-valued configuration items, the latest value wins.  For
> multi-valued configuration items, values accumulate in that order.
> 
> For example, this allows setting a credential helper globally in
> ~/.gitconfig that git will try to use in all repositories, regardless
> of whether they additionally provide another helper.  This is usually
> a nice thing --- e.g. I can install helpers to use my OS keychain and
> to cache credentials for a short period of time globally.
> 
> Sometimes people want to be able to override an inherited setting.
> For the credential.helper setting, this is done by setting the
> configuration item to empty before giving it a new value.  This is
> already documented by the documentation is hard to find ---
> git-config(1) says to look at gitcredentials(7) and the config
> reference in gitcredentials(7) doesn't mention this issue.
> 
> Move the documentation to the config reference to make it easier to
> find.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Brandon Williams wrote:
> 
> >> Noticed while trying to set credential.helper during a clone to use a
> >> specific helper without inheriting from ~/.gitconfig and
> >> /etc/gitconfig.  That is, I ran
> >>
> >> 	git clone -c credential.helper= \
> >> 		-c credential.helper=myhelper \
> >> 		https://example.com/repo
> >>
> >> intending to produce the configuration
> >>
> >> 	[credential]
> >> 		helper =
> >> 		helper = myhelper
> >>
> >> Without this patch, the 'helper =' line is not included and the
> >> credential helper from /etc/gitconfig gets used.
> >>
> >> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> >> ---
> >> Thoughts?
> >
> > After reading this I'm still a little fuzzy on why the empty helper line
> > is needed to avoid using the credential helper from /etc/gitconfig.
> 
> See "git help credentials":
> 
>        If there are multiple instances of the credential.helper configuration
>        variable, each helper will be tried in turn, and may provide a
>        username, password, or nothing. Once Git has acquired both a username
>        and a password, no more helpers will be tried.
> 
>        If credential.helper is configured to the empty string, this resets the
>        helper list to empty (so you may override a helper set by a
>        lower-priority config file by configuring the empty-string helper,
>        followed by whatever set of helpers you would like).
> 
> That's a bit obscure, though --- I didn't find it when I looked in "git
> help config".  How about this patch?
> 
> Tested using 'make -C Documentation gitcredentials.7'.
> 
>  Documentation/gitcredentials.txt | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt
> index f3a75d1ce1..f970196bc1 100644
> --- a/Documentation/gitcredentials.txt
> +++ b/Documentation/gitcredentials.txt
> @@ -101,16 +101,6 @@ $ git help credential-foo
>  $ git config --global credential.helper foo
>  -------------------------------------------
>  
> -If there are multiple instances of the `credential.helper` configuration
> -variable, each helper will be tried in turn, and may provide a username,
> -password, or nothing. Once Git has acquired both a username and a
> -password, no more helpers will be tried.
> -
> -If `credential.helper` is configured to the empty string, this resets
> -the helper list to empty (so you may override a helper set by a
> -lower-priority config file by configuring the empty-string helper,
> -followed by whatever set of helpers you would like).
> -
>  
>  CREDENTIAL CONTEXTS
>  -------------------
> @@ -162,6 +152,16 @@ helper::
>  	shell (so, for example, setting this to `foo --option=bar` will execute
>  	`git credential-foo --option=bar` via the shell. See the manual of
>  	specific helpers for examples of their use.
> ++
> +If there are multiple instances of the `credential.helper` configuration
> +variable, each helper will be tried in turn, and may provide a username,
> +password, or nothing. Once Git has acquired both a username and a
> +password, no more helpers will be tried.
> ++
> +If `credential.helper` is configured to the empty string, this resets
> +the helper list to empty (so you may override a helper set by a
> +lower-priority config file by configuring the empty-string helper,
> +followed by whatever set of helpers you would like).

Thanks, this clears up the confusion.

>  
>  username::
>  
> -- 
> 2.13.0.rc1.294.g07d810a77f
> 

-- 
Brandon Williams

Re: [PATCH] credential doc: make multiple-helper behavior more prominent (Re: [PATCH] clone: handle empty config values in -c)

[Jonathan Nieder]

Starting out the reviews:

Jonathan Nieder wrote:

[...]
> configuration item to empty before giving it a new value.  This is
> already documented by the documentation is hard to find ---
                     ^^
s/by/but/

Sorry for the confusion.

[...]
> +++ b/Documentation/gitcredentials.txt
[...]
> @@ -162,6 +152,16 @@ helper::
>  	shell (so, for example, setting this to `foo --option=bar` will execute
>  	`git credential-foo --option=bar` via the shell. See the manual of
>  	specific helpers for examples of their use.
> ++
> +If there are multiple instances of the `credential.helper` configuration
> +variable, each helper will be tried in turn, and may provide a username,
> +password, or nothing. Once Git has acquired both a username and a
> +password, no more helpers will be tried.
> ++
> +If `credential.helper` is configured to the empty string, this resets
> +the helper list to empty (so you may override a helper set by a
> +lower-priority config file by configuring the empty-string helper,

It's not necessarily obvious to a new user what "lower-priority" means.

Since this text is an example, maybe it should say something like "so,
for example, you can override a helper set in /etc/gitconfig by
configuring the empty-string helper followed by whatever set of
helpers you would like in ~/.gitconfig".

That's orthogonal to this patch but it should be straightforward to
roll in if it makes sense.

Thanks,
Jonathan

Re: [PATCH] credential doc: make multiple-helper behavior more prominent (Re: [PATCH] clone: handle empty config values in -c)

[Jeff King]

On Mon, May 01, 2017 at 05:30:10PM -0700, Jonathan Nieder wrote:

> > @@ -162,6 +152,16 @@ helper::
> >  	shell (so, for example, setting this to `foo --option=bar` will execute
> >  	`git credential-foo --option=bar` via the shell. See the manual of
> >  	specific helpers for examples of their use.
> > ++
> > +If there are multiple instances of the `credential.helper` configuration
> > +variable, each helper will be tried in turn, and may provide a username,
> > +password, or nothing. Once Git has acquired both a username and a
> > +password, no more helpers will be tried.
> > ++
> > +If `credential.helper` is configured to the empty string, this resets
> > +the helper list to empty (so you may override a helper set by a
> > +lower-priority config file by configuring the empty-string helper,
> 
> It's not necessarily obvious to a new user what "lower-priority" means.
> 
> Since this text is an example, maybe it should say something like "so,
> for example, you can override a helper set in /etc/gitconfig by
> configuring the empty-string helper followed by whatever set of
> helpers you would like in ~/.gitconfig".
> 
> That's orthogonal to this patch but it should be straightforward to
> roll in if it makes sense.

I'm not sure we want to get into explaining last-one-wins versus lists
versus list-resets for each option. The "FILES" section of git-config(1)
does cover this, though I could well believe it doesn't go into enough
detail or is too hard to find (my mind is sufficiently poisoned that it
all makes sense to me, but that says little about an average Git user).

The phrase "lower-priority config file" is used for push.gpgSign, too
(though there it really does seem like repeating things that are true
for all the config).

-Peff

Re: [PATCH] credential doc: make multiple-helper behavior more prominent (Re: [PATCH] clone: handle empty config values in -c)

[Jeff King]

On Mon, May 01, 2017 at 05:21:14PM -0700, Jonathan Nieder wrote:

> Subject: credential doc: make multiple-helper behavior more prominent
> 
> Git's configuration system works by reading multiple configuration
> files in order, from general to specific:
> 
>  - first, the system configuration /etc/gitconfig
>  - then the user's configuration (~/.gitconfig or ~/.config/git/config)
>  - then the repository configuration (.git/config)
> 
> For single-valued configuration items, the latest value wins.  For
> multi-valued configuration items, values accumulate in that order.
> 
> For example, this allows setting a credential helper globally in
> ~/.gitconfig that git will try to use in all repositories, regardless
> of whether they additionally provide another helper.  This is usually
> a nice thing --- e.g. I can install helpers to use my OS keychain and
> to cache credentials for a short period of time globally.
> 
> Sometimes people want to be able to override an inherited setting.
> For the credential.helper setting, this is done by setting the
> configuration item to empty before giving it a new value.  This is
> already documented by the documentation is hard to find ---
> git-config(1) says to look at gitcredentials(7) and the config
> reference in gitcredentials(7) doesn't mention this issue.
> 
> Move the documentation to the config reference to make it easier to
> find.

I know I am the last person in the world to criticize somebody for being
too verbose in a commit message, but... :)

I think this background about how multi-values are handled isn't that
useful here, because we're not changing anything to do with that. We're
just moving the documentation around. So I think a more compelling
explanation would focus on the documentation locations. Like:

  The behavior of multiple credential.helper config values is explained
  in the rather-large "AVOIDING REPETITION" section of
  gitcredentials(7), but not mentions at all in the "CONFIGURATION
  OPTIONS" section. That's OK if the user is reading the manpage from
  top to bottom, but users often don't do that. The entry for
  credential.helper in git-config(1) points them to gitcredentials(7),
  which would make it reasonable for them to skip straight to the
  "CONFIGURATION OPTIONS" section.

With or without the suggested commit message, this looks like an
improvement to me.

> > After reading this I'm still a little fuzzy on why the empty helper line
> > is needed to avoid using the credential helper from /etc/gitconfig.
> 
> See "git help credentials":
> 
>        If there are multiple instances of the credential.helper configuration
>        variable, each helper will be tried in turn, and may provide a
>        username, password, or nothing. Once Git has acquired both a username
>        and a password, no more helpers will be tried.
> 
>        If credential.helper is configured to the empty string, this resets the
>        helper list to empty (so you may override a helper set by a
>        lower-priority config file by configuring the empty-string helper,
>        followed by whatever set of helpers you would like).
> 
> That's a bit obscure, though --- I didn't find it when I looked in "git
> help config".  How about this patch?

So I think this looks fine, but I wonder if we should discuss multi-vars
some in Documentation/config.txt. It would be really nice if we could
claim "the usual behavior for multi-vars is to reset the list upon
seeing an empty entry". But probably we should make sure more of them
handle that before making such a claim. :)

-Peff

Re: [PATCH] clone: handle empty config values in -c

[Jeff King]

On Mon, May 01, 2017 at 05:05:15PM -0700, Jonathan Nieder wrote:

> "git clone --config" uses the following incantation to add an item to
> a config file, instead of replacing an existing value:
> 
> 	git_config_set_multivar_gently(key, value, "^$", 0)
> 
> As long as no existing value matches the regex ^$, that works as
> intended and adds to the config.  When a value is empty, though, it
> replaces the existing value.
> [...]
> ---
> Thoughts?

Yeah, I think this is the exact reason we introduced CONFIG_REGEX_NONE
in the first place. At the time we fixed "config --add", but "clone -c"
needs the same treatment.

Grepping around, it looks like we should probably be using this in other
places, too:

  - writing fetch refspecs in clone

  - throughout remote.c for urls and refspecs

I don't think an empty variable has meaning in those places, so probably
nobody really cares. But passing CONFIG_REGEX_NONE seems to better match
the original intent. And in the long run I think we probably ought to
make an empty "remote.foo.url" mean "reset the url list to empty", as
we've started to do with other multivars (like credential helpers).

So your patch looks fine, but I'd be pleased if you wanted to take it
further and eradicate this "^$" anti-pattern through the code base.

-Peff

PS I notice that the documentation for "config --add" explicitly
   mentions that it behaves like "^$". This isn't accurate anymore. We
   should probably update that. I wondered if callers of git-config
   would need some way to specify "--replace-all --no-value" or
   something, but that is precisely what "--add" is. So I think it's
   just a documentation problem.