RE: [PATCH v2 0/8] post-v2.33 "drop support for ancient curl" follow-up

On September 10, 2021 12:53 PM, Junio C Hamano wrote:

To: Ævar Arnfjörð Bjarmason <redacted>
Cc: git@vger.kernel.org; Jeff King <redacted>; brian m . carlson <redacted>; Bagas Sanjaya
[off-list ref]
Subject: Re: [PATCH v2 0/8] post-v2.33 "drop support for ancient curl" follow-up

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

quoted

Version 1 of this had a really bad bug where we'd effectively make all
supported curl versions act like 7.19.4, i.e. the oldest supported
version except for a couple of our supported features. This is because
most of the things checked with the "ifdef" checks are enum fields,
not macros. So basically the "devil's advocate" Jeff King pointed out
in [2] was already the case. Oops!

Wow.  Thanks for bothering to actually test ;-)

Because we learned that "#ifdef CURL_SOME_FEATURE_WE_WANT" is not a generally applicable way to conditionally build for various
features and we'd need to switch on the version numbers, it is now clear (at least to me) that the central registry approach would be a
direction to go.

quoted

In this v2 we're instead checking LIBCURL_VERSION_NUM consistently,
even in those cases where we are checking things that are defined via
macros.

Nice.

quoted

    ++ * For each X symbol we need from curl we define our own
    ++ * GIT_CURL_HAVE_X. If multiple similar symbols with the same prefix
    ++ * were defined in the same version we pick one and check for that name.
     + *
     + * Keep any symbols in date order of when their support was
     + * introduced, oldest first, in the official version of cURL library.
    @@ git-curl-compat.h (new)
     +/**
     + * CURLOPT_TCP_KEEPALIVE was added in 7.25.0, released in March 2012.
     + */
    -+#ifdef CURLOPT_TCP_KEEPALIVE
    ++#if LIBCURL_VERSION_NUM >= 0x071900
     +#define GITCURL_HAVE_CURLOPT_TCP_KEEPALIVE 1
     +#endif

What we have in the posted patch is perfectly OK and serviceable, but this organization somewhat surprised me.

Instead of one #if...#endif block per a feature macro, I expected to see a sequence of

/* 7.34.0 (0x072200) - Dec 2013 */
#if 0x072200 < VERSION
#define HAVE_FOO 1
#define HAVE_BAR 1
...
#endif

/* 7.25.0 (0x071900) - March 2012 */
#if 0x071900 < VERSION
#define HAVE_BAZ 1
#define HAVE_QUX 1
...
       #endif


because it would make it more clear which features can now be unconditionally used when we raise the cut-off point for the oldest
supported version if we group these entries along the versions.

Thanks.

Not that it might matter much, but both NonStop platforms are now on curl 7.78.0 and we are deploying git under OpenSSL 3 this week or next. So any objections I might previously have had are now fortunately really moot.
Cheers,
-Randall