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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 26, 2016, 19:34 UTC
Message-ID: <xmqqk2lrjmff.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqa8mnl71v.fsf@gitster.mtv.corp.google.com>

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

Show 32 quoted lines

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