Re: [PATCH v2 7/8] http: centralize the accounting of libcurl dependencies

From: Jeff King <peff@peff.net>
Date: Sep 10, 2021, 15:15 UTC
Message-ID: <YTt2nHMXAkyfol9v@coredump.intra.peff.net>
In-Reply-To: <patch-v2-7.8-8e57a8409c5-20210910T105523Z-avarab@gmail.com>

On Fri, Sep 10, 2021 at 01:04:32PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 14 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 symbols present in curl, as some distros have been
> known to backport features.
> 
> However, while some of the curl_easy_setopt() arguments we rely on are
> macros, others are enum, and we can't assume that those that are
> macros won't change into enums in the future.
> 
> So we're still going to have to check LIBCURL_VERSION_NUM, but by
> doing that in one central place and using a macro definition of our
> own, anyone who's backporting features can define it themselves, and
> thus have access to more modern curl features that they backported,
> even if they didn't bump the LIBCURL_VERSION_NUM.

I think this lays out a nicer argument for this centralized registry than the previous round.

I looked over the changes themselves and didn't notice any problems. I didn't cross-check every moved-version field, since that seemed like an unlikely mistake to make. I did check for easy mistakes at the use-sites, like making sure to use #ifndef for "version < X" and #ifdef for "version >= X".

> -#if LIBCURL_VERSION_NUM >=0x072b00
> +#ifdef GIT_CURL_HAVE_CURL_HTTP_VERSION_2

Good, I was going to complain about the whitespace in this one when you touched it earlier, but now it goes away. :)

-Peff