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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 26, 2016, 17:23 UTC
Message-ID: <xmqqa8mnl71v.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160226105135.GA30215@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

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

s/Sine/Since/;

Show 17 quoted lines

> 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 ;-)