Re: [PATCH] git-curl-compat.h: addition of all symbols defined by curl

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 15, 2022, 15:47 UTC
Message-ID: <xmqqzglrfcas.fsf@gitster.g>
In-Reply-To: <20220315131638.924669-1-gitter.spiros@gmail.com>

Elia Pinto <gitter.spiros@gmail.com> writes:

Show 18 quoted lines

> This file was produced from a modified version of symbols.pl
> (https://github.com/curl/curl/blob/master/docs/libcurl/symbols.pl) and
> by manually adding the previous comments describing the dates of release
> of some curl versions not currently reported in the symbols-in-versions.
>
> To do this the symbols are listed in the order defined in the file
> symbols-in-versions rather than as they were previously inserted based
> on release dates.
>
> Most of these symbols are not used by git today. However, inserting
> them all starting from an automatic tool makes it largely unnecessary
> to update this file and therefore reduces the possibility
> of introducing possible errors in the future.
>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
>  git-curl-compat.h | 2944 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 2875 insertions(+), 69 deletions(-)

Hmmm. While the basic idea may be sound, I do not think this alone would be sufficient out of the box.

Show 18 quoted lines

> diff --git a/git-curl-compat.h b/git-curl-compat.h
> index 56a83b6bbd..dc9086710a 100644
> --- a/git-curl-compat.h
> +++ b/git-curl-compat.h
> @@ -24,25 +24,1228 @@
>   * doing so is sufficient to add support for it to older versions that
>   * don't have it.
>   *
> - * Keep any symbols in date order of when their support was
> - * introduced, oldest first, in the official version of cURL library.
> - */
> -
> -/**
> - * CURL_SOCKOPT_OK was added in 7.21.5, released in April 2011.
> - */
> -#if LIBCURL_VERSION_NUM < 0x071505
> -#define CURL_SOCKOPT_OK 0
> -#endif

Even though you define GIT_HAVE_CURL_SOCKOPT_OK macro below, the existing codebase is not expecting to handle the lack of CURL_SOCKOPT_OK by checking the macro. Instead, it expects the macro to be defined to be 0 for older versions, exactly like this.

And you do not give a replacement definition.

> -#if LIBCURL_VERSION_NUM >= 0x071900
> -#define GITCURL_HAVE_CURLOPT_TCP_KEEPALIVE 1
> -#endif

This is an unfortunate oddball that lacks _ between GIT and CURL.

$ git grep -e '#define ' --and --not -e GIT_CURL_ git-curl-compat.h

> -#if LIBCURL_VERSION_NUM >= 0x071900
> -#define GITCURL_HAVE_CURLOPT_TCP_KEEPALIVE 1
> -#endif

You define GIT_CURL_HAVE_CURLOPT_TCP_KEEPALIVE but naturally, existing callers do not pay attention to it. They care about the oddball variant GITCURL_HAVE_CURLOPT_TCP_KEEPALIVE instead.

Luckily, these two are the only irregulars, it seems.

$ git grep -e '#define ' --and --not -e GIT_CURL_ git-curl-compat.h git-curl-compat.h:#define CURL_SOCKOPT_OK 0 git-curl-compat.h:#define GITCURL_HAVE_CURLOPT_TCP_KEEPALIVE 1

So, I think you would need a bit of preliminary clean-up patch inserted before this automated compatibility layer, perhaps like this.

--- >8 ---
Subject: curl: streamline conditional compilation

Earlier we introduced git-curl-compat.h that defines bunch of GIT_CURL_HAVE_X where X is a feature of cURL library we care about, to make it easily manageable to conditionally compile code against the version of cURL library we are given.

There however are two oddball macros. Instead of checking GIT_CURL_HAVE_CURL_SOCKOPT_OK and using a fallback definition for CURL_SOCKOPT_OK macro, we just defined CURL_SOCKOPT_OK to a safe value when compiling against an old version that lack the symbol. Also, the macro to check CURLOPT_TCP_KEEPALIVE (alone) was named GITCURL_HAVE_CURLOPT_TCP_KEEPALIVE.

Introduce GIT_CURL_HAVE_CURL_SOCKOPT_OK and define it for the versions of cURL where we used to use our fallback definition for CURL_SOCKOPT_OK, and use the fallback definition based on the new GIT_CURL_HAVE_CURL_SOCKOPT_OK symbol at its sole use site.

To better conform the naming convention of other symbols, rename GITCURL_HAVE_CURLOPT_TCP_KEEPALIVE to GIT_CURL_HAVE_CURL_SOCKOPT_OK and update its sole use site.

After this, conditional compilation with cURL library is all controlled uniformly with GIT_CURL_HAVE_X mechanism.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-curl-compat.h | 2 +-
 http.c            | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git c/git-curl-compat.h w/git-curl-compat.h
index 56a83b6bbd..802d11e7be 100644
--- c/git-curl-compat.h
+++ w/git-curl-compat.h
@@ -39,7 +39,7 @@
  * CURLOPT_TCP_KEEPALIVE was added in 7.25.0, released in March 2012.
  */
 #if LIBCURL_VERSION_NUM >= 0x071900
-#define GITCURL_HAVE_CURLOPT_TCP_KEEPALIVE 1
+#define GIT_CURL_HAVE_CURLOPT_TCP_KEEPALIVE 1
 #endif
 
 
diff --git c/http.c w/http.c
index 2f67fbb33c..8d9a66b5ca 100644
--- c/http.c
+++ w/http.c
@@ -517,7 +517,7 @@ static int has_proxy_cert_password(void)
 }
 #endif
 
-#ifdef GITCURL_HAVE_CURLOPT_TCP_KEEPALIVE
+#ifdef GIT_CURL_HAVE_CURLOPT_TCP_KEEPALIVE
 static void set_curl_keepalive(CURL *c)
 {
 	curl_easy_setopt(c, CURLOPT_TCP_KEEPALIVE, 1);
@@ -537,6 +537,9 @@ static int sockopt_callback(void *client, curl_socket_t fd, curlsocktype type)
 	if (rc < 0)
 		warning_errno("unable to set SO_KEEPALIVE on socket");
 
+#ifndef GIT_CURL_HAVE_CURL_SOCKOPT_OK
+#define CURL_SOCKOPT_OK 0
+#endif
 	return CURL_SOCKOPT_OK;
 }