[PATCH] credential: let empty credential specs reset helper list

[PATCH] credential: let empty credential specs reset helper list

[Jeff King]

Sine the credential.helper key is a multi-valued config
list, there's no way to "unset" a helper once it's been set.
So if your system /etc/gitconfig sets one, you can never
avoid running it, but only add your own helpers on top.

Since an empty value for credential.helper is nonsensical
(it would just try to run "git-credential-"), we can assume
nobody is using it. Let's define it to reset the helper
list, letting you override lower-priority instances which
have come before.

Signed-off-by: Jeff King <peff@peff.net>
---
This doesn't solve the multi-value config problem in the general sense,
but it's simple and easy. And even if we do solve it later, it's not a
big deal to keep supporting this scheme for compatibility.

 Documentation/config.txt         |  5 +++--
 Documentation/gitcredentials.txt |  5 +++++
 credential.c                     |  9 ++++++---
 t/t0300-credentials.sh           | 11 +++++++++++
 4 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..4b85095 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1113,8 +1113,9 @@ commit.template::
 credential.helper::
 	Specify an external helper to be called when a username or
 	password credential is needed; the helper may consult external
-	storage to avoid prompting the user for the credentials. See
-	linkgit:gitcredentials[7] for details.
+	storage to avoid prompting the user for the credentials. Note
+	that multiple helpers may be defined. See linkgit:gitcredentials[7]
+	for details.
 
 credential.useHttpPath::
 	When acquiring credentials, consider the "path" component of an http
diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt
index 1c75be0..f3a75d1 100644
--- a/Documentation/gitcredentials.txt
+++ b/Documentation/gitcredentials.txt
@@ -106,6 +106,11 @@ 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
 -------------------
diff --git a/credential.c b/credential.c
index 7d6501d..aa99666 100644
--- a/credential.c
+++ b/credential.c
@@ -63,9 +63,12 @@ static int credential_config_callback(const char *var, const char *value,
 		key = dot + 1;
 	}
 
-	if (!strcmp(key, "helper"))
-		string_list_append(&c->helpers, value);
-	else if (!strcmp(key, "username")) {
+	if (!strcmp(key, "helper")) {
+		if (*value)
+			string_list_append(&c->helpers, value);
+		else
+			string_list_clear(&c->helpers, 0);
+	} else if (!strcmp(key, "username")) {
 		if (!c->username)
 			c->username = xstrdup(value);
 	}
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index d7ef44b..03bd31e 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -298,4 +298,15 @@ test_expect_success 'helpers can abort the process' '
 	test_cmp expect stdout
 '
 
+test_expect_success 'empty helper spec resets helper list' '
+	test_config credential.helper "verbatim file file" &&
+	check fill "" "verbatim cmdline cmdline" <<-\EOF
+	--
+	username=cmdline
+	password=cmdline
+	--
+	verbatim: get
+	EOF
+'
+
 test_done
-- 
2.7.2.767.g705917e

Re: [PATCH] credential: let empty credential specs reset helper list

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Sine the credential.helper key is a multi-valued config

s/Sine/Since/;

> diff --git a/credential.c b/credential.c
> index 7d6501d..aa99666 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -63,9 +63,12 @@ static int credential_config_callback(const char *var, const char *value,
>  		key = dot + 1;
>  	}
>  
> -	if (!strcmp(key, "helper"))
> -		string_list_append(&c->helpers, value);
> -	else if (!strcmp(key, "username")) {
> +	if (!strcmp(key, "helper")) {
> +		if (*value)
> +			string_list_append(&c->helpers, value);
> +		else
> +			string_list_clear(&c->helpers, 0);
> +	} else if (!strcmp(key, "username")) {

I wondered why neither the existing code nor the updated one has a
check for !value, but this callback assumes no credential
configuration variable will ever be a boolean and rejects it
upfront, so this code before or after the change is safe.

Not pointing out anything that needs to be changed; demonstrating
that I did read this sufficiently well to say that I have reviewed
it ;-)

Re: [PATCH] credential: let empty credential specs reset helper list

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> Sine the credential.helper key is a multi-valued config
>
> s/Sine/Since/;
>
>> diff --git a/credential.c b/credential.c
>> index 7d6501d..aa99666 100644
>> --- a/credential.c
>> +++ b/credential.c
>> @@ -63,9 +63,12 @@ static int credential_config_callback(const char *var, const char *value,
>>  		key = dot + 1;
>>  	}
>>  
>> -	if (!strcmp(key, "helper"))
>> -		string_list_append(&c->helpers, value);
>> -	else if (!strcmp(key, "username")) {
>> +	if (!strcmp(key, "helper")) {
>> +		if (*value)
>> +			string_list_append(&c->helpers, value);
>> +		else
>> +			string_list_clear(&c->helpers, 0);
>> +	} else if (!strcmp(key, "username")) {
>
> I wondered why neither the existing code nor the updated one has a
> check for !value, but this callback assumes no credential
> configuration variable will ever be a boolean and rejects it
> upfront, so this code before or after the change is safe.
>
> Not pointing out anything that needs to be changed; demonstrating
> that I did read this sufficiently well to say that I have reviewed
> it ;-)

This reminds me of one thing.  The only reason why we are hesitant
to introduce a new syntax like

	[credential]
        	!helper ;# clear
                helper = ...

to allow explicit clearing of accumulated values so far IIRC is
because such a _file_ will not be readable by existing versions of
Git.  Am I correct?

If that is the case, then that reasoning will still not prevent us
from adding corresponding support for a command-line overide, i.e.
either one or both of these:

	$ git -c credential.!helper cmd
	$ git -c !credential.helper cmd

no?

Of course, the code in the configuration subsystem for updated
version of Git needs to become aware of the new syntax, and those
that deal with the multi-value variables need custom code, which is
similar to the way you special cased an empty value in the above
patch, so I am not sure how much this would help.

Re: [PATCH] credential: let empty credential specs reset helper list

[Jacob Keller]

On Fri, Feb 26, 2016 at 11:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Of course, the code in the configuration subsystem for updated
> version of Git needs to become aware of the new syntax, and those
> that deal with the multi-value variables need custom code, which is
> similar to the way you special cased an empty value in the above
> patch, so I am not sure how much this would help.
>

For most cases, I think the previous approach of setting to empty
string makes sense and is more easilly intuitive, but would only work
for those values which the empty string has no valid meaning.

I would think either approach here is fine, and i think *most* cases
of multivariable configuration don't need to be cleared from within a
configuration file but only on the command line would be acceptable.

If we go this route I would prefer "-c !<config>" vs "-c <namespace>.!<config>"

Thanks,
Jake

Re: [PATCH] credential: let empty credential specs reset helper list

[Jeff King]

On Fri, Feb 26, 2016 at 11:34:12AM -0800, Junio C Hamano wrote:

> >> -	if (!strcmp(key, "helper"))
> >> -		string_list_append(&c->helpers, value);
> >> -	else if (!strcmp(key, "username")) {
> >> +	if (!strcmp(key, "helper")) {
> >> +		if (*value)
> >> +			string_list_append(&c->helpers, value);
> >> +		else
> >> +			string_list_clear(&c->helpers, 0);
> >> +	} else if (!strcmp(key, "username")) {
> >
> > I wondered why neither the existing code nor the updated one has a
> > check for !value, but this callback assumes no credential
> > configuration variable will ever be a boolean and rejects it
> > upfront, so this code before or after the change is safe.
> >
> > Not pointing out anything that needs to be changed; demonstrating
> > that I did read this sufficiently well to say that I have reviewed
> > it ;-)
> 
> This reminds me of one thing.  The only reason why we are hesitant
> to introduce a new syntax like
> 
> 	[credential]
>         	!helper ;# clear
>                 helper = ...
> 
> to allow explicit clearing of accumulated values so far IIRC is
> because such a _file_ will not be readable by existing versions of
> Git.  Am I correct?

I think there is another reason, which is that the interface we expose
to config callbacks (and via "config --get-all") is to sequentially pass
in all values. How does that interact with this "reset"? For example,
what is the output of:

  git config foo.bar one
  git -c '!foo.bar' config --get-all foo.bar

?

Do we continue to output the "reset" values, or do we quietly munge the
list on behalf of the caller? If the former, how do we represent that in
the output? I can see arguments both ways.

Implementation-wise (both for git-config and for internal callbacks), it
means we cannot parse the config as a single pass anymore. That's
probably OK; we've already moved partially toward that with the
configset stuff. If we _just_ support this via command-line options, we
could do an initial pass over those, looking for negatives, and then
simply skip all negatives while parsing the config files.

> If that is the case, then that reasoning will still not prevent us
> from adding corresponding support for a command-line overide, i.e.
> either one or both of these:
> 
> 	$ git -c credential.!helper cmd
> 	$ git -c !credential.helper cmd
> 
> no?

Yes, that would work, though to me it really feels like a
half-implemented feature. You cannot override a bad /etc/gitconfig line
via your ~/.gitconfig or repo-specific .git/config. Those things are
useful.

One other thing that occurred to me is that Apple Git hard-codes the
osxkeychain helper (rather than putting it into the system-wide
gitconfig <sigh>). No config-based system can "undo" that, but my patch
does. I admit that's probably not the best argument; hitting Apple with
a clue-stick is a cleaner approach.

> Of course, the code in the configuration subsystem for updated
> version of Git needs to become aware of the new syntax, and those
> that deal with the multi-value variables need custom code, which is
> similar to the way you special cased an empty value in the above
> patch, so I am not sure how much this would help.

I think you could get away without changing the users of the multi-value
variables, using the "negative" approach I mentioned above. Basically:

  1. parse GIT_CONFIG_PARAMETERS looking for negatives; stick them in a
     string-list or whatever.

  2. parse the files; look up each key in the string-list, and if it
     matches, don't even send it to the callback

  3. clear the string-list

  4. parse GIT_CONFIG_PARAMETERS again, ignoring any negatives

But like I said, that does feel somewhat half-implemented to me, since
it treats the command-line specially.

-Peff

Re: [PATCH] credential: let empty credential specs reset helper list

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think there is another reason, which is that the interface we expose
> to config callbacks (and via "config --get-all") is to sequentially pass
> in all values. How does that interact with this "reset"? For example,
> what is the output of:
>
>   git config foo.bar one
>   git -c '!foo.bar' config --get-all foo.bar

The callback API needs to be extended to include this "reset" bit,
not just "var" and "value", obviously.  And then of course the users
of the API need to be updated to act on that "reset" thing.

That is what I meant by "multi-value things need custom code".  The
single-valued ones can do "last one wins", and "reset" can be turned
into "the initial state" assignment for the variable; multi-value
cumulative ones already have custom code to accumulate things into
list, and they need to learn to empty the list upon seeing the
"reset" bit.

But that is not as fundamental as "new syntax breaks old clients".
If we want our new code to do useful things, obviously we would need
to work on making it do so ;-)

> I think you could get away without changing the users of the multi-value
> variables, using the "negative" approach I mentioned above. Basically:
> ...
> But like I said, that does feel somewhat half-implemented to me, since
> it treats the command-line specially.

I agree with you that that is a kludge and not the right way
forward.