Re: [PATCH] http.c: fix parsing of http.sslCertPasswordProtected variable

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 12, 2013, 19:05 UTC
Message-ID: <20130712190531.GD8482@google.com>
In-Reply-To: <7v38rjlif4.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

> The existing code triggers only when the configuration variable is
> set to true.  Once the variable is set to true in a more generic
> configuration file (e.g. ~/.gitconfig), it cannot be overriden to
> false in the repository specific one (e.g. .git/config).

[...]

Show 9 quoted lines

> --- a/http.c
> +++ b/http.c
> @@ -160,8 +160,7 @@ static int http_options(const char *var, const char *value, void *cb)
>  	if (!strcmp("http.sslcainfo", var))
>  		return git_config_string(&ssl_cainfo, var, value);
>  	if (!strcmp("http.sslcertpasswordprotected", var)) {
> -		if (git_config_bool(var, value))
> -			ssl_cert_password_required = 1;
> +		ssl_cert_password_required = git_config_bool(var, value);

Thanks for catching it. The documentation doesn't say anything about this "can only enable and cannot disable" behavior and the usual pattern is to allow later settings to override earlier ones, so this change looks good.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

FWIW the GIT_SSL_CERT_PASSWORD_PROTECTED envvar has a similar "can only enable" behavior, but since it's documented, that's not as big of a problem. Do you remember why it was written that way?

When that setting was first added[1], there was some mention of autodetection if libcurl could learn to do that. Did it happen? (Please forgive my ignorance.)

Thanks, Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/122793/focus=122816