git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/4] dropping support for older curl
@ 2017-08-09 12:00 Jeff King
  2017-08-09 12:01 ` [PATCH 1/4] http: drop support for curl < 7.11.1 Jeff King
                   ` (6 more replies)
  0 siblings, 7 replies; 70+ messages in thread
From: Jeff King @ 2017-08-09 12:00 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH 1/4] http: drop support for curl < 7.11.1
  2017-08-09 12:00 [PATCH 0/4] dropping support for older curl Jeff King
@ 2017-08-09 12:01 ` Jeff King
  2017-08-09 12:01 ` [PATCH 2/4] http: drop support for curl < 7.16.0 Jeff King
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 70+ messages in thread
From: Jeff King @ 2017-08-09 12:01 UTC (permalink / raw)
  To: git

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 <peff@peff.net>
---
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(-)

diff --git a/http.c b/http.c
index c6c010f881..a3675a0eaa 100644
--- a/http.c
+++ b/http.c
@@ -13,19 +13,11 @@
 #include "transport.h"
 
 static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
-#if LIBCURL_VERSION_NUM >= 0x070a08
 long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;
-#else
-long int git_curl_ipresolve;
-#endif
 int active_requests;
 int http_is_verbose;
 ssize_t http_post_buffer = 16 * LARGE_PACKET_MAX;
 
-#if LIBCURL_VERSION_NUM >= 0x070a06
-#define LIBCURL_CAN_HANDLE_AUTH_ANY
-#endif
-
 static int min_curl_sessions = 1;
 static int curl_session_count;
 #ifdef USE_CURL_MULTI
@@ -58,12 +50,8 @@ static struct {
 	{ "tlsv1.2", CURL_SSLVERSION_TLSv1_2 },
 #endif
 };
-#if LIBCURL_VERSION_NUM >= 0x070903
 static const char *ssl_key;
-#endif
-#if LIBCURL_VERSION_NUM >= 0x070908
 static const char *ssl_capath;
-#endif
 #if LIBCURL_VERSION_NUM >= 0x072c00
 static const char *ssl_pinnedkey;
 #endif
@@ -82,9 +70,7 @@ static struct {
 	{ "digest", CURLAUTH_DIGEST },
 	{ "negotiate", CURLAUTH_GSSNEGOTIATE },
 	{ "ntlm", CURLAUTH_NTLM },
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 	{ "anyauth", CURLAUTH_ANY },
-#endif
 	/*
 	 * CURLAUTH_DIGEST_IE has no corresponding command-line option in
 	 * curl(1) and is not included in CURLAUTH_ANY, so we leave it out
@@ -124,7 +110,6 @@ enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
 
 static struct credential cert_auth = CREDENTIAL_INIT;
 static int ssl_cert_password_required;
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 static unsigned long http_auth_methods = CURLAUTH_ANY;
 static int http_auth_methods_restricted;
 /* Modes for which empty_auth cannot actually help us. */
@@ -134,7 +119,6 @@ static unsigned long empty_auth_useless =
 	| CURLAUTH_DIGEST_IE
 #endif
 	| CURLAUTH_DIGEST;
-#endif
 
 static struct curl_slist *pragma_header;
 static struct curl_slist *no_pragma_header;
@@ -208,12 +192,8 @@ static void finish_active_slot(struct active_request_slot *slot)
 	if (slot->results != NULL) {
 		slot->results->curl_result = slot->curl_result;
 		slot->results->http_code = slot->http_code;
-#if LIBCURL_VERSION_NUM >= 0x070a08
 		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL,
 				  &slot->results->auth_avail);
-#else
-		slot->results->auth_avail = 0;
-#endif
 
 		curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
 			&slot->results->http_connectcode);
@@ -273,14 +253,10 @@ static int http_options(const char *var, const char *value, void *cb)
 		return git_config_string(&ssl_version, var, value);
 	if (!strcmp("http.sslcert", var))
 		return git_config_string(&ssl_cert, var, value);
-#if LIBCURL_VERSION_NUM >= 0x070903
 	if (!strcmp("http.sslkey", var))
 		return git_config_string(&ssl_key, var, value);
-#endif
-#if LIBCURL_VERSION_NUM >= 0x070908
 	if (!strcmp("http.sslcapath", var))
 		return git_config_pathname(&ssl_capath, var, value);
-#endif
 	if (!strcmp("http.sslcainfo", var))
 		return git_config_pathname(&ssl_cainfo, var, value);
 	if (!strcmp("http.sslcertpasswordprotected", var)) {
@@ -401,12 +377,6 @@ static int curl_empty_auth_enabled(void)
 	if (curl_empty_auth >= 0)
 		return curl_empty_auth;
 
-#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY
-	/*
-	 * Our libcurl is too old to do AUTH_ANY in the first place;
-	 * just default to turning the feature off.
-	 */
-#else
 	/*
 	 * In the automatic case, kick in the empty-auth
 	 * hack as long as we would potentially try some
@@ -419,7 +389,6 @@ static int curl_empty_auth_enabled(void)
 	if (http_auth_methods_restricted &&
 	    (http_auth_methods & ~empty_auth_useless))
 		return 1;
-#endif
 	return 0;
 }
 
@@ -490,7 +459,6 @@ static void init_curl_proxy_auth(CURL *result)
 
 	var_override(&http_proxy_authmethod, getenv("GIT_HTTP_PROXY_AUTHMETHOD"));
 
-#if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */
 	if (http_proxy_authmethod) {
 		int i;
 		for (i = 0; i < ARRAY_SIZE(proxy_authmethods); i++) {
@@ -508,7 +476,6 @@ static void init_curl_proxy_auth(CURL *result)
 	}
 	else
 		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
-#endif
 }
 
 static int has_cert_password(void)
@@ -710,12 +677,8 @@ static CURL *get_curl_handle(void)
 		curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 2);
 	}
 
-#if LIBCURL_VERSION_NUM >= 0x070907
 	curl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
-#endif
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 	curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
-#endif
 
 #if LIBCURL_VERSION_NUM >= 0x071600
 	if (curl_deleg) {
@@ -762,14 +725,10 @@ static CURL *get_curl_handle(void)
 		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
 	if (has_cert_password())
 		curl_easy_setopt(result, CURLOPT_KEYPASSWD, cert_auth.password);
-#if LIBCURL_VERSION_NUM >= 0x070903
 	if (ssl_key != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key);
-#endif
-#if LIBCURL_VERSION_NUM >= 0x070908
 	if (ssl_capath != NULL)
 		curl_easy_setopt(result, CURLOPT_CAPATH, ssl_capath);
-#endif
 #if LIBCURL_VERSION_NUM >= 0x072c00
 	if (ssl_pinnedkey != NULL)
 		curl_easy_setopt(result, CURLOPT_PINNEDPUBLICKEY, ssl_pinnedkey);
@@ -945,12 +904,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 		curl_ssl_verify = 0;
 
 	set_from_env(&ssl_cert, "GIT_SSL_CERT");
-#if LIBCURL_VERSION_NUM >= 0x070903
 	set_from_env(&ssl_key, "GIT_SSL_KEY");
-#endif
-#if LIBCURL_VERSION_NUM >= 0x070908
 	set_from_env(&ssl_capath, "GIT_SSL_CAPATH");
-#endif
 	set_from_env(&ssl_cainfo, "GIT_SSL_CAINFO");
 
 	set_from_env(&user_agent, "GIT_HTTP_USER_AGENT");
@@ -1120,12 +1075,8 @@ struct active_request_slot *get_active_slot(void)
 	else
 		curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 0);
 
-#if LIBCURL_VERSION_NUM >= 0x070a08
 	curl_easy_setopt(slot->curl, CURLOPT_IPRESOLVE, git_curl_ipresolve);
-#endif
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
-#endif
 	if (http_auth.password || curl_empty_auth_enabled())
 		init_curl_http_auth(slot->curl);
 
@@ -1392,13 +1343,11 @@ static int handle_curl_result(struct slot_results *results)
 			credential_reject(&http_auth);
 			return HTTP_NOAUTH;
 		} else {
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
 			if (results->auth_avail) {
 				http_auth_methods &= results->auth_avail;
 				http_auth_methods_restricted = 1;
 			}
-#endif
 			return HTTP_REAUTH;
 		}
 	} else {
diff --git a/http.h b/http.h
index f7bd3b26b0..90b20a711a 100644
--- a/http.h
+++ b/http.h
@@ -22,21 +22,10 @@
 #define DEFAULT_MAX_REQUESTS 5
 #endif
 
-#if LIBCURL_VERSION_NUM < 0x070704
-#define curl_global_cleanup() do { /* nothing */ } while (0)
-#endif
-#if LIBCURL_VERSION_NUM < 0x070800
-#define curl_global_init(a) do { /* nothing */ } while (0)
-#endif
-
 #if (LIBCURL_VERSION_NUM < 0x070c04) || (LIBCURL_VERSION_NUM == 0x071000)
 #define NO_CURL_EASY_DUPHANDLE
 #endif
 
-#if LIBCURL_VERSION_NUM < 0x070a03
-#define CURLE_HTTP_RETURNED_ERROR CURLE_HTTP_NOT_FOUND
-#endif
-
 #if LIBCURL_VERSION_NUM < 0x070c03
 #define NO_CURL_IOCTL
 #endif
diff --git a/remote-curl.c b/remote-curl.c
index 0053b09549..23e2a1f3ac 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -144,8 +144,6 @@ static int set_option(const char *name, const char *value)
 	} else if (!strcmp(name, "push-option")) {
 		string_list_append(&options.push_options, value);
 		return 0;
-
-#if LIBCURL_VERSION_NUM >= 0x070a08
 	} else if (!strcmp(name, "family")) {
 		if (!strcmp(value, "ipv4"))
 			git_curl_ipresolve = CURL_IPRESOLVE_V4;
@@ -156,7 +154,6 @@ static int set_option(const char *name, const char *value)
 		else
 			return -1;
 		return 0;
-#endif /* LIBCURL_VERSION_NUM >= 0x070a08 */
 	} else {
 		return 1 /* unsupported */;
 	}
-- 
2.14.0.609.gd2d1f7ddf


^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH 2/4] http: drop support for curl < 7.16.0
  2017-08-09 12:00 [PATCH 0/4] dropping support for older curl Jeff King
  2017-08-09 12:01 ` [PATCH 1/4] http: drop support for curl < 7.11.1 Jeff King
@ 2017-08-09 12:01 ` Jeff King
  2017-08-09 17:29   ` Stefan Beller
  2017-08-09 17:40   ` Junio C Hamano
  2017-08-09 12:02 ` [PATCH 3/4] http: drop support for curl < 7.19.4 Jeff King
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 70+ messages in thread
From: Jeff King @ 2017-08-09 12:01 UTC (permalink / raw)
  To: git

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 <peff@peff.net>
---
 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(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d5c9c4cab6..07e5fab98a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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
diff --git a/http-push.c b/http-push.c
index c91f40a610..366af210a9 100644
--- a/http-push.c
+++ b/http-push.c
@@ -198,10 +198,8 @@ static void curl_setup_http(CURL *curl, const char *url,
 	curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
 	curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);
 	curl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);
-#ifndef NO_CURL_IOCTL
 	curl_easy_setopt(curl, CURLOPT_IOCTLFUNCTION, ioctl_buffer);
 	curl_easy_setopt(curl, CURLOPT_IOCTLDATA, buffer);
-#endif
 	curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_fn);
 	curl_easy_setopt(curl, CURLOPT_NOBODY, 0);
 	curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, custom_req);
@@ -244,8 +242,6 @@ static void process_response(void *callback_data)
 	finish_request(request);
 }
 
-#ifdef USE_CURL_MULTI
-
 static void start_fetch_loose(struct transfer_request *request)
 {
 	struct active_request_slot *slot;
@@ -294,7 +290,6 @@ static void start_mkcol(struct transfer_request *request)
 		FREE_AND_NULL(request->url);
 	}
 }
-#endif
 
 static void start_fetch_packed(struct transfer_request *request)
 {
@@ -596,7 +591,6 @@ static void finish_request(struct transfer_request *request)
 	}
 }
 
-#ifdef USE_CURL_MULTI
 static int is_running_queue;
 static int fill_active_slot(void *unused)
 {
@@ -620,7 +614,6 @@ static int fill_active_slot(void *unused)
 	}
 	return 0;
 }
-#endif
 
 static void get_remote_object_list(unsigned char parent);
 
@@ -649,10 +642,8 @@ static void add_fetch_request(struct object *obj)
 	request->next = request_queue_head;
 	request_queue_head = request;
 
-#ifdef USE_CURL_MULTI
 	fill_active_slots();
 	step_active_slots();
-#endif
 }
 
 static int add_send_request(struct object *obj, struct remote_lock *lock)
@@ -687,10 +678,8 @@ static int add_send_request(struct object *obj, struct remote_lock *lock)
 	request->next = request_queue_head;
 	request_queue_head = request;
 
-#ifdef USE_CURL_MULTI
 	fill_active_slots();
 	step_active_slots();
-#endif
 
 	return 1;
 }
@@ -1666,21 +1655,15 @@ static int delete_remote_branch(const char *pattern, int force)
 
 static void run_request_queue(void)
 {
-#ifdef USE_CURL_MULTI
 	is_running_queue = 1;
 	fill_active_slots();
 	add_fill_function(NULL, fill_active_slot);
-#endif
 	do {
 		finish_all_active_slots();
-#ifdef USE_CURL_MULTI
 		fill_active_slots();
-#endif
 	} while (request_queue_head && !aborted);
 
-#ifdef USE_CURL_MULTI
 	is_running_queue = 0;
-#endif
 }
 
 int cmd_main(int argc, const char **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_MULTI
 	is_running_queue = 0;
-#endif
 
 	/* Verify DAV compliance/lock support */
 	if (!locking_available()) {
diff --git a/http-walker.c b/http-walker.c
index ee049cb13d..b5b8e03b0b 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -119,7 +119,6 @@ static void release_object_request(struct object_request *obj_req)
 	free(obj_req);
 }
 
-#ifdef USE_CURL_MULTI
 static int fill_active_slot(struct walker *walker)
 {
 	struct object_request *obj_req;
@@ -138,7 +137,6 @@ static int fill_active_slot(struct walker *walker)
 	}
 	return 0;
 }
-#endif
 
 static void prefetch(struct walker *walker, unsigned char *sha1)
 {
@@ -155,10 +153,8 @@ static void prefetch(struct walker *walker, unsigned char *sha1)
 	http_is_verbose = walker->get_verbosely;
 	list_add_tail(&newreq->node, &object_queue_head);
 
-#ifdef USE_CURL_MULTI
 	fill_active_slots();
 	step_active_slots();
-#endif
 }
 
 static int is_alternate_allowed(const char *url)
@@ -346,11 +342,9 @@ static void fetch_alternates(struct walker *walker, const char *base)
 	 * wait for them to arrive and return to processing this request's
 	 * curl message
 	 */
-#ifdef USE_CURL_MULTI
 	while (cdata->got_alternates == 0) {
 		step_active_slots();
 	}
-#endif
 
 	/* Nothing to do if they've already been fetched */
 	if (cdata->got_alternates == 1)
@@ -493,12 +487,8 @@ static int fetch_object(struct walker *walker, unsigned char *sha1)
 		return 0;
 	}
 
-#ifdef USE_CURL_MULTI
 	while (obj_req->state == WAITING)
 		step_active_slots();
-#else
-	start_object_request(walker, obj_req);
-#endif
 
 	/*
 	 * obj_req->req might change when fetching alternates in the callback
@@ -618,9 +608,7 @@ struct walker *get_http_walker(const char *url)
 	walker->cleanup = cleanup;
 	walker->data = data;
 
-#ifdef USE_CURL_MULTI
 	add_fill_function(walker, (int (*)(void *)) fill_active_slot);
-#endif
 
 	return walker;
 }
diff --git a/http.c b/http.c
index a3675a0eaa..6e5f4ce5f9 100644
--- a/http.c
+++ b/http.c
@@ -20,10 +20,8 @@ ssize_t http_post_buffer = 16 * LARGE_PACKET_MAX;
 
 static int min_curl_sessions = 1;
 static int curl_session_count;
-#ifdef USE_CURL_MULTI
 static int max_requests = -1;
 static CURLM *curlm;
-#endif
 #ifndef NO_CURL_EASY_DUPHANDLE
 static CURL *curl_default;
 #endif
@@ -100,14 +98,6 @@ static int curl_empty_auth = -1;
 
 enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
 
-#if LIBCURL_VERSION_NUM >= 0x071700
-/* Use CURLOPT_KEYPASSWD as is */
-#elif LIBCURL_VERSION_NUM >= 0x070903
-#define CURLOPT_KEYPASSWD CURLOPT_SSLKEYPASSWD
-#else
-#define CURLOPT_KEYPASSWD CURLOPT_SSLCERTPASSWD
-#endif
-
 static struct credential cert_auth = CREDENTIAL_INIT;
 static int ssl_cert_password_required;
 static unsigned long http_auth_methods = CURLAUTH_ANY;
@@ -141,7 +131,6 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 	return size;
 }
 
-#ifndef NO_CURL_IOCTL
 curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
 {
 	struct buffer *buffer = clientp;
@@ -158,7 +147,6 @@ curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
 		return CURLIOE_UNKNOWNCMD;
 	}
 }
-#endif
 
 size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
@@ -206,12 +194,9 @@ static void finish_active_slot(struct active_request_slot *slot)
 
 static void xmulti_remove_handle(struct active_request_slot *slot)
 {
-#ifdef USE_CURL_MULTI
 	curl_multi_remove_handle(curlm, slot->curl);
-#endif
 }
 
-#ifdef USE_CURL_MULTI
 static void process_curl_messages(void)
 {
 	int num_messages;
@@ -239,7 +224,6 @@ static void process_curl_messages(void)
 		curl_message = curl_multi_info_read(curlm, &num_messages);
 	}
 }
-#endif
 
 static int http_options(const char *var, const char *value, void *cb)
 {
@@ -269,18 +253,14 @@ static int http_options(const char *var, const char *value, void *cb)
 	}
 	if (!strcmp("http.minsessions", var)) {
 		min_curl_sessions = git_config_int(var, value);
-#ifndef USE_CURL_MULTI
 		if (min_curl_sessions > 1)
 			min_curl_sessions = 1;
-#endif
 		return 0;
 	}
-#ifdef USE_CURL_MULTI
 	if (!strcmp("http.maxrequests", var)) {
 		max_requests = git_config_int(var, value);
 		return 0;
 	}
-#endif
 	if (!strcmp("http.lowspeedlimit", var)) {
 		curl_low_speed_limit = (long)git_config_int(var, value);
 		return 0;
@@ -497,7 +477,7 @@ static void set_curl_keepalive(CURL *c)
 	curl_easy_setopt(c, CURLOPT_TCP_KEEPALIVE, 1);
 }
 
-#elif LIBCURL_VERSION_NUM >= 0x071000
+#else
 static int sockopt_callback(void *client, curl_socket_t fd, curlsocktype type)
 {
 	int ka = 1;
@@ -518,12 +498,6 @@ static void set_curl_keepalive(CURL *c)
 {
 	curl_easy_setopt(c, CURLOPT_SOCKOPTFUNCTION, sockopt_callback);
 }
-
-#else
-static void set_curl_keepalive(CURL *c)
-{
-	/* not supported on older curl versions */
-}
 #endif
 
 static void redact_sensitive_header(struct strbuf *header)
@@ -888,7 +862,6 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 	no_pragma_header = curl_slist_append(http_copy_default_headers(),
 		"Pragma:");
 
-#ifdef USE_CURL_MULTI
 	{
 		char *http_max_requests = getenv("GIT_HTTP_MAX_REQUESTS");
 		if (http_max_requests != NULL)
@@ -898,7 +871,6 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 	curlm = curl_multi_init();
 	if (!curlm)
 		die("curl_multi_init failed");
-#endif
 
 	if (getenv("GIT_SSL_NO_VERIFY"))
 		curl_ssl_verify = 0;
@@ -921,10 +893,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 		curl_ssl_verify = 1;
 
 	curl_session_count = 0;
-#ifdef USE_CURL_MULTI
 	if (max_requests < 1)
 		max_requests = DEFAULT_MAX_REQUESTS;
-#endif
 
 	if (getenv("GIT_CURL_FTP_NO_EPSV"))
 		curl_ftp_no_epsv = 1;
@@ -961,9 +931,7 @@ void http_cleanup(void)
 	curl_easy_cleanup(curl_default);
 #endif
 
-#ifdef USE_CURL_MULTI
 	curl_multi_cleanup(curlm);
-#endif
 	curl_global_cleanup();
 
 	curl_slist_free_all(extra_http_headers);
@@ -1005,7 +973,6 @@ struct active_request_slot *get_active_slot(void)
 	struct active_request_slot *slot = active_queue_head;
 	struct active_request_slot *newslot;
 
-#ifdef USE_CURL_MULTI
 	int num_transfers;
 
 	/* Wait for a slot to open up if the queue is full */
@@ -1014,7 +981,6 @@ struct active_request_slot *get_active_slot(void)
 		if (num_transfers < active_requests)
 			process_curl_messages();
 	}
-#endif
 
 	while (slot != NULL && slot->in_use)
 		slot = slot->next;
@@ -1085,7 +1051,6 @@ struct active_request_slot *get_active_slot(void)
 
 int start_active_slot(struct active_request_slot *slot)
 {
-#ifdef USE_CURL_MULTI
 	CURLMcode curlm_result = curl_multi_add_handle(curlm, slot->curl);
 	int num_transfers;
 
@@ -1103,11 +1068,9 @@ int start_active_slot(struct active_request_slot *slot)
 	 * something.
 	 */
 	curl_multi_perform(curlm, &num_transfers);
-#endif
 	return 1;
 }
 
-#ifdef USE_CURL_MULTI
 struct fill_chain {
 	void *data;
 	int (*fill)(void *);
@@ -1166,11 +1129,9 @@ void step_active_slots(void)
 		fill_active_slots();
 	}
 }
-#endif
 
 void run_active_slot(struct active_request_slot *slot)
 {
-#ifdef USE_CURL_MULTI
 	fd_set readfds;
 	fd_set writefds;
 	fd_set excfds;
@@ -1183,7 +1144,6 @@ void run_active_slot(struct active_request_slot *slot)
 		step_active_slots();
 
 		if (slot->in_use) {
-#if LIBCURL_VERSION_NUM >= 0x070f04
 			long curl_timeout;
 			curl_multi_timeout(curlm, &curl_timeout);
 			if (curl_timeout == 0) {
@@ -1195,10 +1155,6 @@ void run_active_slot(struct active_request_slot *slot)
 				select_timeout.tv_sec  =  curl_timeout / 1000;
 				select_timeout.tv_usec = (curl_timeout % 1000) * 1000;
 			}
-#else
-			select_timeout.tv_sec  = 0;
-			select_timeout.tv_usec = 50000;
-#endif
 
 			max_fd = -1;
 			FD_ZERO(&readfds);
@@ -1221,12 +1177,6 @@ void run_active_slot(struct active_request_slot *slot)
 			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
 		}
 	}
-#else
-	while (slot->in_use) {
-		slot->curl_result = curl_easy_perform(slot->curl);
-		finish_active_slot(slot);
-	}
-#endif
 }
 
 static void release_active_slot(struct active_request_slot *slot)
@@ -1240,9 +1190,7 @@ static void release_active_slot(struct active_request_slot *slot)
 			curl_session_count--;
 		}
 	}
-#ifdef USE_CURL_MULTI
 	fill_active_slots();
-#endif
 }
 
 void finish_all_active_slots(void)
@@ -1353,12 +1301,10 @@ static int handle_curl_result(struct slot_results *results)
 	} else {
 		if (results->http_connectcode == 407)
 			credential_reject(&proxy_auth);
-#if LIBCURL_VERSION_NUM >= 0x070c00
 		if (!curl_errorstr[0])
 			strlcpy(curl_errorstr,
 				curl_easy_strerror(results->curl_result),
 				sizeof(curl_errorstr));
-#endif
 		return HTTP_ERROR;
 	}
 }
diff --git a/http.h b/http.h
index 90b20a711a..57e97c128d 100644
--- a/http.h
+++ b/http.h
@@ -10,26 +10,12 @@
 #include "remote.h"
 #include "url.h"
 
-/*
- * We detect based on the cURL version if multi-transfer is
- * usable in this implementation and define this symbol accordingly.
- * This shouldn't be set by the Makefile or by the user (e.g. via CFLAGS).
- */
-#undef USE_CURL_MULTI
-
-#if LIBCURL_VERSION_NUM >= 0x071000
-#define USE_CURL_MULTI
 #define DEFAULT_MAX_REQUESTS 5
-#endif
 
-#if (LIBCURL_VERSION_NUM < 0x070c04) || (LIBCURL_VERSION_NUM == 0x071000)
+#if LIBCURL_VERSION_NUM == 0x071000
 #define NO_CURL_EASY_DUPHANDLE
 #endif
 
-#if LIBCURL_VERSION_NUM < 0x070c03
-#define NO_CURL_IOCTL
-#endif
-
 /*
  * CURLOPT_USE_SSL was known as CURLOPT_FTP_SSL up to 7.16.4,
  * and the constants were known as CURLFTPSSL_*
@@ -67,9 +53,7 @@ struct buffer {
 extern size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
 extern size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
 extern size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
-#ifndef NO_CURL_IOCTL
 extern curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);
-#endif
 
 /* Slot lifecycle functions */
 extern struct active_request_slot *get_active_slot(void);
@@ -86,11 +70,9 @@ extern void finish_all_active_slots(void);
 int run_one_slot(struct active_request_slot *slot,
 		 struct slot_results *results);
 
-#ifdef USE_CURL_MULTI
 extern void fill_active_slots(void);
 extern void add_fill_function(void *data, int (*fill)(void *));
 extern void step_active_slots(void);
-#endif
 
 extern void http_init(struct remote *remote, const char *url,
 		      int proactive_auth);
diff --git a/remote-curl.c b/remote-curl.c
index 23e2a1f3ac..333cca33b6 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -435,7 +435,6 @@ static size_t rpc_out(void *ptr, size_t eltsize,
 	return avail;
 }
 
-#ifndef NO_CURL_IOCTL
 static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
 {
 	struct rpc_state *rpc = clientp;
@@ -456,7 +455,6 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
 		return CURLIOE_UNKNOWNCMD;
 	}
 }
-#endif
 
 static size_t rpc_in(char *ptr, size_t eltsize,
 		size_t nmemb, void *buffer_)
@@ -602,10 +600,8 @@ static int post_rpc(struct rpc_state *rpc)
 		rpc->initial_buffer = 1;
 		curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
 		curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
-#ifndef NO_CURL_IOCTL
 		curl_easy_setopt(slot->curl, CURLOPT_IOCTLFUNCTION, rpc_ioctl);
 		curl_easy_setopt(slot->curl, CURLOPT_IOCTLDATA, rpc);
-#endif
 		if (options.verbosity > 1) {
 			fprintf(stderr, "POST %s (chunked)\n", rpc->service_name);
 			fflush(stderr);
-- 
2.14.0.609.gd2d1f7ddf


^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH 3/4] http: drop support for curl < 7.19.4
  2017-08-09 12:00 [PATCH 0/4] dropping support for older curl Jeff King
  2017-08-09 12:01 ` [PATCH 1/4] http: drop support for curl < 7.11.1 Jeff King
  2017-08-09 12:01 ` [PATCH 2/4] http: drop support for curl < 7.16.0 Jeff King
@ 2017-08-09 12:02 ` Jeff King
  2017-08-09 13:14   ` Ævar Arnfjörð Bjarmason
                     ` (2 more replies)
  2017-08-09 12:02 ` [PATCH 4/4] http: #error on too-old curl Jeff King
                   ` (3 subsequent siblings)
  6 siblings, 3 replies; 70+ messages in thread
From: Jeff King @ 2017-08-09 12:02 UTC (permalink / raw)
  To: git

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 <peff@peff.net>
---
 http.c | 46 ----------------------------------------------
 http.h |  4 ----
 2 files changed, 50 deletions(-)

diff --git a/http.c b/http.c
index 6e5f4ce5f9..5280511c74 100644
--- a/http.c
+++ b/http.c
@@ -22,9 +22,7 @@ static int min_curl_sessions = 1;
 static int curl_session_count;
 static int max_requests = -1;
 static CURLM *curlm;
-#ifndef NO_CURL_EASY_DUPHANDLE
 static CURL *curl_default;
-#endif
 
 #define PREV_BUF_SIZE 4096
 
@@ -382,24 +380,8 @@ static void init_curl_http_auth(CURL *result)
 
 	credential_fill(&http_auth);
 
-#if LIBCURL_VERSION_NUM >= 0x071301
 	curl_easy_setopt(result, CURLOPT_USERNAME, http_auth.username);
 	curl_easy_setopt(result, CURLOPT_PASSWORD, http_auth.password);
-#else
-	{
-		static struct strbuf up = STRBUF_INIT;
-		/*
-		 * Note that we assume we only ever have a single set of
-		 * credentials in a given program run, so we do not have
-		 * to worry about updating this buffer, only setting its
-		 * initial value.
-		 */
-		if (!up.len)
-			strbuf_addf(&up, "%s:%s",
-				http_auth.username, http_auth.password);
-		curl_easy_setopt(result, CURLOPT_USERPWD, up.buf);
-	}
-#endif
 }
 
 /* *var must be free-able */
@@ -413,20 +395,10 @@ static void var_override(const char **var, char *value)
 
 static void set_proxyauth_name_password(CURL *result)
 {
-#if LIBCURL_VERSION_NUM >= 0x071301
 		curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
 			proxy_auth.username);
 		curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
 			proxy_auth.password);
-#else
-		struct strbuf s = STRBUF_INIT;
-
-		strbuf_addstr_urlencode(&s, proxy_auth.username, 1);
-		strbuf_addch(&s, ':');
-		strbuf_addstr_urlencode(&s, proxy_auth.password, 1);
-		curl_proxyuserpwd = strbuf_detach(&s, NULL);
-		curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);
-#endif
 }
 
 static void init_curl_proxy_auth(CURL *result)
@@ -718,20 +690,12 @@ static CURL *get_curl_handle(void)
 	}
 
 	curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
-#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);
-#endif
-#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
 	if (getenv("GIT_CURL_VERBOSE"))
 		curl_easy_setopt(result, CURLOPT_VERBOSE, 1L);
 	setup_curl_trace(result);
@@ -807,11 +771,9 @@ static CURL *get_curl_handle(void)
 			die("Invalid proxy URL '%s'", curl_http_proxy);
 
 		curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
-#if LIBCURL_VERSION_NUM >= 0x071304
 		var_override(&curl_no_proxy, getenv("NO_PROXY"));
 		var_override(&curl_no_proxy, getenv("no_proxy"));
 		curl_easy_setopt(result, CURLOPT_NOPROXY, curl_no_proxy);
-#endif
 	}
 	init_curl_proxy_auth(result);
 
@@ -907,9 +869,7 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 			ssl_cert_password_required = 1;
 	}
 
-#ifndef NO_CURL_EASY_DUPHANDLE
 	curl_default = get_curl_handle();
-#endif
 }
 
 void http_cleanup(void)
@@ -927,9 +887,7 @@ void http_cleanup(void)
 	}
 	active_queue_head = NULL;
 
-#ifndef NO_CURL_EASY_DUPHANDLE
 	curl_easy_cleanup(curl_default);
-#endif
 
 	curl_multi_cleanup(curlm);
 	curl_global_cleanup();
@@ -1003,11 +961,7 @@ struct active_request_slot *get_active_slot(void)
 	}
 
 	if (slot->curl == NULL) {
-#ifdef NO_CURL_EASY_DUPHANDLE
-		slot->curl = get_curl_handle();
-#else
 		slot->curl = curl_easy_duphandle(curl_default);
-#endif
 		curl_session_count++;
 	}
 
diff --git a/http.h b/http.h
index 57e97c128d..da4d8589d8 100644
--- a/http.h
+++ b/http.h
@@ -12,10 +12,6 @@
 
 #define DEFAULT_MAX_REQUESTS 5
 
-#if LIBCURL_VERSION_NUM == 0x071000
-#define NO_CURL_EASY_DUPHANDLE
-#endif
-
 /*
  * CURLOPT_USE_SSL was known as CURLOPT_FTP_SSL up to 7.16.4,
  * and the constants were known as CURLFTPSSL_*
-- 
2.14.0.609.gd2d1f7ddf


^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH 4/4] http: #error on too-old curl
  2017-08-09 12:00 [PATCH 0/4] dropping support for older curl Jeff King
                   ` (2 preceding siblings ...)
  2017-08-09 12:02 ` [PATCH 3/4] http: drop support for curl < 7.19.4 Jeff King
@ 2017-08-09 12:02 ` Jeff King
  2017-08-09 17:37   ` Stefan Beller
  2017-08-09 21:42 ` [PATCH 0/4] dropping support for older curl Johannes Schindelin
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 70+ messages in thread
From: Jeff King @ 2017-08-09 12:02 UTC (permalink / raw)
  To: git

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 <peff@peff.net>
---
 http.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/http.h b/http.h
index da4d8589d8..29acfe8c55 100644
--- a/http.h
+++ b/http.h
@@ -10,6 +10,10 @@
 #include "remote.h"
 #include "url.h"
 
+#if LIBCURL_VERSION_NUM < 0x071304
+#error "your libcurl version is too old; Git requires curl >= 7.19.4"
+#endif
+
 #define DEFAULT_MAX_REQUESTS 5
 
 /*
-- 
2.14.0.609.gd2d1f7ddf

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 3/4] http: drop support for curl < 7.19.4
  2017-08-09 12:02 ` [PATCH 3/4] http: drop support for curl < 7.19.4 Jeff King
@ 2017-08-09 13:14   ` Ævar Arnfjörð Bjarmason
  2017-08-09 13:38     ` Jeff King
  2017-08-09 17:34   ` [PATCH 3/4] http: drop support for curl < 7.19.4 Stefan Beller
  2017-08-10 12:36   ` Mischa POSLAWSKY
  2 siblings, 1 reply; 70+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-08-09 13:14 UTC (permalink / raw)
  To: Jeff King; +Cc: git


On Wed, Aug 09 2017, Jeff King jotted:

> 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

diff --git a/http.c b/http.c
index 5280511c74..527bc56dc2 100644
--- a/http.c
+++ b/http.c
@@ -103,9 +103,7 @@ static int http_auth_methods_restricted;
 /* Modes for which empty_auth cannot actually help us. */
 static unsigned long empty_auth_useless =
        CURLAUTH_BASIC
-#ifdef CURLAUTH_DIGEST_IE
        | CURLAUTH_DIGEST_IE
-#endif
        | CURLAUTH_DIGEST;

 static struct curl_slist *pragma_header;
@@ -706,10 +704,8 @@ static CURL *get_curl_handle(void)
        if (curl_ftp_no_epsv)
                curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);

-#ifdef CURLOPT_USE_SSL
        if (curl_ssl_try)
                curl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);
-#endif

        /*
         * CURL also examines these variables as a fallback; but we need to query
diff --git a/http.h b/http.h
index 29acfe8c55..66d2d3c539 100644
--- a/http.h
+++ b/http.h
@@ -16,15 +16,6 @@

 #define DEFAULT_MAX_REQUESTS 5

-/*
- * CURLOPT_USE_SSL was known as CURLOPT_FTP_SSL up to 7.16.4,
- * and the constants were known as CURLFTPSSL_*
-*/
-#if !defined(CURLOPT_USE_SSL) && defined(CURLOPT_FTP_SSL)
-#define CURLOPT_USE_SSL CURLOPT_FTP_SSL
-#define CURLUSESSL_TRY CURLFTPSSL_TRY
-#endif
-
 struct slot_results {
        CURLcode curl_result;
        long http_code;

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 3/4] http: drop support for curl < 7.19.4
  2017-08-09 13:14   ` Ævar Arnfjörð Bjarmason
@ 2017-08-09 13:38     ` Jeff King
  2017-08-09 13:49       ` [PATCH 5/4] curl: remove ifdef'd code never used with curl >=7.19.4 Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 70+ messages in thread
From: Jeff King @ 2017-08-09 13:38 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git

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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH 5/4] curl: remove ifdef'd code never used with curl >=7.19.4
  2017-08-09 13:38     ` Jeff King
@ 2017-08-09 13:49       ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 70+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-08-09 13:49 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason

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 <avarab@gmail.com>
---

On Wed, Aug 9, 2017 at 3:38 PM, Jeff King <peff@peff.net> wrote:
> 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.

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(-)

diff --git a/http.c b/http.c
index 5280511c74..527bc56dc2 100644
--- a/http.c
+++ b/http.c
@@ -103,9 +103,7 @@ static int http_auth_methods_restricted;
 /* Modes for which empty_auth cannot actually help us. */
 static unsigned long empty_auth_useless =
 	CURLAUTH_BASIC
-#ifdef CURLAUTH_DIGEST_IE
 	| CURLAUTH_DIGEST_IE
-#endif
 	| CURLAUTH_DIGEST;
 
 static struct curl_slist *pragma_header;
@@ -706,10 +704,8 @@ static CURL *get_curl_handle(void)
 	if (curl_ftp_no_epsv)
 		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
 
-#ifdef CURLOPT_USE_SSL
 	if (curl_ssl_try)
 		curl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);
-#endif
 
 	/*
 	 * CURL also examines these variables as a fallback; but we need to query
diff --git a/http.h b/http.h
index 29acfe8c55..66d2d3c539 100644
--- a/http.h
+++ b/http.h
@@ -16,15 +16,6 @@
 
 #define DEFAULT_MAX_REQUESTS 5
 
-/*
- * CURLOPT_USE_SSL was known as CURLOPT_FTP_SSL up to 7.16.4,
- * and the constants were known as CURLFTPSSL_*
-*/
-#if !defined(CURLOPT_USE_SSL) && defined(CURLOPT_FTP_SSL)
-#define CURLOPT_USE_SSL CURLOPT_FTP_SSL
-#define CURLUSESSL_TRY CURLFTPSSL_TRY
-#endif
-
 struct slot_results {
 	CURLcode curl_result;
 	long http_code;
-- 
2.14.0.rc1.383.gd1ce394fe2


^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 2/4] http: drop support for curl < 7.16.0
  2017-08-09 12:01 ` [PATCH 2/4] http: drop support for curl < 7.16.0 Jeff King
@ 2017-08-09 17:29   ` Stefan Beller
  2017-08-09 21:13     ` Jeff King
  2017-08-09 17:40   ` Junio C Hamano
  1 sibling, 1 reply; 70+ messages in thread
From: Stefan Beller @ 2017-08-09 17:29 UTC (permalink / raw)
  To: Jeff King; +Cc: git

On Wed, Aug 9, 2017 at 5:01 AM, Jeff King <peff@peff.net> 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 <peff@peff.net>
> ---
>  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?

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 3/4] http: drop support for curl < 7.19.4
  2017-08-09 12:02 ` [PATCH 3/4] http: drop support for curl < 7.19.4 Jeff King
  2017-08-09 13:14   ` Ævar Arnfjörð Bjarmason
@ 2017-08-09 17:34   ` Stefan Beller
  2017-08-09 21:19     ` Jeff King
  2017-08-10 12:36   ` Mischa POSLAWSKY
  2 siblings, 1 reply; 70+ messages in thread
From: Stefan Beller @ 2017-08-09 17:34 UTC (permalink / raw)
  To: Jeff King; +Cc: git

On Wed, Aug 9, 2017 at 5:02 AM, Jeff King <peff@peff.net> 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.

I would not state it as bland, see how
https://public-inbox.org/git/20170806233850.14711-1-avarab@gmail.com/
came here to the mailing list.


> 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!

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 4/4] http: #error on too-old curl
  2017-08-09 12:02 ` [PATCH 4/4] http: #error on too-old curl Jeff King
@ 2017-08-09 17:37   ` Stefan Beller
  0 siblings, 0 replies; 70+ messages in thread
From: Stefan Beller @ 2017-08-09 17:37 UTC (permalink / raw)
  To: Jeff King; +Cc: git

On Wed, Aug 9, 2017 at 5:02 AM, Jeff King <peff@peff.net> wrote:
> 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 <peff@peff.net>
> ---
>  http.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/http.h b/http.h
> index da4d8589d8..29acfe8c55 100644
> --- a/http.h
> +++ b/http.h
> @@ -10,6 +10,10 @@
>  #include "remote.h"
>  #include "url.h"
>
> +#if LIBCURL_VERSION_NUM < 0x071304

Oh, it's hex. 0x13 == 19. Makes sense.

Thanks,
Stefan

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 2/4] http: drop support for curl < 7.16.0
  2017-08-09 12:01 ` [PATCH 2/4] http: drop support for curl < 7.16.0 Jeff King
  2017-08-09 17:29   ` Stefan Beller
@ 2017-08-09 17:40   ` Junio C Hamano
  2017-08-09 18:03     ` Nicolas Morey-Chaisemartin
  2017-08-09 21:15     ` Jeff King
  1 sibling, 2 replies; 70+ messages in thread
From: Junio C Hamano @ 2017-08-09 17:40 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Jeff King <peff@peff.net> writes:

> -#if LIBCURL_VERSION_NUM >= 0x071700
> -/* Use CURLOPT_KEYPASSWD as is */
> -#elif LIBCURL_VERSION_NUM >= 0x070903
> -#define CURLOPT_KEYPASSWD CURLOPT_SSLKEYPASSWD
> -#else
> -#define CURLOPT_KEYPASSWD CURLOPT_SSLCERTPASSWD
> -#endif
> -

This part I am not sure.  Don't we still need to substitute
CURLOPT_KEYPASSWD with CURLOPT_SSLKEYPASSWD for versions below
071700, e.g. 071000 which is 7.16.0?

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 2/4] http: drop support for curl < 7.16.0
  2017-08-09 17:40   ` Junio C Hamano
@ 2017-08-09 18:03     ` Nicolas Morey-Chaisemartin
  2017-08-09 21:17       ` Jeff King
  2017-08-09 21:15     ` Jeff King
  1 sibling, 1 reply; 70+ messages in thread
From: Nicolas Morey-Chaisemartin @ 2017-08-09 18:03 UTC (permalink / raw)
  To: Junio C Hamano, Jeff King; +Cc: git



Le 09/08/2017 à 19:40, Junio C Hamano a écrit :
> Jeff King <peff@peff.net> writes:
>
>> -#if LIBCURL_VERSION_NUM >= 0x071700
>> -/* Use CURLOPT_KEYPASSWD as is */
>> -#elif LIBCURL_VERSION_NUM >= 0x070903
>> -#define CURLOPT_KEYPASSWD CURLOPT_SSLKEYPASSWD
>> -#else
>> -#define CURLOPT_KEYPASSWD CURLOPT_SSLCERTPASSWD
>> -#endif
>> -
> This part I am not sure.  Don't we still need to substitute
> CURLOPT_KEYPASSWD with CURLOPT_SSLKEYPASSWD for versions below
> 071700, e.g. 071000 which is 7.16.0?
According to the documentation:

https://curl.haxx.se/libcurl/c/CURLOPT_KEYPASSWD.html
This option was known as CURLOPT_SSLKEYPASSWD up to 7.16.4 and CURLOPT_SSLCERTPASSWD up to 7.9.2.


So the patch breaks things (broken for 7.16.[0-4]). But the series does not as the next patch ensure at least 7.19.4



^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 2/4] http: drop support for curl < 7.16.0
  2017-08-09 17:29   ` Stefan Beller
@ 2017-08-09 21:13     ` Jeff King
  0 siblings, 0 replies; 70+ messages in thread
From: Jeff King @ 2017-08-09 21:13 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git

On Wed, Aug 09, 2017 at 10:29:04AM -0700, Stefan Beller wrote:

> >  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?

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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 2/4] http: drop support for curl < 7.16.0
  2017-08-09 17:40   ` Junio C Hamano
  2017-08-09 18:03     ` Nicolas Morey-Chaisemartin
@ 2017-08-09 21:15     ` Jeff King
  1 sibling, 0 replies; 70+ messages in thread
From: Jeff King @ 2017-08-09 21:15 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Wed, Aug 09, 2017 at 10:40:13AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > -#if LIBCURL_VERSION_NUM >= 0x071700
> > -/* Use CURLOPT_KEYPASSWD as is */
> > -#elif LIBCURL_VERSION_NUM >= 0x070903
> > -#define CURLOPT_KEYPASSWD CURLOPT_SSLKEYPASSWD
> > -#else
> > -#define CURLOPT_KEYPASSWD CURLOPT_SSLCERTPASSWD
> > -#endif
> > -
> 
> This part I am not sure.  Don't we still need to substitute
> CURLOPT_KEYPASSWD with CURLOPT_SSLKEYPASSWD for versions below
> 071700, e.g. 071000 which is 7.16.0?

Yeah, you're right. I'm not sure how I botched that.

Thanks for reading carefully. I'll fix it in a re-roll.

-Peff

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 2/4] http: drop support for curl < 7.16.0
  2017-08-09 18:03     ` Nicolas Morey-Chaisemartin
@ 2017-08-09 21:17       ` Jeff King
  2017-08-09 21:29         ` Nicolas Morey-Chaisemartin
  0 siblings, 1 reply; 70+ messages in thread
From: Jeff King @ 2017-08-09 21:17 UTC (permalink / raw)
  To: Nicolas Morey-Chaisemartin; +Cc: Junio C Hamano, git

On Wed, Aug 09, 2017 at 08:03:05PM +0200, Nicolas Morey-Chaisemartin wrote:

> >> -#if LIBCURL_VERSION_NUM >= 0x071700
> >> -/* Use CURLOPT_KEYPASSWD as is */
> >> -#elif LIBCURL_VERSION_NUM >= 0x070903
> >> -#define CURLOPT_KEYPASSWD CURLOPT_SSLKEYPASSWD
> >> -#else
> >> -#define CURLOPT_KEYPASSWD CURLOPT_SSLCERTPASSWD
> >> -#endif
> >> -
> > This part I am not sure.  Don't we still need to substitute
> > CURLOPT_KEYPASSWD with CURLOPT_SSLKEYPASSWD for versions below
> > 071700, e.g. 071000 which is 7.16.0?
> According to the documentation:
> 
> https://curl.haxx.se/libcurl/c/CURLOPT_KEYPASSWD.html
> This option was known as CURLOPT_SSLKEYPASSWD up to 7.16.4 and
> CURLOPT_SSLCERTPASSWD up to 7.9.2.
> 
> 
> So the patch breaks things (broken for 7.16.[0-4]). But the series
> does not as the next patch ensure at least 7.19.4

But the #ifdef above says 071700, which is 7.23.0. I wonder if we just
got it wrong back then (maybe hex confusion with 7.17.0?). I have a
build setup for old versions of curl, so I'll double-check that 7.19.4
builds with KEYPASSWD. And dig in the history to see if there's any
comment on this mismatch.

-Peff

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 3/4] http: drop support for curl < 7.19.4
  2017-08-09 17:34   ` [PATCH 3/4] http: drop support for curl < 7.19.4 Stefan Beller
@ 2017-08-09 21:19     ` Jeff King
  0 siblings, 0 replies; 70+ messages in thread
From: Jeff King @ 2017-08-09 21:19 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git

On Wed, Aug 09, 2017 at 10:34:09AM -0700, Stefan Beller wrote:

> On Wed, Aug 9, 2017 at 5:02 AM, Jeff King <peff@peff.net> 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.
> 
> I would not state it as bland, see how
> https://public-inbox.org/git/20170806233850.14711-1-avarab@gmail.com/
> came here to the mailing list.

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.

> > 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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 2/4] http: drop support for curl < 7.16.0
  2017-08-09 21:17       ` Jeff King
@ 2017-08-09 21:29         ` Nicolas Morey-Chaisemartin
  2017-08-09 21:49           ` Jeff King
  0 siblings, 1 reply; 70+ messages in thread
From: Nicolas Morey-Chaisemartin @ 2017-08-09 21:29 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git



Le 09/08/2017 à 23:17, Jeff King a écrit :
> On Wed, Aug 09, 2017 at 08:03:05PM +0200, Nicolas Morey-Chaisemartin wrote:
>
>>>> -#if LIBCURL_VERSION_NUM >= 0x071700
>>>> -/* Use CURLOPT_KEYPASSWD as is */
>>>> -#elif LIBCURL_VERSION_NUM >= 0x070903
>>>> -#define CURLOPT_KEYPASSWD CURLOPT_SSLKEYPASSWD
>>>> -#else
>>>> -#define CURLOPT_KEYPASSWD CURLOPT_SSLCERTPASSWD
>>>> -#endif
>>>> -
>>> This part I am not sure.  Don't we still need to substitute
>>> CURLOPT_KEYPASSWD with CURLOPT_SSLKEYPASSWD for versions below
>>> 071700, e.g. 071000 which is 7.16.0?
>> According to the documentation:
>>
>> https://curl.haxx.se/libcurl/c/CURLOPT_KEYPASSWD.html
>> This option was known as CURLOPT_SSLKEYPASSWD up to 7.16.4 and
>> CURLOPT_SSLCERTPASSWD up to 7.9.2.
>>
>>
>> So the patch breaks things (broken for 7.16.[0-4]). But the series
>> does not as the next patch ensure at least 7.19.4
> But the #ifdef above says 071700, which is 7.23.0. I wonder if we just
> got it wrong back then (maybe hex confusion with 7.17.0?). I have a
> build setup for old versions of curl, so I'll double-check that 7.19.4
> builds with KEYPASSWD. And dig in the history to see if there's any
> comment on this mismatch.
>
> -Peff
It seems to be a decimal/hex issue:
docs/libcurl/symbols-in-versions:153:CURLOPT_KEYPASSWD               7.17.0

I guess it should still work because it is now defined like this:
curl.h:#define CURLOPT_SSLKEYPASSWD CURLOPT_KEYPASSWD

If I'm not mistaken on cpp behaviour it means CURLOPT_KEYPASSWD is evaluated to CURLOPT_SSLKEYPASSWD (git define) which is evaluated into CURLOPT_KEYPASSWD (curl define).
It should stop here as CURLOPT_KEYPASSWD was not a defined macro when the curl one was evaluated.
It might be worth cleaning though, specially it wouldn't work anymore if the git macro is ever moved before the curl include.

Nicolas



^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-09 12:00 [PATCH 0/4] dropping support for older curl Jeff King
                   ` (3 preceding siblings ...)
  2017-08-09 12:02 ` [PATCH 4/4] http: #error on too-old curl Jeff King
@ 2017-08-09 21:42 ` Johannes Schindelin
  2017-08-09 21:47   ` Jeff King
  2017-08-09 23:39   ` [PATCH 0/4] dropping support for older curl Ævar Arnfjörð Bjarmason
       [not found] ` <87zib8g8ub.fsf@gmail.com>
  2021-07-21 22:22 ` [PATCH v2 0/5] drop support for ancient curl Ævar Arnfjörð Bjarmason
  6 siblings, 2 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-08-09 21:42 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Hi Peff,

On Wed, 9 Aug 2017, Jeff King wrote:

> This is a resurrection of the thread from April:
> 
>   https://public-inbox.org/git/20170404025438.bgxz5sfmrawqswcj@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?

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-09 21:42 ` [PATCH 0/4] dropping support for older curl Johannes Schindelin
@ 2017-08-09 21:47   ` Jeff King
  2017-08-10  9:01     ` Tom G. Christensen
                       ` (2 more replies)
  2017-08-09 23:39   ` [PATCH 0/4] dropping support for older curl Ævar Arnfjörð Bjarmason
  1 sibling, 3 replies; 70+ messages in thread
From: Jeff King @ 2017-08-09 21:47 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git

On Wed, Aug 09, 2017 at 11:42:12PM +0200, Johannes Schindelin wrote:

> > This is a resurrection of the thread from April:
> > 
> >   https://public-inbox.org/git/20170404025438.bgxz5sfmrawqswcj@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.

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/

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 2/4] http: drop support for curl < 7.16.0
  2017-08-09 21:29         ` Nicolas Morey-Chaisemartin
@ 2017-08-09 21:49           ` Jeff King
  0 siblings, 0 replies; 70+ messages in thread
From: Jeff King @ 2017-08-09 21:49 UTC (permalink / raw)
  To: Nicolas Morey-Chaisemartin; +Cc: Junio C Hamano, git

On Wed, Aug 09, 2017 at 11:29:30PM +0200, Nicolas Morey-Chaisemartin wrote:

> > But the #ifdef above says 071700, which is 7.23.0. I wonder if we just
> > got it wrong back then (maybe hex confusion with 7.17.0?). I have a
> > build setup for old versions of curl, so I'll double-check that 7.19.4
> > builds with KEYPASSWD. And dig in the history to see if there's any
> > comment on this mismatch.
>
> It seems to be a decimal/hex issue:
> docs/libcurl/symbols-in-versions:153:CURLOPT_KEYPASSWD               7.17.0
> 
> I guess it should still work because it is now defined like this:
> curl.h:#define CURLOPT_SSLKEYPASSWD CURLOPT_KEYPASSWD
> 
> If I'm not mistaken on cpp behaviour it means CURLOPT_KEYPASSWD is
> evaluated to CURLOPT_SSLKEYPASSWD (git define) which is evaluated into
> CURLOPT_KEYPASSWD (curl define).
>
> It should stop here as CURLOPT_KEYPASSWD was not a defined macro when
> the curl one was evaluated.  It might be worth cleaning though,
> specially it wouldn't work anymore if the git macro is ever moved
> before the curl include.

Hmph. That makes me think the original should have just been using
CURLOPT_SSLKEYPASSWD through the code, if curl was providing
a backwards-compatible macro. But it won't matter either way if we just
get rid of it. :)

Thanks for digging up the curl history.

-Peff

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-09 21:42 ` [PATCH 0/4] dropping support for older curl Johannes Schindelin
  2017-08-09 21:47   ` Jeff King
@ 2017-08-09 23:39   ` Ævar Arnfjörð Bjarmason
  1 sibling, 0 replies; 70+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-08-09 23:39 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Jeff King, git


On Wed, Aug 09 2017, Johannes Schindelin jotted:

> Hi Peff,
>
> On Wed, 9 Aug 2017, Jeff King wrote:
>
>> This is a resurrection of the thread from April:
>>
>>   https://public-inbox.org/git/20170404025438.bgxz5sfmrawqswcj@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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-09 21:47   ` Jeff King
@ 2017-08-10  9:01     ` Tom G. Christensen
  2017-08-10  9:36     ` Johannes Schindelin
  2017-08-10 20:33     ` Tom G. Christensen
  2 siblings, 0 replies; 70+ messages in thread
From: Tom G. Christensen @ 2017-08-10  9:01 UTC (permalink / raw)
  To: Jeff King, Johannes Schindelin; +Cc: git

On 09/08/17 23:47, Jeff King wrote:
> On Wed, Aug 09, 2017 at 11:42:12PM +0200, Johannes Schindelin wrote:
>> 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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-09 21:47   ` Jeff King
  2017-08-10  9:01     ` Tom G. Christensen
@ 2017-08-10  9:36     ` Johannes Schindelin
  2017-08-10 21:33       ` Jeff King
       [not found]       ` <CAHVLzcnnrABmkYNg31Aq99NgBbyuCKEM60pHGygyjXbjmaUEYQ@mail.gmail.com>
  2017-08-10 20:33     ` Tom G. Christensen
  2 siblings, 2 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-08-10  9:36 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Hi Peff,

On Wed, 9 Aug 2017, Jeff King wrote:

> On Wed, Aug 09, 2017 at 11:42:12PM +0200, Johannes Schindelin wrote:
> 
> > > This is a resurrection of the thread from April:
> > > 
> > >   https://public-inbox.org/git/20170404025438.bgxz5sfmrawqswcj@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:

> > 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.

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: Dropping support for older perl
       [not found] ` <87zib8g8ub.fsf@gmail.com>
@ 2017-08-10 10:04   ` Tom G. Christensen
  0 siblings, 0 replies; 70+ messages in thread
From: Tom G. Christensen @ 2017-08-10 10:04 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, Jeff King
  Cc: git, Tor Arntsen, Randal L. Schwartz, Jakub Narebski

On 09/08/17 15:38, Ævar Arnfjörð Bjarmason wrote:

> RHEL/CentOS 5.x has perl 5.8.8, but it also has curl 7.15.5[1] which is
> obseleted by these curl patches. Maybe we'd want to be more conservative
> with perl for whatever reason, but I'd like to at least bump our
> requirenment of 5.8.0 to 5.8.8. Those releases are 4 years apart, and a
> lot of bugs were fixed[2], and some constructs / modules have newer APIs
> we could use.
> 
> But if we do the thing corresponding to these curl patches we should
> bump the dependency to 5.10.1, that was released in August 2009 (and the
> curl version JK is bumping us to in March 2009), and 5.10.1 is shipped
> with RHEL/CentOS 6.
> 

I agree with your thoughts.
Though I'm a bit biased since I only really care about RHEL/CentOS in 
the context of being able to use vendor provided versions of curl and perl.

> The bump to 5.10.1 may be a bad idea, I know AIX/HPUX/Solaris and some
> others have historically been more conservative about upgrading perl
> than stuff like libcurl since it's in the base system.
> 

AFAIK it used to be common to build updated versions at least on Solaris.
I provide perl 5.16.x and a recent curl for Solaris 2.6-9 as part of 
tgcware(1) and Solaris 10/11 users can use OpenCSW which seems to have 
5.10.1 available.

-tgc

1) https://jupiterrise.com/tgcware/

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 3/4] http: drop support for curl < 7.19.4
  2017-08-09 12:02 ` [PATCH 3/4] http: drop support for curl < 7.19.4 Jeff King
  2017-08-09 13:14   ` Ævar Arnfjörð Bjarmason
  2017-08-09 17:34   ` [PATCH 3/4] http: drop support for curl < 7.19.4 Stefan Beller
@ 2017-08-10 12:36   ` Mischa POSLAWSKY
  2017-08-10 17:34     ` Jeff King
  2 siblings, 1 reply; 70+ messages in thread
From: Mischa POSLAWSKY @ 2017-08-10 12:36 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Jeff King wrote:
> -#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);
> -#endif

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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 3/4] http: drop support for curl < 7.19.4
  2017-08-10 12:36   ` Mischa POSLAWSKY
@ 2017-08-10 17:34     ` Jeff King
  0 siblings, 0 replies; 70+ messages in thread
From: Jeff King @ 2017-08-10 17:34 UTC (permalink / raw)
  To: Mischa POSLAWSKY; +Cc: git

On Thu, Aug 10, 2017 at 02:36:41PM +0200, Mischa POSLAWSKY wrote:

> Jeff King wrote:
> > -#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);
> > -#endif
> 
> 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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-09 21:47   ` Jeff King
  2017-08-10  9:01     ` Tom G. Christensen
  2017-08-10  9:36     ` Johannes Schindelin
@ 2017-08-10 20:33     ` Tom G. Christensen
  2017-08-10 21:32       ` Jeff King
  2 siblings, 1 reply; 70+ messages in thread
From: Tom G. Christensen @ 2017-08-10 20:33 UTC (permalink / raw)
  To: Jeff King, Johannes Schindelin; +Cc: git

[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:
>> 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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-10 20:33     ` Tom G. Christensen
@ 2017-08-10 21:32       ` Jeff King
  2017-08-10 22:23         ` Tom G. Christensen
  0 siblings, 1 reply; 70+ messages in thread
From: Jeff King @ 2017-08-10 21:32 UTC (permalink / raw)
  To: Tom G. Christensen; +Cc: Johannes Schindelin, git

On Thu, Aug 10, 2017 at 10:33:18PM +0200, Tom G. Christensen wrote:

> > 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).

> > 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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-10  9:36     ` Johannes Schindelin
@ 2017-08-10 21:33       ` Jeff King
  2017-08-10 22:17         ` Junio C Hamano
       [not found]       ` <CAHVLzcnnrABmkYNg31Aq99NgBbyuCKEM60pHGygyjXbjmaUEYQ@mail.gmail.com>
  1 sibling, 1 reply; 70+ messages in thread
From: Jeff King @ 2017-08-10 21:33 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git

On Thu, Aug 10, 2017 at 11:36:41AM +0200, Johannes Schindelin wrote:

> Hopefully I had better luck expressing my concerns this time?

I understand your argument much better now. I'm still not sure I agree.

-Peff

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-10 21:33       ` Jeff King
@ 2017-08-10 22:17         ` Junio C Hamano
  2017-08-10 23:09           ` Jeff King
  0 siblings, 1 reply; 70+ messages in thread
From: Junio C Hamano @ 2017-08-10 22:17 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin, git

Jeff King <peff@peff.net> writes:

> On Thu, Aug 10, 2017 at 11:36:41AM +0200, Johannes Schindelin wrote:
>
>> Hopefully I had better luck expressing my concerns this time?
>
> I understand your argument much better now. I'm still not sure I agree.
>
> -Peff

I do not think "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." was why you were suggesting to
clean up the (apparent) support of older curl in the code, though.

Isn't the reason why your series simplifies these #ifdefs away
because we by accident started using some features that require a
version that is even newer than any of these #ifdef's try to cater
to and yet nobody complained?  That is a lot more similar to the
removal of rsync transport that happened in a not so distant past,
where the reason for removal was "We have been shipping code that
couldn't have possibly worked for some time and nobody complained
---we know nobody is depending on it."

Or "We accidentally started shipping code with comma after the last
element of enum decl and nobody compalined---everybody's compiler
must be ready" ;-)

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-10 21:32       ` Jeff King
@ 2017-08-10 22:23         ` Tom G. Christensen
  2017-08-10 22:54           ` Jeff King
  0 siblings, 1 reply; 70+ messages in thread
From: Tom G. Christensen @ 2017-08-10 22:23 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin, git

On 10/08/17 23:32, Jeff King wrote:
> On Thu, Aug 10, 2017 at 10:33:18PM +0200, Tom G. Christensen wrote:
> 
>>> 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?
>
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.).

>> 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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-10 22:23         ` Tom G. Christensen
@ 2017-08-10 22:54           ` Jeff King
  2017-08-10 23:17             ` Tom G. Christensen
  0 siblings, 1 reply; 70+ messages in thread
From: Jeff King @ 2017-08-10 22:54 UTC (permalink / raw)
  To: Tom G. Christensen; +Cc: Johannes Schindelin, git

On Fri, Aug 11, 2017 at 12:23:42AM +0200, Tom G. Christensen wrote:

> > > 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.

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.

> > 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 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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-10 22:17         ` Junio C Hamano
@ 2017-08-10 23:09           ` Jeff King
  2017-08-11  0:17             ` Jeff King
  0 siblings, 1 reply; 70+ messages in thread
From: Jeff King @ 2017-08-10 23:09 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git

On Thu, Aug 10, 2017 at 03:17:06PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Thu, Aug 10, 2017 at 11:36:41AM +0200, Johannes Schindelin wrote:
> >
> >> Hopefully I had better luck expressing my concerns this time?
> >
> > I understand your argument much better now. I'm still not sure I agree.
> >
> > -Peff
> 
> I do not think "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." was why you were suggesting to
> clean up the (apparent) support of older curl in the code, though.
> 
> Isn't the reason why your series simplifies these #ifdefs away
> because we by accident started using some features that require a
> version that is even newer than any of these #ifdef's try to cater
> to and yet nobody complained?  That is a lot more similar to the
> removal of rsync transport that happened in a not so distant past,
> where the reason for removal was "We have been shipping code that
> couldn't have possibly worked for some time and nobody complained
> ---we know nobody is depending on it."

I think there are two questions to be asked, and their answers come from
different lines of reasoning.

The first is "should we eventually drop support for antiquated versions
of dependencies?". And the argument in favor is the one I was making
here: besides lowering maintenance cost, it is more honest to our users
about what to expect[1].

The second is "how far back should we keep support?".

And there are two lines of thought there.

One is to do it by date and what dependencies are in long-term OS
releases, and then compare that to the benefit. Requiring curl 7.11.1
still keeps us working back to rhel4, which was already end-of-lifed
completely after a 12 year run. Bumping to 7.16.0 drops rhel4 and rhel5,
the latter of which is in its final "barely supported" phase after 10
years. But it gives us a bit more bang for our buck by making CURL_MULTI
uconditional[2].  Requiring 7.19.4 actually doesn't drop any more rhel
releases. So by that metric, we might as well go there.

And the second line of thought is: it was already broken and nobody
reported it or offered up a fix. And that's where the "similar to rsync"
thing comes in. Though in this case we do have some evidence that people
(at least Tom) was patching and distributing behind the scenes. And our
breakage period was much shorter (since v2.12.0, but that's only months
or so).

-Peff

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-10 22:54           ` Jeff King
@ 2017-08-10 23:17             ` Tom G. Christensen
  2017-08-10 23:23               ` Jeff King
  0 siblings, 1 reply; 70+ messages in thread
From: Tom G. Christensen @ 2017-08-10 23:17 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin, git

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.

>>> 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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-10 23:17             ` Tom G. Christensen
@ 2017-08-10 23:23               ` Jeff King
  2017-08-10 23:36                 ` Tom G. Christensen
  0 siblings, 1 reply; 70+ messages in thread
From: Jeff King @ 2017-08-10 23:23 UTC (permalink / raw)
  To: Tom G. Christensen; +Cc: Johannes Schindelin, git

On Fri, Aug 11, 2017 at 01:17:51AM +0200, Tom G. Christensen wrote:

> > 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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-10 23:23               ` Jeff King
@ 2017-08-10 23:36                 ` Tom G. Christensen
  2017-08-11 16:37                   ` [PATCH 0/2] http: handle curl with vendor backports Tom G. Christensen
                                     ` (2 more replies)
  0 siblings, 3 replies; 70+ messages in thread
From: Tom G. Christensen @ 2017-08-10 23:36 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin, git

On 11/08/17 01:23, Jeff King wrote:
> On Fri, Aug 11, 2017 at 01:17:51AM +0200, Tom G. Christensen wrote:
> 
>>> 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.

>> 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.
> 

Yes, a feature test would be better.

-tgc

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
  2017-08-10 23:09           ` Jeff King
@ 2017-08-11  0:17             ` Jeff King
  0 siblings, 0 replies; 70+ messages in thread
From: Jeff King @ 2017-08-11  0:17 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git

On Thu, Aug 10, 2017 at 07:09:02PM -0400, Jeff King wrote:

> The first is "should we eventually drop support for antiquated versions
> of dependencies?". And the argument in favor is the one I was making
> here: besides lowering maintenance cost, it is more honest to our users
> about what to expect[1].

As usual, I forgot all my footnotes.

[1] When I've talked about keeping expectations reasonable, I'm a lot
    less interested in "oops, I built Git and this particular feature
    didn't work". It's easy to write that off as "well, you have an old
    version of curl, patches welcome". I'm much more concerned about
    security issues. Curl is network-facing. Leaving aside security
    vulnerabilities in curl itself (which hopefully distros with 10-year
    support periods would fix), I wouldn't be surprised if there are
    bad interactions possible due to our tangle of ifdefs.

    One way to address that would be more careful auditing. But then
    that goes back to the cost/benefit thing.

> One is to do it by date and what dependencies are in long-term OS
> releases, and then compare that to the benefit. Requiring curl 7.11.1
> still keeps us working back to rhel4, which was already end-of-lifed
> completely after a 12 year run. Bumping to 7.16.0 drops rhel4 and rhel5,
> the latter of which is in its final "barely supported" phase after 10
> years. But it gives us a bit more bang for our buck by making CURL_MULTI
> uconditional[2].  Requiring 7.19.4 actually doesn't drop any more rhel
> releases. So by that metric, we might as well go there.

[2] The line-count change from dropping CURL_MULTI isn't _too_ exciting.
    But a lot of the tangled design of our http code revolves around
    the abstractions we've introduced. I have a feeling that it will
    enable further cleanups as we move forward (OTOH, a lot of the worst
    parts of our design are because of _using_ curl_multi for dumb http,
    which of course hardly anyone does these days. But I have a feeling
    if I suggested removing that, people would really scream).

-Peff

^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH 0/2] http: handle curl with vendor backports
  2017-08-10 23:36                 ` Tom G. Christensen
@ 2017-08-11 16:37                   ` Tom G. Christensen
  2017-08-11 22:15                     ` Junio C Hamano
  2017-08-11 16:37                   ` [PATCH 1/2] http: Fix handling of missing CURLPROTO_* Tom G. Christensen
  2017-08-11 16:37                   ` [PATCH 2/2] http: use a feature check to enable GSSAPI delegation control Tom G. Christensen
  2 siblings, 1 reply; 70+ messages in thread
From: Tom G. Christensen @ 2017-08-11 16:37 UTC (permalink / raw)
  To: git; +Cc: Tom G. Christensen

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


^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH 1/2] http: Fix handling of missing CURLPROTO_*
  2017-08-10 23:36                 ` Tom G. Christensen
  2017-08-11 16:37                   ` [PATCH 0/2] http: handle curl with vendor backports Tom G. Christensen
@ 2017-08-11 16:37                   ` Tom G. Christensen
  2017-08-12  0:30                     ` Junio C Hamano
  2017-08-11 16:37                   ` [PATCH 2/2] http: use a feature check to enable GSSAPI delegation control Tom G. Christensen
  2 siblings, 1 reply; 70+ messages in thread
From: Tom G. Christensen @ 2017-08-11 16:37 UTC (permalink / raw)
  To: git; +Cc: Tom G. Christensen

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 <tgc@jupiterrise.com>
---
 http.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/http.c b/http.c
index e00264cff..569909e8a 100644
--- a/http.c
+++ b/http.c
@@ -685,6 +685,7 @@ 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;
@@ -700,6 +701,7 @@ static long get_curl_allowed_protocols(int from_user)
 
 	return allowed_protocols;
 }
+#endif
 
 static CURL *get_curl_handle(void)
 {
@@ -798,7 +800,7 @@ static CURL *get_curl_handle(void)
 #elif LIBCURL_VERSION_NUM >= 0x071101
 	curl_easy_setopt(result, CURLOPT_POST301, 1);
 #endif
-#if LIBCURL_VERSION_NUM >= 0x071304
+#ifdef CURLPROTO_HTTP
 	curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
 			 get_curl_allowed_protocols(0));
 	curl_easy_setopt(result, CURLOPT_PROTOCOLS,
-- 
2.14.1


^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH 2/2] http: use a feature check to enable GSSAPI delegation control
  2017-08-10 23:36                 ` Tom G. Christensen
  2017-08-11 16:37                   ` [PATCH 0/2] http: handle curl with vendor backports Tom G. Christensen
  2017-08-11 16:37                   ` [PATCH 1/2] http: Fix handling of missing CURLPROTO_* Tom G. Christensen
@ 2017-08-11 16:37                   ` Tom G. Christensen
  2 siblings, 0 replies; 70+ messages in thread
From: Tom G. Christensen @ 2017-08-11 16:37 UTC (permalink / raw)
  To: git; +Cc: Tom G. Christensen

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 <tgc@jupiterrise.com>
---
 http.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/http.c b/http.c
index 569909e8a..a3ae58f13 100644
--- a/http.c
+++ b/http.c
@@ -91,7 +91,7 @@ static struct {
 	 * here, too
 	 */
 };
-#if LIBCURL_VERSION_NUM >= 0x071600
+#ifdef CURLGSSAPI_DELEGATION_FLAG
 static const char *curl_deleg;
 static struct {
 	const char *name;
@@ -356,7 +356,7 @@ static int http_options(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp("http.delegation", var)) {
-#if LIBCURL_VERSION_NUM >= 0x071600
+#ifdef CURLGSSAPI_DELEGATION_FLAG
 		return git_config_string(&curl_deleg, var, value);
 #else
 		warning(_("Delegation control is not supported with cURL < 7.22.0"));
@@ -727,7 +727,7 @@ static CURL *get_curl_handle(void)
 	curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
 #endif
 
-#if LIBCURL_VERSION_NUM >= 0x071600
+#ifdef CURLGSSAPI_DELEGATION_FLAG
 	if (curl_deleg) {
 		int i;
 		for (i = 0; i < ARRAY_SIZE(curl_deleg_levels); i++) {
-- 
2.14.1


^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/2] http: handle curl with vendor backports
  2017-08-11 16:37                   ` [PATCH 0/2] http: handle curl with vendor backports Tom G. Christensen
@ 2017-08-11 22:15                     ` Junio C Hamano
  2017-08-12  6:20                       ` Tom G. Christensen
  2017-08-20  8:47                       ` Jeff King
  0 siblings, 2 replies; 70+ messages in thread
From: Junio C Hamano @ 2017-08-11 22:15 UTC (permalink / raw)
  To: Tom G. Christensen; +Cc: git, Jeff King

"Tom G. Christensen" <tgc@jupiterrise.com> writes:

> 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.

Thanks; these feature macros ought to be more dependable, and I
think this moves things in the right direction (regardless of which
features we might later pick as mandatory and cut off supports for
older versions).

> 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.

Hmph, what does "(fails)" mean here?

>
> 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(-)

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 1/2] http: Fix handling of missing CURLPROTO_*
  2017-08-11 16:37                   ` [PATCH 1/2] http: Fix handling of missing CURLPROTO_* Tom G. Christensen
@ 2017-08-12  0:30                     ` Junio C Hamano
  2017-08-12  9:04                       ` Tom G. Christensen
  2017-08-20  8:59                       ` Jeff King
  0 siblings, 2 replies; 70+ messages in thread
From: Junio C Hamano @ 2017-08-12  0:30 UTC (permalink / raw)
  To: Tom G. Christensen; +Cc: git, Jeff King

"Tom G. Christensen" <tgc@jupiterrise.com> writes:

> 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 <tgc@jupiterrise.com>
> ---
>  http.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/http.c b/http.c
> index e00264cff..569909e8a 100644
> --- a/http.c
> +++ b/http.c
> @@ -685,6 +685,7 @@ 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;
> @@ -700,6 +701,7 @@ static long get_curl_allowed_protocols(int from_user)
>  
>  	return allowed_protocols;
>  }
> +#endif
>  
>  static CURL *get_curl_handle(void)
>  {
> @@ -798,7 +800,7 @@ static CURL *get_curl_handle(void)
>  #elif LIBCURL_VERSION_NUM >= 0x071101
>  	curl_easy_setopt(result, CURLOPT_POST301, 1);
>  #endif
> -#if LIBCURL_VERSION_NUM >= 0x071304
> +#ifdef CURLPROTO_HTTP
>  	curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
>  			 get_curl_allowed_protocols(0));
>  	curl_easy_setopt(result, CURLOPT_PROTOCOLS,

This may make the code to _compile_, but is it sensible to let the
code build and be used by the end users without the "these protocols
are safe" filter, I wonder?  

Granted, ancient code was unsafe and people were happily using it,
but now we know better, and more importantly, we have since added
users of transport (e.g. blindly fetch submodules recursively) that
may _rely_ on this layer of the code safely filtering unsafe
protocols, so...


^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/2] http: handle curl with vendor backports
  2017-08-11 22:15                     ` Junio C Hamano
@ 2017-08-12  6:20                       ` Tom G. Christensen
  2017-08-20  8:47                       ` Jeff King
  1 sibling, 0 replies; 70+ messages in thread
From: Tom G. Christensen @ 2017-08-12  6:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King

On 12/08/17 00:15, Junio C Hamano wrote:
> "Tom G. Christensen" <tgc@jupiterrise.com> writes:

>> 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.
> 
> Hmph, what does "(fails)" mean here?
> 

It means building against 7.12.0 fails which is expected because it is 
missing CURLINFO_SSL_DATA_{IN,OUT}. There are patches in the other 
thread that would add support for curl < 7.12.1 if necessary.

-tgc

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 1/2] http: Fix handling of missing CURLPROTO_*
  2017-08-12  0:30                     ` Junio C Hamano
@ 2017-08-12  9:04                       ` Tom G. Christensen
  2017-08-20  8:59                       ` Jeff King
  1 sibling, 0 replies; 70+ messages in thread
From: Tom G. Christensen @ 2017-08-12  9:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King

On 12/08/17 02:30, Junio C Hamano wrote:
> This may make the code to _compile_, but is it sensible to let the
> code build and be used by the end users without the "these protocols
> are safe" filter, I wonder?
> 

Git will display a warning at runtime if this is not available but 
perhaps this warning could be worded more strongly and/or make reference 
to CVE-2009-0037.

-tgc

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/4] dropping support for older curl
       [not found]       ` <CAHVLzcnnrABmkYNg31Aq99NgBbyuCKEM60pHGygyjXbjmaUEYQ@mail.gmail.com>
@ 2017-08-14 21:50         ` Johannes Schindelin
  0 siblings, 0 replies; 70+ messages in thread
From: Johannes Schindelin @ 2017-08-14 21:50 UTC (permalink / raw)
  To: Paolo Ciarrocchi; +Cc: git, Jeff King

Hi Paolo,

On Thu, 10 Aug 2017, Paolo Ciarrocchi wrote:

> Il 10 ago 2017 11:39 AM, "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
> ha scritto:
> 
> 
> 
> 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.
> 
> 
> Isn't SumbitGit a possible answer to your doubts (I strongly agree with
> you) about the current development process?

No. I hate to say that SubmitGit neither integrates well with GitHub Pull
Requests (code comments on GitHub are approximately 1,523x easier to
write, read, associate with the actual code, and see the current state of,
compared to the mailing list, and SubmitGit does not even hint at
integrating with that user experience).

Also, the barrier to start using SubmitGit is rather high. If you open a
Pull Request on github.com/git/git, you get *no* indication that SubmitGit
is an option to *actually* get the code into Git. There are also concerns
about required permissions that Junio Hamano himself would not accept.

Now, let's assume that you submitted the code via SubmitGit. The
challenges of the patch submission process do not end there, yet SubmitGit
goes home and has a beer. But the hard part, the discussions on the
mailing list, the status updates in the completely separate What's cooking
mails, the missing links back to the original source code (let alone the
information in which worktree on your computer under which branch name
that topic was developed again?), the diverging mail threads, the
"rerolls" that should not forget to Cc: all reviewers of previous rounds,
all that jazz is still all very, very manual.

And even if there was an easier path from having a local branch that works
to finally getting it onto the list in the required form, your mail is an
eloquent example of one of the most preposterous hurdles along the way: we
pride ourselves with the openness we demonstrate by communicating via a
mailing list, everybody has a mail address, amirite? But of course, HTML
mails, like, about 130% of all mails on the internet (or so it feels),
including yours, are dropped. Silently. Not so open anymore, are we.

It is all so frustrating, really. I work in a team (Visual Studio Team
Services, you can think of it as kind of a Microsoft take on Git hosting
plus a lot of other tooling) where we get really, really positive feedback
regarding our user experience, in particular the frequent enhancements to
PRs. It is really powerful to open, review and merge PRs, interact with
other developers, see the state of PRs and their respective discussions,
open and resolve issues, automate workflows and build tasks [*1*]. And
then I try to convince people here on this mailing list that it really
makes a difference if you start using tools to lighten the load of
everybody, and... little changes.

At least thanks to Lars Schneider's incredible efforts we have Continuous
Testing (I know how much time he spent on this, it is another thing
deserving the label "preposterous", and I know how much time I spent on
top to add the Windows part which mostly works). If only we could push it
further to true Continuous Integration. Or at least to accepting PRs from
professionals who simply have no time to fight with our patch contribution
process and whose expertise we lose as a consequence.

Ciao,
Johannes

Footnote *1*: The funniest part about this is that I do get mails about
all of this all the time. When I am pulled in as a reviewer. When a build
failed. When a previously failing task was fixed by a new build. When
somebody responded to my comments. The difference to the mailing
list-centric approach is of course that those mails are only
notifications, and link back to the tool appropriately supporting what
I, the user, want to get done.

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/2] http: handle curl with vendor backports
  2017-08-11 22:15                     ` Junio C Hamano
  2017-08-12  6:20                       ` Tom G. Christensen
@ 2017-08-20  8:47                       ` Jeff King
  2017-08-20 16:28                         ` Junio C Hamano
  1 sibling, 1 reply; 70+ messages in thread
From: Jeff King @ 2017-08-20  8:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Tom G. Christensen, git

On Fri, Aug 11, 2017 at 03:15:06PM -0700, Junio C Hamano wrote:

> "Tom G. Christensen" <tgc@jupiterrise.com> writes:
> 
> > 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.
> 
> Thanks; these feature macros ought to be more dependable, and I
> think this moves things in the right direction (regardless of which
> features we might later pick as mandatory and cut off supports for
> older versions).

Yes, I agree that these are an improvement regardless. If we follow
through on the cut-off to 7.19.4, then the CURLPROTO ones all go away.
But I don't mind rebasing any cut-off proposal on top of this work.

-Peff

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 1/2] http: Fix handling of missing CURLPROTO_*
  2017-08-12  0:30                     ` Junio C Hamano
  2017-08-12  9:04                       ` Tom G. Christensen
@ 2017-08-20  8:59                       ` Jeff King
  1 sibling, 0 replies; 70+ messages in thread
From: Jeff King @ 2017-08-20  8:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Tom G. Christensen, git

On Fri, Aug 11, 2017 at 05:30:34PM -0700, Junio C Hamano wrote:

> > -#if LIBCURL_VERSION_NUM >= 0x071304
> > +#ifdef CURLPROTO_HTTP
> >  	curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> >  			 get_curl_allowed_protocols(0));
> >  	curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> 
> This may make the code to _compile_, but is it sensible to let the
> code build and be used by the end users without the "these protocols
> are safe" filter, I wonder?  
> 
> Granted, ancient code was unsafe and people were happily using it,
> but now we know better, and more importantly, we have since added
> users of transport (e.g. blindly fetch submodules recursively) that
> may _rely_ on this layer of the code safely filtering unsafe
> protocols, so...

I don't think Tom's patch changes this in any meaningful way. The
"fallback to skipping the safety and showing a warning" dates back to
the original introduction of the feature.

But FWIW, that is exactly the kind of thing that led me to wanting to
implement a hard cutoff in the first place. The warning is small
consolation if Git allows an attack through anyway. Or worse, you don't
even see the warning because it's an automated process that is being
exploited.

There's a good chance if you have such an antique curl that it is also
riddled with other curl-specific bugs that have since been fixed. And
that would argue that we don't need to care that much anyway; people
running old curl have decided that it's not worth caring about the
security implications.

But in the case of RHEL, in theory they are patching security bugs in
curl but just not implementing new features. So if we have a
vulnerability introduced by using an old version of curl, we really are
making things worse. And that argues for having a hard cutoff.

But as Tom's series demonstrates, they are backporting _some_ features
(presumably ones needed by other programs like Git to fix security
bugs). Which argues for having #ifdefs that handle those backports,
which in theory gives us a secure Git on systems that do careful
backporting, and gives us an insecure-but-not-worse-than-it-already-was
Git on systems that don't do backporting.

I dunno. There were a lot of assumptions and mental gymnastics there.
I'm still tempted to target curl >= 7.19.4 just based on timing and
RHEL5's support life-cycle.

-Peff

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/2] http: handle curl with vendor backports
  2017-08-20  8:47                       ` Jeff King
@ 2017-08-20 16:28                         ` Junio C Hamano
  2017-08-23 15:41                           ` Jeff King
  0 siblings, 1 reply; 70+ messages in thread
From: Junio C Hamano @ 2017-08-20 16:28 UTC (permalink / raw)
  To: Jeff King; +Cc: Tom G. Christensen, git

Jeff King <peff@peff.net> writes:

> On Fri, Aug 11, 2017 at 03:15:06PM -0700, Junio C Hamano wrote:
>
>> "Tom G. Christensen" <tgc@jupiterrise.com> writes:
>> 
>> > 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.
>> 
>> Thanks; these feature macros ought to be more dependable, and I
>> think this moves things in the right direction (regardless of which
>> features we might later pick as mandatory and cut off supports for
>> older versions).
>
> Yes, I agree that these are an improvement regardless. If we follow
> through on the cut-off to 7.19.4, then the CURLPROTO ones all go away.
> But I don't mind rebasing any cut-off proposal on top of this work.

Yeah I came to a similar conclusion and was about asking if you feel
the same way that your series should be made on top of Tom's fixes.

The aspect of that series I do like the most is to base our
decisions on features, not versions, and I also wonder if we can do
similar in your "abandon too old ones" series, too.

Thanks.

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH 0/2] http: handle curl with vendor backports
  2017-08-20 16:28                         ` Junio C Hamano
@ 2017-08-23 15:41                           ` Jeff King
  0 siblings, 0 replies; 70+ messages in thread
From: Jeff King @ 2017-08-23 15:41 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Tom G. Christensen, git

On Sun, Aug 20, 2017 at 09:28:20AM -0700, Junio C Hamano wrote:

> > Yes, I agree that these are an improvement regardless. If we follow
> > through on the cut-off to 7.19.4, then the CURLPROTO ones all go away.
> > But I don't mind rebasing any cut-off proposal on top of this work.
> 
> Yeah I came to a similar conclusion and was about asking if you feel
> the same way that your series should be made on top of Tom's fixes.
> 
> The aspect of that series I do like the most is to base our
> decisions on features, not versions, and I also wonder if we can do
> similar in your "abandon too old ones" series, too.

Yeah, I don't mind moving to feature flags where we can (though some
features do not have a useful flag; e.g., the only way to know whether
we must be strdup curl_easy_setopt() arguments is by checking the curl
version).

One annoying thing about "feature" flags instead of version flags is
that it takes a lot of legwork to figure out how old those features are
(whereas with the versions I was able to look that up in the curl
history pretty easily).  Since people adding the feature flag generally
do that legwork, it's probably worth having a comment for each
mentioning the general vintage (or maybe the commit message is an OK
place for that).

I actually wonder if it is worth defining our own readable flags in a
big table at the beginning of the file, like:

  /*
   * introduced in curl 7.19.4, but backported by some distros like
   * RHEL. We can identify it by the presence of the PROTO flags.
   */
  #ifdef CURLPROTO_HTTP
  #define CURL_SUPPORTS_PROTOCOL_REDIRECTION
  #endif

That keeps the logic in one place (where it can be changed if we later
find that the define we picked for our feature isn't quite accurate).
And then the #ifdefs sprinkled through the code itself become
self-documenting.

-Peff

^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH v2 0/5] drop support for ancient curl
  2017-08-09 12:00 [PATCH 0/4] dropping support for older curl Jeff King
                   ` (5 preceding siblings ...)
       [not found] ` <87zib8g8ub.fsf@gmail.com>
@ 2021-07-21 22:22 ` Ævar Arnfjörð Bjarmason
  2021-07-21 22:22   ` [PATCH v2 1/5] http: drop support for curl < 7.11.1 Ævar Arnfjörð Bjarmason
                     ` (8 more replies)
  6 siblings, 9 replies; 70+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-21 22:22 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Nicolas Morey-Chaisemartin,
	Tom G . Christensen, Mischa POSLAWSKY, Johannes Schindelin,
	Ævar Arnfjörð Bjarmason

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 <peff@peff.net>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +
      ## 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 <peff@peff.net>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +
      ## 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 <peff@peff.net>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## 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


^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH v2 1/5] http: drop support for curl < 7.11.1
  2021-07-21 22:22 ` [PATCH v2 0/5] drop support for ancient curl Ævar Arnfjörð Bjarmason
@ 2021-07-21 22:22   ` Ævar Arnfjörð Bjarmason
  2021-07-21 22:56     ` Junio C Hamano
  2021-07-21 22:22   ` [PATCH v2 2/5] http: drop support for curl < 7.16.0 Ævar Arnfjörð Bjarmason
                     ` (7 subsequent siblings)
  8 siblings, 1 reply; 70+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-21 22:22 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Nicolas Morey-Chaisemartin,
	Tom G . Christensen, Mischa POSLAWSKY, Johannes Schindelin,
	Ævar Arnfjörð Bjarmason

From: Jeff King <peff@peff.net>

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 <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 http.c        | 53 ---------------------------------------------------
 http.h        | 12 +-----------
 remote-curl.c |  3 ---
 3 files changed, 1 insertion(+), 67 deletions(-)

diff --git a/http.c b/http.c
index 8119247149a..56182a89e25 100644
--- a/http.c
+++ b/http.c
@@ -19,19 +19,11 @@
 static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
 static int trace_curl_data = 1;
 static int trace_curl_redact = 1;
-#if LIBCURL_VERSION_NUM >= 0x070a08
 long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;
-#else
-long int git_curl_ipresolve;
-#endif
 int active_requests;
 int http_is_verbose;
 ssize_t http_post_buffer = 16 * LARGE_PACKET_MAX;
 
-#if LIBCURL_VERSION_NUM >= 0x070a06
-#define LIBCURL_CAN_HANDLE_AUTH_ANY
-#endif
-
 static int min_curl_sessions = 1;
 static int curl_session_count;
 #ifdef USE_CURL_MULTI
@@ -68,15 +60,9 @@ static struct {
 	{ "tlsv1.3", CURL_SSLVERSION_TLSv1_3 },
 #endif
 };
-#if LIBCURL_VERSION_NUM >= 0x070903
 static const char *ssl_key;
-#endif
-#if LIBCURL_VERSION_NUM >= 0x070908
 static const char *ssl_capath;
-#endif
-#if LIBCURL_VERSION_NUM >= 0x071304
 static const char *curl_no_proxy;
-#endif
 #if LIBCURL_VERSION_NUM >= 0x072c00
 static const char *ssl_pinnedkey;
 #endif
@@ -101,9 +87,7 @@ static struct {
 	{ "digest", CURLAUTH_DIGEST },
 	{ "negotiate", CURLAUTH_GSSNEGOTIATE },
 	{ "ntlm", CURLAUTH_NTLM },
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 	{ "anyauth", CURLAUTH_ANY },
-#endif
 	/*
 	 * CURLAUTH_DIGEST_IE has no corresponding command-line option in
 	 * curl(1) and is not included in CURLAUTH_ANY, so we leave it out
@@ -143,7 +127,6 @@ enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
 
 static struct credential cert_auth = CREDENTIAL_INIT;
 static int ssl_cert_password_required;
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 static unsigned long http_auth_methods = CURLAUTH_ANY;
 static int http_auth_methods_restricted;
 /* Modes for which empty_auth cannot actually help us. */
@@ -153,7 +136,6 @@ static unsigned long empty_auth_useless =
 	| CURLAUTH_DIGEST_IE
 #endif
 	| CURLAUTH_DIGEST;
-#endif
 
 static struct curl_slist *pragma_header;
 static struct curl_slist *no_pragma_header;
@@ -237,12 +219,8 @@ static void finish_active_slot(struct active_request_slot *slot)
 	if (slot->results != NULL) {
 		slot->results->curl_result = slot->curl_result;
 		slot->results->http_code = slot->http_code;
-#if LIBCURL_VERSION_NUM >= 0x070a08
 		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL,
 				  &slot->results->auth_avail);
-#else
-		slot->results->auth_avail = 0;
-#endif
 
 		curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
 			&slot->results->http_connectcode);
@@ -305,14 +283,10 @@ static int http_options(const char *var, const char *value, void *cb)
 		return git_config_string(&ssl_version, var, value);
 	if (!strcmp("http.sslcert", var))
 		return git_config_pathname(&ssl_cert, var, value);
-#if LIBCURL_VERSION_NUM >= 0x070903
 	if (!strcmp("http.sslkey", var))
 		return git_config_pathname(&ssl_key, var, value);
-#endif
-#if LIBCURL_VERSION_NUM >= 0x070908
 	if (!strcmp("http.sslcapath", var))
 		return git_config_pathname(&ssl_capath, var, value);
-#endif
 	if (!strcmp("http.sslcainfo", var))
 		return git_config_pathname(&ssl_cainfo, var, value);
 	if (!strcmp("http.sslcertpasswordprotected", var)) {
@@ -461,12 +435,6 @@ static int curl_empty_auth_enabled(void)
 	if (curl_empty_auth >= 0)
 		return curl_empty_auth;
 
-#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY
-	/*
-	 * Our libcurl is too old to do AUTH_ANY in the first place;
-	 * just default to turning the feature off.
-	 */
-#else
 	/*
 	 * In the automatic case, kick in the empty-auth
 	 * hack as long as we would potentially try some
@@ -479,7 +447,6 @@ static int curl_empty_auth_enabled(void)
 	if (http_auth_methods_restricted &&
 	    (http_auth_methods & ~empty_auth_useless))
 		return 1;
-#endif
 	return 0;
 }
 
@@ -552,7 +519,6 @@ static void init_curl_proxy_auth(CURL *result)
 
 	var_override(&http_proxy_authmethod, getenv("GIT_HTTP_PROXY_AUTHMETHOD"));
 
-#if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */
 	if (http_proxy_authmethod) {
 		int i;
 		for (i = 0; i < ARRAY_SIZE(proxy_authmethods); i++) {
@@ -570,7 +536,6 @@ static void init_curl_proxy_auth(CURL *result)
 	}
 	else
 		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
-#endif
 }
 
 static int has_cert_password(void)
@@ -879,12 +844,8 @@ static CURL *get_curl_handle(void)
     }
 #endif
 
-#if LIBCURL_VERSION_NUM >= 0x070907
 	curl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
-#endif
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 	curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
-#endif
 
 #ifdef CURLGSSAPI_DELEGATION_FLAG
 	if (curl_deleg) {
@@ -940,14 +901,10 @@ static CURL *get_curl_handle(void)
 		curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
 	if (has_cert_password())
 		curl_easy_setopt(result, CURLOPT_KEYPASSWD, cert_auth.password);
-#if LIBCURL_VERSION_NUM >= 0x070903
 	if (ssl_key != NULL)
 		curl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key);
-#endif
-#if LIBCURL_VERSION_NUM >= 0x070908
 	if (ssl_capath != NULL)
 		curl_easy_setopt(result, CURLOPT_CAPATH, ssl_capath);
-#endif
 #if LIBCURL_VERSION_NUM >= 0x072c00
 	if (ssl_pinnedkey != NULL)
 		curl_easy_setopt(result, CURLOPT_PINNEDPUBLICKEY, ssl_pinnedkey);
@@ -1180,12 +1137,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 		curl_ssl_verify = 0;
 
 	set_from_env(&ssl_cert, "GIT_SSL_CERT");
-#if LIBCURL_VERSION_NUM >= 0x070903
 	set_from_env(&ssl_key, "GIT_SSL_KEY");
-#endif
-#if LIBCURL_VERSION_NUM >= 0x070908
 	set_from_env(&ssl_capath, "GIT_SSL_CAPATH");
-#endif
 	set_from_env(&ssl_cainfo, "GIT_SSL_CAINFO");
 
 	set_from_env(&user_agent, "GIT_HTTP_USER_AGENT");
@@ -1367,12 +1320,8 @@ struct active_request_slot *get_active_slot(void)
 	else
 		curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 0);
 
-#if LIBCURL_VERSION_NUM >= 0x070a08
 	curl_easy_setopt(slot->curl, CURLOPT_IPRESOLVE, git_curl_ipresolve);
-#endif
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
-#endif
 	if (http_auth.password || curl_empty_auth_enabled())
 		init_curl_http_auth(slot->curl);
 
@@ -1654,13 +1603,11 @@ static int handle_curl_result(struct slot_results *results)
 			credential_reject(&http_auth);
 			return HTTP_NOAUTH;
 		} else {
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
 			if (results->auth_avail) {
 				http_auth_methods &= results->auth_avail;
 				http_auth_methods_restricted = 1;
 			}
-#endif
 			return HTTP_REAUTH;
 		}
 	} else {
diff --git a/http.h b/http.h
index bf3d1270ad8..d2f8cc56617 100644
--- a/http.h
+++ b/http.h
@@ -22,13 +22,7 @@
 #define DEFAULT_MAX_REQUESTS 5
 #endif
 
-#if LIBCURL_VERSION_NUM < 0x070704
-#define curl_global_cleanup() do { /* nothing */ } while (0)
-#endif
-
-#if LIBCURL_VERSION_NUM < 0x070800
-#define curl_global_init(a) do { /* nothing */ } while (0)
-#elif LIBCURL_VERSION_NUM >= 0x070c00
+#if LIBCURL_VERSION_NUM >= 0x070c00
 #define curl_global_init(a) curl_global_init_mem(a, xmalloc, free, \
 						xrealloc, xstrdup, xcalloc)
 #endif
@@ -37,10 +31,6 @@
 #define NO_CURL_EASY_DUPHANDLE
 #endif
 
-#if LIBCURL_VERSION_NUM < 0x070a03
-#define CURLE_HTTP_RETURNED_ERROR CURLE_HTTP_NOT_FOUND
-#endif
-
 #if LIBCURL_VERSION_NUM < 0x070c03
 #define NO_CURL_IOCTL
 #endif
diff --git a/remote-curl.c b/remote-curl.c
index 9d432c299a2..9e6918468e4 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -185,8 +185,6 @@ static int set_option(const char *name, const char *value)
 						 strbuf_detach(&unquoted, NULL));
 		}
 		return 0;
-
-#if LIBCURL_VERSION_NUM >= 0x070a08
 	} else if (!strcmp(name, "family")) {
 		if (!strcmp(value, "ipv4"))
 			git_curl_ipresolve = CURL_IPRESOLVE_V4;
@@ -197,7 +195,6 @@ static int set_option(const char *name, const char *value)
 		else
 			return -1;
 		return 0;
-#endif /* LIBCURL_VERSION_NUM >= 0x070a08 */
 	} else if (!strcmp(name, "from-promisor")) {
 		options.from_promisor = 1;
 		return 0;
-- 
2.32.0.874.ge7a9d58bfcf


^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH v2 2/5] http: drop support for curl < 7.16.0
  2021-07-21 22:22 ` [PATCH v2 0/5] drop support for ancient curl Ævar Arnfjörð Bjarmason
  2021-07-21 22:22   ` [PATCH v2 1/5] http: drop support for curl < 7.11.1 Ævar Arnfjörð Bjarmason
@ 2021-07-21 22:22   ` Ævar Arnfjörð Bjarmason
  2021-07-21 22:22   ` [PATCH v2 3/5] http: drop support for curl < 7.19.4 Ævar Arnfjörð Bjarmason
                     ` (6 subsequent siblings)
  8 siblings, 0 replies; 70+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-21 22:22 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Nicolas Morey-Chaisemartin,
	Tom G . Christensen, Mischa POSLAWSKY, Johannes Schindelin,
	Ævar Arnfjörð Bjarmason

From: Jeff King <peff@peff.net>

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 <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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(-)

diff --git a/http-push.c b/http-push.c
index d7cb1675a2d..aa3de7c1086 100644
--- a/http-push.c
+++ b/http-push.c
@@ -203,10 +203,8 @@ static void curl_setup_http(CURL *curl, const char *url,
 	curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
 	curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);
 	curl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);
-#ifndef NO_CURL_IOCTL
 	curl_easy_setopt(curl, CURLOPT_IOCTLFUNCTION, ioctl_buffer);
 	curl_easy_setopt(curl, CURLOPT_IOCTLDATA, buffer);
-#endif
 	curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_fn);
 	curl_easy_setopt(curl, CURLOPT_NOBODY, 0);
 	curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, custom_req);
@@ -249,8 +247,6 @@ static void process_response(void *callback_data)
 	finish_request(request);
 }
 
-#ifdef USE_CURL_MULTI
-
 static void start_fetch_loose(struct transfer_request *request)
 {
 	struct active_request_slot *slot;
@@ -299,7 +295,6 @@ static void start_mkcol(struct transfer_request *request)
 		FREE_AND_NULL(request->url);
 	}
 }
-#endif
 
 static void start_fetch_packed(struct transfer_request *request)
 {
@@ -605,7 +600,6 @@ static void finish_request(struct transfer_request *request)
 	}
 }
 
-#ifdef USE_CURL_MULTI
 static int is_running_queue;
 static int fill_active_slot(void *unused)
 {
@@ -629,7 +623,6 @@ static int fill_active_slot(void *unused)
 	}
 	return 0;
 }
-#endif
 
 static void get_remote_object_list(unsigned char parent);
 
@@ -658,10 +651,8 @@ static void add_fetch_request(struct object *obj)
 	request->next = request_queue_head;
 	request_queue_head = request;
 
-#ifdef USE_CURL_MULTI
 	fill_active_slots();
 	step_active_slots();
-#endif
 }
 
 static int add_send_request(struct object *obj, struct remote_lock *lock)
@@ -696,10 +687,8 @@ static int add_send_request(struct object *obj, struct remote_lock *lock)
 	request->next = request_queue_head;
 	request_queue_head = request;
 
-#ifdef USE_CURL_MULTI
 	fill_active_slots();
 	step_active_slots();
-#endif
 
 	return 1;
 }
@@ -1682,21 +1671,15 @@ static int delete_remote_branch(const char *pattern, int force)
 
 static void run_request_queue(void)
 {
-#ifdef USE_CURL_MULTI
 	is_running_queue = 1;
 	fill_active_slots();
 	add_fill_function(NULL, fill_active_slot);
-#endif
 	do {
 		finish_all_active_slots();
-#ifdef USE_CURL_MULTI
 		fill_active_slots();
-#endif
 	} while (request_queue_head && !aborted);
 
-#ifdef USE_CURL_MULTI
 	is_running_queue = 0;
-#endif
 }
 
 int cmd_main(int argc, const char **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_MULTI
 	is_running_queue = 0;
-#endif
 
 	/* Verify DAV compliance/lock support */
 	if (!locking_available()) {
diff --git a/http-walker.c b/http-walker.c
index 90d8ecb57ef..19e31623f04 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -127,7 +127,6 @@ static void release_object_request(struct object_request *obj_req)
 	free(obj_req);
 }
 
-#ifdef USE_CURL_MULTI
 static int fill_active_slot(struct walker *walker)
 {
 	struct object_request *obj_req;
@@ -146,7 +145,6 @@ static int fill_active_slot(struct walker *walker)
 	}
 	return 0;
 }
-#endif
 
 static void prefetch(struct walker *walker, unsigned char *sha1)
 {
@@ -163,10 +161,8 @@ static void prefetch(struct walker *walker, unsigned char *sha1)
 	http_is_verbose = walker->get_verbosely;
 	list_add_tail(&newreq->node, &object_queue_head);
 
-#ifdef USE_CURL_MULTI
 	fill_active_slots();
 	step_active_slots();
-#endif
 }
 
 static int is_alternate_allowed(const char *url)
@@ -357,11 +353,9 @@ static void fetch_alternates(struct walker *walker, const char *base)
 	 * wait for them to arrive and return to processing this request's
 	 * curl message
 	 */
-#ifdef USE_CURL_MULTI
 	while (cdata->got_alternates == 0) {
 		step_active_slots();
 	}
-#endif
 
 	/* Nothing to do if they've already been fetched */
 	if (cdata->got_alternates == 1)
@@ -505,12 +499,8 @@ static int fetch_object(struct walker *walker, unsigned char *hash)
 		return 0;
 	}
 
-#ifdef USE_CURL_MULTI
 	while (obj_req->state == WAITING)
 		step_active_slots();
-#else
-	start_object_request(walker, obj_req);
-#endif
 
 	/*
 	 * obj_req->req might change when fetching alternates in the callback
@@ -623,9 +613,7 @@ struct walker *get_http_walker(const char *url)
 	walker->cleanup = cleanup;
 	walker->data = data;
 
-#ifdef USE_CURL_MULTI
 	add_fill_function(walker, (int (*)(void *)) fill_active_slot);
-#endif
 
 	return walker;
 }
diff --git a/http.c b/http.c
index 56182a89e25..ef00e930232 100644
--- a/http.c
+++ b/http.c
@@ -26,10 +26,8 @@ ssize_t http_post_buffer = 16 * LARGE_PACKET_MAX;
 
 static int min_curl_sessions = 1;
 static int curl_session_count;
-#ifdef USE_CURL_MULTI
 static int max_requests = -1;
 static CURLM *curlm;
-#endif
 #ifndef NO_CURL_EASY_DUPHANDLE
 static CURL *curl_default;
 #endif
@@ -117,14 +115,6 @@ static int curl_empty_auth = -1;
 
 enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
 
-#if LIBCURL_VERSION_NUM >= 0x071700
-/* Use CURLOPT_KEYPASSWD as is */
-#elif LIBCURL_VERSION_NUM >= 0x070903
-#define CURLOPT_KEYPASSWD CURLOPT_SSLKEYPASSWD
-#else
-#define CURLOPT_KEYPASSWD CURLOPT_SSLCERTPASSWD
-#endif
-
 static struct credential cert_auth = CREDENTIAL_INIT;
 static int ssl_cert_password_required;
 static unsigned long http_auth_methods = CURLAUTH_ANY;
@@ -168,7 +158,6 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 	return size / eltsize;
 }
 
-#ifndef NO_CURL_IOCTL
 curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
 {
 	struct buffer *buffer = clientp;
@@ -185,7 +174,6 @@ curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
 		return CURLIOE_UNKNOWNCMD;
 	}
 }
-#endif
 
 size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
@@ -233,12 +221,9 @@ static void finish_active_slot(struct active_request_slot *slot)
 
 static void xmulti_remove_handle(struct active_request_slot *slot)
 {
-#ifdef USE_CURL_MULTI
 	curl_multi_remove_handle(curlm, slot->curl);
-#endif
 }
 
-#ifdef USE_CURL_MULTI
 static void process_curl_messages(void)
 {
 	int num_messages;
@@ -266,7 +251,6 @@ static void process_curl_messages(void)
 		curl_message = curl_multi_info_read(curlm, &num_messages);
 	}
 }
-#endif
 
 static int http_options(const char *var, const char *value, void *cb)
 {
@@ -315,18 +299,14 @@ static int http_options(const char *var, const char *value, void *cb)
 
 	if (!strcmp("http.minsessions", var)) {
 		min_curl_sessions = git_config_int(var, value);
-#ifndef USE_CURL_MULTI
 		if (min_curl_sessions > 1)
 			min_curl_sessions = 1;
-#endif
 		return 0;
 	}
-#ifdef USE_CURL_MULTI
 	if (!strcmp("http.maxrequests", var)) {
 		max_requests = git_config_int(var, value);
 		return 0;
 	}
-#endif
 	if (!strcmp("http.lowspeedlimit", var)) {
 		curl_low_speed_limit = (long)git_config_int(var, value);
 		return 0;
@@ -574,7 +554,7 @@ static void set_curl_keepalive(CURL *c)
 	curl_easy_setopt(c, CURLOPT_TCP_KEEPALIVE, 1);
 }
 
-#elif LIBCURL_VERSION_NUM >= 0x071000
+#else
 static int sockopt_callback(void *client, curl_socket_t fd, curlsocktype type)
 {
 	int ka = 1;
@@ -595,12 +575,6 @@ static void set_curl_keepalive(CURL *c)
 {
 	curl_easy_setopt(c, CURLOPT_SOCKOPTFUNCTION, sockopt_callback);
 }
-
-#else
-static void set_curl_keepalive(CURL *c)
-{
-	/* not supported on older curl versions */
-}
 #endif
 
 static void redact_sensitive_header(struct strbuf *header)
@@ -1121,7 +1095,6 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 	no_pragma_header = curl_slist_append(http_copy_default_headers(),
 		"Pragma:");
 
-#ifdef USE_CURL_MULTI
 	{
 		char *http_max_requests = getenv("GIT_HTTP_MAX_REQUESTS");
 		if (http_max_requests != NULL)
@@ -1131,7 +1104,6 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 	curlm = curl_multi_init();
 	if (!curlm)
 		die("curl_multi_init failed");
-#endif
 
 	if (getenv("GIT_SSL_NO_VERIFY"))
 		curl_ssl_verify = 0;
@@ -1154,10 +1126,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 		curl_ssl_verify = 1;
 
 	curl_session_count = 0;
-#ifdef USE_CURL_MULTI
 	if (max_requests < 1)
 		max_requests = DEFAULT_MAX_REQUESTS;
-#endif
 
 	set_from_env(&http_proxy_ssl_cert, "GIT_PROXY_SSL_CERT");
 	set_from_env(&http_proxy_ssl_key, "GIT_PROXY_SSL_KEY");
@@ -1201,9 +1171,7 @@ void http_cleanup(void)
 	curl_easy_cleanup(curl_default);
 #endif
 
-#ifdef USE_CURL_MULTI
 	curl_multi_cleanup(curlm);
-#endif
 	curl_global_cleanup();
 
 	string_list_clear(&extra_http_headers, 0);
@@ -1250,7 +1218,6 @@ struct active_request_slot *get_active_slot(void)
 	struct active_request_slot *slot = active_queue_head;
 	struct active_request_slot *newslot;
 
-#ifdef USE_CURL_MULTI
 	int num_transfers;
 
 	/* Wait for a slot to open up if the queue is full */
@@ -1259,7 +1226,6 @@ struct active_request_slot *get_active_slot(void)
 		if (num_transfers < active_requests)
 			process_curl_messages();
 	}
-#endif
 
 	while (slot != NULL && slot->in_use)
 		slot = slot->next;
@@ -1330,7 +1296,6 @@ struct active_request_slot *get_active_slot(void)
 
 int start_active_slot(struct active_request_slot *slot)
 {
-#ifdef USE_CURL_MULTI
 	CURLMcode curlm_result = curl_multi_add_handle(curlm, slot->curl);
 	int num_transfers;
 
@@ -1348,11 +1313,9 @@ int start_active_slot(struct active_request_slot *slot)
 	 * something.
 	 */
 	curl_multi_perform(curlm, &num_transfers);
-#endif
 	return 1;
 }
 
-#ifdef USE_CURL_MULTI
 struct fill_chain {
 	void *data;
 	int (*fill)(void *);
@@ -1411,11 +1374,9 @@ void step_active_slots(void)
 		fill_active_slots();
 	}
 }
-#endif
 
 void run_active_slot(struct active_request_slot *slot)
 {
-#ifdef USE_CURL_MULTI
 	fd_set readfds;
 	fd_set writefds;
 	fd_set excfds;
@@ -1428,7 +1389,6 @@ void run_active_slot(struct active_request_slot *slot)
 		step_active_slots();
 
 		if (slot->in_use) {
-#if LIBCURL_VERSION_NUM >= 0x070f04
 			long curl_timeout;
 			curl_multi_timeout(curlm, &curl_timeout);
 			if (curl_timeout == 0) {
@@ -1440,10 +1400,6 @@ void run_active_slot(struct active_request_slot *slot)
 				select_timeout.tv_sec  =  curl_timeout / 1000;
 				select_timeout.tv_usec = (curl_timeout % 1000) * 1000;
 			}
-#else
-			select_timeout.tv_sec  = 0;
-			select_timeout.tv_usec = 50000;
-#endif
 
 			max_fd = -1;
 			FD_ZERO(&readfds);
@@ -1466,12 +1422,6 @@ void run_active_slot(struct active_request_slot *slot)
 			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
 		}
 	}
-#else
-	while (slot->in_use) {
-		slot->curl_result = curl_easy_perform(slot->curl);
-		finish_active_slot(slot);
-	}
-#endif
 }
 
 static void release_active_slot(struct active_request_slot *slot)
@@ -1485,9 +1435,7 @@ static void release_active_slot(struct active_request_slot *slot)
 			curl_session_count--;
 		}
 	}
-#ifdef USE_CURL_MULTI
 	fill_active_slots();
-#endif
 }
 
 void finish_all_active_slots(void)
@@ -1613,12 +1561,10 @@ static int handle_curl_result(struct slot_results *results)
 	} else {
 		if (results->http_connectcode == 407)
 			credential_reject(&proxy_auth);
-#if LIBCURL_VERSION_NUM >= 0x070c00
 		if (!curl_errorstr[0])
 			strlcpy(curl_errorstr,
 				curl_easy_strerror(results->curl_result),
 				sizeof(curl_errorstr));
-#endif
 		return HTTP_ERROR;
 	}
 }
diff --git a/http.h b/http.h
index d2f8cc56617..cb092622a73 100644
--- a/http.h
+++ b/http.h
@@ -10,31 +10,12 @@
 #include "remote.h"
 #include "url.h"
 
-/*
- * We detect based on the cURL version if multi-transfer is
- * usable in this implementation and define this symbol accordingly.
- * This shouldn't be set by the Makefile or by the user (e.g. via CFLAGS).
- */
-#undef USE_CURL_MULTI
-
-#if LIBCURL_VERSION_NUM >= 0x071000
-#define USE_CURL_MULTI
 #define DEFAULT_MAX_REQUESTS 5
-#endif
-
-#if LIBCURL_VERSION_NUM >= 0x070c00
-#define curl_global_init(a) curl_global_init_mem(a, xmalloc, free, \
-						xrealloc, xstrdup, xcalloc)
-#endif
 
-#if (LIBCURL_VERSION_NUM < 0x070c04) || (LIBCURL_VERSION_NUM == 0x071000)
+#if LIBCURL_VERSION_NUM == 0x071000
 #define NO_CURL_EASY_DUPHANDLE
 #endif
 
-#if LIBCURL_VERSION_NUM < 0x070c03
-#define NO_CURL_IOCTL
-#endif
-
 /*
  * CURLOPT_USE_SSL was known as CURLOPT_FTP_SSL up to 7.16.4,
  * and the constants were known as CURLFTPSSL_*
@@ -72,9 +53,7 @@ struct buffer {
 size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
 size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
 size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
-#ifndef NO_CURL_IOCTL
 curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);
-#endif
 
 /* Slot lifecycle functions */
 struct active_request_slot *get_active_slot(void);
@@ -91,11 +70,9 @@ void finish_all_active_slots(void);
 int run_one_slot(struct active_request_slot *slot,
 		 struct slot_results *results);
 
-#ifdef USE_CURL_MULTI
 void fill_active_slots(void);
 void add_fill_function(void *data, int (*fill)(void *));
 void step_active_slots(void);
-#endif
 
 void http_init(struct remote *remote, const char *url,
 	       int proactive_auth);
diff --git a/imap-send.c b/imap-send.c
index bb085d66d10..9844328b7b3 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1526,11 +1526,7 @@ 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);
 	}
 
diff --git a/remote-curl.c b/remote-curl.c
index 9e6918468e4..482d5a4656d 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -706,7 +706,6 @@ static size_t rpc_out(void *ptr, size_t eltsize,
 	return avail;
 }
 
-#ifndef NO_CURL_IOCTL
 static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
 {
 	struct rpc_state *rpc = clientp;
@@ -727,7 +726,6 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
 		return CURLIOE_UNKNOWNCMD;
 	}
 }
-#endif
 
 struct check_pktline_state {
 	char len_buf[4];
@@ -946,10 +944,8 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
 		rpc->initial_buffer = 1;
 		curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
 		curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
-#ifndef NO_CURL_IOCTL
 		curl_easy_setopt(slot->curl, CURLOPT_IOCTLFUNCTION, rpc_ioctl);
 		curl_easy_setopt(slot->curl, CURLOPT_IOCTLDATA, rpc);
-#endif
 		if (options.verbosity > 1) {
 			fprintf(stderr, "POST %s (chunked)\n", rpc->service_name);
 			fflush(stderr);
-- 
2.32.0.874.ge7a9d58bfcf


^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH v2 3/5] http: drop support for curl < 7.19.4
  2021-07-21 22:22 ` [PATCH v2 0/5] drop support for ancient curl Ævar Arnfjörð Bjarmason
  2021-07-21 22:22   ` [PATCH v2 1/5] http: drop support for curl < 7.11.1 Ævar Arnfjörð Bjarmason
  2021-07-21 22:22   ` [PATCH v2 2/5] http: drop support for curl < 7.16.0 Ævar Arnfjörð Bjarmason
@ 2021-07-21 22:22   ` Ævar Arnfjörð Bjarmason
  2021-07-21 23:05     ` Junio C Hamano
  2021-07-21 22:22   ` [PATCH v2 4/5] http: drop support for curl < 7.19.3 and < 7.16.4 (again) Ævar Arnfjörð Bjarmason
                     ` (5 subsequent siblings)
  8 siblings, 1 reply; 70+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-21 22:22 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Nicolas Morey-Chaisemartin,
	Tom G . Christensen, Mischa POSLAWSKY, Johannes Schindelin,
	Ævar Arnfjörð Bjarmason

From: Jeff King <peff@peff.net>

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 <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 http.c | 50 --------------------------------------------------
 http.h |  4 ----
 2 files changed, 54 deletions(-)

diff --git a/http.c b/http.c
index ef00e930232..1f0d7664d35 100644
--- a/http.c
+++ b/http.c
@@ -28,9 +28,7 @@ static int min_curl_sessions = 1;
 static int curl_session_count;
 static int max_requests = -1;
 static CURLM *curlm;
-#ifndef NO_CURL_EASY_DUPHANDLE
 static CURL *curl_default;
-#endif
 
 #define PREV_BUF_SIZE 4096
 
@@ -440,24 +438,8 @@ static void init_curl_http_auth(CURL *result)
 
 	credential_fill(&http_auth);
 
-#if LIBCURL_VERSION_NUM >= 0x071301
 	curl_easy_setopt(result, CURLOPT_USERNAME, http_auth.username);
 	curl_easy_setopt(result, CURLOPT_PASSWORD, http_auth.password);
-#else
-	{
-		static struct strbuf up = STRBUF_INIT;
-		/*
-		 * Note that we assume we only ever have a single set of
-		 * credentials in a given program run, so we do not have
-		 * to worry about updating this buffer, only setting its
-		 * initial value.
-		 */
-		if (!up.len)
-			strbuf_addf(&up, "%s:%s",
-				http_auth.username, http_auth.password);
-		curl_easy_setopt(result, CURLOPT_USERPWD, up.buf);
-	}
-#endif
 }
 
 /* *var must be free-able */
@@ -471,22 +453,10 @@ static void var_override(const char **var, char *value)
 
 static void set_proxyauth_name_password(CURL *result)
 {
-#if LIBCURL_VERSION_NUM >= 0x071301
 		curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
 			proxy_auth.username);
 		curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
 			proxy_auth.password);
-#else
-		struct strbuf s = STRBUF_INIT;
-
-		strbuf_addstr_urlencode(&s, proxy_auth.username,
-					is_rfc3986_unreserved);
-		strbuf_addch(&s, ':');
-		strbuf_addstr_urlencode(&s, proxy_auth.password,
-					is_rfc3986_unreserved);
-		curl_proxyuserpwd = strbuf_detach(&s, NULL);
-		curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);
-#endif
 }
 
 static void init_curl_proxy_auth(CURL *result)
@@ -748,7 +718,6 @@ 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;
@@ -764,7 +733,6 @@ 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)
@@ -906,19 +874,11 @@ static CURL *get_curl_handle(void)
 	}
 
 	curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
-#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);
-#endif
-#ifdef CURLPROTO_HTTP
 	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 supported with cURL < 7.19.4"));
-#endif
 	if (getenv("GIT_CURL_VERBOSE"))
 		http_trace_curl_no_data();
 	setup_curl_trace(result);
@@ -1012,11 +972,9 @@ static CURL *get_curl_handle(void)
 			die("Invalid proxy URL '%s'", curl_http_proxy);
 
 		curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
-#if LIBCURL_VERSION_NUM >= 0x071304
 		var_override(&curl_no_proxy, getenv("NO_PROXY"));
 		var_override(&curl_no_proxy, getenv("no_proxy"));
 		curl_easy_setopt(result, CURLOPT_NOPROXY, curl_no_proxy);
-#endif
 	}
 	init_curl_proxy_auth(result);
 
@@ -1147,9 +1105,7 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 			ssl_cert_password_required = 1;
 	}
 
-#ifndef NO_CURL_EASY_DUPHANDLE
 	curl_default = get_curl_handle();
-#endif
 }
 
 void http_cleanup(void)
@@ -1167,9 +1123,7 @@ void http_cleanup(void)
 	}
 	active_queue_head = NULL;
 
-#ifndef NO_CURL_EASY_DUPHANDLE
 	curl_easy_cleanup(curl_default);
-#endif
 
 	curl_multi_cleanup(curlm);
 	curl_global_cleanup();
@@ -1248,11 +1202,7 @@ struct active_request_slot *get_active_slot(void)
 	}
 
 	if (slot->curl == NULL) {
-#ifdef NO_CURL_EASY_DUPHANDLE
-		slot->curl = get_curl_handle();
-#else
 		slot->curl = curl_easy_duphandle(curl_default);
-#endif
 		curl_session_count++;
 	}
 
diff --git a/http.h b/http.h
index cb092622a73..19f19dbe74c 100644
--- a/http.h
+++ b/http.h
@@ -12,10 +12,6 @@
 
 #define DEFAULT_MAX_REQUESTS 5
 
-#if LIBCURL_VERSION_NUM == 0x071000
-#define NO_CURL_EASY_DUPHANDLE
-#endif
-
 /*
  * CURLOPT_USE_SSL was known as CURLOPT_FTP_SSL up to 7.16.4,
  * and the constants were known as CURLFTPSSL_*
-- 
2.32.0.874.ge7a9d58bfcf


^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH v2 4/5] http: drop support for curl < 7.19.3 and < 7.16.4 (again)
  2021-07-21 22:22 ` [PATCH v2 0/5] drop support for ancient curl Ævar Arnfjörð Bjarmason
                     ` (2 preceding siblings ...)
  2021-07-21 22:22   ` [PATCH v2 3/5] http: drop support for curl < 7.19.4 Ævar Arnfjörð Bjarmason
@ 2021-07-21 22:22   ` Ævar Arnfjörð Bjarmason
  2021-07-21 23:17     ` Junio C Hamano
  2021-07-21 22:22   ` [PATCH v2 5/5] http: rename CURLOPT_FILE to CURLOPT_WRITEDATA Ævar Arnfjörð Bjarmason
                     ` (4 subsequent siblings)
  8 siblings, 1 reply; 70+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-21 22:22 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Nicolas Morey-Chaisemartin,
	Tom G . Christensen, Mischa POSLAWSKY, Johannes Schindelin,
	Ævar Arnfjörð Bjarmason

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 <avarab@gmail.com>
---
 http.c | 4 ----
 http.h | 9 ---------
 2 files changed, 13 deletions(-)

diff --git a/http.c b/http.c
index 1f0d7664d35..e9446850a62 100644
--- a/http.c
+++ b/http.c
@@ -120,9 +120,7 @@ static int http_auth_methods_restricted;
 /* Modes for which empty_auth cannot actually help us. */
 static unsigned long empty_auth_useless =
 	CURLAUTH_BASIC
-#ifdef CURLAUTH_DIGEST_IE
 	| CURLAUTH_DIGEST_IE
-#endif
 	| CURLAUTH_DIGEST;
 
 static struct curl_slist *pragma_header;
@@ -893,10 +891,8 @@ static CURL *get_curl_handle(void)
 	if (curl_ftp_no_epsv)
 		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
 
-#ifdef CURLOPT_USE_SSL
 	if (curl_ssl_try)
 		curl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);
-#endif
 
 	/*
 	 * CURL also examines these variables as a fallback; but we need to query
diff --git a/http.h b/http.h
index 19f19dbe74c..3db5a0cf320 100644
--- a/http.h
+++ b/http.h
@@ -12,15 +12,6 @@
 
 #define DEFAULT_MAX_REQUESTS 5
 
-/*
- * CURLOPT_USE_SSL was known as CURLOPT_FTP_SSL up to 7.16.4,
- * and the constants were known as CURLFTPSSL_*
-*/
-#if !defined(CURLOPT_USE_SSL) && defined(CURLOPT_FTP_SSL)
-#define CURLOPT_USE_SSL CURLOPT_FTP_SSL
-#define CURLUSESSL_TRY CURLFTPSSL_TRY
-#endif
-
 struct slot_results {
 	CURLcode curl_result;
 	long http_code;
-- 
2.32.0.874.ge7a9d58bfcf


^ permalink raw reply	[flat|nested] 70+ messages in thread

* [PATCH v2 5/5] http: rename CURLOPT_FILE to CURLOPT_WRITEDATA
  2021-07-21 22:22 ` [PATCH v2 0/5] drop support for ancient curl Ævar Arnfjörð Bjarmason
                     ` (3 preceding siblings ...)
  2021-07-21 22:22   ` [PATCH v2 4/5] http: drop support for curl < 7.19.3 and < 7.16.4 (again) Ævar Arnfjörð Bjarmason
@ 2021-07-21 22:22   ` Ævar Arnfjörð Bjarmason
  2021-07-21 23:19     ` Junio C Hamano
  2021-07-21 22:39   ` [PATCH v2 0/5] drop support for ancient curl Junio C Hamano
                     ` (3 subsequent siblings)
  8 siblings, 1 reply; 70+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-21 22:22 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Nicolas Morey-Chaisemartin,
	Tom G . Christensen, Mischa POSLAWSKY, Johannes Schindelin,
	Ævar Arnfjörð Bjarmason

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 <avarab@gmail.com>
---
 http-push.c   | 6 +++---
 http-walker.c | 2 +-
 http.c        | 6 +++---
 remote-curl.c | 4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/http-push.c b/http-push.c
index aa3de7c1086..3309aaf004a 100644
--- a/http-push.c
+++ b/http-push.c
@@ -883,7 +883,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	slot->results = &results;
 	curl_setup_http(slot->curl, url, DAV_LOCK, &out_buffer, fwrite_buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, &in_buffer);
+	curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &in_buffer);
 
 	CALLOC_ARRAY(lock, 1);
 	lock->timeout = -1;
@@ -1142,7 +1142,7 @@ static void remote_ls(const char *path, int flags,
 	curl_setup_http(slot->curl, url, DAV_PROPFIND,
 			&out_buffer, fwrite_buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, &in_buffer);
+	curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &in_buffer);
 
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
@@ -1216,7 +1216,7 @@ static int locking_available(void)
 	curl_setup_http(slot->curl, repo->url, DAV_PROPFIND,
 			&out_buffer, fwrite_buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, &in_buffer);
+	curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &in_buffer);
 
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
diff --git a/http-walker.c b/http-walker.c
index 19e31623f04..910fae539b8 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -378,7 +378,7 @@ static void fetch_alternates(struct walker *walker, const char *base)
 	alt_req.walker = walker;
 	slot->callback_data = &alt_req;
 
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
+	curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_URL, url.buf);
 
diff --git a/http.c b/http.c
index e9446850a62..a0f169d2fe5 100644
--- a/http.c
+++ b/http.c
@@ -1769,7 +1769,7 @@ static int http_request(const char *url,
 		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
 	} else {
 		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
-		curl_easy_setopt(slot->curl, CURLOPT_FILE, result);
+		curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, result);
 
 		if (target == HTTP_REQUEST_FILE) {
 			off_t posn = ftello(result);
@@ -2186,7 +2186,7 @@ struct http_pack_request *new_direct_http_pack_request(
 	}
 
 	preq->slot = get_active_slot();
-	curl_easy_setopt(preq->slot->curl, CURLOPT_FILE, preq->packfile);
+	curl_easy_setopt(preq->slot->curl, CURLOPT_WRITEDATA, preq->packfile);
 	curl_easy_setopt(preq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite);
 	curl_easy_setopt(preq->slot->curl, CURLOPT_URL, preq->url);
 	curl_easy_setopt(preq->slot->curl, CURLOPT_HTTPHEADER,
@@ -2357,7 +2357,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 
 	freq->slot = get_active_slot();
 
-	curl_easy_setopt(freq->slot->curl, CURLOPT_FILE, freq);
+	curl_easy_setopt(freq->slot->curl, CURLOPT_WRITEDATA, freq);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_FAILONERROR, 0);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_ERRORBUFFER, freq->errorstr);
diff --git a/remote-curl.c b/remote-curl.c
index 482d5a4656d..bf795f90c6e 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -853,7 +853,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, 4);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);
+	curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &buf);
 
 	err = run_slot(slot, results);
 
@@ -1016,7 +1016,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
 	rpc_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);
 
 
-- 
2.32.0.874.ge7a9d58bfcf


^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 0/5] drop support for ancient curl
  2021-07-21 22:22 ` [PATCH v2 0/5] drop support for ancient curl Ævar Arnfjörð Bjarmason
                     ` (4 preceding siblings ...)
  2021-07-21 22:22   ` [PATCH v2 5/5] http: rename CURLOPT_FILE to CURLOPT_WRITEDATA Ævar Arnfjörð Bjarmason
@ 2021-07-21 22:39   ` Junio C Hamano
  2021-07-21 22:56   ` brian m. carlson
                     ` (2 subsequent siblings)
  8 siblings, 0 replies; 70+ messages in thread
From: Junio C Hamano @ 2021-07-21 22:39 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Nicolas Morey-Chaisemartin, Tom G . Christensen,
	Mischa POSLAWSKY, Johannes Schindelin

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> This series simplifies the http code by dropping support for curl
> versions older than 7.19.4, released in March 2009.

Thanks.

Will take a look and may have some comments, but I'd prefer to see
an Ack from Peff as well.

> 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 <peff@peff.net>
>     +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>     +
>       ## 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 <peff@peff.net>
>     +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>     +
>       ## 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 <peff@peff.net>
>     +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>      
>       ## 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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 0/5] drop support for ancient curl
  2021-07-21 22:22 ` [PATCH v2 0/5] drop support for ancient curl Ævar Arnfjörð Bjarmason
                     ` (5 preceding siblings ...)
  2021-07-21 22:39   ` [PATCH v2 0/5] drop support for ancient curl Junio C Hamano
@ 2021-07-21 22:56   ` brian m. carlson
  2021-07-22  7:09     ` Ævar Arnfjörð Bjarmason
  2021-07-22  6:27   ` Bagas Sanjaya
  2021-07-23 10:16   ` Jeff King
  8 siblings, 1 reply; 70+ messages in thread
From: brian m. carlson @ 2021-07-21 22:56 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Jeff King, Nicolas Morey-Chaisemartin,
	Tom G . Christensen, Mischa POSLAWSKY, Johannes Schindelin

[-- Attachment #1: Type: text/plain, Size: 1791 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 1/5] http: drop support for curl < 7.11.1
  2021-07-21 22:22   ` [PATCH v2 1/5] http: drop support for curl < 7.11.1 Ævar Arnfjörð Bjarmason
@ 2021-07-21 22:56     ` Junio C Hamano
  0 siblings, 0 replies; 70+ messages in thread
From: Junio C Hamano @ 2021-07-21 22:56 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Nicolas Morey-Chaisemartin, Tom G . Christensen,
	Mischa POSLAWSKY, Johannes Schindelin

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> From: Jeff King <peff@peff.net>
>
> 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

13+4=17; in 2/5 you say 2006 is 15 years ago, and I think this
should be updated, too.  Is 21-4=17 close enough?

> 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).

Yup.  I sense that we'd be dropping anything older than 7.19.4
because nobody complained about our dependency on CURLPROTO_* for
the past 5 years?

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 3/5] http: drop support for curl < 7.19.4
  2021-07-21 22:22   ` [PATCH v2 3/5] http: drop support for curl < 7.19.4 Ævar Arnfjörð Bjarmason
@ 2021-07-21 23:05     ` Junio C Hamano
  0 siblings, 0 replies; 70+ messages in thread
From: Junio C Hamano @ 2021-07-21 23:05 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Nicolas Morey-Chaisemartin, Tom G . Christensen,
	Mischa POSLAWSKY, Johannes Schindelin

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> From: Jeff King <peff@peff.net>
>
> 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).

Hmph, doesn't the proposed log message of 1/5 need updating then?
The above says CURLPROTO_* breakage was only during a few months in
2017 and hints that we've been OK for the past 4 years, but 1/5 says
we need further work to if we want to get stuff working with a
version of cURL without CURLPROTO_* stuff, which directly contradicts
with "we fixed it at v2.15.0" above.

> 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.

Yeah, with RHEL 6 at 7.19.7 (from the proposed log for 2/5), I agree
that we'd not be worried too much about pre 7.19.4 as we used to.

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 4/5] http: drop support for curl < 7.19.3 and < 7.16.4 (again)
  2021-07-21 22:22   ` [PATCH v2 4/5] http: drop support for curl < 7.19.3 and < 7.16.4 (again) Ævar Arnfjörð Bjarmason
@ 2021-07-21 23:17     ` Junio C Hamano
  0 siblings, 0 replies; 70+ messages in thread
From: Junio C Hamano @ 2021-07-21 23:17 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Nicolas Morey-Chaisemartin, Tom G . Christensen,
	Mischa POSLAWSKY, Johannes Schindelin

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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

My go-to place for cURL version information is:

https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions

and it says CURLOPT_USE_SSL has been available since 7.17.0.  If
7.16.4 was the last version in the 7.16.x series, then these two
sources are consistent.

[2] says that the feature was available under a different name in
the past and up to version 7.16.4, meaning with 7.16.4, USE_SSL was
not usable. You'd need to do "<= 7.16.4" on the title, but it is
simpler to follow the symbols-in-versions table and say "< 7.17.0",
I would think.

The patch text looks OK.

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 5/5] http: rename CURLOPT_FILE to CURLOPT_WRITEDATA
  2021-07-21 22:22   ` [PATCH v2 5/5] http: rename CURLOPT_FILE to CURLOPT_WRITEDATA Ævar Arnfjörð Bjarmason
@ 2021-07-21 23:19     ` Junio C Hamano
  0 siblings, 0 replies; 70+ messages in thread
From: Junio C Hamano @ 2021-07-21 23:19 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Nicolas Morey-Chaisemartin, Tom G . Christensen,
	Mischa POSLAWSKY, Johannes Schindelin

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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].

Yes!  _FILE has been deprecated since that version so this is
definitely the right thing to do.

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 0/5] drop support for ancient curl
  2021-07-21 22:22 ` [PATCH v2 0/5] drop support for ancient curl Ævar Arnfjörð Bjarmason
                     ` (6 preceding siblings ...)
  2021-07-21 22:56   ` brian m. carlson
@ 2021-07-22  6:27   ` Bagas Sanjaya
  2021-07-23 10:16   ` Jeff King
  8 siblings, 0 replies; 70+ messages in thread
From: Bagas Sanjaya @ 2021-07-22  6:27 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Jeff King, Nicolas Morey-Chaisemartin,
	Tom G . Christensen, Mischa POSLAWSKY, Johannes Schindelin

On 22/07/21 05.22, Æ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.

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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 0/5] drop support for ancient curl
  2021-07-21 22:56   ` brian m. carlson
@ 2021-07-22  7:09     ` Ævar Arnfjörð Bjarmason
  2021-07-22 22:56       ` brian m. carlson
  0 siblings, 1 reply; 70+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-22  7:09 UTC (permalink / raw)
  To: brian m. carlson
  Cc: git, Junio C Hamano, Jeff King, Nicolas Morey-Chaisemartin,
	Tom G . Christensen, Mischa POSLAWSKY, Johannes Schindelin


On Wed, Jul 21 2021, brian m. carlson wrote:

> [[PGP Signed Part:Undecided]]
> 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.

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).

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 0/5] drop support for ancient curl
  2021-07-22  7:09     ` Ævar Arnfjörð Bjarmason
@ 2021-07-22 22:56       ` brian m. carlson
  2021-07-23  7:17         ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 70+ messages in thread
From: brian m. carlson @ 2021-07-22 22:56 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Jeff King, Nicolas Morey-Chaisemartin,
	Tom G . Christensen, Mischa POSLAWSKY, Johannes Schindelin

[-- Attachment #1: Type: text/plain, Size: 4439 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 0/5] drop support for ancient curl
  2021-07-22 22:56       ` brian m. carlson
@ 2021-07-23  7:17         ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 70+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-23  7:17 UTC (permalink / raw)
  To: brian m. carlson
  Cc: git, Junio C Hamano, Jeff King, Nicolas Morey-Chaisemartin,
	Tom G . Christensen, Mischa POSLAWSKY, Johannes Schindelin


On Thu, Jul 22 2021, brian m. carlson wrote:

> [[PGP Signed Part:Undecided]]
> 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.

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.

>> 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).

>> 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.

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 0/5] drop support for ancient curl
  2021-07-21 22:22 ` [PATCH v2 0/5] drop support for ancient curl Ævar Arnfjörð Bjarmason
                     ` (7 preceding siblings ...)
  2021-07-22  6:27   ` Bagas Sanjaya
@ 2021-07-23 10:16   ` Jeff King
  2021-07-23 16:21     ` Junio C Hamano
  8 siblings, 1 reply; 70+ messages in thread
From: Jeff King @ 2021-07-23 10:16 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Nicolas Morey-Chaisemartin,
	Tom G . Christensen, Mischa POSLAWSKY, Johannes Schindelin

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.

This seemed weirdly familiar. Looks like it was part of a series last
year, but the trickier parts built on top merited a re-roll that never
came:

  https://lore.kernel.org/git/20201013191729.2524700-2-smcallis@google.com/

> 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

^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 0/5] drop support for ancient curl
  2021-07-23 10:16   ` Jeff King
@ 2021-07-23 16:21     ` Junio C Hamano
  2021-07-23 16:49       ` Randall S. Becker
  2021-07-24  1:19       ` Jeff King
  0 siblings, 2 replies; 70+ messages in thread
From: Junio C Hamano @ 2021-07-23 16:21 UTC (permalink / raw)
  To: Jeff King
  Cc: Ævar Arnfjörð Bjarmason, git,
	Nicolas Morey-Chaisemartin, Tom G . Christensen,
	Mischa POSLAWSKY, Johannes Schindelin

Jeff King <peff@peff.net> writes:

> 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. :-/

Thanks.

> 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.

Good.  S-o-b is merely "I can let the project use it" and does not
say "I agree this is (still) relevant in the context of the code
this is being submitted to", so the above note is very much
appreciated.

> 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.

Me neither.  Those who are vanilla would not be helped by having it,
as their build would fail if their cURL is too old anyway even
without it.  Those who backported would have a build that may or may
not work, but diagnosing it is part of the job of backporting their
cURL anyway.  So in practice, I think "#error if you are older than
X" primarily would serve documentation purposes (which may be worth
doing, but requirements listed in INSTALL would probably be a better
alternative anyway).

Thanks.

^ permalink raw reply	[flat|nested] 70+ messages in thread

* RE: [PATCH v2 0/5] drop support for ancient curl
  2021-07-23 16:21     ` Junio C Hamano
@ 2021-07-23 16:49       ` Randall S. Becker
  2021-07-24  1:19       ` Jeff King
  1 sibling, 0 replies; 70+ messages in thread
From: Randall S. Becker @ 2021-07-23 16:49 UTC (permalink / raw)
  To: 'Junio C Hamano', 'Jeff King'
  Cc: 'Ævar Arnfjörð Bjarmason',
	git, 'Nicolas Morey-Chaisemartin',
	'Tom G . Christensen', 'Mischa POSLAWSKY',
	'Johannes Schindelin'

On July 23, 2021 12:21 PM, Junio C Hamano wrote:
>Jeff King <peff@peff.net> writes:
>
>> 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. :-/
>
>Thanks.
>
>> 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.
>
>Good.  S-o-b is merely "I can let the project use it" and does not say "I agree this is (still) relevant in the context of the code this is being
>submitted to", so the above note is very much appreciated.
>
>> 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.
>
>Me neither.  Those who are vanilla would not be helped by having it, as their build would fail if their cURL is too old anyway even without
>it.  Those who backported would have a build that may or may not work, but diagnosing it is part of the job of backporting their cURL
>anyway.  So in practice, I think "#error if you are older than X" primarily would serve documentation purposes (which may be worth doing,
>but requirements listed in INSTALL would probably be a better alternative anyway).

This is probably a red-herring, but from what I am observing, the curl 7.70 version is required for OpenSSL 3.0.0. Once we move there, which my team is working on, the near recent older version of curl could also be problematic and incompatible. This is rather unpleasant because the current standard libcurl on our platform is 7.65, which is too old to be compatible anyway, so we're going to have to put out a separate libcurl build. Curl seems to need to be closer to the bleeding edge to retain imminent compatibility.

-Randall


^ permalink raw reply	[flat|nested] 70+ messages in thread

* Re: [PATCH v2 0/5] drop support for ancient curl
  2021-07-23 16:21     ` Junio C Hamano
  2021-07-23 16:49       ` Randall S. Becker
@ 2021-07-24  1:19       ` Jeff King
  1 sibling, 0 replies; 70+ messages in thread
From: Jeff King @ 2021-07-24  1:19 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Ævar Arnfjörð Bjarmason, git,
	Nicolas Morey-Chaisemartin, Tom G . Christensen,
	Mischa POSLAWSKY, Johannes Schindelin

On Fri, Jul 23, 2021 at 09:21:11AM -0700, Junio C Hamano wrote:

> > 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.
> 
> Me neither.  Those who are vanilla would not be helped by having it,
> as their build would fail if their cURL is too old anyway even
> without it.  Those who backported would have a build that may or may
> not work, but diagnosing it is part of the job of backporting their
> cURL anyway.  So in practice, I think "#error if you are older than
> X" primarily would serve documentation purposes (which may be worth
> doing, but requirements listed in INSTALL would probably be a better
> alternative anyway).

Yeah, it is purely for documentation to help people who are confused by
a broken build. But I agree that INSTALL is probably reasonable there
(not to mention that the whole point is that these versions are so old
hardly anybody should be using them anyway). So let's forget about the
#error thing, then.

-Peff

^ permalink raw reply	[flat|nested] 70+ messages in thread

end of thread, other threads:[~2021-07-24  1:19 UTC | newest]

Thread overview: 70+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-09 12:00 [PATCH 0/4] dropping support for older curl Jeff King
2017-08-09 12:01 ` [PATCH 1/4] http: drop support for curl < 7.11.1 Jeff King
2017-08-09 12:01 ` [PATCH 2/4] http: drop support for curl < 7.16.0 Jeff King
2017-08-09 17:29   ` Stefan Beller
2017-08-09 21:13     ` Jeff King
2017-08-09 17:40   ` Junio C Hamano
2017-08-09 18:03     ` Nicolas Morey-Chaisemartin
2017-08-09 21:17       ` Jeff King
2017-08-09 21:29         ` Nicolas Morey-Chaisemartin
2017-08-09 21:49           ` Jeff King
2017-08-09 21:15     ` Jeff King
2017-08-09 12:02 ` [PATCH 3/4] http: drop support for curl < 7.19.4 Jeff King
2017-08-09 13:14   ` Ævar Arnfjörð Bjarmason
2017-08-09 13:38     ` Jeff King
2017-08-09 13:49       ` [PATCH 5/4] curl: remove ifdef'd code never used with curl >=7.19.4 Ævar Arnfjörð Bjarmason
2017-08-09 17:34   ` [PATCH 3/4] http: drop support for curl < 7.19.4 Stefan Beller
2017-08-09 21:19     ` Jeff King
2017-08-10 12:36   ` Mischa POSLAWSKY
2017-08-10 17:34     ` Jeff King
2017-08-09 12:02 ` [PATCH 4/4] http: #error on too-old curl Jeff King
2017-08-09 17:37   ` Stefan Beller
2017-08-09 21:42 ` [PATCH 0/4] dropping support for older curl Johannes Schindelin
2017-08-09 21:47   ` Jeff King
2017-08-10  9:01     ` Tom G. Christensen
2017-08-10  9:36     ` Johannes Schindelin
2017-08-10 21:33       ` Jeff King
2017-08-10 22:17         ` Junio C Hamano
2017-08-10 23:09           ` Jeff King
2017-08-11  0:17             ` Jeff King
     [not found]       ` <CAHVLzcnnrABmkYNg31Aq99NgBbyuCKEM60pHGygyjXbjmaUEYQ@mail.gmail.com>
2017-08-14 21:50         ` Johannes Schindelin
2017-08-10 20:33     ` Tom G. Christensen
2017-08-10 21:32       ` Jeff King
2017-08-10 22:23         ` Tom G. Christensen
2017-08-10 22:54           ` Jeff King
2017-08-10 23:17             ` Tom G. Christensen
2017-08-10 23:23               ` Jeff King
2017-08-10 23:36                 ` Tom G. Christensen
2017-08-11 16:37                   ` [PATCH 0/2] http: handle curl with vendor backports Tom G. Christensen
2017-08-11 22:15                     ` Junio C Hamano
2017-08-12  6:20                       ` Tom G. Christensen
2017-08-20  8:47                       ` Jeff King
2017-08-20 16:28                         ` Junio C Hamano
2017-08-23 15:41                           ` Jeff King
2017-08-11 16:37                   ` [PATCH 1/2] http: Fix handling of missing CURLPROTO_* Tom G. Christensen
2017-08-12  0:30                     ` Junio C Hamano
2017-08-12  9:04                       ` Tom G. Christensen
2017-08-20  8:59                       ` Jeff King
2017-08-11 16:37                   ` [PATCH 2/2] http: use a feature check to enable GSSAPI delegation control Tom G. Christensen
2017-08-09 23:39   ` [PATCH 0/4] dropping support for older curl Ævar Arnfjörð Bjarmason
     [not found] ` <87zib8g8ub.fsf@gmail.com>
2017-08-10 10:04   ` Dropping support for older perl Tom G. Christensen
2021-07-21 22:22 ` [PATCH v2 0/5] drop support for ancient curl Ævar Arnfjörð Bjarmason
2021-07-21 22:22   ` [PATCH v2 1/5] http: drop support for curl < 7.11.1 Ævar Arnfjörð Bjarmason
2021-07-21 22:56     ` Junio C Hamano
2021-07-21 22:22   ` [PATCH v2 2/5] http: drop support for curl < 7.16.0 Ævar Arnfjörð Bjarmason
2021-07-21 22:22   ` [PATCH v2 3/5] http: drop support for curl < 7.19.4 Ævar Arnfjörð Bjarmason
2021-07-21 23:05     ` Junio C Hamano
2021-07-21 22:22   ` [PATCH v2 4/5] http: drop support for curl < 7.19.3 and < 7.16.4 (again) Ævar Arnfjörð Bjarmason
2021-07-21 23:17     ` Junio C Hamano
2021-07-21 22:22   ` [PATCH v2 5/5] http: rename CURLOPT_FILE to CURLOPT_WRITEDATA Ævar Arnfjörð Bjarmason
2021-07-21 23:19     ` Junio C Hamano
2021-07-21 22:39   ` [PATCH v2 0/5] drop support for ancient curl Junio C Hamano
2021-07-21 22:56   ` brian m. carlson
2021-07-22  7:09     ` Ævar Arnfjörð Bjarmason
2021-07-22 22:56       ` brian m. carlson
2021-07-23  7:17         ` Ævar Arnfjörð Bjarmason
2021-07-22  6:27   ` Bagas Sanjaya
2021-07-23 10:16   ` Jeff King
2021-07-23 16:21     ` Junio C Hamano
2021-07-23 16:49       ` Randall S. Becker
2021-07-24  1:19       ` Jeff King

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git