Re: [PATCH 4/5] http: centralize the accounting of libcurl dependencies

From: Jeff King <peff@peff.net>
Date: Sep 8, 2021, 19:31 UTC
Message-ID: <YTkPfyAYTU4ZgRgb@coredump.intra.peff.net>
In-Reply-To: <patch-4.5-47b513a261b-20210908T152807Z-avarab@gmail.com>

On Wed, Sep 08, 2021 at 05:31:55PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 7 quoted lines

> As discussed in 644de29e220 (http: drop support for curl < 7.19.4,
> 2021-07-30) checking against LIBCURL_VERSION_NUM isn't as reliable as
> checking specific defines in curl, as some distros have been known to
> backport features. Furthermore as shown in the preceding commit doing
> these version checks makes for hard to read and possibly buggy code,
> as shown by the bug fixed there where we were conflating base 10 for
> base 16 when comparing the version.

Just playing devil's advocate for a moment: we are making the assumption here that curl will use preprocessor macros to implement these constants (as opposed to, say, enums). I think that has been historically true, but it is an extra dependency we're adding on curl's internal-ish details.

Show 14 quoted lines

> +/**
> + * CURLOPT_TCP_KEEPALIVE was added in 7.25.0, released in March 2012.
> + */
> +#ifdef CURLOPT_TCP_KEEPALIVE
> +#define GITCURL_HAVE_CURLOPT_TCP_KEEPALIVE 1
> +#endif
>
> [...]
>
> -#if LIBCURL_VERSION_NUM >= 0x071900
> +#ifdef GITCURL_HAVE_CURLOPT_TCP_KEEPALIVE
>  static void set_curl_keepalive(CURL *c)
>  {
>  	curl_easy_setopt(c, CURLOPT_TCP_KEEPALIVE, 1);

Part of me is a little sad at the duplication this creates. We could just be checking

  #ifdef CURLOPT_TCP_KEEPALIVE

in the second hunk, without the first one at all. That does make it harder to see which ones are in use (and we'd still want a comment to take note of the versions). I dunno. I guess having a central-ish registry of these is worth the duplication.

-Peff