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

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