Re: [PATCH 2/3] http: add support for disabling SSL revocation checks in cURL

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 16, 2018, 04:23 UTC
Message-ID: <xmqq1s8q34g2.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <764791d13d20478639402e7af95e901223444240.1539598481.git.gitgitgadget@gmail.com>

"Brendan Forster via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Note: an earlier iteration tried to use the config setting
> http.schannel.checkRevoke, but the http.* config settings can be limited
> to specific URLs via http.<url>.* (which would mistake `schannel` for a
> URL).

Yeah, "http.schannel.anything" would not work, but is this note relevant here? As far as the git development community as a whole is concerned, this is the first iteration of the patch we see and review.

In any case, you can use "http.<url>.$variable" to say "I want the http.$variable to be in effect but only when I am talking to <url>". Does it make sense for this new variable, too? That is, does it benefit the users to be able to do something like this?

    [http] schannelCheckRevoke = no
    [http "https://microsoft.com/"] schannelCheckRevoke = yes

I am guessing that the answer is yes.

I guess the same comment applies to the previous step, but I suspect that the code structure may not allow us to switch the SSL backend so late in the game (e.g. "when talking to microsoft, use schannel, but when talking to github, use openssl").

Show 6 quoted lines

> +#if LIBCURL_VERSION_NUM >= 0x072c00
> +		curl_easy_setopt(result, CURLOPT_SSL_OPTIONS, CURLSSLOPT_NO_REVOKE);
> +#else
> +		warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options because\n"
> +			"your curl version is too old (>= 7.44.0)");
> +#endif

That ">=" is hard to grok. I think you meant it to be pronounced "requries at least", but that is not a common reading. People more commonly pronounce it "is greater than or equal to".

> +	}
> +
>  	if (http_proactive_auth)
>  		init_curl_http_auth(result);