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

>
> Elia Pinto <gitter.spiros@gmail.com> writes:
>
> > 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.
>
> > 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;
>  }
>
>