From: Jeff King <hidden> Date: 2017-08-09 12:00:32
This is a resurrection of the thread from April:
https://public-inbox.org/git/20170404025438.bgxz5sfmrawqswcj@sigill.intra.peff.net/
The general idea is that we should drop support for very old curl
versions, which already fail to compile. I'm sympathetic to the case
where people actually have systems with really old versions of curl. But
at the same time, I think we may be better off informing them that Git
isn't tested with these ancient versions at all (and I have a suspicion
that there are lurking bugs; see the commit messages or read that other
thread).
I've broken the changes into three patches. That helps a bit with
reviewing the diffs, but it also means we don't have to apply them all
at once (though I think we should; but it would likewise help if end up
wanting to revert one of them later).
The first cutoff is based on having more compilation breakages than the
other (and also just being incredibly old). The second is just a sweet
spot of bang-for-the-buck and age. In the absence of other data, it's
probably what I would suggest. The third one uses the existing compile
breakage from v2.12.0 as a guide.
[1/4]: http: drop support for curl < 7.11.1
[2/4]: http: drop support for curl < 7.16.0
[3/4]: http: drop support for curl < 7.19.4
[4/4]: http: #error on too-old curl
Documentation/config.txt | 3 +-
http-push.c | 23 -------
http-walker.c | 12 ----
http.c | 153 +----------------------------------------------
http.h | 35 +----------
remote-curl.c | 7 ---
6 files changed, 5 insertions(+), 228 deletions(-)
-Peff
From: Jeff King <hidden> Date: 2017-08-09 12:01:32
Recent versions of Git will not build with curl older than
7.11.1 due to (at least) two issues:
- our use of CURLOPT_POSTFIELDSIZE in 37ee680d9b
(http.postbuffer: allow full range of ssize_t values,
2017-04-11). This field was introduced in curl 7.11.1.
- our use of CURLPROTO_* outside any #ifdef in aeae4db174
(http: create function to get curl allowed protocols,
2016-12-14). These were introduced in curl 7.19.4.
We could solve these compilation problems with more #ifdefs,
but it's not worth the trouble. Version 7.11.1 came out in
March of 2004, over 13 years ago. Let's declare that too old
and drop any existing ifdefs that go further back. One
obvious benefit is that we'll have fewer conditional bits
cluttering the code.
But more importantly, we're doing a disservice to users to
pretend that Git works with old versions. It's clear that
nobody is testing modern Git with such old versions of curl
(we've had 3 released versions with the CURLPROTO issue
without a report of anyone seeing the breakage in the wild).
And there are a lot of subtle ways we could be getting this
wrong (for instance, curl prior to 7.17.0 did not copy
string arguments to curl_easy_setopt(), which means that
using an old copy of curl could produce use-after-free
bugs that are not present with more recent versions).
This patch drops all #ifdefs that reference older versions
(note that curl's preprocessor macros are in hex, so we're
looking for 070b01, not 071101).
Signed-off-by: Jeff King <redacted>
---
There may be other problems, too. I couldn't actually get a version of
curl older than 7.12.2 to compile due to bison/yacc woes.
http.c | 51 ---------------------------------------------------
http.h | 11 -----------
remote-curl.c | 3 ---
3 files changed, 65 deletions(-)
From: Jeff King <hidden> Date: 2017-08-09 12:02:04
As discussed in the previous commit, Git is not well-tested
with old versions of curl (and in fact since v2.12.0 does
not even compile with versions older than 7.19.4). Let's
stop pretending we support curl that old and drop any
now-obslete #ifdefs.
Choosing 7.16.0 is a somewhat arbitrary cutoff, but:
1. it came out in October of 2006, over 10 years ago.
Besides being a nice round number, it's a common
end-of-life support period, even for conservative
distributions.
2. that version introduced the curl_multi interface, which
gives us a lot of bang for the buck in removing #ifdefs
Signed-off-by: Jeff King <redacted>
---
Documentation/config.txt | 3 +--
http-push.c | 23 --------------------
http-walker.c | 12 -----------
http.c | 56 +-----------------------------------------------
http.h | 20 +----------------
remote-curl.c | 4 ----
6 files changed, 3 insertions(+), 115 deletions(-)
@@ -1971,8 +1971,7 @@ http.maxRequests:: http.minSessions:: The number of curl sessions (counted across slots) to be kept across requests. They will not be ended with curl_easy_cleanup() until- http_cleanup() is invoked. If USE_CURL_MULTI is not defined, this- value will be capped at 1. Defaults to 1.+ http_cleanup() is invoked. Defaults to 1. http.postBuffer:: Maximum size in bytes of the buffer used by smart HTTP
@@ -1666,21 +1655,15 @@ static int delete_remote_branch(const char *pattern, int force)staticvoidrun_request_queue(void){-#ifdef USE_CURL_MULTIis_running_queue=1;fill_active_slots();add_fill_function(NULL,fill_active_slot);-#endifdo{finish_all_active_slots();-#ifdef USE_CURL_MULTIfill_active_slots();-#endif}while(request_queue_head&&!aborted);-#ifdef USE_CURL_MULTIis_running_queue=0;-#endif}intcmd_main(intargc,constchar**argv)
@@ -1756,10 +1739,6 @@ int cmd_main(int argc, const char **argv)break;}-#ifndef USE_CURL_MULTI-die("git-push is not available for http/https repository when not compiled with USE_CURL_MULTI");-#endif-if(!repo->url)usage(http_push_usage);
@@ -1772,9 +1751,7 @@ int cmd_main(int argc, const char **argv)http_init(NULL,repo->url,1);-#ifdef USE_CURL_MULTIis_running_queue=0;-#endif/* Verify DAV compliance/lock support */if(!locking_available()){
@@ -1005,7 +973,6 @@ struct active_request_slot *get_active_slot(void)structactive_request_slot*slot=active_queue_head;structactive_request_slot*newslot;-#ifdef USE_CURL_MULTIintnum_transfers;/* Wait for a slot to open up if the queue is full */
From: Jeff King <hidden> Date: 2017-08-09 12:02:08
Since v2.12.0, Git does not compile with versions of curl
older than 7.19.4. That version of curl is about 8 years
old. This means it may still be used in some distributions
with long-running support periods. But the fact that we
haven't received a single bug report about the compile-time
breakage implies that nobody cares about building recent
releases on such platforms.
As discussed in the previous two commits, this cleans up the
code and gives a more realistic signal to users about which
versions of Git are actually tested (in particular, this
moves us past the potential use-after-free issues with curl
older than 7.17.0).
Signed-off-by: Jeff King <redacted>
---
http.c | 46 ----------------------------------------------
http.h | 4 ----
2 files changed, 50 deletions(-)
@@ -718,20 +690,12 @@ static CURL *get_curl_handle(void)}curl_easy_setopt(result,CURLOPT_MAXREDIRS,20);-#if LIBCURL_VERSION_NUM >= 0x071301curl_easy_setopt(result,CURLOPT_POSTREDIR,CURL_REDIR_POST_ALL);-#elif LIBCURL_VERSION_NUM >= 0x071101curl_easy_setopt(result,CURLOPT_POST301,1);-#endif-#if LIBCURL_VERSION_NUM >= 0x071304curl_easy_setopt(result,CURLOPT_REDIR_PROTOCOLS,get_curl_allowed_protocols(0));curl_easy_setopt(result,CURLOPT_PROTOCOLS,get_curl_allowed_protocols(-1));-#else-warning("protocol restrictions not applied to curl redirects because\n"-"your curl version is too old (>= 7.19.4)");-#endifif(getenv("GIT_CURL_VERBOSE"))curl_easy_setopt(result,CURLOPT_VERBOSE,1L);setup_curl_trace(result);
From: Jeff King <hidden> Date: 2017-08-09 12:02:14
We already fail to build with versions of curl older than
7.19.4. But doing an explicit check with an #error has two
benefits.
One is that it makes it clear to users that the build
failure is intentional, so they don't waste time trying to
debug it.
And two is that it documents our current "too old"
assumption, so that we know whether we need use an #ifdef
when using newer curl features in future patches.
Signed-off-by: Jeff King <redacted>
---
http.h | 4 ++++
1 file changed, 4 insertions(+)
Since v2.12.0, Git does not compile with versions of curl
older than 7.19.4. That version of curl is about 8 years
old. This means it may still be used in some distributions
with long-running support periods. But the fact that we
haven't received a single bug report about the compile-time
breakage implies that nobody cares about building recent
releases on such platforms.
This whole series looks good to me. As I commented on in the thread you
referenced in 0/4 I think this is the right trade-off, and people like
me who occasionally compile git on older systems can just easily package
a newer curl as well if we need it.
My reading of the curl history/docs is that you should squash this into
this last patch. It's code that's now dead since we require
7.19.4.
CURLAUTH_DIGEST_IE was added in 7.19.3, and as a comment this squash
removes indicates CURLOPT_USE_SSL hasn't been needed since 7.16.4:
https://curl.haxx.se/libcurl/c/CURLOPT_USE_SSL.html
@@ -103,9 +103,7 @@ static int http_auth_methods_restricted;/* Modes for which empty_auth cannot actually help us. */staticunsignedlongempty_auth_useless=CURLAUTH_BASIC-#ifdef CURLAUTH_DIGEST_IE|CURLAUTH_DIGEST_IE-#endif|CURLAUTH_DIGEST;staticstructcurl_slist*pragma_header;
From: Jeff King <hidden> Date: 2017-08-09 13:38:51
On Wed, Aug 09, 2017 at 03:14:22PM +0200, Ævar Arnfjörð Bjarmason wrote:
This whole series looks good to me. As I commented on in the thread you
referenced in 0/4 I think this is the right trade-off, and people like
me who occasionally compile git on older systems can just easily package
a newer curl as well if we need it.
My reading of the curl history/docs is that you should squash this into
this last patch. It's code that's now dead since we require
7.19.4.
CURLAUTH_DIGEST_IE was added in 7.19.3, and as a comment this squash
removes indicates CURLOPT_USE_SSL hasn't been needed since 7.16.4:
https://curl.haxx.se/libcurl/c/CURLOPT_USE_SSL.html
Thanks. Do you mind formatting this as a patch on top instead of a
squash? I think it's sufficiently subtle that it should be separate from
the main cleanup, which is just dropping our own internal #ifdefs.
I guess that would make reverting harder, though.
-Peff
As the LIBCURL_VERSION_NUM check at the top of http.h shows we require
curl >= 7.19.4. This means we can remove previously added ifdef's
needed to support older curl versions.
The CURLAUTH_DIGEST_IE macro conditionally used since [1] was added in
7.19.3 (see CURLOPT_HTTPAUTH(3)).
The CURLOPT_USE_SSL macro used since [2] was added in 7.16.4 (see
CURLOPT_USE_SSL(3)).
1. 40a18fc77c ("http: add an "auto" mode for http.emptyauth",
2017-02-25)
2. 4bc444eb64 ("Support FTP-over-SSL/TLS for regular FTP", 2013-04-07)
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
On Wed, Aug 9, 2017 at 3:38 PM, Jeff King [off-list ref] wrote:
On Wed, Aug 09, 2017 at 03:14:22PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
This whole series looks good to me. As I commented on in the thread you
referenced in 0/4 I think this is the right trade-off, and people like
me who occasionally compile git on older systems can just easily package
a newer curl as well if we need it.
My reading of the curl history/docs is that you should squash this into
this last patch. It's code that's now dead since we require
7.19.4.
CURLAUTH_DIGEST_IE was added in 7.19.3, and as a comment this squash
removes indicates CURLOPT_USE_SSL hasn't been needed since 7.16.4:
https://curl.haxx.se/libcurl/c/CURLOPT_USE_SSL.html
Thanks. Do you mind formatting this as a patch on top instead of a
squash? I think it's sufficiently subtle that it should be separate from
the main cleanup, which is just dropping our own internal #ifdefs.
No problem. Here it is. Intended to be placed after your 4/4 since the
commit message references the new error message in http.h.
http.c | 4 ----
http.h | 9 ---------
2 files changed, 13 deletions(-)
@@ -103,9 +103,7 @@ static int http_auth_methods_restricted;/* Modes for which empty_auth cannot actually help us. */staticunsignedlongempty_auth_useless=CURLAUTH_BASIC-#ifdef CURLAUTH_DIGEST_IE|CURLAUTH_DIGEST_IE-#endif|CURLAUTH_DIGEST;staticstructcurl_slist*pragma_header;
From: Stefan Beller <hidden> Date: 2017-08-09 17:29:12
On Wed, Aug 9, 2017 at 5:01 AM, Jeff King [off-list ref] wrote:
As discussed in the previous commit, Git is not well-tested
with old versions of curl (and in fact since v2.12.0 does
not even compile with versions older than 7.19.4). Let's
stop pretending we support curl that old and drop any
now-obslete #ifdefs.
Choosing 7.16.0 is a somewhat arbitrary cutoff, but:
1. it came out in October of 2006, over 10 years ago.
Besides being a nice round number, it's a common
end-of-life support period, even for conservative
distributions.
2. that version introduced the curl_multi interface, which
gives us a lot of bang for the buck in removing #ifdefs
Signed-off-by: Jeff King <redacted>
---
Documentation/config.txt | 3 +--
http-push.c | 23 --------------------
http-walker.c | 12 -----------
http.c | 56 +-----------------------------------------------
http.h | 20 +----------------
remote-curl.c | 4 ----
6 files changed, 3 insertions(+), 115 deletions(-)
`git grep USE_CURL_MULTI` also yields
Documentation/config.txt
t/t5540-http-push-webdav.sh
Would these also need adaption in this patch?
From: Stefan Beller <hidden> Date: 2017-08-09 17:34:17
On Wed, Aug 9, 2017 at 5:02 AM, Jeff King [off-list ref] wrote:
Since v2.12.0, Git does not compile with versions of curl
older than 7.19.4. That version of curl is about 8 years
old. This means it may still be used in some distributions
with long-running support periods. But the fact that we
haven't received a single bug report about the compile-time
breakage implies that nobody cares about building recent
releases on such platforms.
As discussed in the previous two commits, this cleans up the
code and gives a more realistic signal to users about which
versions of Git are actually tested (in particular, this
moves us past the potential use-after-free issues with curl
older than 7.17.0).
This is a good reason for this patch, though, so maybe just elide
the "nobody cares" part?
Thanks for these cleanups!
From: Stefan Beller <hidden> Date: 2017-08-09 17:37:52
On Wed, Aug 9, 2017 at 5:02 AM, Jeff King [off-list ref] wrote:
quoted hunk
We already fail to build with versions of curl older than
7.19.4. But doing an explicit check with an #error has two
benefits.
One is that it makes it clear to users that the build
failure is intentional, so they don't waste time trying to
debug it.
And two is that it documents our current "too old"
assumption, so that we know whether we need use an #ifdef
when using newer curl features in future patches.
Signed-off-by: Jeff King <redacted>
---
http.h | 4 ++++
1 file changed, 4 insertions(+)
`git grep USE_CURL_MULTI` also yields
Documentation/config.txt
t/t5540-http-push-webdav.sh
Would these also need adaption in this patch?
That one threw me off for a minute. How does a test even know about
USE_CURL_MULTI?
But it is just a bad error message. :) You cannot fix it by compiling
with "make USE_CURL_MULTI=1". The right message is more like "skipping
http-push tests, your curl is too old".
But that does mean there's another problem: Makefile can drop its
conditional curl_check for http-push.o. I'll add that to a re-roll.
-Peff
From: Jeff King <hidden> Date: 2017-08-09 21:19:35
On Wed, Aug 09, 2017 at 10:34:09AM -0700, Stefan Beller wrote:
On Wed, Aug 9, 2017 at 5:02 AM, Jeff King [off-list ref] wrote:
quoted
Since v2.12.0, Git does not compile with versions of curl
older than 7.19.4. That version of curl is about 8 years
old. This means it may still be used in some distributions
with long-running support periods. But the fact that we
haven't received a single bug report about the compile-time
breakage implies that nobody cares about building recent
releases on such platforms.
Heh, I almost added "Or they are happy patching Git themselves". This
_does_ make patching Git harder for them, because now there are a lot
more spots to patch.
quoted
As discussed in the previous two commits, this cleans up the
code and gives a more realistic signal to users about which
versions of Git are actually tested (in particular, this
moves us past the potential use-after-free issues with curl
older than 7.17.0).
This is a good reason for this patch, though, so maybe just elide
the "nobody cares" part?
I think I'd rather elaborate than elide. One of the reasons to split
this into multiple patches is that it's a ready-made patch for a
distributor to apply (in reverse) if they really want to.
-Peff
As before, I would like to point out that people running with older cURL
are most likely not at liberty to change the system libraries.
I know that I didn't when I was working on a very expensive microscope
whose only certified control computer ran a very old version of CentOS,
and I really needed to install Git on it.
In such a case, it is often preferable to be able to build against an old
cURL -- even if some of the fancier features might be broken, and even if
some minor compile errors need to be fixed.
I know I was happy to compile Git against an ancient cURL back then.
Just so you understand where I come from when I would like to caution
against dropping support for older cURL unless it *really* adds an
*enormous* amount of maintenance burden.
I mean, if we even go out of our way to support the completely outdated
and obsolete .git/branches/ for what is likely a single user, it may not
be the worst to keep those couple of #ifdef guards to keep at least
nominal support for older cURLs?
Ciao,
Dscho
As before, I would like to point out that people running with older cURL
are most likely not at liberty to change the system libraries.
I know that I didn't when I was working on a very expensive microscope
whose only certified control computer ran a very old version of CentOS,
and I really needed to install Git on it.
In such a case, it is often preferable to be able to build against an old
cURL -- even if some of the fancier features might be broken, and even if
some minor compile errors need to be fixed.
I know I was happy to compile Git against an ancient cURL back then.
Just so you understand where I come from when I would like to caution
against dropping support for older cURL unless it *really* adds an
*enormous* amount of maintenance burden.
I mean, if we even go out of our way to support the completely outdated
and obsolete .git/branches/ for what is likely a single user, it may not
be the worst to keep those couple of #ifdef guards to keep at least
nominal support for older cURLs?
You've totally ignored the argument I made back then[1], and which I
reiterated in this thread. So I'll say it one more time: the more
compelling reason is not the #ifdefs, but the fact that the older
versions are totally untested. In fact, they do not even compile, and
yet I have not seen any patches to fix that.
So IMHO this is about being honest with users about which versions we
_actually_ support.
-Peff
[1] https://public-inbox.org/git/20170410182215.figy7hm4sogwipyz@sigill.intra.peff.net/
As before, I would like to point out that people running with older cURL
are most likely not at liberty to change the system libraries.
I know that I didn't when I was working on a very expensive microscope
whose only certified control computer ran a very old version of CentOS,
and I really needed to install Git on it.
In such a case, it is often preferable to be able to build against an old
cURL -- even if some of the fancier features might be broken, and even if
some minor compile errors need to be fixed.
I know I was happy to compile Git against an ancient cURL back then.
Just so you understand where I come from when I would like to caution
against dropping support for older cURL unless it *really* adds an
*enormous* amount of maintenance burden.
I mean, if we even go out of our way to support the completely outdated
and obsolete .git/branches/ for what is likely a single user, it may not
be the worst to keep those couple of #ifdef guards to keep at least
nominal support for older cURLs?
I too compile against ancient CentOS crap often where I need a newer
library and upgrading the system library is not an option, and the
problem you're describing is easily solved.
You grab the source RPM for e.g. curl, search-replace both the package
name and the installation paths to something else, e.g. name it
avar-curl and install it in /usr/local/avar-curl/{lib,bin,include}, then
make your new git package {Requires,BuildRequires}: avar-curl{,-dev}.
You then get a brand new curl on your system without touching anything
that needed the ancient system-library curl, because your new custom
curl lives under other paths, you then compile the package you actually
wanted against those.
Is it painless? No, of course it would be easier for me if I could just
"yum upgrade" and every package tested all 10 year old versions of their
dependencies, but it's often not realistic that they do that.
It usually takes no more than 10 minutes to give a package this
treatment, since I can usually grab a SRPM that already works for that
OS version, I just need to change the name & installation paths.
At $WORK we have hundreds of RPMs that have been given this treatment
for one reason or another.
Some of those are because upstream has decided to support the stuff
found on our systems. In some cases it's trivial to fix and they're
willing to take a patch, but in other cases it's reasonable of them to
say "just upgrade". I think looking at the diffstat of this series that
this is such a case, especially given Jeff's argument in
20170809214758.p77fqrwxanb4zn5a@sigill.intra.peff.net
As before, I would like to point out that people running with older cURL
are most likely not at liberty to change the system libraries.
I know that I didn't when I was working on a very expensive microscope
whose only certified control computer ran a very old version of CentOS,
and I really needed to install Git on it.
In such a case, it is often preferable to be able to build against an old
cURL -- even if some of the fancier features might be broken, and even if
some minor compile errors need to be fixed.
I know I was happy to compile Git against an ancient cURL back then.
Just so you understand where I come from when I would like to caution
against dropping support for older cURL unless it *really* adds an
*enormous* amount of maintenance burden.
I mean, if we even go out of our way to support the completely outdated
and obsolete .git/branches/ for what is likely a single user, it may not
be the worst to keep those couple of #ifdef guards to keep at least
nominal support for older cURLs?
You've totally ignored the argument I made back then[1], and which I
reiterated in this thread. So I'll say it one more time: the more
compelling reason is not the #ifdefs, but the fact that the older
versions are totally untested. In fact, they do not even compile, and
yet I have not seen any patches to fix that.
Let me re-quote from above:
quoted
In such a case, it is often preferable to be able to build against an
old cURL -- even if some of the fancier features might be broken, and
even if some minor compile errors need to be fixed.
As far as I remember, I *did* have to fix a minor compile error. Took
something like 15 minutes from first compile error to fully running test
suite.
Compare that effort to the effort of compiling a current cURL, possibly
having to compile newer c-ares, spdylay, jansson, nghttp2, openssl,
nettle, libunistring, libtasn1, libidn, libmetalink, rtmpdump and whatever
else.
So IMHO this is about being honest with users about which versions we
_actually_ support.
We will most likely never, ever have a fully 100% bug free system. That
does not mean that we should rip out everything that is not totally,
completely working.
Instead, we try [*1*] to welcome patches.
I can buy some argument like: this support is so invasive, so brittle, and
nobody takes care of it, and if it breaks, it is hard to fix, and those
who could, won't, so let's remove it.
That argument is why Git for Windows dropped XP support.
I cannot buy the argument: there are a dozen #ifdefs and I don't know
whether they still work. I don't know whether anybody (who most likely has
better things to do than read the Git mailing list) is still using those.
So let's just remove them.
That argument was what let us go overboard, and actually go too far by
removing the fallback when REG_STARTEND is missing, even while fixing a
very real bug. And that overzealous action hurt users. It also cost *us*
time, having to deal with the ensuing conversation, but we deserved to be
paying for this, the users didn't.
I did not have time to look closely over your patches to remove cURL
support for older versions. From a cursory look, I did not get the
impression that there is a lot of maintenance burden there, though.
Therefore, I currently believe that the downsides of removing the support
outweigh the benefits.
Mind, I agree that cURL should be upgrade to version 7.55.0 wherever
possible. But it is a huge mistake to assume that everybody who wants to
build, or just use, Git is at liberty to perform that upgrade in their
setup. To make that assumption is really harmful to users who are stuck in
a bad place out of no fault of their own. It is also not very nice.
Hopefully I had better luck expressing my concerns this time?
Ciao,
Dscho
Footnote *1*: It is no secret that I find our patch submission less than
inviting. Granted, *I* use it. *I* did not have problems entering the
mailing list. But then, my mails were not swallowed silently, because my
mail program does not send HTML by default. And prepared by the German
school system (I learned the term "sugar coating" only when exposed to
some US culture), I had little emotional problems with being criticized
and not thanked for my contribution, I persisted nevertheless. The opinion
that the Git contribution process is a lot less inviting than it could be
is not only my view, by the way. I hear this a lot. I give you that we are
not quite as textbook "keep out from here unless you look like us, smell
like us, talk like us, have the same genital setup like us" as the Linux
kernel mailing list, but we are in a different universe compared to, say,
the Drupal community. And their universe is a lot nicer to live in.
This seems to be an unintended behavioural change: the second condition
wouldn't have applied previously and overrides the first option
(equivalent to CURLOPT_POSTREDIR = CURL_REDIR_POST_301).
--
Mischa
This seems to be an unintended behavioural change: the second condition
wouldn't have applied previously and overrides the first option
(equivalent to CURLOPT_POSTREDIR = CURL_REDIR_POST_301).
Thanks, you're right. I'll fix it in my re-roll.
-Peff
From: Tom G. Christensen <hidden> Date: 2017-08-10 20:33:27
[I am resending this since the original does not seem to have made it to
the list, at least I cannot find it in any archives]
On 09/08/17 23:47, Jeff King wrote:
On Wed, Aug 09, 2017 at 11:42:12PM +0200, Johannes Schindelin wrote:
quoted
I mean, if we even go out of our way to support the completely outdated
and obsolete .git/branches/ for what is likely a single user, it may not
be the worst to keep those couple of #ifdef guards to keep at least
nominal support for older cURLs?
You've totally ignored the argument I made back then[1], and which I
reiterated in this thread. So I'll say it one more time: the more
compelling reason is not the #ifdefs, but the fact that the older
versions are totally untested.
Perhaps you forgot but I stated in the original thread that I build RPMS
for RHEL/CentOS 3, 4, 5, 6 and 7. I still do and I run the testsuite
every single time.
I currently have 2.13.3 up for el4, el5, el6 and el7.
Only el4 requires any patches, the rest will build out of the box with
the vendor supplied version of curl.
The plan was to drop the el4 builds for 2.14.0 to get rid of the patches.
In fact, they do not even compile, and
yet I have not seen any patches to fix that.
I just built a pristine 2.14.0 on CentOS 5 with curl 7.15.5. No problems
at all neither with building nor with running the testsuite.
So IMHO this is about being honest with users about which versions we
_actually_ support.
I have no problem with you wanting to drop support for older curl
releases (such as 7.15.5) but don't use the argument that it doesn't
currently build and nobody cares.
Also FWIW Red Hat continues to support RHEL 5 with the Extended
Life-cycle Support program until 2020-11-30.
-tgc
From: Tom G. Christensen <hidden> Date: 2017-08-10 21:05:39
On 09/08/17 23:47, Jeff King wrote:
On Wed, Aug 09, 2017 at 11:42:12PM +0200, Johannes Schindelin wrote:
quoted
I mean, if we even go out of our way to support the completely outdated
and obsolete .git/branches/ for what is likely a single user, it may not
be the worst to keep those couple of #ifdef guards to keep at least
nominal support for older cURLs?
You've totally ignored the argument I made back then[1], and which I
reiterated in this thread. So I'll say it one more time: the more
compelling reason is not the #ifdefs, but the fact that the older
versions are totally untested.
Perhaps you forgot but I stated in the original thread that I build RPMS
for RHEL/CentOS 3, 4, 5, 6 and 7. I still do and I run the testsuite
every single time.
I currently have 2.13.3 up for el4, el5, el6 and el7.
Only el4 requires any patches, the rest will build out of the box with
the vendor supplied version of curl.
The plan was to drop the el4 builds for 2.14.0 to get rid of the patches.
In fact, they do not even compile, and
yet I have not seen any patches to fix that.
I just built a pristine 2.14.0 on CentOS 5 with curl 7.15.5. No problems
at all neither with building nor with running the testsuite.
So IMHO this is about being honest with users about which versions we
_actually_ support.
I have no problem with you wanting to drop support for older curl
releases (such as 7.15.5) but don't use the argument that it doesn't
currently build and nobody cares.
Also FWIW Red Hat continues to support RHEL 5 with the Extended
Life-cycle Support program until 2020-11-30.
-tgc
From: Jeff King <hidden> Date: 2017-08-10 21:32:42
On Thu, Aug 10, 2017 at 10:33:18PM +0200, Tom G. Christensen wrote:
quoted
You've totally ignored the argument I made back then[1], and which I
reiterated in this thread. So I'll say it one more time: the more
compelling reason is not the #ifdefs, but the fact that the older
versions are totally untested.
Perhaps you forgot but I stated in the original thread that I build RPMS for
RHEL/CentOS 3, 4, 5, 6 and 7. I still do and I run the testsuite every
single time.
I didn't forget. I actually double-checked the patches you sent at the
time, but I didn't see one for the CURLPROTO issue. And indeed, it is
still broken for me:
$ cd /path/to/curl/repo
$ git checkout curl-7_15_5
$ ./buildconf && ./configure --prefix=/tmp/foo && make install
$ cd /path/to/git
$ git checkout v2.14.0
$ make CURLDIR=/tmp/foo V=1 http.o
gcc -o http.o -c -MF ./.depend/http.o.d -MQ http.o -MMD -MP -g -O0 -Wall -Werror -Wdeclaration-after-statement -Wpointer-arith -Wstrict-prototypes -Wvla -Wold-style-declaration -Wold-style-definition -Wno-error -Wno-cpp -Wno-unused-value -Wno-strict-prototypes -I. -DUSE_LIBPCRE1 -DHAVE_ALLOCA_H -I/tmp/foo/include -DUSE_CURL_FOR_IMAP_SEND -DNO_GETTEXT -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\"" -DSHA1DC_CUSTOM_TRAILING_INCLUDE_SHA1_C="\"sha1dc_git.c\"" -DSHA1DC_CUSTOM_TRAILING_INCLUDE_SHA1_H="\"sha1dc_git.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='"/bin/sh"' -DPAGER_ENV='"LESS=FRX LV=-c"' http.c
http.c: In function ‘get_curl_allowed_protocols’:
http.c:685:24: error: ‘CURLPROTO_HTTP’ undeclared (first use in this function); did you mean ‘CURLPROXY_HTTP’?
allowed_protocols |= CURLPROTO_HTTP;
^~~~~~~~~~~~~~
CURLPROXY_HTTP
[and so on]
I just built a pristine 2.14.0 on CentOS 5 with curl 7.15.5. No problems at
all neither with building nor with running the testsuite.
As you can see, this does not compile for me. What's going on?
I don't see how it could work, as CURLPROTO_HTTP is not defined at all
in that version of curl. Can you please double-check that you're
building against the correct version of curl, and that you are building
the HTTP parts of Git (which _are_ optional, and the test suite will
pass without them).
quoted
So IMHO this is about being honest with users about which versions we
_actually_ support.
I have no problem with you wanting to drop support for older curl releases
(such as 7.15.5) but don't use the argument that it doesn't currently build
and nobody cares.
My argument isn't quite that nobody cares. It's that we do users a
disservice by shipping a version of the code that very well may have
hidden problems like security holes (for instance, we do not handle
redirects safely in old versions of curl). So if you can get it to build
it may _seem_ fine, but it's a bit of a booby-trap waiting to spring.
I also won't claim any absolutes. I think we all agree this is a
cost/benefit tradeoff. But there are a lot of options for building on a
very old system. For instance, building without http if you don't need
it. Or building a more recent libcurl (and even linking statically for
simplicity).
I'd find arguments against the latter more compelling if recent curl
were hard to compile on old systems. I don't know whether that's the
case (certainly on a modern system, it's much easier to get newer
versions of curl to compile than older ones).
Also FWIW Red Hat continues to support RHEL 5 with the Extended Life-cycle
Support program until 2020-11-30.
I saw that, too. But as I understand it, they provide no code updates:
no bugfixes and no security updates. They just promise to answer the
phone and help you with troubleshooting. It's possible my perception is
wrong, though; I'm certainly not one of their customers.
-Peff
From: Tom G. Christensen <hidden> Date: 2017-08-10 22:23:52
On 10/08/17 23:32, Jeff King wrote:
On Thu, Aug 10, 2017 at 10:33:18PM +0200, Tom G. Christensen wrote:
quoted
quoted
You've totally ignored the argument I made back then[1], and which I
reiterated in this thread. So I'll say it one more time: the more
compelling reason is not the #ifdefs, but the fact that the older
versions are totally untested.
Perhaps you forgot but I stated in the original thread that I build RPMS for
RHEL/CentOS 3, 4, 5, 6 and 7. I still do and I run the testsuite every
single time.
I didn't forget. I actually double-checked the patches you sent at the
time, but I didn't see one for the CURLPROTO issue. And indeed, it is
still broken for me:
$ cd /path/to/curl/repo
$ git checkout curl-7_15_5
$ ./buildconf && ./configure --prefix=/tmp/foo && make install
$ cd /path/to/git
$ git checkout v2.14.0
$ make CURLDIR=/tmp/foo V=1 http.o
gcc -o http.o -c -MF ./.depend/http.o.d -MQ http.o -MMD -MP -g -O0 -Wall -Werror -Wdeclaration-after-statement -Wpointer-arith -Wstrict-prototypes -Wvla -Wold-style-declaration -Wold-style-definition -Wno-error -Wno-cpp -Wno-unused-value -Wno-strict-prototypes -I. -DUSE_LIBPCRE1 -DHAVE_ALLOCA_H -I/tmp/foo/include -DUSE_CURL_FOR_IMAP_SEND -DNO_GETTEXT -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\"" -DSHA1DC_CUSTOM_TRAILING_INCLUDE_SHA1_C="\"sha1dc_git.c\"" -DSHA1DC_CUSTOM_TRAILING_INCLUDE_SHA1_H="\"sha1dc_git.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='
"/bin/sh"' -DPAGER_ENV='"LESS=FRX LV=-c"' http.c
http.c: In function ‘get_curl_allowed_protocols’:
http.c:685:24: error: ‘CURLPROTO_HTTP’ undeclared (first use in this function); did you mean ‘CURLPROXY_HTTP’?
allowed_protocols |= CURLPROTO_HTTP;
^~~~~~~~~~~~~~
CURLPROXY_HTTP
[and so on]
quoted
I just built a pristine 2.14.0 on CentOS 5 with curl 7.15.5. No problems at
all neither with building nor with running the testsuite.
As you can see, this does not compile for me. What's going on?
The call site for get_curl_allowed_protocols() in http.c is still
protected by an #if:
#if LIBCURL_VERSION_NUM >= 0x071304
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
get_curl_allowed_protocols(0));
curl_easy_setopt(result, CURLOPT_PROTOCOLS,
get_curl_allowed_protocols(-1));
#else
warning("protocol restrictions not applied to curl redirects
because\n"
"your curl version is too old (>= 7.19.4)");
#endif
I don't see how it could work, as CURLPROTO_HTTP is not defined at all
in that version of curl.
Indeed but the #if will handle that.
Can you please double-check that you're
building against the correct version of curl, and that you are building
the HTTP parts of Git (which _are_ optional, and the test suite will
pass without them).
I use a mock buildroot and there is no other curl than the vendor
supplied 7.15.5 installed:
# pwd
/var/lib/mock/jrpms-el5-x86_64/root
# find . -name 'curlver.h'
./usr/include/curl/curlver.h
# grep LIBCURL_VERSION_NUM ./usr/include/curl/curlver.h
parsing and comparions by programs. The LIBCURL_VERSION_NUM define will
#define LIBCURL_VERSION_NUM 0x070f05
#
[root@c5-32bit-01 ~]# rpm -q git
git-2.14.1-1.el5.jr
[root@c5-32bit-01 ~]# ldd /usr/libexec/git-core/git-http-fetch |grep libcurl
libcurl.so.3 => /usr/lib/libcurl.so.3 (0x001e7000)
[root@c5-32bit-01 ~]# rpm -qf /usr/lib/libcurl.so.3
curl-7.15.5-17.el5_9
[root@c5-32bit-01 ~]# git --version
git version 2.14.1
[root@c5-32bit-01 ~]# git clone
https://github.com/tgc/tgcware-for-solaris.git
Cloning into 'tgcware-for-solaris'...
warning: protocol restrictions not applied to curl redirects because
your curl version is too old (>= 7.19.4)
remote: Counting objects: 2793, done.
remote: Total 2793 (delta 0), reused 0 (delta 0), pack-reused 2793
Receiving objects: 100% (2793/2793), 780.88 KiB | 639.00 KiB/s, done.
Resolving deltas: 100% (1233/1233), done.
[root@c5-32bit-01 ~]#
<snip>
I also won't claim any absolutes. I think we all agree this is a
cost/benefit tradeoff. But there are a lot of options for building on a
very old system. For instance, building without http if you don't need
it. Or building a more recent libcurl (and even linking statically for
simplicity).
Of course that is always an option but it does complicate things.
I'd find arguments against the latter more compelling if recent curl
were hard to compile on old systems. I don't know whether that's the
case (certainly on a modern system, it's much easier to get newer
versions of curl to compile than older ones).
I have no experience with building curl on older Linux systems. I know
that I can build it on old Solaris releases but that is not quite the
same since there I am also building against recent versions of curls
dependecies (openssl etc.).
quoted
Also FWIW Red Hat continues to support RHEL 5 with the Extended Life-cycle
Support program until 2020-11-30.
I saw that, too. But as I understand it, they provide no code updates:
no bugfixes and no security updates. They just promise to answer the
phone and help you with troubleshooting. It's possible my perception is
wrong, though; I'm certainly not one of their customers.
I am refering to the Extended Life-cycle Support product (ELS), which
promises:
"the ELS Add-On delivers certain critical-impact security fixes and
selected urgent priority bug fixes and troubleshooting for the last
minor release"
The full description is here:
https://access.redhat.com/support/policy/updates/errata#Extended_Life_Cycle_Phase
-tgc
From: Jeff King <hidden> Date: 2017-08-10 22:54:40
On Fri, Aug 11, 2017 at 12:23:42AM +0200, Tom G. Christensen wrote:
quoted
quoted
I just built a pristine 2.14.0 on CentOS 5 with curl 7.15.5. No problems at
all neither with building nor with running the testsuite.
As you can see, this does not compile for me. What's going on?
The call site for get_curl_allowed_protocols() in http.c is still protected
by an #if:
#if LIBCURL_VERSION_NUM >= 0x071304
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
get_curl_allowed_protocols(0));
curl_easy_setopt(result, CURLOPT_PROTOCOLS,
get_curl_allowed_protocols(-1));
#else
warning("protocol restrictions not applied to curl redirects
because\n"
"your curl version is too old (>= 7.19.4)");
#endif
quoted
I don't see how it could work, as CURLPROTO_HTTP is not defined at all
in that version of curl.
Indeed but the #if will handle that.
Er, sorry if I'm being dense, but how? Are you suggesting that by
removing the callsite of get_curl_allowed_protocols(), the compiler
might elide the now-dead code completely? I could certainly see it being
dropped after the compilation, but I'm surprised that it wouldn't
complain about the undeclared identifiers in the first place.
And if that _is_ what is happening...that seems like a very fragile and
unportable thing to be depending on.
quoted
Can you please double-check that you're
building against the correct version of curl, and that you are building
the HTTP parts of Git (which _are_ optional, and the test suite will
pass without them).
I use a mock buildroot and there is no other curl than the vendor supplied
7.15.5 installed:
[...]
OK, thanks for double-checking. I'm still puzzled why your build
succeeds and mine does not.
quoted
I saw that, too. But as I understand it, they provide no code updates:
no bugfixes and no security updates. They just promise to answer the
phone and help you with troubleshooting. It's possible my perception is
wrong, though; I'm certainly not one of their customers.
I am refering to the Extended Life-cycle Support product (ELS), which
promises:
"the ELS Add-On delivers certain critical-impact security fixes and selected
urgent priority bug fixes and troubleshooting for the last minor release"
The full description is here:
https://access.redhat.com/support/policy/updates/errata#Extended_Life_Cycle_Phase
That was the same page I was looking at. The bit I read was:
For versions of products in the Extended Life Phase, Red Hat will
provide limited ongoing technical support. No bug fixes, security
fixes, hardware enablement or root-cause analysis will be available
during this phase, and support will be provided on existing
installations only.
But I missed the bit about the "ELS add-on" below there, which I guess
is an extra thing. I do suspect that "install arbitrary new versions of
Git" is outside of their scope of "urgent priority bug fixes". But in a
sense it doesn't really matter. What is much more interesting is whether
there's a significant population that is running RHEL5 and has a strong
need for newer versions of Git. That I'm not sure about.
-Peff
From: Tom G. Christensen <hidden> Date: 2017-08-10 23:17:59
On 11/08/17 00:54, Jeff King wrote:
On Fri, Aug 11, 2017 at 12:23:42AM +0200, Tom G. Christensen wrote:
Er, sorry if I'm being dense, but how? Are you suggesting that by
removing the callsite of get_curl_allowed_protocols(), the compiler
might elide the now-dead code completely? I could certainly see it being
dropped after the compilation, but I'm surprised that it wouldn't
complain about the undeclared identifiers in the first place.
You're right, that should not be able to handle it.
quoted
quoted
Can you please double-check that you're
building against the correct version of curl, and that you are building
the HTTP parts of Git (which _are_ optional, and the test suite will
pass without them).
I use a mock buildroot and there is no other curl than the vendor supplied
7.15.5 installed:
[...]
OK, thanks for double-checking. I'm still puzzled why your build
succeeds and mine does not.
I know what's going on now and it's so simple.
Red Hats version of curl 7.15.5 includes a number of patches including
one that backports support for CURLPROTO_* (as part of a fix for
CVE-2009-0037).
I haven't checked el6 but I would not be surprised if there where
similar things going on there.
So in conclusion version based #ifdefs are misleading when used with
curl as shipped with RHEL.
-tgc
From: Jeff King <hidden> Date: 2017-08-10 23:23:21
On Fri, Aug 11, 2017 at 01:17:51AM +0200, Tom G. Christensen wrote:
quoted
OK, thanks for double-checking. I'm still puzzled why your build
succeeds and mine does not.
I know what's going on now and it's so simple.
Red Hats version of curl 7.15.5 includes a number of patches including one
that backports support for CURLPROTO_* (as part of a fix for CVE-2009-0037).
I haven't checked el6 but I would not be surprised if there where similar
things going on there.
el6 should have it already as part of 7.19.7, right?
So in conclusion version based #ifdefs are misleading when used with curl as
shipped with RHEL.
Yeah, that's certainly an interesting finding. In this case your builds
are missing out on redirect protection that we _could_ be providing.
If we do keep the compat ifdefs around this feature, it may be worth
converting them to "#ifdef CURLPROTO_HTTP" to more directly check the
feature.
-Peff
From: Tom G. Christensen <hidden> Date: 2017-08-10 23:36:17
On 11/08/17 01:23, Jeff King wrote:
On Fri, Aug 11, 2017 at 01:17:51AM +0200, Tom G. Christensen wrote:
quoted
quoted
OK, thanks for double-checking. I'm still puzzled why your build
succeeds and mine does not.
I know what's going on now and it's so simple.
Red Hats version of curl 7.15.5 includes a number of patches including one
that backports support for CURLPROTO_* (as part of a fix for CVE-2009-0037).
I haven't checked el6 but I would not be surprised if there where similar
things going on there.
el6 should have it already as part of 7.19.7, right?
Yes of course.
quoted
So in conclusion version based #ifdefs are misleading when used with curl as
shipped with RHEL.
Yeah, that's certainly an interesting finding. In this case your builds
are missing out on redirect protection that we _could_ be providing.
Yes and I'm looking into that right now.
If we do keep the compat ifdefs around this feature, it may be worth
converting them to "#ifdef CURLPROTO_HTTP" to more directly check the
feature.
From: Tom G. Christensen <hidden> Date: 2017-08-11 16:37:55
Turn the version check into a feature check to ensure this functionality
is also enabled with vendor supported curl versions where the feature
may have been backported.
Signed-off-by: Tom G. Christensen <redacted>
---
http.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Tom G. Christensen <hidden> Date: 2017-08-11 16:37:56
The curl packages provided by Red Hat for RHEL contain several
backports of features from later curl releases.
This causes problems with current version based checks in http.c.
Here is an overview of the features that have been backported:
7.10.6 (el3) Backports CURLPROTO_*
7.12.1 (el4) Backports CURLPROTO_*
7.15.5 (el5) Backports GSSAPI_DELEGATION_*
Backports CURLPROTO_*
7.19.7 (el6) Backports GSSAPI_DELEGATION_*
Backports CURL_SSL_VERSION_TLSv1_{0,1,2}
7.29.0 (el7) Backports CURL_SSL_VERSION_TLSv1_{0,1,2}
This patch series will update the current version based checks for
protocol restriction and GSSAPI delegation control support to ones
based on features to properly deal with the above listed backports.
The fine grained TLS version support does not seem to be
distinguishable via a preprocessor macro so I've left that alone.
I have build tested these changes against upstream curl 7.12.0 (fails),
7.12.1 and 7.15.5. I have also built and run the testsuite against the
Red Hat provided curl versions listed above.
Tom G. Christensen (2):
http: Fix handling of missing CURLPROTO_*
http: use a feature check to enable GSSAPI delegation control
http.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
--
2.14.1
From: Tom G. Christensen <hidden> Date: 2017-08-11 16:37:58
Commit aeae4db1 refactored the handling of the curl protocol restriction
support into a function but failed to add a version check for older
versions of curl that lack CURLPROTO_* support.
This adds the missing check and at the same time converts it to a feature
check instead of a version based check.
This is done to ensure that vendor supported curl versions that have had
CURLPROTO_* support backported are handled correctly.
Signed-off-by: Tom G. Christensen <redacted>
---
http.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
This series simplifies the http code by dropping support for curl
versions older than 7.19.4, released in March 2009.
This was last discussed on-list in 2017:
http://lore.kernel.org/git/20170809120024.7phdjzjv54uv5dpz@sigill.intra.peff.net
My reading of why it didn't get integrated at the time was:
- The original commit messages are opinionated about git not working
on these versions anyway, as noted in the original thread that's
only true of vanilla curl, but anyone impacted by these issues at
the time was probably using e.g. RHEL, which had backports that
confused the issue.
- While in 2017 these versions were already ancient, RHEL 5 (released
in 2007) was still seeing some notable production use.
It finally got "we really mean it now" EOL'd in late 2020 when
extended life-cycle support ended (see
https://access.redhat.com/support/policy/updates/errata). RHEL 6
does not have a libcurl affected by these changes.
- It ended with a patch to "error on too-old curl", i.e. to make
compiling on versions older than 7.19.4 an error. I've ejected that
per the discussion about backports confusing that issue.
This series is a re-roll of patches found in Peff's GitHub repo at
jk/no-ancient-curl, which were already-rebased versions of those
patches. His original on-list version had his Signed-off-by, but the
range-diff is against that branch, hence the addition of
Signed-off-by in the range-diff.
Peff's original 3/4 had a subtle bug in keeping the "CURLOPT_POST301"
branch of an ifdef/elif, spotted by Mischa POSLAWSKY, a fix for that
is squashed in here. See
https://lore.kernel.org/git/20170810123641.GG2363@shiar.net/
I then added a couple of patches on top, one is based on my comments
on the v1 http://lore.kernel.org/git/871sokhoi9.fsf@gmail.com,
i.e. the CURLAUTH_DIGEST_IE and CURLOPT_USE_SSL flags are also
version-based, and we can drop support for curls that don't have them.
I then renamed the ancient CURLOPT_FILE alias to
CURLOPT_WRITEDATA. Incidentally that's how I remembered to dig up this
series, i.e. I tried to search for "CURLOPT_FILE" in API documentation
while reading our HTTP code, but had a hard time finding it, turns out
we were using a very ancient synonym for the preferred name.
Jeff King (3):
http: drop support for curl < 7.11.1
http: drop support for curl < 7.16.0
http: drop support for curl < 7.19.4
Ævar Arnfjörð Bjarmason (2):
http: drop support for curl < 7.19.3 and < 7.16.4 (again)
http: rename CURLOPT_FILE to CURLOPT_WRITEDATA
http-push.c | 29 +--------
http-walker.c | 14 +----
http.c | 169 ++------------------------------------------------
http.h | 46 --------------
imap-send.c | 4 --
remote-curl.c | 11 +---
6 files changed, 10 insertions(+), 263 deletions(-)
Range-diff against v1:
1: 8793735cc2c ! 1: dcbb6f95652 http: drop support for curl < 7.11.1
@@ Metadata
## Commit message ##
http: drop support for curl < 7.11.1
- Recent versions of Git will not build with curl older than
- 7.11.1 due to (at least) two issues:
+ Drop support for this ancient version of curl and simplify the code by
+ allowing us get rid of some "#ifdef"'s.
+
+ Git will not build with vanilla curl older than 7.11.1 due to (at
+ least) two issues:
- our use of CURLOPT_POSTFIELDSIZE in 37ee680d9b
(http.postbuffer: allow full range of ssize_t values,
@@ Commit message
obvious benefit is that we'll have fewer conditional bits
cluttering the code.
- But more importantly, we're doing a disservice to users to
- pretend that Git works with old versions. It's clear that
- nobody is testing modern Git with such old versions of curl
- (we've had 3 released versions with the CURLPROTO issue
- without a report of anyone seeing the breakage in the wild).
- And there are a lot of subtle ways we could be getting this
- wrong (for instance, curl prior to 7.17.0 did not copy
- string arguments to curl_easy_setopt(), which means that
- using an old copy of curl could produce use-after-free
- bugs that are not present with more recent versions).
-
This patch drops all #ifdefs that reference older versions
(note that curl's preprocessor macros are in hex, so we're
looking for 070b01, not 071101).
+ Signed-off-by: Jeff King [off-list ref]
+ Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
+
## http.c ##
@@
static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
2: 15638cd1856 ! 2: 1c9f3bc031b http: drop support for curl < 7.16.0
@@ Metadata
## Commit message ##
http: drop support for curl < 7.16.0
- As discussed in the previous commit, Git is not well-tested
- with old versions of curl (and in fact since v2.12.0 does
- not even compile with versions older than 7.19.4). Let's
- stop pretending we support curl that old and drop any
- now-obslete #ifdefs.
+ In the last commit we dropped support for curl < 7.11.1, let's
+ continue that and drop support for versions older than 7.16.0. This
+ allows us to get rid of some now-obsolete #ifdefs.
- Choosing 7.16.0 is a somewhat arbitrary cutoff, but:
+ Choosing 7.16.0 is a somewhat arbitrary cutoff:
- 1. it came out in October of 2006, over 10 years ago.
- Besides being a nice round number, it's a common
- end-of-life support period, even for conservative
+ 1. It came out in October of 2006, almost 15 years ago.
+ Besides being a nice round number, around 10 years is
+ a common end-of-life support period, even for conservative
distributions.
- 2. that version introduced the curl_multi interface, which
+ 2. That version introduced the curl_multi interface, which
gives us a lot of bang for the buck in removing #ifdefs
+ RHEL 5 came with curl 7.15.5[1] (released in August 2006). RHEL 5's
+ extended life cycle program ended on 2020-11-30[1]. RHEL 6 comes with
+ curl 7.19.7 (released in November 2009), and RHEL 7 comes with
+ 7.29.0 (released in February 2013).
+
+ 1. http://lore.kernel.org/git/873e1f31-2a96-5b72-2f20-a5816cad1b51@jupiterrise.com
+
+ Signed-off-by: Jeff King [off-list ref]
+ Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
+
## http-push.c ##
@@ http-push.c: static void curl_setup_http(CURL *curl, const char *url,
curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
@@ http.h: void finish_all_active_slots(void);
void http_init(struct remote *remote, const char *url,
int proactive_auth);
+ ## imap-send.c ##
+@@ imap-send.c: static int curl_append_msgs_to_imap(struct imap_server_conf *server,
+ if (cred.username) {
+ if (res == CURLE_OK)
+ credential_approve(&cred);
+-#if LIBCURL_VERSION_NUM >= 0x070d01
+ else if (res == CURLE_LOGIN_DENIED)
+-#else
+- else
+-#endif
+ credential_reject(&cred);
+ }
+
+
## remote-curl.c ##
@@ remote-curl.c: static size_t rpc_out(void *ptr, size_t eltsize,
return avail;
3: 335046de7bc ! 3: faae88b7fec http: drop support for curl < 7.19.4
@@ Metadata
## Commit message ##
http: drop support for curl < 7.19.4
- Since v2.12.0, Git does not compile with versions of curl
- older than 7.19.4. That version of curl is about 8 years
- old. This means it may still be used in some distributions
- with long-running support periods. But the fact that we
- haven't received a single bug report about the compile-time
- breakage implies that nobody cares about building recent
- releases on such platforms.
+ In the last commit we dropped support for curl < 7.16.0, let's
+ continue that and drop support for versions older than 7.19.4. This
+ allows us to simplify the code by getting rid of some "#ifdef"'s.
- As discussed in the previous two commits, this cleans up the
- code and gives a more realistic signal to users about which
- versions of Git are actually tested (in particular, this
- moves us past the potential use-after-free issues with curl
- older than 7.17.0).
+ Git was broken with vanilla curl < 7.19.4 from v2.12.0 until
+ v2.15.0. Compiling with it was broken by using CURLPROTO_* outside any
+ "#ifdef" in aeae4db174 (http: create function to get curl allowed
+ protocols, 2016-12-14), and fixed in v2.15.0 in f18777ba6ef (http: fix
+ handling of missing CURLPROTO_*, 2017-08-11).
+
+ It's unclear how much anyone was impacted by that in practice, since
+ as noted in [1] RHEL versions using curl older than that still
+ compiled, because RedHat backported some features. Perhaps other
+ vendors did the same.
+
+ Still, it's one datapoint indicating that it wasn't in active use at
+ the time. That (the v2.12.0 release) was in Feb 24, 2017, with v2.15.0
+ on Oct 30, 2017, it's now mid-2021.
+
+ 1. http://lore.kernel.org/git/c8a2716d-76ac-735c-57f9-175ca3acbcb0@jupiterrise.com;
+ followed-up by f18777ba6ef (http: fix handling of missing CURLPROTO_*,
+ 2017-08-11)
+
+ Signed-off-by: Jeff King [off-list ref]
+ Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
## http.c ##
@@ http.c: static int min_curl_sessions = 1;
@@ http.c: static void var_override(const char **var, char *value)
}
static void init_curl_proxy_auth(CURL *result)
+@@ http.c: void setup_curl_trace(CURL *handle)
+ curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
+ }
+
+-#ifdef CURLPROTO_HTTP
+ static long get_curl_allowed_protocols(int from_user)
+ {
+ long allowed_protocols = 0;
+@@ http.c: static long get_curl_allowed_protocols(int from_user)
+
+ return allowed_protocols;
+ }
+-#endif
+
+ #if LIBCURL_VERSION_NUM >=0x072f00
+ static int get_curl_http_version_opt(const char *version_string, long *opt)
@@ http.c: static CURL *get_curl_handle(void)
}
@@ http.c: static CURL *get_curl_handle(void)
-#if LIBCURL_VERSION_NUM >= 0x071301
curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
-#elif LIBCURL_VERSION_NUM >= 0x071101
- curl_easy_setopt(result, CURLOPT_POST301, 1);
+- curl_easy_setopt(result, CURLOPT_POST301, 1);
-#endif
-#ifdef CURLPROTO_HTTP
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
4: e049f37357a < -: ----------- http: #error on too-old curl
-: ----------- > 4: 9a30e92520c http: drop support for curl < 7.19.3 and < 7.16.4 (again)
-: ----------- > 5: 64e510b4a6b http: rename CURLOPT_FILE to CURLOPT_WRITEDATA
--
2.32.0.874.ge7a9d58bfcf
From: Jeff King <redacted>
Drop support for this ancient version of curl and simplify the code by
allowing us get rid of some "#ifdef"'s.
Git will not build with vanilla curl older than 7.11.1 due to (at
least) two issues:
- our use of CURLOPT_POSTFIELDSIZE in 37ee680d9b
(http.postbuffer: allow full range of ssize_t values,
2017-04-11). This field was introduced in curl 7.11.1.
- our use of CURLPROTO_* outside any #ifdef in aeae4db174
(http: create function to get curl allowed protocols,
2016-12-14). These were introduced in curl 7.19.4.
We could solve these compilation problems with more #ifdefs,
but it's not worth the trouble. Version 7.11.1 came out in
March of 2004, over 13 years ago. Let's declare that too old
and drop any existing ifdefs that go further back. One
obvious benefit is that we'll have fewer conditional bits
cluttering the code.
This patch drops all #ifdefs that reference older versions
(note that curl's preprocessor macros are in hex, so we're
looking for 070b01, not 071101).
Signed-off-by: Jeff King <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 53 ---------------------------------------------------
http.h | 12 +-----------
remote-curl.c | 3 ---
3 files changed, 1 insertion(+), 67 deletions(-)
From: Jeff King <redacted>
In the last commit we dropped support for curl < 7.11.1, let's
continue that and drop support for versions older than 7.16.0. This
allows us to get rid of some now-obsolete #ifdefs.
Choosing 7.16.0 is a somewhat arbitrary cutoff:
1. It came out in October of 2006, almost 15 years ago.
Besides being a nice round number, around 10 years is
a common end-of-life support period, even for conservative
distributions.
2. That version introduced the curl_multi interface, which
gives us a lot of bang for the buck in removing #ifdefs
RHEL 5 came with curl 7.15.5[1] (released in August 2006). RHEL 5's
extended life cycle program ended on 2020-11-30[1]. RHEL 6 comes with
curl 7.19.7 (released in November 2009), and RHEL 7 comes with
7.29.0 (released in February 2013).
1. http://lore.kernel.org/git/873e1f31-2a96-5b72-2f20-a5816cad1b51@jupiterrise.com
Signed-off-by: Jeff King <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http-push.c | 23 ---------------------
http-walker.c | 12 -----------
http.c | 56 +--------------------------------------------------
http.h | 25 +----------------------
imap-send.c | 4 ----
remote-curl.c | 4 ----
6 files changed, 2 insertions(+), 122 deletions(-)
@@ -1682,21 +1671,15 @@ static int delete_remote_branch(const char *pattern, int force)staticvoidrun_request_queue(void){-#ifdef USE_CURL_MULTIis_running_queue=1;fill_active_slots();add_fill_function(NULL,fill_active_slot);-#endifdo{finish_all_active_slots();-#ifdef USE_CURL_MULTIfill_active_slots();-#endif}while(request_queue_head&&!aborted);-#ifdef USE_CURL_MULTIis_running_queue=0;-#endif}intcmd_main(intargc,constchar**argv)
@@ -1770,10 +1753,6 @@ int cmd_main(int argc, const char **argv)break;}-#ifndef USE_CURL_MULTI-die("git-push is not available for http/https repository when not compiled with USE_CURL_MULTI");-#endif-if(!repo->url)usage(http_push_usage);
@@ -1786,9 +1765,7 @@ int cmd_main(int argc, const char **argv)http_init(NULL,repo->url,1);-#ifdef USE_CURL_MULTIis_running_queue=0;-#endif/* Verify DAV compliance/lock support */if(!locking_available()){
@@ -1250,7 +1218,6 @@ struct active_request_slot *get_active_slot(void)structactive_request_slot*slot=active_queue_head;structactive_request_slot*newslot;-#ifdef USE_CURL_MULTIintnum_transfers;/* Wait for a slot to open up if the queue is full */
From: Jeff King <redacted>
In the last commit we dropped support for curl < 7.16.0, let's
continue that and drop support for versions older than 7.19.4. This
allows us to simplify the code by getting rid of some "#ifdef"'s.
Git was broken with vanilla curl < 7.19.4 from v2.12.0 until
v2.15.0. Compiling with it was broken by using CURLPROTO_* outside any
"#ifdef" in aeae4db174 (http: create function to get curl allowed
protocols, 2016-12-14), and fixed in v2.15.0 in f18777ba6ef (http: fix
handling of missing CURLPROTO_*, 2017-08-11).
It's unclear how much anyone was impacted by that in practice, since
as noted in [1] RHEL versions using curl older than that still
compiled, because RedHat backported some features. Perhaps other
vendors did the same.
Still, it's one datapoint indicating that it wasn't in active use at
the time. That (the v2.12.0 release) was in Feb 24, 2017, with v2.15.0
on Oct 30, 2017, it's now mid-2021.
1. http://lore.kernel.org/git/c8a2716d-76ac-735c-57f9-175ca3acbcb0@jupiterrise.com;
followed-up by f18777ba6ef (http: fix handling of missing CURLPROTO_*,
2017-08-11)
Signed-off-by: Jeff King <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 50 --------------------------------------------------
http.h | 4 ----
2 files changed, 54 deletions(-)
Remove the conditional use of CURLAUTH_DIGEST_IE and
CURLOPT_USE_SSL. These two have been split from earlier simpler checks
against LIBCURL_VERSION_NUM for ease of review.
The CURLAUTH_DIGEST_IE flag was added in n 7.19.3[1], and
CURLOPT_USE_SSL in 7.16.4[2], as noted in [2] it was then renamed from
the older CURLOPT_FTP_SSL.
1. https://curl.se/libcurl/c/CURLOPT_HTTPAUTH.html
2. https://curl.se/libcurl/c/CURLOPT_USE_SSL.html
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 4 ----
http.h | 9 ---------
2 files changed, 13 deletions(-)
@@ -120,9 +120,7 @@ static int http_auth_methods_restricted;/* Modes for which empty_auth cannot actually help us. */staticunsignedlongempty_auth_useless=CURLAUTH_BASIC-#ifdef CURLAUTH_DIGEST_IE|CURLAUTH_DIGEST_IE-#endif|CURLAUTH_DIGEST;staticstructcurl_slist*pragma_header;
The CURLOPT_FILE name is an alias for CURLOPT_WRITEDATA, the
CURLOPT_WRITEDATA name has been preferred since curl 7.9.7, released
in May 2002[1].
1. https://curl.se/libcurl/c/CURLOPT_WRITEDATA.html
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http-push.c | 6 +++---
http-walker.c | 2 +-
http.c | 6 +++---
remote-curl.c | 4 ++--
4 files changed, 9 insertions(+), 9 deletions(-)
@@ -1016,7 +1016,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_recerpc_in_data.slot=slot;rpc_in_data.check_pktline=stateless_connect;memset(&rpc_in_data.pktline_state,0,sizeof(rpc_in_data.pktline_state));-curl_easy_setopt(slot->curl,CURLOPT_FILE,&rpc_in_data);+curl_easy_setopt(slot->curl,CURLOPT_WRITEDATA,&rpc_in_data);curl_easy_setopt(slot->curl,CURLOPT_FAILONERROR,0);
From: brian m. carlson <hidden> Date: 2021-07-21 22:56:42
On 2021-07-21 at 22:22:11, Ævar Arnfjörð Bjarmason wrote:
This series simplifies the http code by dropping support for curl
versions older than 7.19.4, released in March 2009.
This was last discussed on-list in 2017:
http://lore.kernel.org/git/20170809120024.7phdjzjv54uv5dpz@sigill.intra.peff.net
My reading of why it didn't get integrated at the time was:
- The original commit messages are opinionated about git not working
on these versions anyway, as noted in the original thread that's
only true of vanilla curl, but anyone impacted by these issues at
the time was probably using e.g. RHEL, which had backports that
confused the issue.
- While in 2017 these versions were already ancient, RHEL 5 (released
in 2007) was still seeing some notable production use.
It finally got "we really mean it now" EOL'd in late 2020 when
extended life-cycle support ended (see
https://access.redhat.com/support/policy/updates/errata). RHEL 6
does not have a libcurl affected by these changes.
- It ended with a patch to "error on too-old curl", i.e. to make
compiling on versions older than 7.19.4 an error. I've ejected that
per the discussion about backports confusing that issue.
I'm in favor of this series. I'm actually in favor of dropping support
for RHEL 6 as well, since there is nobody providing public security
support for it, and therefore nobody but people paying Red Hat (that is,
not this project) can be expected to safely run it. I also think ten
years is about the reasonable maximum lifetime of software.
So, with or without those changes, this seems like a good approach to
me.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
This series simplifies the http code by dropping support for curl
versions older than 7.19.4, released in March 2009.
But INSTALL says:
- "libcurl" library is used by git-http-fetch, git-fetch, and, if
the curl version >= 7.34.0, for git-imap-send. You might also
want the "curl" executable for debugging purposes. If you do not
use http:// or https:// repositories, and do not want to put
patches into an IMAP mailbox, you do not have to have them
(use NO_CURL).
I think it's worth mentioning minimal required curl version (7.19.4) there.
--
An old man doll... just what I always wanted! - Clara
[[PGP Signed Part:Undecided]]
On 2021-07-21 at 22:22:11, Ævar Arnfjörð Bjarmason wrote:
quoted
This series simplifies the http code by dropping support for curl
versions older than 7.19.4, released in March 2009.
This was last discussed on-list in 2017:
http://lore.kernel.org/git/20170809120024.7phdjzjv54uv5dpz@sigill.intra.peff.net
My reading of why it didn't get integrated at the time was:
- The original commit messages are opinionated about git not working
on these versions anyway, as noted in the original thread that's
only true of vanilla curl, but anyone impacted by these issues at
the time was probably using e.g. RHEL, which had backports that
confused the issue.
- While in 2017 these versions were already ancient, RHEL 5 (released
in 2007) was still seeing some notable production use.
It finally got "we really mean it now" EOL'd in late 2020 when
extended life-cycle support ended (see
https://access.redhat.com/support/policy/updates/errata). RHEL 6
does not have a libcurl affected by these changes.
- It ended with a patch to "error on too-old curl", i.e. to make
compiling on versions older than 7.19.4 an error. I've ejected that
per the discussion about backports confusing that issue.
I'm in favor of this series. I'm actually in favor of dropping support
for RHEL 6 as well, since there is nobody providing public security
support for it, and therefore nobody but people paying Red Hat (that is,
not this project) can be expected to safely run it. I also think ten
years is about the reasonable maximum lifetime of software.
So, with or without those changes, this seems like a good approach to
me.
I'll clarify this along with other fixes in a re-roll, but I think our
policy shouldn't have anything to do with upstream promises of support,
but merely the trade-off of how easy it is for us to support old
software & how likely it is that people use it in practice along with
git.
So as an example we still say we support Perl 5.8, which is ridiculously
ancient as far as any notion of upstream security support goes (and as
an aside, does have real DoS issues exposed by e.g. the gitweb we ship).
But while we could probably bump that to something more modern nowadays
in practice we're not a mostly-Perl project, so I haven't found it to be
worth it to bump it when working on the relevant code.
I'm only using RHEL 5 as a shorthand for a system that's usually the
most ancient thing people want to build new gits with in practice.
It's just not the case that you can't run RHEL 5 or even RHEL 4 "safely"
even today. Upstream has just abandoned it, but that doesn't mean users
in the wild have. There's also CentOS, not everyone cares about IBM
corporate support policies.
E.g. in practice at a past-job I've had to build git using system
libcurl in a mixed environment which (and I forget the details) included
mostly today's equivalent of RHEL 8 and 7, but there was some system
using RHEL 5 in a closet somewhere still using puppet automation.
Why? Because (and I forget the details, but this example will do)
because it needed to operate some proprietary dongle requiring a RHEL 5
kernel driver that its vendor had since abandoned.
There were plans to move away from it, but that was maybe 1-2 years away
at the time. Meanwhile I had to build a git across the fleet, and it
would be a hassle to need to ship my own libcurl just because this
project wanted to have paternalistic version dependency policies.
I mean, if it's a matter of supporting that version being painful then
fair enough. I had some comments in the 2017 thread (or something linked
from it) about needing to package your dependencies not being *that* big
a deal.
Hence this series, I think on balance the improvement in maintainability
of the http code makes it worth it.
But let's not justify it with a user not being able to run such software
securely, in my example those ancient boxes were externally firewalled,
and in any case any practical security issues were probably with some
vendor's admin interface on them, not whatever ancient kernel they had.
On the other hand there's surely people who are running RHEL 5 today who
are running insecure setup, but let's not make it our job to force them
to move by virtue of being overly annoying about dependency version
requirements.
We should have the view that git's critical infrastructure and we should
be wary of breaking things. It would also just be counter-productive,
the result would probably be that the ancient box wouldn't get an
upgraded git, and would still have preventable CVE's in git itself
present (e.g. the gitmodules RCE).
From: brian m. carlson <hidden> Date: 2021-07-22 22:56:48
On 2021-07-22 at 07:09:59, Ævar Arnfjörð Bjarmason wrote:
I'll clarify this along with other fixes in a re-roll, but I think our
policy shouldn't have anything to do with upstream promises of support,
but merely the trade-off of how easy it is for us to support old
software & how likely it is that people use it in practice along with
git.
I don't think I agree. We should try to support major operating systems
well provided we can adequately be expected to test on them, and that
means that they should have publicly available security support. In
other words, a developer on the relevant operating system should be able
to test on that OS without paying ongoing money for the privilege of doing
so securely.
Once an operating system is no longer supported security-wise, we should
no longer support it, either, since we can't be expected to test or
develop on it securely. Nobody could responsibly run such an image on
a CI system or test with it on an Internet-connected computer, so we
should no longer consider it worthy of our support.
So as an example we still say we support Perl 5.8, which is ridiculously
ancient as far as any notion of upstream security support goes (and as
an aside, does have real DoS issues exposed by e.g. the gitweb we ship).
But while we could probably bump that to something more modern nowadays
in practice we're not a mostly-Perl project, so I haven't found it to be
worth it to bump it when working on the relevant code.
I've actually argued in favor of bumping the version to 5.14 a long time
ago. I can send a patch for that. It has a bunch of nice new features
we could take advantage of.
I'm only using RHEL 5 as a shorthand for a system that's usually the
most ancient thing people want to build new gits with in practice.
It's just not the case that you can't run RHEL 5 or even RHEL 4 "safely"
even today. Upstream has just abandoned it, but that doesn't mean users
in the wild have. There's also CentOS, not everyone cares about IBM
corporate support policies.
Yes, and CentOS has dropped support earlier than Red Hat has.
Just because users want to run new versions of Git on systems that
should long ago have been abandoned[0] does not mean we should take the
burden of maintaining that code for them. Since they have the source
code, they can build and maintain Git on those old systems and apply
any necessary patches. If this becomes burdensome, then perhaps the
cost of maintaining the system will be an incentive to replace it with a
secure system.
I am unconvinced that we should make it easier for people to run
insecure operating systems because they pose a hazard to the Internet
when connected to it. Just because it is behind some firewall doesn't
mean that it cannot be compromised, and once it is, it can then become
a source of spam and abuse. This is not an idle thought experiment; it
does practically happen with great frequency on the Internet today. An
unsupported system might be acceptable if it has no network connectivity
at all, but then it would not need a newer version of Git.
It is not that I have not experienced such load-bearing obsolete systems
before: I have, and I have done my best to support them. But I've also
been happy to be clear to management and/or customers about what that
means in terms of costs and that we were taking a real, substantial
risk, and been clear what the consequences were. In no situation,
however, did I try to convince outside parties that my obsolete OS was
deserving of someone else's maintenance burden or argue that the system
should not be replaced as soon as possible.
We should have the view that git's critical infrastructure and we should
be wary of breaking things. It would also just be counter-productive,
the result would probably be that the ancient box wouldn't get an
upgraded git, and would still have preventable CVE's in git itself
present (e.g. the gitmodules RCE).
Considering that the machine already has multiple CVEs, probably
including root code execution vulnerabilities, I'm not sure how much
worse we could make it. It's already trivial to compromise with or
without a newer version of Git.
[0] I should point out that ten years of support is already extremely
generous.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[[PGP Signed Part:Undecided]]
On 2021-07-22 at 07:09:59, Ævar Arnfjörð Bjarmason wrote:
quoted
I'll clarify this along with other fixes in a re-roll, but I think our
policy shouldn't have anything to do with upstream promises of support,
but merely the trade-off of how easy it is for us to support old
software & how likely it is that people use it in practice along with
git.
I don't think I agree. We should try to support major operating systems
well provided we can adequately be expected to test on them, and that
means that they should have publicly available security support. In
other words, a developer on the relevant operating system should be able
to test on that OS without paying ongoing money for the privilege of doing
so securely.
Doesn't drawing that line in the sand for Linux distributions by
implication leave out support for Windows, OSX and any other proprietary
system? You need to pay for security and other updates for those from
day one.
Once an operating system is no longer supported security-wise, we should
no longer support it, either, since we can't be expected to test or
develop on it securely. Nobody could responsibly run such an image on
a CI system or test with it on an Internet-connected computer, so we
should no longer consider it worthy of our support.
Yes, I do think we disagree. I just think we should focus narrowly on
whether it's a hassle for us to support older libcurl, whether some
version of it is packaged with an old OS that's known to be in wide use
or not is ultimately just a useful heuristic.
quoted
So as an example we still say we support Perl 5.8, which is ridiculously
ancient as far as any notion of upstream security support goes (and as
an aside, does have real DoS issues exposed by e.g. the gitweb we ship).
But while we could probably bump that to something more modern nowadays
in practice we're not a mostly-Perl project, so I haven't found it to be
worth it to bump it when working on the relevant code.
I've actually argued in favor of bumping the version to 5.14 a long time
ago. I can send a patch for that. It has a bunch of nice new features
we could take advantage of.
Sure, I'm not opposed. Just noting the in-tree nicer features for us
v.s. more aggressive versioning policy for packagers and users (not that
Perl 5.14 is aggressive).
quoted
I'm only using RHEL 5 as a shorthand for a system that's usually the
most ancient thing people want to build new gits with in practice.
It's just not the case that you can't run RHEL 5 or even RHEL 4 "safely"
even today. Upstream has just abandoned it, but that doesn't mean users
in the wild have. There's also CentOS, not everyone cares about IBM
corporate support policies.
Yes, and CentOS has dropped support earlier than Red Hat has.
Just because users want to run new versions of Git on systems that
should long ago have been abandoned[0] does not mean we should take the
burden of maintaining that code for them. Since they have the source
code, they can build and maintain Git on those old systems and apply
any necessary patches. If this becomes burdensome, then perhaps the
cost of maintaining the system will be an incentive to replace it with a
secure system.
I am unconvinced that we should make it easier for people to run
insecure operating systems because they pose a hazard to the Internet
when connected to it. Just because it is behind some firewall doesn't
mean that it cannot be compromised, and once it is, it can then become
a source of spam and abuse. This is not an idle thought experiment; it
does practically happen with great frequency on the Internet today. An
unsupported system might be acceptable if it has no network connectivity
at all, but then it would not need a newer version of Git.
Aren't you assuming that any network connectivity is equal to
connectivity to the open internet?
In any case, I think the notion that we should make git slightly more
painful to use on these systems as a distant proxy variable to forcing
OS upgrades is several levels away from where I think we should be
drawing the line, which is closer to "is it painful in-tree?" and "is
someone sending us patches to make it work?" etc.
From: Jeff King <hidden> Date: 2021-07-23 10:16:21
On Thu, Jul 22, 2021 at 12:22:11AM +0200, Ævar Arnfjörð Bjarmason wrote:
This series is a re-roll of patches found in Peff's GitHub repo at
jk/no-ancient-curl, which were already-rebased versions of those
patches. His original on-list version had his Signed-off-by, but the
range-diff is against that branch, hence the addition of
Signed-off-by in the range-diff.
Heh, OK. It's a little surprising to see random junk pulled out of my
GitHub repo, but in this case I was holding onto them with the intent of
eventually resending after more time passed.
So I'm happy to see these cleaned up and posted. I think what's on that
branch should be good-ish, in the sense that I've been rebasing it
forward as part of my daily routine, and it's part of the build that I
use day-to-day. Though apparently I never applied the CURLOPT_POST301
fix. :-/
I know my S-o-b was on the originals to the list, but just to make
clear: I am fine with using them on the rebased versions you grabbed.
I then added a couple of patches on top, one is based on my comments
on the v1 http://lore.kernel.org/git/871sokhoi9.fsf@gmail.com,
i.e. the CURLAUTH_DIGEST_IE and CURLOPT_USE_SSL flags are also
version-based, and we can drop support for curls that don't have them.
Seems reasonable.
I then renamed the ancient CURLOPT_FILE alias to
CURLOPT_WRITEDATA. Incidentally that's how I remembered to dig up this
series, i.e. I tried to search for "CURLOPT_FILE" in API documentation
while reading our HTTP code, but had a hard time finding it, turns out
we were using a very ancient synonym for the preferred name.
Jeff King (3):
http: drop support for curl < 7.11.1
http: drop support for curl < 7.16.0
http: drop support for curl < 7.19.4
Ævar Arnfjörð Bjarmason (2):
http: drop support for curl < 7.19.3 and < 7.16.4 (again)
http: rename CURLOPT_FILE to CURLOPT_WRITEDATA
So modulo the commit message tweaks that Junio suggested, this all looks
fine. I actually think my original "#error on too-old curl" is still
reasonable. Yes, people whose distro has backported all of these
features could possibly still use it. But in that case they likely know
what's going on and can rip out the #error. It seems much more likely
to me that it _won't_ work, and they'll get confused by obscure errors
when they try to use an old curl.
But I don't feel too stronlgy about it either way.
-Peff
This should address the feedback on v2, except that I did not update
the INSTALL document as Bagas Sanjaya suggested. We did not have
existing discussion of the generally supported minimal support minimal
version there, that would make sense as a follow-up patch.
There's minor commit message changes here, and two new patches, one is
another missed ifdef we could remove because the version comparion was
wrong, and the last commit entirely gets rid of LIBCURL_VERSION_NUM
checks in the codebase in favor of checking specific defined macros.
Jeff King (3):
http: drop support for curl < 7.11.1
http: drop support for curl < 7.16.0
http: drop support for curl < 7.19.4
Ævar Arnfjörð Bjarmason (4):
http: drop support for curl < 7.19.3 and <= 7.16.4 (or <7.17.0)
(again)
http: drop support for curl < 7.18.0 (again)
http: rename CURLOPT_FILE to CURLOPT_WRITEDATA
http: centralize the accounting of libcurl dependencies
git-curl-compat.h | 119 ++++++++++++++++++++++++++
http-push.c | 29 +------
http-walker.c | 14 +--
http.c | 212 ++++++----------------------------------------
http.h | 46 ----------
imap-send.c | 10 +--
remote-curl.c | 11 +--
7 files changed, 153 insertions(+), 288 deletions(-)
create mode 100644 git-curl-compat.h
Range-diff against v2:
1: dcbb6f95652 ! 1: 6bd41764a54 http: drop support for curl < 7.11.1
@@ Commit message
Drop support for this ancient version of curl and simplify the code by
allowing us get rid of some "#ifdef"'s.
- Git will not build with vanilla curl older than 7.11.1 due to (at
- least) two issues:
-
- - our use of CURLOPT_POSTFIELDSIZE in 37ee680d9b
- (http.postbuffer: allow full range of ssize_t values,
- 2017-04-11). This field was introduced in curl 7.11.1.
-
- - our use of CURLPROTO_* outside any #ifdef in aeae4db174
- (http: create function to get curl allowed protocols,
- 2016-12-14). These were introduced in curl 7.19.4.
+ Git will not build with vanilla curl older than 7.11.1 due our use of
+ CURLOPT_POSTFIELDSIZE in 37ee680d9b
+ (http.postbuffer: allow full range of ssize_t values,
+ 2017-04-11). This field was introduced in curl 7.11.1.
We could solve these compilation problems with more #ifdefs,
but it's not worth the trouble. Version 7.11.1 came out in
- March of 2004, over 13 years ago. Let's declare that too old
+ March of 2004, over 17 years ago. Let's declare that too old
and drop any existing ifdefs that go further back. One
obvious benefit is that we'll have fewer conditional bits
cluttering the code.
2: 1c9f3bc031b = 2: fb308258e2b http: drop support for curl < 7.16.0
3: faae88b7fec = 3: 9fcd3a3e486 http: drop support for curl < 7.19.4
4: 9a30e92520c ! 4: 230b968382f http: drop support for curl < 7.19.3 and < 7.16.4 (again)
@@ Metadata
Author: Ævar Arnfjörð Bjarmason [off-list ref]
## Commit message ##
- http: drop support for curl < 7.19.3 and < 7.16.4 (again)
+ http: drop support for curl < 7.19.3 and <= 7.16.4 (or <7.17.0) (again)
Remove the conditional use of CURLAUTH_DIGEST_IE and
CURLOPT_USE_SSL. These two have been split from earlier simpler checks
against LIBCURL_VERSION_NUM for ease of review.
- The CURLAUTH_DIGEST_IE flag was added in n 7.19.3[1], and
- CURLOPT_USE_SSL in 7.16.4[2], as noted in [2] it was then renamed from
- the older CURLOPT_FTP_SSL.
+ The CURLAUTH_DIGEST_IE flag was added in 7.19.3[1], and
+ CURLOPT_USE_SSL in 7.16.4[2] or 7.17.0[3], depending on the source. As
+ noted in [2] it was then renamed around that time from the older
+ CURLOPT_FTP_SSL.
1. https://curl.se/libcurl/c/CURLOPT_HTTPAUTH.html
2. https://curl.se/libcurl/c/CURLOPT_USE_SSL.html
+ 3. https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
-: ----------- > 5: b857a9ef7b1 http: drop support for curl < 7.18.0 (again)
5: 64e510b4a6b = 6: 95e42b17ce8 http: rename CURLOPT_FILE to CURLOPT_WRITEDATA
-: ----------- > 7: 93a2775d0ee http: centralize the accounting of libcurl dependencies
--
2.32.0.1069.g516d52f3d85
From: Jeff King <redacted>
Drop support for this ancient version of curl and simplify the code by
allowing us get rid of some "#ifdef"'s.
Git will not build with vanilla curl older than 7.11.1 due our use of
CURLOPT_POSTFIELDSIZE in 37ee680d9b
(http.postbuffer: allow full range of ssize_t values,
2017-04-11). This field was introduced in curl 7.11.1.
We could solve these compilation problems with more #ifdefs,
but it's not worth the trouble. Version 7.11.1 came out in
March of 2004, over 17 years ago. Let's declare that too old
and drop any existing ifdefs that go further back. One
obvious benefit is that we'll have fewer conditional bits
cluttering the code.
This patch drops all #ifdefs that reference older versions
(note that curl's preprocessor macros are in hex, so we're
looking for 070b01, not 071101).
Signed-off-by: Jeff King <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 53 ---------------------------------------------------
http.h | 12 +-----------
remote-curl.c | 3 ---
3 files changed, 1 insertion(+), 67 deletions(-)
From: Jeff King <redacted>
In the last commit we dropped support for curl < 7.11.1, let's
continue that and drop support for versions older than 7.16.0. This
allows us to get rid of some now-obsolete #ifdefs.
Choosing 7.16.0 is a somewhat arbitrary cutoff:
1. It came out in October of 2006, almost 15 years ago.
Besides being a nice round number, around 10 years is
a common end-of-life support period, even for conservative
distributions.
2. That version introduced the curl_multi interface, which
gives us a lot of bang for the buck in removing #ifdefs
RHEL 5 came with curl 7.15.5[1] (released in August 2006). RHEL 5's
extended life cycle program ended on 2020-11-30[1]. RHEL 6 comes with
curl 7.19.7 (released in November 2009), and RHEL 7 comes with
7.29.0 (released in February 2013).
1. http://lore.kernel.org/git/873e1f31-2a96-5b72-2f20-a5816cad1b51@jupiterrise.com
Signed-off-by: Jeff King <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http-push.c | 23 ---------------------
http-walker.c | 12 -----------
http.c | 56 +--------------------------------------------------
http.h | 25 +----------------------
imap-send.c | 4 ----
remote-curl.c | 4 ----
6 files changed, 2 insertions(+), 122 deletions(-)
@@ -1682,21 +1671,15 @@ static int delete_remote_branch(const char *pattern, int force)staticvoidrun_request_queue(void){-#ifdef USE_CURL_MULTIis_running_queue=1;fill_active_slots();add_fill_function(NULL,fill_active_slot);-#endifdo{finish_all_active_slots();-#ifdef USE_CURL_MULTIfill_active_slots();-#endif}while(request_queue_head&&!aborted);-#ifdef USE_CURL_MULTIis_running_queue=0;-#endif}intcmd_main(intargc,constchar**argv)
@@ -1770,10 +1753,6 @@ int cmd_main(int argc, const char **argv)break;}-#ifndef USE_CURL_MULTI-die("git-push is not available for http/https repository when not compiled with USE_CURL_MULTI");-#endif-if(!repo->url)usage(http_push_usage);
@@ -1786,9 +1765,7 @@ int cmd_main(int argc, const char **argv)http_init(NULL,repo->url,1);-#ifdef USE_CURL_MULTIis_running_queue=0;-#endif/* Verify DAV compliance/lock support */if(!locking_available()){
@@ -1250,7 +1218,6 @@ struct active_request_slot *get_active_slot(void)structactive_request_slot*slot=active_queue_head;structactive_request_slot*newslot;-#ifdef USE_CURL_MULTIintnum_transfers;/* Wait for a slot to open up if the queue is full */
From: Jeff King <redacted>
In the last commit we dropped support for curl < 7.16.0, let's
continue that and drop support for versions older than 7.19.4. This
allows us to simplify the code by getting rid of some "#ifdef"'s.
Git was broken with vanilla curl < 7.19.4 from v2.12.0 until
v2.15.0. Compiling with it was broken by using CURLPROTO_* outside any
"#ifdef" in aeae4db174 (http: create function to get curl allowed
protocols, 2016-12-14), and fixed in v2.15.0 in f18777ba6ef (http: fix
handling of missing CURLPROTO_*, 2017-08-11).
It's unclear how much anyone was impacted by that in practice, since
as noted in [1] RHEL versions using curl older than that still
compiled, because RedHat backported some features. Perhaps other
vendors did the same.
Still, it's one datapoint indicating that it wasn't in active use at
the time. That (the v2.12.0 release) was in Feb 24, 2017, with v2.15.0
on Oct 30, 2017, it's now mid-2021.
1. http://lore.kernel.org/git/c8a2716d-76ac-735c-57f9-175ca3acbcb0@jupiterrise.com;
followed-up by f18777ba6ef (http: fix handling of missing CURLPROTO_*,
2017-08-11)
Signed-off-by: Jeff King <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 50 --------------------------------------------------
http.h | 4 ----
2 files changed, 54 deletions(-)
In a preceding commit we dropped support for curl < 7.19.4, so we can
drop support for this non-obvious dependency on curl < 7.18.0.
It's non-obvious because in curl's hex version notation 0x071800 is
version 7.24.0, *not* 7.18.0, so at a glance this patch looks
incorrect.
But it's correct, because the existing version check being removed
here is wrong. The check guards use of the following curl defines:
CURLPROXY_SOCKS4 7.10
CURLPROXY_SOCKS4A 7.18.0
CURLPROXY_SOCKS5 7.10
CURLPROXY_SOCKS5_HOSTNAME 7.18.0
I.e. the oldest version that has these is in fact 7.18.0, not
7.24.0. That we were checking 7.24.0 is just an mistake in
6d7afe07f29 (remote-http(s): support SOCKS proxies, 2015-10-26),
i.e. its author confusing base 10 and base 16.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 2 --
1 file changed, 2 deletions(-)
Remove the conditional use of CURLAUTH_DIGEST_IE and
CURLOPT_USE_SSL. These two have been split from earlier simpler checks
against LIBCURL_VERSION_NUM for ease of review.
The CURLAUTH_DIGEST_IE flag was added in 7.19.3[1], and
CURLOPT_USE_SSL in 7.16.4[2] or 7.17.0[3], depending on the source. As
noted in [2] it was then renamed around that time from the older
CURLOPT_FTP_SSL.
1. https://curl.se/libcurl/c/CURLOPT_HTTPAUTH.html
2. https://curl.se/libcurl/c/CURLOPT_USE_SSL.html
3. https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 4 ----
http.h | 9 ---------
2 files changed, 13 deletions(-)
@@ -120,9 +120,7 @@ static int http_auth_methods_restricted;/* Modes for which empty_auth cannot actually help us. */staticunsignedlongempty_auth_useless=CURLAUTH_BASIC-#ifdef CURLAUTH_DIGEST_IE|CURLAUTH_DIGEST_IE-#endif|CURLAUTH_DIGEST;staticstructcurl_slist*pragma_header;
The CURLOPT_FILE name is an alias for CURLOPT_WRITEDATA, the
CURLOPT_WRITEDATA name has been preferred since curl 7.9.7, released
in May 2002[1].
1. https://curl.se/libcurl/c/CURLOPT_WRITEDATA.html
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http-push.c | 6 +++---
http-walker.c | 2 +-
http.c | 6 +++---
remote-curl.c | 4 ++--
4 files changed, 9 insertions(+), 9 deletions(-)
@@ -1016,7 +1016,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_recerpc_in_data.slot=slot;rpc_in_data.check_pktline=stateless_connect;memset(&rpc_in_data.pktline_state,0,sizeof(rpc_in_data.pktline_state));-curl_easy_setopt(slot->curl,CURLOPT_FILE,&rpc_in_data);+curl_easy_setopt(slot->curl,CURLOPT_WRITEDATA,&rpc_in_data);curl_easy_setopt(slot->curl,CURLOPT_FAILONERROR,0);
As noted in preceding commits 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 we had at
least one case of someone confusing base 10 for base 16 in comparing
the version.
Let's add a git-curl-compat.h header that'll keep track of these
dependencies, and make the next round of deprecations easier. This
patch is mostly obvious, with the following exceptions:
- In aeff8a61216 (http: implement public key pinning, 2016-02-15) a
dependency and warning() was added if curl older than 7.44.0 was
used, but the relevant code depended on CURLOPT_PINNEDPUBLICKEY,
introduced in 7.39.0.
- We did not use CURL_SOCKOPT_OK and instead hardcoded its value in
a15d069a198 (http: enable keepalive on TCP sockets, 2013-10-12),
let's use it and guard its use by a new GITCURL_HAVE_* check.
- We guarded the support for http.version with a check for curl
7.47.0, looking at the history of how that happen reveals that an
earlier on-list version of d73019feb44 (http: add support selecting
http version, 2018-11-08) depended on CURL_HTTP_VERSION_2TLS (which
was added in 7.47.0).
But the version this code depended on was in fact 7.43.0, since
that's when CURL_HTTP_VERSION_2_0 was added.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
git-curl-compat.h | 119 ++++++++++++++++++++++++++++++++++++++++++++++
http.c | 41 ++++++++--------
imap-send.c | 6 +--
3 files changed, 143 insertions(+), 23 deletions(-)
create mode 100644 git-curl-compat.h
@@ -536,7 +533,11 @@ static int sockopt_callback(void *client, curl_socket_t fd, curlsocktype type)if(rc<0)warning_errno("unable to set SO_KEEPALIVE on socket");-return0;/* CURL_SOCKOPT_OK only exists since curl 7.21.5 */+#ifdef GITCURL_HAVE_CURL_SOCKOPT_OK+returnCURL_SOCKOPT_OK;+#else+return0;+#endif}staticvoidset_curl_keepalive(CURL*c)
@@ -1441,14 +1441,14 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)curl_easy_setopt(curl,CURLOPT_PORT,server.port);if(server.auth_method){-#if LIBCURL_VERSION_NUM < 0x072200-warning("No LOGIN_OPTIONS support in this cURL version");-#else+#ifdef GIT_CURL_HAVE_CURLOPT_LOGIN_OPTIONSstructstrbufauth=STRBUF_INIT;strbuf_addstr(&auth,"AUTH=");strbuf_addstr(&auth,server.auth_method);curl_easy_setopt(curl,CURLOPT_LOGIN_OPTIONS,auth.buf);strbuf_release(&auth);+#else+warning("No LOGIN_OPTIONS support in this cURL version");#endif}
Per the feature creep feedback on v3 this v4 ejects the two new
changes new in v3. The below range-diff is against v2, not v3.
I dug into the 7.16.4 v.s. 7.17.0 documentation issue and found that
it's bug in curl's docs, for which I submitted a patch.
I considered keeping
[off-list ref], but
sequencing it in made the range diff quite a bit larger, so per the
feature creep feedback I ejected it too. Junio: Perhaps you'd like to
cherry-pick it on top too, or it can be dug up post-release.
Jeff King (3):
http: drop support for curl < 7.11.1
http: drop support for curl < 7.16.0
http: drop support for curl < 7.19.4
Ævar Arnfjörð Bjarmason (2):
http: drop support for curl < 7.19.3 and < 7.17.0 (again)
http: rename CURLOPT_FILE to CURLOPT_WRITEDATA
http-push.c | 29 +--------
http-walker.c | 14 +----
http.c | 169 ++------------------------------------------------
http.h | 46 --------------
imap-send.c | 4 --
remote-curl.c | 11 +---
6 files changed, 10 insertions(+), 263 deletions(-)
Range-diff against v1:
1: dcbb6f95652 ! 1: 6bd41764a54 http: drop support for curl < 7.11.1
@@ Commit message
Drop support for this ancient version of curl and simplify the code by
allowing us get rid of some "#ifdef"'s.
- Git will not build with vanilla curl older than 7.11.1 due to (at
- least) two issues:
-
- - our use of CURLOPT_POSTFIELDSIZE in 37ee680d9b
- (http.postbuffer: allow full range of ssize_t values,
- 2017-04-11). This field was introduced in curl 7.11.1.
-
- - our use of CURLPROTO_* outside any #ifdef in aeae4db174
- (http: create function to get curl allowed protocols,
- 2016-12-14). These were introduced in curl 7.19.4.
+ Git will not build with vanilla curl older than 7.11.1 due our use of
+ CURLOPT_POSTFIELDSIZE in 37ee680d9b
+ (http.postbuffer: allow full range of ssize_t values,
+ 2017-04-11). This field was introduced in curl 7.11.1.
We could solve these compilation problems with more #ifdefs,
but it's not worth the trouble. Version 7.11.1 came out in
- March of 2004, over 13 years ago. Let's declare that too old
+ March of 2004, over 17 years ago. Let's declare that too old
and drop any existing ifdefs that go further back. One
obvious benefit is that we'll have fewer conditional bits
cluttering the code.
2: 1c9f3bc031b = 2: fb308258e2b http: drop support for curl < 7.16.0
3: faae88b7fec ! 3: fba5560a3ba http: drop support for curl < 7.19.4
@@ Commit message
http: drop support for curl < 7.19.4
In the last commit we dropped support for curl < 7.16.0, let's
- continue that and drop support for versions older than 7.19.4. This
+ continue that and drop support for versions older than 7.19.3. This
allows us to simplify the code by getting rid of some "#ifdef"'s.
Git was broken with vanilla curl < 7.19.4 from v2.12.0 until
4: 9a30e92520c ! 4: 42d1c72ff7e http: drop support for curl < 7.19.3 and < 7.16.4 (again)
@@ Metadata
Author: Ævar Arnfjörð Bjarmason [off-list ref]
## Commit message ##
- http: drop support for curl < 7.19.3 and < 7.16.4 (again)
+ http: drop support for curl < 7.19.3 and < 7.17.0 (again)
Remove the conditional use of CURLAUTH_DIGEST_IE and
CURLOPT_USE_SSL. These two have been split from earlier simpler checks
against LIBCURL_VERSION_NUM for ease of review.
The CURLAUTH_DIGEST_IE flag was added in n 7.19.3[1], and
- CURLOPT_USE_SSL in 7.16.4[2], as noted in [2] it was then renamed from
- the older CURLOPT_FTP_SSL.
+ CURLOPT_USE_SSL in 7.17.0[2][3], as noted in [2] it was then renamed
+ from the older CURLOPT_FTP_SSL.
+
+ The documentation[2] currently claims that it was introduced in
+ 7.16.4, but the symbols-in-versions file correctly states
+ 7.17.0[3].
+
+ I've submitted an upstream
+ patch ([off-list ref]) to the
+ curl-library mailing list fix the documentation.
1. https://curl.se/libcurl/c/CURLOPT_HTTPAUTH.html
2. https://curl.se/libcurl/c/CURLOPT_USE_SSL.html
+ 3. https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
5: 64e510b4a6b = 5: e34ab1d1f65 http: rename CURLOPT_FILE to CURLOPT_WRITEDATA
--
2.32.0.1071.g36f34456314
From: Jeff King <redacted>
Drop support for this ancient version of curl and simplify the code by
allowing us get rid of some "#ifdef"'s.
Git will not build with vanilla curl older than 7.11.1 due our use of
CURLOPT_POSTFIELDSIZE in 37ee680d9b
(http.postbuffer: allow full range of ssize_t values,
2017-04-11). This field was introduced in curl 7.11.1.
We could solve these compilation problems with more #ifdefs,
but it's not worth the trouble. Version 7.11.1 came out in
March of 2004, over 17 years ago. Let's declare that too old
and drop any existing ifdefs that go further back. One
obvious benefit is that we'll have fewer conditional bits
cluttering the code.
This patch drops all #ifdefs that reference older versions
(note that curl's preprocessor macros are in hex, so we're
looking for 070b01, not 071101).
Signed-off-by: Jeff King <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 53 ---------------------------------------------------
http.h | 12 +-----------
remote-curl.c | 3 ---
3 files changed, 1 insertion(+), 67 deletions(-)
From: Jeff King <redacted>
In the last commit we dropped support for curl < 7.11.1, let's
continue that and drop support for versions older than 7.16.0. This
allows us to get rid of some now-obsolete #ifdefs.
Choosing 7.16.0 is a somewhat arbitrary cutoff:
1. It came out in October of 2006, almost 15 years ago.
Besides being a nice round number, around 10 years is
a common end-of-life support period, even for conservative
distributions.
2. That version introduced the curl_multi interface, which
gives us a lot of bang for the buck in removing #ifdefs
RHEL 5 came with curl 7.15.5[1] (released in August 2006). RHEL 5's
extended life cycle program ended on 2020-11-30[1]. RHEL 6 comes with
curl 7.19.7 (released in November 2009), and RHEL 7 comes with
7.29.0 (released in February 2013).
1. http://lore.kernel.org/git/873e1f31-2a96-5b72-2f20-a5816cad1b51@jupiterrise.com
Signed-off-by: Jeff King <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http-push.c | 23 ---------------------
http-walker.c | 12 -----------
http.c | 56 +--------------------------------------------------
http.h | 25 +----------------------
imap-send.c | 4 ----
remote-curl.c | 4 ----
6 files changed, 2 insertions(+), 122 deletions(-)
@@ -1682,21 +1671,15 @@ static int delete_remote_branch(const char *pattern, int force)staticvoidrun_request_queue(void){-#ifdef USE_CURL_MULTIis_running_queue=1;fill_active_slots();add_fill_function(NULL,fill_active_slot);-#endifdo{finish_all_active_slots();-#ifdef USE_CURL_MULTIfill_active_slots();-#endif}while(request_queue_head&&!aborted);-#ifdef USE_CURL_MULTIis_running_queue=0;-#endif}intcmd_main(intargc,constchar**argv)
@@ -1770,10 +1753,6 @@ int cmd_main(int argc, const char **argv)break;}-#ifndef USE_CURL_MULTI-die("git-push is not available for http/https repository when not compiled with USE_CURL_MULTI");-#endif-if(!repo->url)usage(http_push_usage);
@@ -1786,9 +1765,7 @@ int cmd_main(int argc, const char **argv)http_init(NULL,repo->url,1);-#ifdef USE_CURL_MULTIis_running_queue=0;-#endif/* Verify DAV compliance/lock support */if(!locking_available()){
@@ -1250,7 +1218,6 @@ struct active_request_slot *get_active_slot(void)structactive_request_slot*slot=active_queue_head;structactive_request_slot*newslot;-#ifdef USE_CURL_MULTIintnum_transfers;/* Wait for a slot to open up if the queue is full */
From: Jeff King <redacted>
In the last commit we dropped support for curl < 7.16.0, let's
continue that and drop support for versions older than 7.19.3. This
allows us to simplify the code by getting rid of some "#ifdef"'s.
Git was broken with vanilla curl < 7.19.4 from v2.12.0 until
v2.15.0. Compiling with it was broken by using CURLPROTO_* outside any
"#ifdef" in aeae4db174 (http: create function to get curl allowed
protocols, 2016-12-14), and fixed in v2.15.0 in f18777ba6ef (http: fix
handling of missing CURLPROTO_*, 2017-08-11).
It's unclear how much anyone was impacted by that in practice, since
as noted in [1] RHEL versions using curl older than that still
compiled, because RedHat backported some features. Perhaps other
vendors did the same.
Still, it's one datapoint indicating that it wasn't in active use at
the time. That (the v2.12.0 release) was in Feb 24, 2017, with v2.15.0
on Oct 30, 2017, it's now mid-2021.
1. http://lore.kernel.org/git/c8a2716d-76ac-735c-57f9-175ca3acbcb0@jupiterrise.com;
followed-up by f18777ba6ef (http: fix handling of missing CURLPROTO_*,
2017-08-11)
Signed-off-by: Jeff King <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 50 --------------------------------------------------
http.h | 4 ----
2 files changed, 54 deletions(-)
Remove the conditional use of CURLAUTH_DIGEST_IE and
CURLOPT_USE_SSL. These two have been split from earlier simpler checks
against LIBCURL_VERSION_NUM for ease of review.
The CURLAUTH_DIGEST_IE flag was added in n 7.19.3[1], and
CURLOPT_USE_SSL in 7.17.0[2][3], as noted in [2] it was then renamed
from the older CURLOPT_FTP_SSL.
The documentation[2] currently claims that it was introduced in
7.16.4, but the symbols-in-versions file correctly states
7.17.0[3].
I've submitted an upstream
patch ([off-list ref]) to the
curl-library mailing list fix the documentation.
1. https://curl.se/libcurl/c/CURLOPT_HTTPAUTH.html
2. https://curl.se/libcurl/c/CURLOPT_USE_SSL.html
3. https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 4 ----
http.h | 9 ---------
2 files changed, 13 deletions(-)
@@ -120,9 +120,7 @@ static int http_auth_methods_restricted;/* Modes for which empty_auth cannot actually help us. */staticunsignedlongempty_auth_useless=CURLAUTH_BASIC-#ifdef CURLAUTH_DIGEST_IE|CURLAUTH_DIGEST_IE-#endif|CURLAUTH_DIGEST;staticstructcurl_slist*pragma_header;
The CURLOPT_FILE name is an alias for CURLOPT_WRITEDATA, the
CURLOPT_WRITEDATA name has been preferred since curl 7.9.7, released
in May 2002[1].
1. https://curl.se/libcurl/c/CURLOPT_WRITEDATA.html
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http-push.c | 6 +++---
http-walker.c | 2 +-
http.c | 6 +++---
remote-curl.c | 4 ++--
4 files changed, 9 insertions(+), 9 deletions(-)
@@ -1016,7 +1016,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_recerpc_in_data.slot=slot;rpc_in_data.check_pktline=stateless_connect;memset(&rpc_in_data.pktline_state,0,sizeof(rpc_in_data.pktline_state));-curl_easy_setopt(slot->curl,CURLOPT_FILE,&rpc_in_data);+curl_easy_setopt(slot->curl,CURLOPT_WRITEDATA,&rpc_in_data);curl_easy_setopt(slot->curl,CURLOPT_FAILONERROR,0);
This is a follow-up to the already-integrated topic for dropping
support for older curl versions submitted before the v2.33 release[1].
No verions become unsupported in this topic, this is a mere clean-up.
There were a couple of cases where we either already dropped support
for something, but got the curl version wrong in a macro comparison,
or just got the version wrong. That's now corrected.
But more importantly this introduces a git-curl-compat.h header which
centarlizes all our curl version dependencies, and any compatability
shims we've got to deal with those versions. This makes the code much
easier to read and maintain than inline LIBCURL_VERSION_NUM version
comparisons, and will make any future dropping of supported curl
versions in the future easier to perform & review.
1. https://lore.kernel.org/git/cover-v4-0.5-00000000000-20210730T175650Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (5):
http: drop support for curl < 7.18.0 (again)
http: correct curl version check for CURLOPT_PINNEDPUBLICKEY
http: correct version check for CURL_HTTP_VERSION_2_0
http: centralize the accounting of libcurl dependencies
http: don't hardcode the value of CURL_SOCKOPT_OK
git-curl-compat.h | 123 ++++++++++++++++++++++++++++++++++++++++++++++
http.c | 39 +++++++--------
imap-send.c | 2 +-
3 files changed, 141 insertions(+), 23 deletions(-)
create mode 100644 git-curl-compat.h
--
2.33.0.825.g2bf60429931
In 644de29e220 (http: drop support for curl < 7.19.4, 2021-07-30) we
dropped support for curl < 7.19.4, so we can drop support for this
non-obvious dependency on curl < 7.18.0.
It's non-obvious because in curl's hex version notation 0x071800 is
version 7.24.0, *not* 7.18.0, so at a glance this patch looks
incorrect.
But it's correct, because the existing version check being removed
here is wrong. The check guards use of the following curl defines:
CURLPROXY_SOCKS4 7.10
CURLPROXY_SOCKS4A 7.18.0
CURLPROXY_SOCKS5 7.10
CURLPROXY_SOCKS5_HOSTNAME 7.18.0
I.e. the oldest version that has these is in fact 7.18.0, not
7.24.0. That we were checking 7.24.0 is just an mistake in
6d7afe07f29 (remote-http(s): support SOCKS proxies, 2015-10-26),
i.e. its author confusing base 10 and base 16.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 2 --
1 file changed, 2 deletions(-)
In aeff8a61216 (http: implement public key pinning, 2016-02-15) a
dependency and warning() was added if curl older than 7.44.0 was used,
but the relevant code depended on CURLOPT_PINNEDPUBLICKEY, introduced
in 7.39.0.
Let's also remove the macro check before we declare the ssl_pinnedkey
variable, the pattern for other such variables is to declare the
static variable unconditionally, we just may not use it on older
versions. This reduces macro verbosity.
The reduction in verbosity comes at the small cost of issuing a
warning about the unused variable if this code is compiled with curl
versions older than 7.39.0. I think that's an acceptable trade-off,
anyone compiling a new git with a 2014-era toolchain likely has at
least other warning that'll have prompted them not to use -Werror, and
if not maybe this'll prompt them to compile their new git with a more
modern libcurl.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
In d73019feb44 (http: add support selecting http version, 2018-11-08)
a dependency was added on CURL_HTTP_VERSION_2_0, but this feature was
introduced in curl version 7.43.0, not 7.47.0, as the incorrect
version check led us to believe.
As looking through the history of that commit on the mailing list will
reveal[1], the reason for this is that an earlier version of it
depended on CURL_HTTP_VERSION_2TLS, which was introduced in libcurl
7.47.0.
But the version that made it in in d73019feb44 had dropped the
dependency on CURL_HTTP_VERSION_2TLS, but the corresponding version
check was not corrected.
1. https://lore.kernel.org/git/pull.69.git.gitgitgadget@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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.
Let's instead add a new git-curl-compat.h header that'll keep track of
these dependencies. Following this pattern will also make it much
easier to track when we should deprecate curl versions in the future,
as we just did post-v2.33 e48a623dea0 (Merge branch
'ab/http-drop-old-curl', 2021-08-24).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
git-curl-compat.h | 112 ++++++++++++++++++++++++++++++++++++++++++++++
http.c | 31 +++++++------
imap-send.c | 2 +-
3 files changed, 128 insertions(+), 17 deletions(-)
create mode 100644 git-curl-compat.h
Use the new git-curl-compat.h header to define CURL_SOCKOPT_OK to its
known value if we're on an older curl version that doesn't have it. It
was hardcoded in http.c in a15d069a198 (http: enable keepalive on TCP
sockets, 2013-10-12).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
git-curl-compat.h | 11 +++++++++++
http.c | 2 +-
2 files changed, 12 insertions(+), 1 deletion(-)
@@ -533,7 +533,7 @@ static int sockopt_callback(void *client, curl_socket_t fd, curlsocktype type)if(rc<0)warning_errno("unable to set SO_KEEPALIVE on socket");-return0;/* CURL_SOCKOPT_OK only exists since curl 7.21.5 */+returnCURL_SOCKOPT_OK;}staticvoidset_curl_keepalive(CURL*c)
From: Jeff King <hidden> Date: 2021-09-08 19:22:33
On Wed, Sep 08, 2021 at 05:31:53PM +0200, Ævar Arnfjörð Bjarmason wrote:
In aeff8a61216 (http: implement public key pinning, 2016-02-15) a
dependency and warning() was added if curl older than 7.44.0 was used,
but the relevant code depended on CURLOPT_PINNEDPUBLICKEY, introduced
in 7.39.0.
According to the manpage for CURLOPT_PINNEDPUBLICKEY, it looks like
support for various formats and implementations was phased in. In
particular, 7.44.0 picked up sha256 support (I guess for a fingerprint?
I've never used this feature) for most major implementations.
But in terms of compiling, all we care about is that the constant is
there. So I think the cutoff point you found is what we want. Presumably
when the file format isn't supported we'd get some error, though it's
not clear if that would come during the actual curl_*_perform(), or if
we should be checking the curl_easy_setopt() result.
Let's also remove the macro check before we declare the ssl_pinnedkey
variable, the pattern for other such variables is to declare the
static variable unconditionally, we just may not use it on older
versions. This reduces macro verbosity.
The reduction in verbosity comes at the small cost of issuing a
warning about the unused variable if this code is compiled with curl
versions older than 7.39.0. I think that's an acceptable trade-off,
anyone compiling a new git with a 2014-era toolchain likely has at
least other warning that'll have prompted them not to use -Werror, and
if not maybe this'll prompt them to compile their new git with a more
modern libcurl.
OK. That's a bit of a departure from how we've handled variables before,
but it does make the code a bit cleaner. And I am fine with the attitude
of "if you are using ancient tools, you may see some extra warnings". We
already know this is true for older compilers, and it's not worth caring
too much about.
-Peff
From: Jeff King <hidden> Date: 2021-09-08 19:27:19
On Wed, Sep 08, 2021 at 05:31:54PM +0200, Ævar Arnfjörð Bjarmason wrote:
In d73019feb44 (http: add support selecting http version, 2018-11-08)
a dependency was added on CURL_HTTP_VERSION_2_0, but this feature was
introduced in curl version 7.43.0, not 7.47.0, as the incorrect
version check led us to believe.
As looking through the history of that commit on the mailing list will
reveal[1], the reason for this is that an earlier version of it
depended on CURL_HTTP_VERSION_2TLS, which was introduced in libcurl
7.47.0.
But the version that made it in in d73019feb44 had dropped the
dependency on CURL_HTTP_VERSION_2TLS, but the corresponding version
check was not corrected.
According to the curl manpage, if we use CURL_HTTP_VERSION_2_0 instead,
it goes all the way to 7.33.0. I don't have opinion on whether that's
worth covering or not.
-Peff
From: Jeff King <hidden> Date: 2021-09-08 19:31:14
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
From: Jeff King <hidden> Date: 2021-09-08 19:32:15
On Wed, Sep 08, 2021 at 05:31:51PM +0200, Ævar Arnfjörð Bjarmason wrote:
This is a follow-up to the already-integrated topic for dropping
support for older curl versions submitted before the v2.33 release[1].
No verions become unsupported in this topic, this is a mere clean-up.
There were a couple of cases where we either already dropped support
for something, but got the curl version wrong in a macro comparison,
or just got the version wrong. That's now corrected.
But more importantly this introduces a git-curl-compat.h header which
centarlizes all our curl version dependencies, and any compatability
shims we've got to deal with those versions. This makes the code much
easier to read and maintain than inline LIBCURL_VERSION_NUM version
comparisons, and will make any future dropping of supported curl
versions in the future easier to perform & review.
These all seem fine to me. I left a few comments that are mostly
thinking out loud, not really asking for any changes.
-Peff
In 1d53f90ed97 (The "curl" executable is no longer required,
2008-06-15) the wording for requiring curl(1) was changed to the
current "you might also want...".
Mentioning the "curl" executable at all is just confusing, someone
building git might want to use it to debug things, but they might also
just use wget(1) or some other http client. The "curl" executable has
the advantage that you might be able to e.g. reproduce a bug in git's
usage of libcurl with it, but anyone going to those extents is
unlikely to be aided by this note in INSTALL.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
INSTALL | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
@@ -139,11 +139,11 @@ Issues of note: (PPC_SHA1). - "libcurl" library is used by git-http-fetch, git-fetch, and, if- the curl version >= 7.34.0, for git-imap-send. You might also- want the "curl" executable for debugging purposes. If you do not- use http:// or https:// repositories, and do not want to put- patches into an IMAP mailbox, you do not have to have them- (use NO_CURL).+ the curl version >= 7.34.0, for git-imap-send.++ If you do not use http:// or https:// repositories, and do+ not want to put patches into an IMAP mailbox, you do not+ have to have them (use NO_CURL). - "expat" library; git-http-push uses it for remote lock management over DAV. Similar to "curl" above, this is optional
This is a follow-up to the already-integrated topic for dropping
support for older curl versions submitted before the v2.33 release[1].
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!
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.
This means that anyone on an older distro with backported features
will need to -DGIT_CURL_HAVE_* if their version of curl supports some
of these via a backport, not ideal, but an acceptable trade-off. If we
cared about this we could have some "detect-curl" script similar to my
proposed "detect-compiler"[3] (or another homegrown autoconf
replacement).
This also corrects commit messages, removes already-dead code from the
Makefile that we'd missed, and mentions the oldest supported version
in the INSTALL document.
The part where we left behind a potentially warning "ssl_pinnedkey"
variable is also gone, although due to another bug in v1 we'd
unconditionally use it (for config) with the "centralize the
accounting" change there.
1. https://lore.kernel.org/git/cover-v4-0.5-00000000000-20210730T175650Z-avarab@gmail.com/ [1]
2. http://lore.kernel.org/git/YTkPfyAYTU4ZgRgb@coredump.intra.peff.net
3. https://lore.kernel.org/git/87bl6aypke.fsf@evledraar.gmail.com/
Ævar Arnfjörð Bjarmason (8):
INSTALL: don't mention the "curl" executable at all
INSTALL: mention that we need libcurl 7.19.4 or newer to build
Makefile: drop support for curl < 7.9.8 (again)
http: drop support for curl < 7.18.0 (again)
http: correct version check for CURL_HTTP_VERSION_2
http: correct curl version check for CURLOPT_PINNEDPUBLICKEY
http: centralize the accounting of libcurl dependencies
http: don't hardcode the value of CURL_SOCKOPT_OK
INSTALL | 13 +++--
Makefile | 11 +---
git-curl-compat.h | 133 ++++++++++++++++++++++++++++++++++++++++++++++
http.c | 35 ++++++------
imap-send.c | 2 +-
5 files changed, 161 insertions(+), 33 deletions(-)
create mode 100644 git-curl-compat.h
Range-diff against v1:
-: ----------- > 1: ac11cf8cfd1 INSTALL: don't mention the "curl" executable at all
-: ----------- > 2: 4b653cee2d3 INSTALL: mention that we need libcurl 7.19.4 or newer to build
-: ----------- > 3: 76c2aa6e78d Makefile: drop support for curl < 7.9.8 (again)
1: 3ffa2f491dd = 4: e73a9ff1780 http: drop support for curl < 7.18.0 (again)
3: d8192164937 ! 5: 2567b888c3d http: correct version check for CURL_HTTP_VERSION_2_0
@@ Metadata
Author: Ævar Arnfjörð Bjarmason [off-list ref]
## Commit message ##
- http: correct version check for CURL_HTTP_VERSION_2_0
+ http: correct version check for CURL_HTTP_VERSION_2
In d73019feb44 (http: add support selecting http version, 2018-11-08)
- a dependency was added on CURL_HTTP_VERSION_2_0, but this feature was
+ a dependency was added on CURL_HTTP_VERSION_2, but this feature was
introduced in curl version 7.43.0, not 7.47.0, as the incorrect
version check led us to believe.
@@ Commit message
dependency on CURL_HTTP_VERSION_2TLS, but the corresponding version
check was not corrected.
+ The newest symbol we depend on is CURL_HTTP_VERSION_2. It was added in
+ 7.33.0, but the CURL_HTTP_VERSION_2 alias we used was added in
+ 7.47.0. So we could support an even older version here, but let's just
+ correct the checked version.
+
1. https://lore.kernel.org/git/pull.69.git.gitgitgadget@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
2: 511534ce17a ! 6: 397d54a1352 http: correct curl version check for CURLOPT_PINNEDPUBLICKEY
@@ Commit message
but the relevant code depended on CURLOPT_PINNEDPUBLICKEY, introduced
in 7.39.0.
- Let's also remove the macro check before we declare the ssl_pinnedkey
- variable, the pattern for other such variables is to declare the
- static variable unconditionally, we just may not use it on older
- versions. This reduces macro verbosity.
-
- The reduction in verbosity comes at the small cost of issuing a
- warning about the unused variable if this code is compiled with curl
- versions older than 7.39.0. I think that's an acceptable trade-off,
- anyone compiling a new git with a 2014-era toolchain likely has at
- least other warning that'll have prompted them not to use -Werror, and
- if not maybe this'll prompt them to compile their new git with a more
- modern libcurl.
-
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
## http.c ##
@@ http.c: static struct {
static const char *ssl_capath;
static const char *curl_no_proxy;
-#if LIBCURL_VERSION_NUM >= 0x072c00
++#if LIBCURL_VERSION_NUM >= 0x072700
static const char *ssl_pinnedkey;
--#endif
+ #endif
static const char *ssl_cainfo;
- static long curl_low_speed_limit = -1;
- static long curl_low_speed_time = -1;
@@ http.c: static int http_options(const char *var, const char *value, void *cb)
}
4: 47b513a261b ! 7: 8e57a8409c5 http: centralize the accounting of libcurl dependencies
@@ Commit message
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.
+ checking specific symbols present in curl, as some distros have been
+ known to backport features.
- Let's instead add a new git-curl-compat.h header that'll keep track of
- these dependencies. Following this pattern will also make it much
- easier to track when we should deprecate curl versions in the future,
- as we just did post-v2.33 e48a623dea0 (Merge branch
- 'ab/http-drop-old-curl', 2021-08-24).
+ 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.
+
+ More importantly, as shown in a 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.
+
+ By doing them all in one place we'll hopefully reduce the chances of
+ such future mistakes, furthermore it now becomes easier to see at a
+ glance what the oldest supported version is, which makes it easier to
+ reason about any future deprecation similar to the recent
+ e48a623dea0 (Merge branch 'ab/http-drop-old-curl', 2021-08-24).
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
@@ git-curl-compat.h (new)
@@
+#ifndef GIT_CURL_COMPAT_H
+#define GIT_CURL_COMPAT_H
++#include <curl/curl.h>
+
+/**
+ * This header centralizes the declaration of our libcurl dependencies
@@ git-curl-compat.h (new)
+ * inform decisions about removing support for older libcurl in the
+ * future.
+ *
++ * The oldest supported version of curl is documented in the "INSTALL"
++ * document.
++ *
+ * The source of truth for what versions have which symbols is
+ * https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions;
+ * the release dates are taken from curl.git (at
+ * https://github.com/curl/curl/).
+ *
-+ * For each X symbol we need from curl we check if it exists and
-+ * declare our own GIT_CURL_HAVE_X, or if it's for both X and Y
-+ * GIT_CURL_HAVE_X_and_Y, where the "Y" in "X_and_Y" is only the part
-+ * of the symbol name that "X" and "Y" don't have in common.
-+ *
-+ * We avoid comparisons against LIBCURL_VERSION_NUM, enterprise
-+ * distros have been known to backport symbols to their older curl
-+ * versions.
++ * 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
+
@@ git-curl-compat.h (new)
+/**
+ * CURLOPT_LOGIN_OPTIONS was added in 7.34.0, released in December
+ * 2013.
++ *
++ * If we start requiring 7.34.0 we might also be able to remove the
++ * code conditional on USE_CURL_FOR_IMAP_SEND in imap-send.c, see
++ * 1e16b255b95 (git-imap-send: use libcurl for implementation,
++ * 2014-11-09) and the check it added for "072200" in the Makefile.
++
+ */
-+#ifdef CURLOPT_LOGIN_OPTIONS
++#if LIBCURL_VERSION_NUM >= 0x072200
+#define GIT_CURL_HAVE_CURLOPT_LOGIN_OPTIONS 1
+#endif
+
@@ git-curl-compat.h (new)
+ * CURL_SSLVERSION_TLSv1_[012] was added in 7.34.0, released in
+ * December 2013.
+ */
-+#if defined(CURL_SSLVERSION_TLSv1_0) && \
-+ defined(CURL_SSLVERSION_TLSv1_1) && \
-+ defined(CURL_SSLVERSION_TLSv1_2)
-+#define GIT_CURL_HAVE_CURL_SSLVERSION_TLSv1_0_and_1_and_2
++#if LIBCURL_VERSION_NUM >= 0x072200
++#define GIT_CURL_HAVE_CURL_SSLVERSION_TLSv1_0
+#endif
+
+/**
+ * CURLOPT_PINNEDPUBLICKEY was added in 7.39.0, released in November
+ * 2014.
+ */
-+#ifdef CURLOPT_PINNEDPUBLICKEY
++#if LIBCURL_VERSION_NUM >= 0x072c00
+#define GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY 1
+#endif
+
+/**
+ * CURL_HTTP_VERSION_2 was added in 7.43.0, released in June 2015.
++ *
++ * The CURL_HTTP_VERSION_2 alias (but not CURL_HTTP_VERSION_2_0) has
++ * always been a macro, not an enum field (checked on curl version
++ * 7.78.0)
+ */
-+#ifdef CURL_HTTP_VERSION_2
++#if LIBCURL_VERSION_NUM >= 0x072b00
+#define GIT_CURL_HAVE_CURL_HTTP_VERSION_2 1
+#endif
+
+/**
+ * CURLSSLOPT_NO_REVOKE was added in 7.44.0, released in August 2015.
++ *
++ * The CURLSSLOPT_NO_REVOKE is, has always been a macro, not an enum
++ * field (checked on curl version 7.78.0)
+ */
-+#ifdef CURLSSLOPT_NO_REVOKE
++#if LIBCURL_VERSION_NUM >= 0x072c00
+#define GIT_CURL_HAVE_CURLSSLOPT_NO_REVOKE 1
+#endif
+
+/**
+ * CURLOPT_PROXY_CAINFO was added in 7.52.0, released in August 2017.
+ */
-+#ifdef CURLOPT_PROXY_CAINFO
++#if LIBCURL_VERSION_NUM >= 0x073400
+#define GIT_CURL_HAVE_CURLOPT_PROXY_CAINFO 1
+#endif
+
@@ git-curl-compat.h (new)
+ * CURLOPT_PROXY_{KEYPASSWD,SSLCERT,SSLKEY} was added in 7.52.0,
+ * released in August 2017.
+ */
-+#if defined(CURLOPT_PROXY_KEYPASSWD) && \
-+ defined(CURLOPT_PROXY_SSLCERT) && \
-+ defined(CURLOPT_PROXY_SSLKEY)
-+#define GIT_CURL_HAVE_CURLOPT_PROXY_KEYPASSWD_and_SSLCERT_and_SSLKEY 1
++#if LIBCURL_VERSION_NUM >= 0x073400
++#define GIT_CURL_HAVE_CURLOPT_PROXY_KEYPASSWD 1
+#endif
+
+/**
+ * CURL_SSLVERSION_TLSv1_3 was added in 7.53.0, released in February
+ * 2017.
+ */
-+#ifdef CURL_SSLVERSION_TLSv1_3
++#if LIBCURL_VERSION_NUM >= 0x073400
+#define GIT_CURL_HAVE_CURL_SSLVERSION_TLSv1_3 1
+#endif
+
@@ git-curl-compat.h (new)
+ * CURLSSLSET_{NO_BACKENDS,OK,TOO_LATE,UNKNOWN_BACKEND} were added in
+ * 7.56.0, released in September 2017.
+ */
-+#if defined(CURLSSLSET_NO_BACKENDS) && \
-+ defined(CURLSSLSET_OK) && \
-+ defined(CURLSSLSET_TOO_LATE) && \
-+ defined(CURLSSLSET_UNKNOWN_BACKEND)
-+#define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS_and_OK_and_TOO_LATE_and_UNKNOWN_BACKEND 1
++#if LIBCURL_VERSION_NUM >= 0x073800
++#define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS
+#endif
+
+#endif
@@ http.c: static struct {
{ "sslv3", CURL_SSLVERSION_SSLv3 },
{ "tlsv1", CURL_SSLVERSION_TLSv1 },
-#if LIBCURL_VERSION_NUM >= 0x072200
-+#if GIT_CURL_HAVE_CURL_SSLVERSION_TLSv1_0_AND_1_AND_2
++#ifdef GIT_CURL_HAVE_CURL_SSLVERSION_TLSv1_0
{ "tlsv1.0", CURL_SSLVERSION_TLSv1_0 },
{ "tlsv1.1", CURL_SSLVERSION_TLSv1_1 },
{ "tlsv1.2", CURL_SSLVERSION_TLSv1_2 },
#endif
-#if LIBCURL_VERSION_NUM >= 0x073400
-+#if GIT_CURL_HAVE_CURL_SSLVERSION_TLSv1_3
++#ifdef GIT_CURL_HAVE_CURL_SSLVERSION_TLSv1_3
{ "tlsv1.3", CURL_SSLVERSION_TLSv1_3 },
#endif
};
+ static const char *ssl_key;
+ static const char *ssl_capath;
+ static const char *curl_no_proxy;
+-#if LIBCURL_VERSION_NUM >= 0x072700
++#ifdef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY
+ static const char *ssl_pinnedkey;
+ #endif
+ static const char *ssl_cainfo;
@@ http.c: static int http_options(const char *var, const char *value, void *cb)
}
if (!strcmp("http.pinnedpubkey", var)) {
-#if LIBCURL_VERSION_NUM >= 0x072700
-- return git_config_pathname(&ssl_pinnedkey, var, value);
--#else
-+#ifndef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY
++#ifdef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY
+ return git_config_pathname(&ssl_pinnedkey, var, value);
+ #else
warning(_("Public key pinning not supported with cURL < 7.39.0"));
-- return 0;
- #endif
-+ return git_config_pathname(&ssl_pinnedkey, var, value);
- }
-
- if (!strcmp("http.extraheader", var)) {
@@ http.c: static int has_cert_password(void)
return 1;
}
-#if LIBCURL_VERSION_NUM >= 0x073400
-+#ifdef GIT_CURL_HAVE_CURLOPT_PROXY_KEYPASSWD_and_SSLCERT_and_SSLKEY
++#ifdef GIT_CURL_HAVE_CURLOPT_PROXY_KEYPASSWD
static int has_proxy_cert_password(void)
{
if (http_proxy_ssl_cert == NULL || proxy_ssl_cert_password_required != 1)
@@ http.c: static CURL *get_curl_handle(void)
curl_easy_setopt(result,
CURLOPT_PROXYTYPE, CURLPROXY_SOCKS4);
-#if LIBCURL_VERSION_NUM >= 0x073400
-+#ifdef GIT_CURL_HAVE_CURLOPT_PROXY_KEYPASSWD_and_SSLCERT_and_SSLKEY
++#ifdef GIT_CURL_HAVE_CURLOPT_PROXY_KEYPASSWD
else if (starts_with(curl_http_proxy, "https")) {
curl_easy_setopt(result, CURLOPT_PROXYTYPE, CURLPROXY_HTTPS);
@@ http.c: void http_init(struct remote *remote, const char *url, int proactive_aut
string_list_clear(&config.vars, 1);
-#if LIBCURL_VERSION_NUM >= 0x073800
-+#ifdef GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS_and_OK_and_TOO_LATE_and_UNKNOWN_BACKEND
++#ifdef GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS
if (http_ssl_backend) {
const curl_ssl_backend **backends;
struct strbuf buf = STRBUF_INIT;
5: 4f42c0e48b0 ! 8: 465ab33ebda http: don't hardcode the value of CURL_SOCKOPT_OK
@@ Commit message
## git-curl-compat.h ##
@@
- * GIT_CURL_HAVE_X_and_Y, where the "Y" in "X_and_Y" is only the part
- * of the symbol name that "X" and "Y" don't have in common.
+ * 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.
*
+ * We may also define a missing CURL_* symbol to its known value, if
+ * doing so is sufficient to add support for it to older versions that
+ * don't have it.
+ *
- * We avoid comparisons against LIBCURL_VERSION_NUM, enterprise
- * distros have been known to backport symbols to their older curl
- * versions.
-@@
+ * Keep any symbols in date order of when their support was
* introduced, oldest first, in the official version of cURL library.
*/
+/**
+ * CURL_SOCKOPT_OK was added in 7.21.5, released in April 2011.
++ *
++ * This should be safe as CURL_SOCKOPT_OK has always been a macro, not
++ * an enum field (checked on curl version 7.78.0, released on July 19,
++ * 2021). Even if that were to change the value of "0" for "OK" is
++ * unlikely to change.
+ */
+#ifndef CURL_SOCKOPT_OK
+#define CURL_SOCKOPT_OK 0
--
2.33.0.873.g125ff7b9940
Without NO_CURL=Y we require at least version "7.19.4" of libcurl, see
644de29e220 (http: drop support for curl < 7.19.4, 2021-07-30). Let's
document this in the "INSTALL" document.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
INSTALL | 3 +++
1 file changed, 3 insertions(+)
@@ -141,6 +141,9 @@ Issues of note: - "libcurl" library is used by git-http-fetch, git-fetch, and, if the curl version >= 7.34.0, for git-imap-send.+ Git version "7.19.4" of "libcurl" or later to build. This+ version requirement may be bumped in the future.+ If you do not use http:// or https:// repositories, and do not want to put patches into an IMAP mailbox, you do not have to have them (use NO_CURL).
In 1119a15b5c8 (http: drop support for curl < 7.11.1, 2021-07-30)
support for curl versions older than 7.11.1 was removed, and we
currently require at least version 7.19.4, see 644de29e220 (http: drop
support for curl < 7.19.4, 2021-07-30).
In those changes this Makefile-specific check added in
0890098780f (Decide whether to build http-push in the Makefile,
2005-11-18) was missed, now that we're never going to use such an
ancient curl version we don't need to check that we have at least
7.9.8 here. I have no idea what in http-push.c broke on versions older
than that.
This does not impact "NO_CURL" setups, as this is in the "else" branch
after that check.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
In 644de29e220 (http: drop support for curl < 7.19.4, 2021-07-30) we
dropped support for curl < 7.19.4, so we can drop support for this
non-obvious dependency on curl < 7.18.0.
It's non-obvious because in curl's hex version notation 0x071800 is
version 7.24.0, *not* 7.18.0, so at a glance this patch looks
incorrect.
But it's correct, because the existing version check being removed
here is wrong. The check guards use of the following curl defines:
CURLPROXY_SOCKS4 7.10
CURLPROXY_SOCKS4A 7.18.0
CURLPROXY_SOCKS5 7.10
CURLPROXY_SOCKS5_HOSTNAME 7.18.0
I.e. the oldest version that has these is in fact 7.18.0, not
7.24.0. That we were checking 7.24.0 is just an mistake in
6d7afe07f29 (remote-http(s): support SOCKS proxies, 2015-10-26),
i.e. its author confusing base 10 and base 16.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 2 --
1 file changed, 2 deletions(-)
In d73019feb44 (http: add support selecting http version, 2018-11-08)
a dependency was added on CURL_HTTP_VERSION_2, but this feature was
introduced in curl version 7.43.0, not 7.47.0, as the incorrect
version check led us to believe.
As looking through the history of that commit on the mailing list will
reveal[1], the reason for this is that an earlier version of it
depended on CURL_HTTP_VERSION_2TLS, which was introduced in libcurl
7.47.0.
But the version that made it in in d73019feb44 had dropped the
dependency on CURL_HTTP_VERSION_2TLS, but the corresponding version
check was not corrected.
The newest symbol we depend on is CURL_HTTP_VERSION_2. It was added in
7.33.0, but the CURL_HTTP_VERSION_2 alias we used was added in
7.47.0. So we could support an even older version here, but let's just
correct the checked version.
1. https://lore.kernel.org/git/pull.69.git.gitgitgadget@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
In aeff8a61216 (http: implement public key pinning, 2016-02-15) a
dependency and warning() was added if curl older than 7.44.0 was used,
but the relevant code depended on CURLOPT_PINNEDPUBLICKEY, introduced
in 7.39.0.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
http.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
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.
More importantly, as shown in a 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.
By doing them all in one place we'll hopefully reduce the chances of
such future mistakes, furthermore it now becomes easier to see at a
glance what the oldest supported version is, which makes it easier to
reason about any future deprecation similar to the recent
e48a623dea0 (Merge branch 'ab/http-drop-old-curl', 2021-08-24).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
git-curl-compat.h | 117 ++++++++++++++++++++++++++++++++++++++++++++++
http.c | 29 ++++++------
imap-send.c | 2 +-
3 files changed, 133 insertions(+), 15 deletions(-)
create mode 100644 git-curl-compat.h