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

On Fri, Sep 10 2021, Jeff King wrote:

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

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!

Oops. :)

Well, I'm glad I mentioned it, then (I really was just playing devil's
advocate). It does seem a little scary that we'd compile without all of
these features and not even notice it.

I guess it's hard to test for those features when we don't know for sure
that our libcurl supports them. We could perhaps make the tests
optional, but how do we set up the prereqs?

Version checks based on "curl --version" or even "pkg-config
--modversion libcurl" seems error-prone. The curl binary might not match
the library we used, or the user might have given us a specific libcurl
to use rather than the pkg-config version. We could teach Git to tell us
which features it thinks curl supports, but that's depending on the very
thing we're trying to test. We could have Git output the
LIBCURL_VERSION_NUM it saw at build time, but then we're just
implementing the same "if version > X, we have this feature" logic now
in the test suite.

So I dunno. I do not have any clever solution that would have caught
this automatically without creating an even bigger maintenance headache.

Yeah, ideally we'd have tests for these optional features, and we'd then
just add them to GIT-BUILD-OPTIONS and skip dedicated tests
appropriately, then it would be more visible to see those tests
skipped. Presumably someone testing this would run "make test" against a
glob that includes *curl*.

But that's not easy to do in practice since the flags are either not
visible from the outside in terms of behavior, or if they are it's all
something that requires proxies, SSL etc, which we don't test currently.

We can and should test that, but requires e.g. extending lib-httpd.sh to
start apache with ssl support, or maybe we could do it more easily with
an optional stunnel or something...

(Though for other reasons, it might be nice to report the curl version
from "git version --build-options". This is a bit tricky because we
avoid linking at libcurl at all in the main binary. Definitely
orthogonal to your series, anyway).

We could always ship a git-version-curl and shell out to it, or embed
the version curl-config --vernum gave us at compile time via Makefile ->
-DGIT_CURL_VERSION.

That version could be changed at runtime. We could call that a
feature. It's --build-options, not --runtime-libraries :)