| Message ID | 20220316140106.14678-2-gitter.spiros@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | addition of all symbols defined by curl | expand |
On Wed, Mar 16 2022, Elia Pinto wrote: [Meta: Please chehck the -vN and --in-reply-to options to git-format-patch et al, i.e. make a v2 a v2, and have it reply to the v1 patch or cover-letter.] > 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. The way it was being done before was intentional & discused on list. See my original https://lore.kernel.org/git/patch-v3-7.7-93a2775d0ee-20210730T092843Z-avarab@gmail.com/ which did it pretty much like that, and Junio's subsequent follow-up. I.e. this breadcrumb trail: https://lore.kernel.org/git/?q=CURL_SOCKOPT_OK > -#if LIBCURL_VERSION_NUM < 0x071505 > -#define CURL_SOCKOPT_OK 0 > +#if LIBCURL_VERSION_NUM >= 0x071505 > +#define GIT_CURL_HAVE_CURL_SOCKOPT_OK 1 > #endif IOW we should drop this. > /** > * 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 This change is good. > diff --git a/http.c b/http.c > index 229da4d148..d7ad7db1d6 100644 > --- a/http.c > +++ b/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) > { As is this. > curl_easy_setopt(c, CURLOPT_TCP_KEEPALIVE, 1); > @@ -536,7 +536,9 @@ static int sockopt_callback(void *client, curl_socket_t fd, curlsocktype type) > rc = setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, (void *)&ka, len); > 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; > } The whole point of git-curl-compat.h and its big-brother git-compat-util.h is that we'd prefer not to have such hacks inline if at all possible. For most of the GIT_CURL_* stuff we need to since it's conditionally using symbols etc., but in this case we can just define a fallback centrally and not worry about it in the code. So the pre-image really is much better.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > The way it was being done before was intentional & discused on list. > > See my original > https://lore.kernel.org/git/patch-v3-7.7-93a2775d0ee-20210730T092843Z-avarab@gmail.com/ > which did it pretty much like that, and Junio's subsequent > follow-up. I.e. this breadcrumb trail: > https://lore.kernel.org/git/?q=CURL_SOCKOPT_OK > >> -#if LIBCURL_VERSION_NUM < 0x071505 >> -#define CURL_SOCKOPT_OK 0 >> +#if LIBCURL_VERSION_NUM >= 0x071505 >> +#define GIT_CURL_HAVE_CURL_SOCKOPT_OK 1 >> #endif > > IOW we should drop this. I think that depends on the worldview. In a world in which [PATCH 2/2] is a good idea, i.e. "we have a comprehensive catalog of available cURL features, but it expresses its knowledge in one particular way, i.e. HAVE_X", the above, together with the change at the only use site in http.c, are very sensible changes. Given that we do not want to have too many conditionally compiled codepath, I certainly understand that the current approach to keep an ad-hoc list of features we care about may be your preference. I am not sure if that is viable longer term, though. I still am not decided. Thanks.
diff --git a/git-curl-compat.h b/git-curl-compat.h index 56a83b6bbd..b14234f9e7 100644 --- a/git-curl-compat.h +++ b/git-curl-compat.h @@ -31,15 +31,15 @@ /** * CURL_SOCKOPT_OK was added in 7.21.5, released in April 2011. */ -#if LIBCURL_VERSION_NUM < 0x071505 -#define CURL_SOCKOPT_OK 0 +#if LIBCURL_VERSION_NUM >= 0x071505 +#define GIT_CURL_HAVE_CURL_SOCKOPT_OK 1 #endif /** * 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 a/http.c b/http.c index 229da4d148..d7ad7db1d6 100644 --- a/http.c +++ b/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); @@ -536,7 +536,9 @@ static int sockopt_callback(void *client, curl_socket_t fd, curlsocktype type) rc = setsockopt(fd, SOL_SOCKET, SO_KEEPALIVE, (void *)&ka, len); 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; }
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. Co-authored-by: Junio C Hamano <gitster@pobox.com> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com> --- git-curl-compat.h | 6 +++--- http.c | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-)