Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> 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.
> 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.
> ++ * 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.
