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

[Junio C Hamano <gitster@pobox.com>]

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.