On Wed, Sep 08, 2021 at 05:31:55PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 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.
> +/**
> + * 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
