Re: [PATCH 3/3] http: support CURLOPT_PROTOCOLS_STR

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 16, 2023, 16:05 UTC
Message-ID: <xmqqzgaicvmj.fsf@gitster.g>
In-Reply-To: <230116.86edruzk5m.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 8 quoted lines

> +#define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR (LIBCURL_VERSION_NUM >= 0x075500)
> +#if GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR
> +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR CURLOPT_REDIR_PROTOCOLS_STR 
> +#define GIT_CURLOPT_PROTOCOLS_STR CURLOPT_PROTOCOLS_STR
> +#else
> +#define GIT_CURLOPT_REDIR_PROTOCOLS_STR 0
> +#define GIT_CURLOPT_PROTOCOLS_STR 0
>  #endif

I find it a bit ugly that CURLOPT_* being used are all non-zero, but it may be true in practice.

Show 25 quoted lines

> diff --git a/http.c b/http.c
> index e529ebc993d..3224e897f02 100644
> --- a/http.c
> +++ b/http.c
> @@ -933,24 +933,22 @@ static CURL *get_curl_handle(void)
>  	curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
>  	curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
>  
> -#ifdef GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR
> -	{
> +	if (GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR) {
>  		struct strbuf buf = STRBUF_INIT;
> ...
> +	} else {
> +		curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> +				 get_curl_allowed_protocols(0, NULL));
> +		curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> +				 get_curl_allowed_protocols(-1, NULL));
>  	}
> -#else
> -	curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> -			 get_curl_allowed_protocols(0, NULL));
> -	curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> -			 get_curl_allowed_protocols(-1, NULL));
> -#endif

I somehow find the above over-engineered solution looking for a problem. Conditional compilation might be ugly, but what is uglier is a conditional compilation hidden behind what pretends to be a runtime conditional but gets optimized away at compile time. Also, when the non _STR variant changes status from deprecated to removed, the code will cease to build, so I am not sure if the above is the whole story. You'd also need dummy definitions for them when the version of cURL is advanced enough.

It is true that with the above we will always pass both sides to the compiler, which may be an upside, but at the same time doesn't it make it harder to notice and remove the support of the older side once the time comes?

Thanks.