All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] dropping support for ancient versions of curl
@ 2017-04-04  2:54 Jeff King
  2017-04-04  3:08 ` Jeff King
                   ` (3 more replies)
  0 siblings, 4 replies; 48+ messages in thread
From: Jeff King @ 2017-04-04  2:54 UTC (permalink / raw)
  To: git; +Cc: Jonathan Nieder

A nearby thread raised the question of whether we can rely on a version
of libcurl that contains a particular feature. The version in question
is curl 7.11.1, which came out in March 2004.

My feeling is that this is old enough to stop caring about. Which means
we can drop some of the #ifdefs that clutter the HTTP code (and there's
a patch at the end of this mail dropping support for everything older
than 7.11.1). But that made wonder: how old is too old? I think it's
nice that we don't force people to upgrade to the latest version of
curl. But at some point, if you are running a 13-year-old version of
libcurl, how likely are you to run a brand new version of Git? :)

If we declared 7.16.0 as a cutoff, we could unconditionally define
USE_CURL_MULTI, which gets rid of quite a few messy ifdefs.

-Peff

---
 http.c        | 51 -----------------------------------
 http.h        | 11 --------
 remote-curl.c |  3 ---
 3 files changed, 65 deletions(-)

diff --git a/http.c b/http.c
index 8d94e2c63..7a81f0b68 100644
--- a/http.c
+++ b/http.c
@@ -12,19 +12,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;
 size_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
@@ -57,12 +49,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
@@ -81,9 +69,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
@@ -123,7 +109,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. */
@@ -133,7 +118,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;
@@ -207,12 +191,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);
@@ -272,14 +252,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)) {
@@ -398,12 +374,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
@@ -416,7 +386,6 @@ static int curl_empty_auth_enabled(void)
 	if (http_auth_methods_restricted &&
 	    (http_auth_methods & ~empty_auth_useless))
 		return 1;
-#endif
 	return 0;
 }
 
@@ -487,7 +456,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++) {
@@ -505,7 +473,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)
@@ -707,12 +674,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) {
@@ -759,14 +722,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);
@@ -933,12 +892,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");
@@ -1111,12 +1066,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);
 
@@ -1383,13 +1334,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 02bccb7b0..d1de11a3d 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 e953d06f6..c792942f0 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -143,8 +143,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;
@@ -155,7 +153,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 */;
 	}

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-04  2:54 [RFC] dropping support for ancient versions of curl Jeff King
@ 2017-04-04  3:08 ` Jeff King
  2017-04-04  5:44   ` Jessie Hernandez
  2017-04-04  8:17 ` Ævar Arnfjörð Bjarmason
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 48+ messages in thread
From: Jeff King @ 2017-04-04  3:08 UTC (permalink / raw)
  To: git; +Cc: Jonathan Nieder

On Mon, Apr 03, 2017 at 10:54:38PM -0400, Jeff King wrote:

> If we declared 7.16.0 as a cutoff, we could unconditionally define
> USE_CURL_MULTI, which gets rid of quite a few messy ifdefs.

That version came out 11 years ago. Here's what that patch would look
like (on top of my other one, but note I missed one older ifdef in the
last one that's included here).

And we could go further. There are a number of ifdefs around 7.19.1 (Nov
2008), 7.22 (Sep 2011). A 5-year cutoff puts us at 7.24. That's getting
close enough that I'd probably start looking at what long-term distros
like RHEL are still shipping and supporting.

-Peff

---
 Documentation/config.txt |  3 +-
 http-push.c              | 23 --------------
 http-walker.c            | 12 --------
 http.c                   | 57 +----------------------------------
 http.h                   | 18 -----------
 remote-curl.c            |  4 ---
 6 files changed, 2 insertions(+), 115 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d5..2b04c1777 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1942,8 +1942,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 f0e3108f7..40146cdd6 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;
@@ -295,7 +291,6 @@ static void start_mkcol(struct transfer_request *request)
 		request->url = NULL;
 	}
 }
-#endif
 
 static void start_fetch_packed(struct transfer_request *request)
 {
@@ -600,7 +595,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)
 {
@@ -624,7 +618,6 @@ static int fill_active_slot(void *unused)
 	}
 	return 0;
 }
-#endif
 
 static void get_remote_object_list(unsigned char parent);
 
@@ -653,10 +646,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)
@@ -691,10 +682,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;
 }
@@ -1673,21 +1662,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)
@@ -1763,10 +1746,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);
 
@@ -1779,9 +1758,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 ee049cb13..b5b8e03b0 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 e9918f184..d4f8601ba 100644
--- a/http.c
+++ b/http.c
@@ -19,10 +19,8 @@ size_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
@@ -99,14 +97,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;
@@ -140,7 +130,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;
@@ -157,7 +146,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_)
 {
@@ -205,12 +193,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;
@@ -238,7 +223,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)
 {
@@ -268,18 +252,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;
@@ -494,7 +474,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;
@@ -516,13 +496,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)
 {
 	const char *sensitive_header;
@@ -876,7 +849,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)
@@ -886,7 +858,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;
@@ -909,10 +880,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;
@@ -949,9 +918,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);
@@ -996,7 +963,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 */
@@ -1005,7 +971,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;
@@ -1076,7 +1041,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;
 
@@ -1094,11 +1058,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 *);
@@ -1157,11 +1119,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;
@@ -1174,7 +1134,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) {
@@ -1186,10 +1145,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);
@@ -1212,12 +1167,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)
@@ -1231,9 +1180,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)
@@ -1344,12 +1291,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 d1de11a3d..4054af685 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)
 #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 c792942f0..4997cc193 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -434,7 +434,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;
@@ -455,7 +454,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_)
@@ -595,10 +593,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);

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-04  3:08 ` Jeff King
@ 2017-04-04  5:44   ` Jessie Hernandez
  0 siblings, 0 replies; 48+ messages in thread
From: Jessie Hernandez @ 2017-04-04  5:44 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Jonathan Nieder

> On Mon, Apr 03, 2017 at 10:54:38PM -0400, Jeff King wrote:
>
>> If we declared 7.16.0 as a cutoff, we could unconditionally define
>> USE_CURL_MULTI, which gets rid of quite a few messy ifdefs.
>
> That version came out 11 years ago. Here's what that patch would look
> like (on top of my other one, but note I missed one older ifdef in the
> last one that's included here).
>
> And we could go further. There are a number of ifdefs around 7.19.1 (Nov
> 2008), 7.22 (Sep 2011). A 5-year cutoff puts us at 7.24. That's getting
> close enough that I'd probably start looking at what long-term distros
> like RHEL are still shipping and supporting.

Hi Peff,

I am all for making code simpler and dropping ancient versions.
I think we have to look at about the 7.19.1 mark.

I can confirm on my Centos 6.8 machine and on RHEL 6.8 on the client side
curl 7.19.7 [1] is used. I do not know what other distros are using.

------------------------
Jessie

>
> -Peff
>
> ---
>  Documentation/config.txt |  3 +-
>  http-push.c              | 23 --------------
>  http-walker.c            | 12 --------
>  http.c                   | 57 +----------------------------------
>  http.h                   | 18 -----------
>  remote-curl.c            |  4 ---
>  6 files changed, 2 insertions(+), 115 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 475e874d5..2b04c1777 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1942,8 +1942,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 f0e3108f7..40146cdd6 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;
> @@ -295,7 +291,6 @@ static void start_mkcol(struct transfer_request
> *request)
>  		request->url = NULL;
>  	}
>  }
> -#endif
>
>  static void start_fetch_packed(struct transfer_request *request)
>  {
> @@ -600,7 +595,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)
>  {
> @@ -624,7 +618,6 @@ static int fill_active_slot(void *unused)
>  	}
>  	return 0;
>  }
> -#endif
>
>  static void get_remote_object_list(unsigned char parent);
>
> @@ -653,10 +646,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)
> @@ -691,10 +682,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;
>  }
> @@ -1673,21 +1662,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)
> @@ -1763,10 +1746,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);
>
> @@ -1779,9 +1758,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 ee049cb13..b5b8e03b0 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 e9918f184..d4f8601ba 100644
> --- a/http.c
> +++ b/http.c
> @@ -19,10 +19,8 @@ size_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
> @@ -99,14 +97,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;
> @@ -140,7 +130,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;
> @@ -157,7 +146,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_)
>  {
> @@ -205,12 +193,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;
> @@ -238,7 +223,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)
>  {
> @@ -268,18 +252,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;
> @@ -494,7 +474,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;
> @@ -516,13 +496,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)
>  {
>  	const char *sensitive_header;
> @@ -876,7 +849,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)
> @@ -886,7 +858,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;
> @@ -909,10 +880,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;
> @@ -949,9 +918,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);
> @@ -996,7 +963,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 */
> @@ -1005,7 +971,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;
> @@ -1076,7 +1041,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;
>
> @@ -1094,11 +1058,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 *);
> @@ -1157,11 +1119,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;
> @@ -1174,7 +1134,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) {
> @@ -1186,10 +1145,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);
> @@ -1212,12 +1167,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)
> @@ -1231,9 +1180,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)
> @@ -1344,12 +1291,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 d1de11a3d..4054af685 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)
>  #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 c792942f0..4997cc193 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -434,7 +434,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;
> @@ -455,7 +454,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_)
> @@ -595,10 +593,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);
>

[1] curl --version
curl 7.19.7 (x86_64-redhat-linux-gnu) libcurl/7.19.7 NSS/3.21 Basic ECC
zlib/1.2.3 libidn/1.18 libssh2/1.4.2
Protocols: tftp ftp telnet dict ldap ldaps http file https ftps scp sftp
Features: GSS-Negotiate IDN IPv6 Largefile NTLM SSL libz



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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-04  2:54 [RFC] dropping support for ancient versions of curl Jeff King
  2017-04-04  3:08 ` Jeff King
@ 2017-04-04  8:17 ` Ævar Arnfjörð Bjarmason
  2017-04-04  8:33   ` Jeff King
  2017-04-04 13:32 ` Frank Gevaerts
  2017-04-05  9:33 ` Tom G. Christensen
  3 siblings, 1 reply; 48+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-04-04  8:17 UTC (permalink / raw)
  To: Jeff King; +Cc: Git Mailing List, Jonathan Nieder

On Tue, Apr 4, 2017 at 4:54 AM, Jeff King <peff@peff.net> wrote:
> My feeling is that this is old enough to stop caring about. Which means
> we can drop some of the #ifdefs that clutter the HTTP code (and there's
> a patch at the end of this mail dropping support for everything older
> than 7.11.1). But that made wonder: how old is too old? I think it's
> nice that we don't force people to upgrade to the latest version of
> curl. But at some point, if you are running a 13-year-old version of
> libcurl, how likely are you to run a brand new version of Git? :)
>
> If we declared 7.16.0 as a cutoff, we could unconditionally define
> USE_CURL_MULTI, which gets rid of quite a few messy ifdefs.

I don't object to this patch, but just as a general comment, in
enterprise-y environments using an old OS (e.g. CentOS 5/6) & then
compiling some selected packages like git based on OS libraries is
quite common.

E.g. at work we're running git 2.12.0 compiled against CentOS 6
libraries, which has curl 7.20.1, released on
Apr 14 2010. Not so long ago we were still running CentOS 5 which
comes with 7.15.5 released in Aug 7 2006 which would break with your
patch.

Whether we support that is another question, I think it's reasonable
to say that if you're compiling git on such an old system you also
need to compile a libcurl instead of using the OS version. I just
wanted to point out that this *does* happen, someone is going to be
compiling new git releases on CentOS 5 & will be hit by this.

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-04  8:17 ` Ævar Arnfjörð Bjarmason
@ 2017-04-04  8:33   ` Jeff King
  2017-04-04 10:44     ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 48+ messages in thread
From: Jeff King @ 2017-04-04  8:33 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Git Mailing List, Jonathan Nieder

On Tue, Apr 04, 2017 at 10:17:51AM +0200, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Apr 4, 2017 at 4:54 AM, Jeff King <peff@peff.net> wrote:
> > My feeling is that this is old enough to stop caring about. Which means
> > we can drop some of the #ifdefs that clutter the HTTP code (and there's
> > a patch at the end of this mail dropping support for everything older
> > than 7.11.1). But that made wonder: how old is too old? I think it's
> > nice that we don't force people to upgrade to the latest version of
> > curl. But at some point, if you are running a 13-year-old version of
> > libcurl, how likely are you to run a brand new version of Git? :)
> >
> > If we declared 7.16.0 as a cutoff, we could unconditionally define
> > USE_CURL_MULTI, which gets rid of quite a few messy ifdefs.
> 
> I don't object to this patch, but just as a general comment, in
> enterprise-y environments using an old OS (e.g. CentOS 5/6) & then
> compiling some selected packages like git based on OS libraries is
> quite common.
> 
> E.g. at work we're running git 2.12.0 compiled against CentOS 6
> libraries, which has curl 7.20.1, released on
> Apr 14 2010. Not so long ago we were still running CentOS 5 which
> comes with 7.15.5 released in Aug 7 2006 which would break with your
> patch.
> 
> Whether we support that is another question, I think it's reasonable
> to say that if you're compiling git on such an old system you also
> need to compile a libcurl instead of using the OS version. I just
> wanted to point out that this *does* happen, someone is going to be
> compiling new git releases on CentOS 5 & will be hit by this.

Thanks, that's a very useful bit of data. According to:

  https://access.redhat.com/support/policy/updates/errata

RHEL5 (which as I understand it basically the same as CentOS 5 in terms
of packages) ended its 10-year support cycle just a few days ago. That
would perhaps make 7.20.1 a reasonable cutoff.

I dunno. We could also just punt on the whole thing. It was nice to see
a bunch of #ifdefs go away, but the code they're protecting is actually
not that complicated. Most of them do not have an #else at all, and we
just silently skip features on old versions.

I think it might be nice to declare a "too old" version, though, just so
we can stop adding _new_ ifdefs. Maybe 7.11.1 is that version now, and
in another few years we can bump to 7.16.0. :)

-Peff

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-04  8:33   ` Jeff King
@ 2017-04-04 10:44     ` Ævar Arnfjörð Bjarmason
  2017-04-04 11:54       ` Johannes Schindelin
  0 siblings, 1 reply; 48+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-04-04 10:44 UTC (permalink / raw)
  To: Jeff King; +Cc: Git Mailing List, Jonathan Nieder

On Tue, Apr 4, 2017 at 10:33 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Apr 04, 2017 at 10:17:51AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> On Tue, Apr 4, 2017 at 4:54 AM, Jeff King <peff@peff.net> wrote:
>> > My feeling is that this is old enough to stop caring about. Which means
>> > we can drop some of the #ifdefs that clutter the HTTP code (and there's
>> > a patch at the end of this mail dropping support for everything older
>> > than 7.11.1). But that made wonder: how old is too old? I think it's
>> > nice that we don't force people to upgrade to the latest version of
>> > curl. But at some point, if you are running a 13-year-old version of
>> > libcurl, how likely are you to run a brand new version of Git? :)
>> >
>> > If we declared 7.16.0 as a cutoff, we could unconditionally define
>> > USE_CURL_MULTI, which gets rid of quite a few messy ifdefs.
>>
>> I don't object to this patch, but just as a general comment, in
>> enterprise-y environments using an old OS (e.g. CentOS 5/6) & then
>> compiling some selected packages like git based on OS libraries is
>> quite common.
>>
>> E.g. at work we're running git 2.12.0 compiled against CentOS 6
>> libraries, which has curl 7.20.1, released on
>> Apr 14 2010. Not so long ago we were still running CentOS 5 which
>> comes with 7.15.5 released in Aug 7 2006 which would break with your
>> patch.
>>
>> Whether we support that is another question, I think it's reasonable
>> to say that if you're compiling git on such an old system you also
>> need to compile a libcurl instead of using the OS version. I just
>> wanted to point out that this *does* happen, someone is going to be
>> compiling new git releases on CentOS 5 & will be hit by this.
>
> Thanks, that's a very useful bit of data. According to:
>
>   https://access.redhat.com/support/policy/updates/errata
>
> RHEL5 (which as I understand it basically the same as CentOS 5 in terms
> of packages).

Yeah CentOS is just s/RHEL/CentOS/g across the RHEL-provided source
packages. It's a way to use RHEL without paying for (or getting) RHEL
support.

> ended its 10-year support cycle just a few days ago. That
> would perhaps make 7.20.1 a reasonable cutoff.

FWIW these cut-offs don't have a lot to do with how long some
installations run things like RHEL5 or CO5 actually run. RHEL5's EOP3
phase ended days ago, but the End of Extended Life-cycle Support goes
until late 2020, and for anyone running the CentOS flavors these dates
matter very little, since they're about how long RedHat is supporting
it, and if you don't have RedHat support in the first place...

> I dunno. We could also just punt on the whole thing. It was nice to see
> a bunch of #ifdefs go away, but the code they're protecting is actually
> not that complicated. Most of them do not have an #else at all, and we
> just silently skip features on old versions.
>
> I think it might be nice to declare a "too old" version, though, just so
> we can stop adding _new_ ifdefs. Maybe 7.11.1 is that version now, and
> in another few years we can bump to 7.16.0. :)

I think it's completely fine to include your patch as-is. At some
point we need to pass the burden of dealing with these old software
versions, saying that you should use a <10 year old library isn't
unreasonable. Anyone packaging new git on RHEL5 or derivatives can
just package a newer libcurl as well.

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-04 10:44     ` Ævar Arnfjörð Bjarmason
@ 2017-04-04 11:54       ` Johannes Schindelin
  2017-04-04 14:06         ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 48+ messages in thread
From: Johannes Schindelin @ 2017-04-04 11:54 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Jeff King, Git Mailing List, Jonathan Nieder

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

Hi,

On Tue, 4 Apr 2017, Ævar Arnfjörð Bjarmason wrote:

> I think it's completely fine to include your patch as-is. At some
> point we need to pass the burden of dealing with these old software
> versions, saying that you should use a <10 year old library isn't
> unreasonable. Anyone packaging new git on RHEL5 or derivatives can
> just package a newer libcurl as well.

But how much maintenance burden is it, really? Is the continued use of
those #ifdef's really worth this much discussion, let alone applying a
patch that may break users who have so far been happy?

It would be a different thing if we had to have hacks to support old cURL
versions, where we need to ship entire >10kB source files that tap into
internal data structures that may, or may not have changed. Such a hack, I
would be happy to discuss when we could possibly remove it.

But a couple of #ifdef's? C'mon, man, we can carry this *without sweat*
indefinitely ;-)

Ciao,
Dscho

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-04  2:54 [RFC] dropping support for ancient versions of curl Jeff King
  2017-04-04  3:08 ` Jeff King
  2017-04-04  8:17 ` Ævar Arnfjörð Bjarmason
@ 2017-04-04 13:32 ` Frank Gevaerts
  2017-04-05  9:33 ` Tom G. Christensen
  3 siblings, 0 replies; 48+ messages in thread
From: Frank Gevaerts @ 2017-04-04 13:32 UTC (permalink / raw)
  To: git

On Mon, Apr 03, 2017 at 10:54:38PM -0400, Jeff King wrote:
> A nearby thread raised the question of whether we can rely on a version
> of libcurl that contains a particular feature. The version in question
> is curl 7.11.1, which came out in March 2004.

I had a quick look at the 7.11.1 support, and I found that current git
actually doesn't build with anything older than curl 7.19.4. The issue
is aeae4db174, which moved some stuff to a helper function but didn't
copy the corresponding #ifdefs.

With that issue fixed, I could build with versions back to 7.12.2, which
is the oldest curl version I could get to build on my modern system.
Note that while I could build with those versions, I didn't actually
check if the result worked.

I'd say it's going to be increasingly likely with time that similar
issues will crop up for such old versions of dependencies.

Frank

diff --git a/http.c b/http.c
index 96d84bbed..8c782a086 100644
--- a/http.c
+++ b/http.c
@@ -674,6 +674,7 @@ void setup_curl_trace(CURL *handle)
        curl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);
 }
 
+#if LIBCURL_VERSION_NUM >= 0x071304
 static long get_curl_allowed_protocols(int from_user)
 {
        long allowed_protocols = 0;
@@ -689,6 +690,7 @@ static long get_curl_allowed_protocols(int from_user)
 
        return allowed_protocols;
 }
+#endif
 
 static CURL *get_curl_handle(void)
 {

-- 
"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it." - Brian W. Kernighan

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-04 11:54       ` Johannes Schindelin
@ 2017-04-04 14:06         ` Ævar Arnfjörð Bjarmason
  2017-04-04 16:53           ` Brandon Williams
  2017-04-04 20:16           ` Jeff King
  0 siblings, 2 replies; 48+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-04-04 14:06 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Jeff King, Git Mailing List, Jonathan Nieder, Brandon Williams, frank

On Tue, Apr 4, 2017 at 1:54 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Tue, 4 Apr 2017, Ævar Arnfjörð Bjarmason wrote:
>
>> I think it's completely fine to include your patch as-is. At some
>> point we need to pass the burden of dealing with these old software
>> versions, saying that you should use a <10 year old library isn't
>> unreasonable. Anyone packaging new git on RHEL5 or derivatives can
>> just package a newer libcurl as well.
>
> But how much maintenance burden is it, really? Is the continued use of
> those #ifdef's really worth this much discussion, let alone applying a
> patch that may break users who have so far been happy?
>
> It would be a different thing if we had to have hacks to support old cURL
> versions, where we need to ship entire >10kB source files that tap into
> internal data structures that may, or may not have changed. Such a hack, I
> would be happy to discuss when we could possibly remove it.
>
> But a couple of #ifdef's? C'mon, man, we can carry this *without sweat*
> indefinitely ;-)

I don't really care about applying this patch, but I wouldn't mind
seeing it applied.

I just wanted to clarify the counteractive point that it's not unusual
for some (particularly corporate) environments to be compiling fresh
upstream releases of some software against really ancient versions of
other upstream libraries.

But as Frank Gevaerts's reply (thanks!) which came after your reply
points out, this code has already been broken since v2.12.0, so it's
rarely used enough that nobody's reported being unable to compile git
2.12.0 on e.g. CentOS 5 >2 months since release.

I think this is a stronger argument for removing stuff like this. At
some point we're shipping code nobody's tested in combination with the
rest of our code. This can easily becomes a source of bugs as someone
e.g. compiling a new git on co5 becomes literally the first person to
ever test some new combination of codepaths we've added around mostly
unused ifdefs.

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-04 14:06         ` Ævar Arnfjörð Bjarmason
@ 2017-04-04 16:53           ` Brandon Williams
  2017-04-04 22:46             ` Johannes Schindelin
  2017-04-04 20:16           ` Jeff King
  1 sibling, 1 reply; 48+ messages in thread
From: Brandon Williams @ 2017-04-04 16:53 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Johannes Schindelin, Jeff King, Git Mailing List, Jonathan Nieder, frank

On 04/04, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Apr 4, 2017 at 1:54 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> >
> > On Tue, 4 Apr 2017, Ævar Arnfjörð Bjarmason wrote:
> >
> >> I think it's completely fine to include your patch as-is. At some
> >> point we need to pass the burden of dealing with these old software
> >> versions, saying that you should use a <10 year old library isn't
> >> unreasonable. Anyone packaging new git on RHEL5 or derivatives can
> >> just package a newer libcurl as well.
> >
> > But how much maintenance burden is it, really? Is the continued use of
> > those #ifdef's really worth this much discussion, let alone applying a
> > patch that may break users who have so far been happy?
> >
> > It would be a different thing if we had to have hacks to support old cURL
> > versions, where we need to ship entire >10kB source files that tap into
> > internal data structures that may, or may not have changed. Such a hack, I
> > would be happy to discuss when we could possibly remove it.
> >
> > But a couple of #ifdef's? C'mon, man, we can carry this *without sweat*
> > indefinitely ;-)
> 
> I don't really care about applying this patch, but I wouldn't mind
> seeing it applied.
> 
> I just wanted to clarify the counteractive point that it's not unusual
> for some (particularly corporate) environments to be compiling fresh
> upstream releases of some software against really ancient versions of
> other upstream libraries.
> 
> But as Frank Gevaerts's reply (thanks!) which came after your reply
> points out, this code has already been broken since v2.12.0, so it's
> rarely used enough that nobody's reported being unable to compile git
> 2.12.0 on e.g. CentOS 5 >2 months since release.
> 
> I think this is a stronger argument for removing stuff like this. At
> some point we're shipping code nobody's tested in combination with the
> rest of our code. This can easily becomes a source of bugs as someone
> e.g. compiling a new git on co5 becomes literally the first person to
> ever test some new combination of codepaths we've added around mostly
> unused ifdefs.

I'm all for seeing a patch like this applied.  I agree that we can't
expect the world to be running the most up-to-date version of curl but
we should be able to select some "oldest" version we will support which
can be bumped up every couple of years.  

I mean, ensuring that you are running with an up-to-date version of curl
is really important when it comes to all of the security fixes that have
been made in each revision.

-- 
Brandon Williams

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-04 14:06         ` Ævar Arnfjörð Bjarmason
  2017-04-04 16:53           ` Brandon Williams
@ 2017-04-04 20:16           ` Jeff King
  1 sibling, 0 replies; 48+ messages in thread
From: Jeff King @ 2017-04-04 20:16 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Johannes Schindelin, Git Mailing List, Jonathan Nieder,
	Brandon Williams, frank

On Tue, Apr 04, 2017 at 04:06:46PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > But a couple of #ifdef's? C'mon, man, we can carry this *without sweat*
> > indefinitely ;-)
> 
> I don't really care about applying this patch, but I wouldn't mind
> seeing it applied.
> 
> I just wanted to clarify the counteractive point that it's not unusual
> for some (particularly corporate) environments to be compiling fresh
> upstream releases of some software against really ancient versions of
> other upstream libraries.
> 
> But as Frank Gevaerts's reply (thanks!) which came after your reply
> points out, this code has already been broken since v2.12.0, so it's
> rarely used enough that nobody's reported being unable to compile git
> 2.12.0 on e.g. CentOS 5 >2 months since release.

Yeah, this is exactly the kind of thing I was wondering about (but was
too lazy to actually build an ancient version of curl -- thank you,
Frank).

In this case it was a compile error, which was obvious. I'm much more
worried about subtle interactions, or the fact that some of the ifdefs
are around security features that get ignored. In some cases we at least
issue a warning, but not always. E.g., we silently ignore
http.sslcapath.  Depending what you're using it for that could fail
closed (if you were trying to add a CA) or open (if you were trying to
restrict to a specific CA).

-Peff

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-04 16:53           ` Brandon Williams
@ 2017-04-04 22:46             ` Johannes Schindelin
  2017-04-04 23:03               ` Brandon Williams
  2017-04-04 23:03               ` Stefan Beller
  0 siblings, 2 replies; 48+ messages in thread
From: Johannes Schindelin @ 2017-04-04 22:46 UTC (permalink / raw)
  To: Brandon Williams
  Cc: Ævar Arnfjörð Bjarmason, Jeff King,
	Git Mailing List, Jonathan Nieder, frank

Hi Brandon,

On Tue, 4 Apr 2017, Brandon Williams wrote:

> I'm all for seeing a patch like this applied.  I agree that we can't
> expect the world to be running the most up-to-date version of curl but
> we should be able to select some "oldest" version we will support which
> can be bumped up every couple of years.  
> 
> I mean, ensuring that you are running with an up-to-date version of curl
> is really important when it comes to all of the security fixes that have
> been made in each revision.

I am not in the business of dictating to others what software they have to
run. I am in the business of maintaining Git for Windows. And part of that
job is to drag along code that is maybe not the most elegant, but works.

The patch in question resolves such a wart. Sure, it would be a cleanup.
Is it a huge maintenance burden to keep those few #ifdef's, though?
Absolutely not.

Ciao,
Johannes

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-04 22:46             ` Johannes Schindelin
@ 2017-04-04 23:03               ` Brandon Williams
  2017-04-04 23:03               ` Stefan Beller
  1 sibling, 0 replies; 48+ messages in thread
From: Brandon Williams @ 2017-04-04 23:03 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Ævar Arnfjörð Bjarmason, Jeff King,
	Git Mailing List, Jonathan Nieder, frank

On 04/05, Johannes Schindelin wrote:
> Hi Brandon,
> 
> On Tue, 4 Apr 2017, Brandon Williams wrote:
> 
> > I'm all for seeing a patch like this applied.  I agree that we can't
> > expect the world to be running the most up-to-date version of curl but
> > we should be able to select some "oldest" version we will support which
> > can be bumped up every couple of years.  
> > 
> > I mean, ensuring that you are running with an up-to-date version of curl
> > is really important when it comes to all of the security fixes that have
> > been made in each revision.
> 
> I am not in the business of dictating to others what software they have to
> run. I am in the business of maintaining Git for Windows. And part of that
> job is to drag along code that is maybe not the most elegant, but works.
> 
> The patch in question resolves such a wart. Sure, it would be a cleanup.
> Is it a huge maintenance burden to keep those few #ifdef's, though?
> Absolutely not.

haha, very true.  Of course I'm stating my opinion based on my limited
experience and work environment.

-- 
Brandon Williams

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-04 22:46             ` Johannes Schindelin
  2017-04-04 23:03               ` Brandon Williams
@ 2017-04-04 23:03               ` Stefan Beller
  2017-04-05  8:49                 ` Johannes Schindelin
  1 sibling, 1 reply; 48+ messages in thread
From: Stefan Beller @ 2017-04-04 23:03 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Brandon Williams, Ævar Arnfjörð Bjarmason,
	Jeff King, Git Mailing List, Jonathan Nieder, frank

On Tue, Apr 4, 2017 at 3:46 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Brandon,
>
> On Tue, 4 Apr 2017, Brandon Williams wrote:
>
>> I'm all for seeing a patch like this applied.  I agree that we can't
>> expect the world to be running the most up-to-date version of curl but
>> we should be able to select some "oldest" version we will support which
>> can be bumped up every couple of years.
>>
>> I mean, ensuring that you are running with an up-to-date version of curl
>> is really important when it comes to all of the security fixes that have
>> been made in each revision.
>
> I am not in the business of dictating to others what software they have to
> run. I am in the business of maintaining Git for Windows. And part of that
> job is to drag along code that is maybe not the most elegant, but works.
>
> The patch in question resolves such a wart. Sure, it would be a cleanup.
> Is it a huge maintenance burden to keep those few #ifdef's, though?
> Absolutely not.

Keeping them around is the easy part, the hard part is promising to users
that the software you maintain is as good as your reputation, when e.g.
we find out that certain #ifdefs don't even compile.
(See Frank Gevaerts answer)

So from my point of view it ought to be easier to maintain software that
is fully compiled and tested by a lot of people, and not have a long tail
of niche features that are not well tested.

Initially I thought I had a similar stance as you ("A well written line of code
is cheap") but I kept quiet, as I do not have a lot of experience with dealing
"old" Software.

Maybe the git community would want to take a look at the kernel community
(or other similar communities), how they solve the "long term stable" problem
of computer science.

And there are different things to be considered:
(1) the kernel community has "stable" maintainer(s) that are not the same
as the maintainer of the bleeding edge branch. So I would expect that these
maintainers have expertise in "dealing with old stuff, particular from
$DATE_RANGE".
(2) one of the kernels rules is "don't break user space". However sometimes
they do remove code[1]. And then their policy seemed to be: Wait until someone
shows up and we can revert the removal.

[1] http://lkml.iu.edu/hypermail/linux/kernel/1212.1/01152.html

So I would propose to take this patch, but be prepared to revert it in case
some user yells.

Thanks,
Stefan

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-04 23:03               ` Stefan Beller
@ 2017-04-05  8:49                 ` Johannes Schindelin
  2017-04-05  9:29                   ` Jeff King
  0 siblings, 1 reply; 48+ messages in thread
From: Johannes Schindelin @ 2017-04-05  8:49 UTC (permalink / raw)
  To: Stefan Beller
  Cc: Brandon Williams, Ævar Arnfjörð Bjarmason,
	Jeff King, Git Mailing List, Jonathan Nieder, frank

Hi Stefan,

On Tue, 4 Apr 2017, Stefan Beller wrote:

> On Tue, Apr 4, 2017 at 3:46 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Tue, 4 Apr 2017, Brandon Williams wrote:
> >
> >> I'm all for seeing a patch like this applied.  I agree that we can't
> >> expect the world to be running the most up-to-date version of curl
> >> but we should be able to select some "oldest" version we will support
> >> which can be bumped up every couple of years.
> >>
> >> I mean, ensuring that you are running with an up-to-date version of
> >> curl is really important when it comes to all of the security fixes
> >> that have been made in each revision.
> >
> > I am not in the business of dictating to others what software they
> > have to run. I am in the business of maintaining Git for Windows. And
> > part of that job is to drag along code that is maybe not the most
> > elegant, but works.
> >
> > The patch in question resolves such a wart. Sure, it would be a
> > cleanup.  Is it a huge maintenance burden to keep those few #ifdef's,
> > though?  Absolutely not.
> 
> Keeping them around is the easy part, the hard part is promising to
> users that the software you maintain is as good as your reputation, when
> e.g.  we find out that certain #ifdefs don't even compile.  (See Frank
> Gevaerts answer)

From that point of view, we should drop support for platforms as soon as
we have a bug in our code that lets the build fail: there is little
difference between the #ifdef's that let Git build with older dependencies
and #ifdef's that let Git build on platforms other than Linux.

By that reasoning, we would have dropped FreeBSD support a gazillion
times. Or for that matter, Windows support.

I obviously disagree with this stance.

> Initially I thought I had a similar stance as you ("A well written line
> of code is cheap") but I kept quiet, as I do not have a lot of
> experience with dealing "old" Software.

I am speaking from my point of view, of course, maintaining Git for
Windows. Of course I would love to drop support for old MSys. But that's
not going to happen because at least one active contributor has a vested
interest in keeping it going.

But even then, it sometimes takes a while until any breakages get fixed.
Prematurely removing a build option "because it has been broken for a
couple of major versions" would make it infinitely harder for (often
overworked) contributors to fix the breakage.

Anyway, this whole discussion took way more effort from my side than to
maintain a couple of slightly stale #ifdef's. From
https://xkcd.com/1205/'s point of view, I should have ignored the
"let's rip out stuff just because we can" patch.

I just thought that we care a bit more about contributors' experience.

> Maybe the git community would want to take a look at the kernel
> community (or other similar communities), how they solve the "long term
> stable" problem of computer science.

If we want to follow the example of a inclusive community that values
contributors' time and effort, maybe we should look elsewhere?

I vividly remember hearing Greg KH's statement at Git Merge 2016 "we
deliberately waste contributors' time". Very vividly. Very, very vividly.

> So I would propose to take this patch, but be prepared to revert it in case
> some user yells.

It would be much nicer to contributors (who are likely not subscribed to
the Git mailing list, or at least do not read all the mails coming in on
that channel) if they could simply imitate the surrounding #ifdef's and
make that tiny patch that adds a guard around an unused static function.

If you require them to revert a patch first that reinstates an almost
working state with an old dependency, you expect them to know that there
was such a patch in the first place, and to dig it up.

Let's reiterate that we are talking about some #ifdef's here that are a
tiny maintenance burden. That may have a bug here and there, easily fixed.

Again, I am sure that we can provide a much better contributors'
experience.

Also, maybe, just maybe, there are more pressing issues than removing a
couple lines here and there? This discussion vaguely reminds me of the
opening statement of https://en.wikipedia.org/wiki/Law_of_triviality...
Just saying'...

Ciao,
Dscho

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-05  8:49                 ` Johannes Schindelin
@ 2017-04-05  9:29                   ` Jeff King
  0 siblings, 0 replies; 48+ messages in thread
From: Jeff King @ 2017-04-05  9:29 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Stefan Beller, Brandon Williams,
	Ævar Arnfjörð Bjarmason, Git Mailing List,
	Jonathan Nieder, frank

On Wed, Apr 05, 2017 at 10:49:47AM +0200, Johannes Schindelin wrote:

> Let's reiterate that we are talking about some #ifdef's here that are a
> tiny maintenance burden. That may have a bug here and there, easily fixed.

Forget the maintenance cost for a moment. My concern is that we are
doing users a disservice by shipping broken and untested code without
them (or us) realizing it. The compile failure is the _best_ case,
because they know there's a bug to be fixed. A build that quietly fails
to enforce security properties is actively dangerous, and the user would
potentially be better off with an #error.

> Also, maybe, just maybe, there are more pressing issues than removing a
> couple lines here and there? This discussion vaguely reminds me of the
> opening statement of https://en.wikipedia.org/wiki/Law_of_triviality...
> Just saying'...

It's not just removing a couple of lines. It's remembering to check and
#ifdef new lines that get added, too (this conversation started because
of review on another patch which failed to do so). Is our attitude "add
it and when somebody with an ancient curl complains and provides a
patch, we'll #ifdef it"?

-Peff

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-04  2:54 [RFC] dropping support for ancient versions of curl Jeff King
                   ` (2 preceding siblings ...)
  2017-04-04 13:32 ` Frank Gevaerts
@ 2017-04-05  9:33 ` Tom G. Christensen
  2017-04-05 10:51   ` Ævar Arnfjörð Bjarmason
  2017-04-06  9:21   ` Jeff King
  3 siblings, 2 replies; 48+ messages in thread
From: Tom G. Christensen @ 2017-04-05  9:33 UTC (permalink / raw)
  To: Jeff King, git; +Cc: Jonathan Nieder

On 04/04/17 04:54, Jeff King wrote:
> A nearby thread raised the question of whether we can rely on a version
> of libcurl that contains a particular feature. The version in question
> is curl 7.11.1, which came out in March 2004.
>
> My feeling is that this is old enough to stop caring about. Which means
> we can drop some of the #ifdefs that clutter the HTTP code (and there's
> a patch at the end of this mail dropping support for everything older
> than 7.11.1). But that made wonder: how old is too old? I think it's
> nice that we don't force people to upgrade to the latest version of
> curl. But at some point, if you are running a 13-year-old version of
> libcurl, how likely are you to run a brand new version of Git? :)
>

FWIW I maintain freely available updated git packages for RHEL 3, 4, 5, 
6 and 7.

They can be found here:
https://jupiterrise.com/blog/jrpms/

And direct access here:
https://jupiterrise.com/jrpms/ (for el3,el4,el5)
https://jupiterrise.com/jrpmsplus/ (for el6, el7)

They are built against system versions of curl though a few patches are 
required for 7.10.6 (el3) and 7.12.1 (el4) support.
Patches can be found in the src.rpm, though I can also post them here as 
patch series, they cover more than just curl.

I don't use the el3 and el4 versions much any more and el5 use will also 
drop of now as I'm busy converting machines from el5 to el7.

-tgc

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-05  9:33 ` Tom G. Christensen
@ 2017-04-05 10:51   ` Ævar Arnfjörð Bjarmason
  2017-04-05 13:04     ` [PATCH 0/7] Patches to support older RHEL releases Tom G. Christensen
                       ` (2 more replies)
  2017-04-06  9:21   ` Jeff King
  1 sibling, 3 replies; 48+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-04-05 10:51 UTC (permalink / raw)
  To: Tom G. Christensen
  Cc: Jeff King, Git Mailing List, Jonathan Nieder, Todd Zullinger

On Wed, Apr 5, 2017 at 11:33 AM, Tom G. Christensen <tgc@jupiterrise.com> wrote:
> On 04/04/17 04:54, Jeff King wrote:
>>
>> A nearby thread raised the question of whether we can rely on a version
>> of libcurl that contains a particular feature. The version in question
>> is curl 7.11.1, which came out in March 2004.
>>
>> My feeling is that this is old enough to stop caring about. Which means
>> we can drop some of the #ifdefs that clutter the HTTP code (and there's
>> a patch at the end of this mail dropping support for everything older
>> than 7.11.1). But that made wonder: how old is too old? I think it's
>> nice that we don't force people to upgrade to the latest version of
>> curl. But at some point, if you are running a 13-year-old version of
>> libcurl, how likely are you to run a brand new version of Git? :)
>>
>
> FWIW I maintain freely available updated git packages for RHEL 3, 4, 5, 6
> and 7.
>
> They can be found here:
> https://jupiterrise.com/blog/jrpms/
>
> And direct access here:
> https://jupiterrise.com/jrpms/ (for el3,el4,el5)
> https://jupiterrise.com/jrpmsplus/ (for el6, el7)
>
> They are built against system versions of curl though a few patches are
> required for 7.10.6 (el3) and 7.12.1 (el4) support.

Whoah. So my assumption in
<CACBZZX78oKU5HuBEqb9qLy7--wcwhC_mW6x7Q+tB4suxohSCsQ@mail.gmail.com>
that nobody was compiling this & thus not reporting failures was
false. Rather there's an entire community & distribution mechanism
around patching git for older EL versions, but the patches aren't
making it upstream.

$ grep -h -e ^Subject -e ^Date *patch
Date: Tue, 13 Oct 2009 11:34:11 +0200
Subject: [PATCH 1/7] Make NO_PERL_MAKEMAKER behave more like
Date: Fri, 13 Jun 2014 11:02:02 +0200
Subject: [PATCH 2/7] Install man pages when NO_PERL_MAKEMAKER is used
Date: Mon, 22 Sep 2014 13:42:50 +0200
Subject: [PATCH 3/7] Allow svnrdump_sim.py to be used with Python 2.2
Date: Tue, 8 Mar 2016 09:31:56 +0100
Subject: [PATCH 4/7] Handle missing HTTP_CONNECTCODE in curl < 7.10.7
Date: Tue, 23 Aug 2016 10:32:51 +0200
Subject: [PATCH 5/7] Add support for gnupg < 1.4
Date: Tue, 23 Aug 2016 18:15:13 +0200
Subject: [PATCH 6/7] Handle missing CURLINFO_SSL_DATA_{IN,OUT}
Date: Tue, 23 Aug 2016 18:26:54 +0200
Subject: [PATCH 7/7] Do not use curl_easy_strerror with curl < 7.12.0
Date: Wed, 2 Feb 2011 21:24:44 -0500
Subject: [PATCH] Restore vc-git.el for basic compatibility on EL-5
Date: Mon, 23 Mar 2009 00:03:36 -0400
Subject: [PATCH] git-cvsimport: Ignore cvsps-2.2b1 Branches: output

> Patches can be found in the src.rpm, though I can also post them here as
> patch series, they cover more than just curl.
>
> I don't use the el3 and el4 versions much any more and el5 use will also
> drop of now as I'm busy converting machines from el5 to el7.

It would be great to have them on-list, as far as I can tell they were
never submitted? Is there some time/administrative reason for why
you're not submitting them? Some of these are many years old, it would
be great to have them on-list for wider review & included so vanilla
git works on these platforms.

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

* [PATCH 0/7] Patches to support older RHEL releases
  2017-04-05 10:51   ` Ævar Arnfjörð Bjarmason
@ 2017-04-05 13:04     ` Tom G. Christensen
  2017-04-05 13:04       ` [PATCH 1/7] Make NO_PERL_MAKEMAKER behave more like ExtUtils::MakeMaker Tom G. Christensen
                         ` (6 more replies)
  2017-04-05 13:04     ` [RFC] dropping support for ancient versions of curl Tom G. Christensen
  2017-04-06  0:53     ` brian m. carlson
  2 siblings, 7 replies; 48+ messages in thread
From: Tom G. Christensen @ 2017-04-05 13:04 UTC (permalink / raw)
  To: git

These are patches currently needed to support building and running git
on RHEL/CentOS 3 and 4.

Tom G. Christensen (7):
  Make NO_PERL_MAKEMAKER behave more like ExtUtils::MakeMaker
  Install man pages when NO_PERL_MAKEMAKER is used
  Allow svnrdump_sim.py to be used with Python 2.2
  Handle missing HTTP_CONNECTCODE in curl < 7.10.7
  Add support for gnupg < 1.4
  Handle missing CURLINFO_SSL_DATA_{IN,OUT}
  Do not use curl_easy_strerror with curl < 7.12.0

 Makefile                       |  6 ++++++
 contrib/svn-fe/svnrdump_sim.py |  4 ++--
 gpg-interface.c                |  2 ++
 http.c                         | 12 ++++++++++++
 perl/Makefile                  | 20 +++++++++++++++++++-
 5 files changed, 41 insertions(+), 3 deletions(-)

-- 
2.12.2


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

* [PATCH 1/7] Make NO_PERL_MAKEMAKER behave more like ExtUtils::MakeMaker
  2017-04-05 13:04     ` [PATCH 0/7] Patches to support older RHEL releases Tom G. Christensen
@ 2017-04-05 13:04       ` Tom G. Christensen
  2017-04-05 13:04       ` [PATCH 2/7] Install man pages when NO_PERL_MAKEMAKER is used Tom G. Christensen
                         ` (5 subsequent siblings)
  6 siblings, 0 replies; 48+ messages in thread
From: Tom G. Christensen @ 2017-04-05 13:04 UTC (permalink / raw)
  To: git

This change makes NO_PERL_MAKEMAKER behave more as ExtUtils::MakeMaker
by installing the modules into the perl libdir and not $(prefix)/lib.
It will default to sitelib but will allow the user to choose by setting
the INSTALLDIRS variable which is also honored by ExtUtils::MakeMaker.

Signed-off-by: Tom G. Christensen <tgc@jupiterrise.com>
---
 perl/Makefile | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/perl/Makefile b/perl/Makefile
index 15d96fcc7..ce53a240c 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -25,7 +25,12 @@ clean:
 $(makfile): PM.stamp
 
 ifdef NO_PERL_MAKEMAKER
-instdir_SQ = $(subst ','\'',$(prefix)/lib)
+perllib = site
+ifdef INSTALLDIRS
+perllib = $(INSTALLDIRS)
+endif
+perlinstdir := $(shell sh -c "$(PERL_PATH_SQ) -V:install$(perllib)lib | cut -d\' -f2")
+instdir_SQ = $(subst ','\'',$(perlinstdir))
 
 modules += Git
 modules += Git/I18N
-- 
2.12.2


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

* [PATCH 2/7] Install man pages when NO_PERL_MAKEMAKER is used
  2017-04-05 13:04     ` [PATCH 0/7] Patches to support older RHEL releases Tom G. Christensen
  2017-04-05 13:04       ` [PATCH 1/7] Make NO_PERL_MAKEMAKER behave more like ExtUtils::MakeMaker Tom G. Christensen
@ 2017-04-05 13:04       ` Tom G. Christensen
  2017-04-05 13:04       ` [PATCH 3/7] Allow svnrdump_sim.py to be used with Python 2.2 Tom G. Christensen
                         ` (4 subsequent siblings)
  6 siblings, 0 replies; 48+ messages in thread
From: Tom G. Christensen @ 2017-04-05 13:04 UTC (permalink / raw)
  To: git

This adds man page installation when using NO_PERL_MAKEMAKER to ensure
parity with the normal case where ExtUtils::MakeMaker is used.

Signed-off-by: Tom G. Christensen <tgc@jupiterrise.com>
---
 perl/Makefile | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/perl/Makefile b/perl/Makefile
index ce53a240c..c4b96b058 100644
--- a/perl/Makefile
+++ b/perl/Makefile
@@ -31,6 +31,11 @@ perllib = $(INSTALLDIRS)
 endif
 perlinstdir := $(shell sh -c "$(PERL_PATH_SQ) -V:install$(perllib)lib | cut -d\' -f2")
 instdir_SQ = $(subst ','\'',$(perlinstdir))
+ifdef mandir
+mandir_SQ = $(subst ',''\'',$(mandir))
+else
+mandir_SQ = /usr/share/man
+endif
 
 modules += Git
 modules += Git/I18N
@@ -74,9 +79,17 @@ $(makfile): ../GIT-CFLAGS Makefile
 		else \
 			subdir=/$${i%/*}; \
 		fi; \
+		if test -z $$subdir; \
+		then \
+			manpage=$$i; \
+		else \
+			manpage=$$(echo $$i|sed 's#/#::#g'); \
+		fi; \
 		echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/'$$i'.pm"' >> $@; \
 		echo '	mkdir -p "$$(DESTDIR)$(instdir_SQ)'$$subdir'"' >> $@; \
 		echo '	cp '$$i'.pm "$$(DESTDIR)$(instdir_SQ)/'$$i'.pm"' >> $@; \
+		echo '	mkdir -p "$$(DESTDIR)$(mandir_SQ)/man3"' >> $@; \
+		echo '	pod2man '$$i'.pm "$$(DESTDIR)$(mandir_SQ)/man3/'$$manpage'.3pm"' >> $@; \
 	done
 	echo '	$(RM) "$$(DESTDIR)$(instdir_SQ)/Error.pm"' >> $@
 	'$(PERL_PATH_SQ)' -MError -e 'exit($$Error::VERSION < 0.15009)' || \
-- 
2.12.2


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

* [PATCH 3/7] Allow svnrdump_sim.py to be used with Python 2.2
  2017-04-05 13:04     ` [PATCH 0/7] Patches to support older RHEL releases Tom G. Christensen
  2017-04-05 13:04       ` [PATCH 1/7] Make NO_PERL_MAKEMAKER behave more like ExtUtils::MakeMaker Tom G. Christensen
  2017-04-05 13:04       ` [PATCH 2/7] Install man pages when NO_PERL_MAKEMAKER is used Tom G. Christensen
@ 2017-04-05 13:04       ` Tom G. Christensen
  2017-04-05 13:40         ` Ævar Arnfjörð Bjarmason
  2017-04-05 13:04       ` [PATCH 4/7] Handle missing HTTP_CONNECTCODE in curl < 7.10.7 Tom G. Christensen
                         ` (3 subsequent siblings)
  6 siblings, 1 reply; 48+ messages in thread
From: Tom G. Christensen @ 2017-04-05 13:04 UTC (permalink / raw)
  To: git

This allows running the git-svn testsuite with Python 2.2.

Signed-off-by: Tom G. Christensen <tgc@jupiterrise.com>
---
 contrib/svn-fe/svnrdump_sim.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/svn-fe/svnrdump_sim.py b/contrib/svn-fe/svnrdump_sim.py
index 11ac6f692..86bf4a742 100755
--- a/contrib/svn-fe/svnrdump_sim.py
+++ b/contrib/svn-fe/svnrdump_sim.py
@@ -8,9 +8,9 @@ to the highest revision that should be available.
 import sys
 import os
 
-if sys.hexversion < 0x02040000:
+if sys.hexversion < 0x02020000:
     # The limiter is the ValueError() calls. This may be too conservative
-    sys.stderr.write("svnrdump-sim.py: requires Python 2.4 or later.\n")
+    sys.stderr.write("svnrdump-sim.py: requires Python 2.2 or later.\n")
     sys.exit(1)
 
 
-- 
2.12.2


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

* [PATCH 4/7] Handle missing HTTP_CONNECTCODE in curl < 7.10.7
  2017-04-05 13:04     ` [PATCH 0/7] Patches to support older RHEL releases Tom G. Christensen
                         ` (2 preceding siblings ...)
  2017-04-05 13:04       ` [PATCH 3/7] Allow svnrdump_sim.py to be used with Python 2.2 Tom G. Christensen
@ 2017-04-05 13:04       ` Tom G. Christensen
  2017-04-05 13:50         ` Ævar Arnfjörð Bjarmason
  2017-04-05 13:04       ` [PATCH 5/7] Add support for gnupg < 1.4 Tom G. Christensen
                         ` (2 subsequent siblings)
  6 siblings, 1 reply; 48+ messages in thread
From: Tom G. Christensen @ 2017-04-05 13:04 UTC (permalink / raw)
  To: git

With curl < 7.10.7 we cannot get the proxy CONNECT response code.
As a workaround set it to zero which means no response code available.

Signed-off-by: Tom G. Christensen <tgc@jupiterrise.com>
---
 http.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/http.c b/http.c
index 96d84bbed..ce618bdca 100644
--- a/http.c
+++ b/http.c
@@ -214,8 +214,12 @@ static void finish_active_slot(struct active_request_slot *slot)
 		slot->results->auth_avail = 0;
 #endif
 
+#if LIBCURL_VERSION_NUM >= 0x070a07
 		curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
 			&slot->results->http_connectcode);
+#else
+		slot->results->http_connectcode = 0;
+#endif
 	}
 
 	/* Run callback if appropriate */
-- 
2.12.2


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

* [PATCH 5/7] Add support for gnupg < 1.4
  2017-04-05 13:04     ` [PATCH 0/7] Patches to support older RHEL releases Tom G. Christensen
                         ` (3 preceding siblings ...)
  2017-04-05 13:04       ` [PATCH 4/7] Handle missing HTTP_CONNECTCODE in curl < 7.10.7 Tom G. Christensen
@ 2017-04-05 13:04       ` Tom G. Christensen
  2017-04-05 13:45         ` Ævar Arnfjörð Bjarmason
  2017-04-05 13:04       ` [PATCH 6/7] Handle missing CURLINFO_SSL_DATA_{IN,OUT} Tom G. Christensen
  2017-04-05 13:04       ` [PATCH 7/7] Do not use curl_easy_strerror with curl < 7.12.0 Tom G. Christensen
  6 siblings, 1 reply; 48+ messages in thread
From: Tom G. Christensen @ 2017-04-05 13:04 UTC (permalink / raw)
  To: git

This adds an OLD_GNUPG define to the Makefile which when activated will
ensure git does not use the --keyid-format argument when calling the
'gpg' program.
This is consistent with how 'gpg' was used in git < 2.10.0 and slightly
decreases security.

Signed-off-by: Tom G. Christensen <tgc@jupiterrise.com>
---
 Makefile        | 6 ++++++
 gpg-interface.c | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/Makefile b/Makefile
index ca9f16d19..f8f585d21 100644
--- a/Makefile
+++ b/Makefile
@@ -386,6 +386,8 @@ all::
 #
 # to say "export LESS=FRX (and LV=-c) if the environment variable
 # LESS (and LV) is not set, respectively".
+#
+# Define OLD_GNUPG if you need support for gnupg < 1.4.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1529,6 +1531,10 @@ ifndef PAGER_ENV
 PAGER_ENV = LESS=FRX LV=-c
 endif
 
+ifdef OLD_GNUPG
+	BASIC_CFLAGS += -DOLD_GNUPG
+endif
+
 QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
 QUIET_SUBDIR1  =
 
diff --git a/gpg-interface.c b/gpg-interface.c
index e44cc27da..57f1ea792 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -224,7 +224,9 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	argv_array_pushl(&gpg.args,
 			 gpg_program,
 			 "--status-fd=1",
+#ifndef OLD_GNUPG
 			 "--keyid-format=long",
+#endif
 			 "--verify", temp.filename.buf, "-",
 			 NULL);
 
-- 
2.12.2


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

* [PATCH 6/7] Handle missing CURLINFO_SSL_DATA_{IN,OUT}
  2017-04-05 13:04     ` [PATCH 0/7] Patches to support older RHEL releases Tom G. Christensen
                         ` (4 preceding siblings ...)
  2017-04-05 13:04       ` [PATCH 5/7] Add support for gnupg < 1.4 Tom G. Christensen
@ 2017-04-05 13:04       ` Tom G. Christensen
  2017-04-05 13:52         ` Ævar Arnfjörð Bjarmason
  2017-04-05 13:04       ` [PATCH 7/7] Do not use curl_easy_strerror with curl < 7.12.0 Tom G. Christensen
  6 siblings, 1 reply; 48+ messages in thread
From: Tom G. Christensen @ 2017-04-05 13:04 UTC (permalink / raw)
  To: git

Do not try and use CURLINFO_SSL_DATA_{IN,OUT} for curl < 7.12.1.

Signed-off-by: Tom G. Christensen <tgc@jupiterrise.com>
---
 http.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/http.c b/http.c
index ce618bdca..a46ab23af 100644
--- a/http.c
+++ b/http.c
@@ -649,10 +649,12 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,
 		text = "=> Send data";
 		curl_dump_data(text, (unsigned char *)data, size);
 		break;
+#if LIBCURL_VERSION_NUM >= 0x070c01
 	case CURLINFO_SSL_DATA_OUT:
 		text = "=> Send SSL data";
 		curl_dump_data(text, (unsigned char *)data, size);
 		break;
+#endif
 	case CURLINFO_HEADER_IN:
 		text = "<= Recv header";
 		curl_dump_header(text, (unsigned char *)data, size, NO_FILTER);
@@ -661,10 +663,12 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,
 		text = "<= Recv data";
 		curl_dump_data(text, (unsigned char *)data, size);
 		break;
+#if LIBCURL_VERSION_NUM >= 0x070c01
 	case CURLINFO_SSL_DATA_IN:
 		text = "<= Recv SSL data";
 		curl_dump_data(text, (unsigned char *)data, size);
 		break;
+#endif
 	}
 	return 0;
 }
-- 
2.12.2


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

* [PATCH 7/7] Do not use curl_easy_strerror with curl < 7.12.0
  2017-04-05 13:04     ` [PATCH 0/7] Patches to support older RHEL releases Tom G. Christensen
                         ` (5 preceding siblings ...)
  2017-04-05 13:04       ` [PATCH 6/7] Handle missing CURLINFO_SSL_DATA_{IN,OUT} Tom G. Christensen
@ 2017-04-05 13:04       ` Tom G. Christensen
  2017-04-05 13:53         ` Ævar Arnfjörð Bjarmason
  2017-04-06  9:18         ` Jeff King
  6 siblings, 2 replies; 48+ messages in thread
From: Tom G. Christensen @ 2017-04-05 13:04 UTC (permalink / raw)
  To: git

Commit 17966c0a added an unguarded use of curl_easy_strerror.
This adds a guard so it is not used with curl < 7.12.0.

Signed-off-by: Tom G. Christensen <tgc@jupiterrise.com>
---
 http.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/http.c b/http.c
index a46ab23af..104caaa75 100644
--- a/http.c
+++ b/http.c
@@ -2116,8 +2116,12 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,
 		CURLcode c = curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE,
 						&slot->http_code);
 		if (c != CURLE_OK)
+#if LIBCURL_VERSION_NUM >= 0x070c00
 			die("BUG: curl_easy_getinfo for HTTP code failed: %s",
 				curl_easy_strerror(c));
+#else
+			die("BUG: curl_easy_getinfo for HTTP code failed");
+#endif
 		if (slot->http_code >= 300)
 			return size;
 	}
-- 
2.12.2


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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-05 10:51   ` Ævar Arnfjörð Bjarmason
  2017-04-05 13:04     ` [PATCH 0/7] Patches to support older RHEL releases Tom G. Christensen
@ 2017-04-05 13:04     ` Tom G. Christensen
  2017-04-06  0:53     ` brian m. carlson
  2 siblings, 0 replies; 48+ messages in thread
From: Tom G. Christensen @ 2017-04-05 13:04 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Jeff King, Git Mailing List, Jonathan Nieder, Todd Zullinger

On 05/04/17 12:51, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Apr 5, 2017 at 11:33 AM, Tom G. Christensen <tgc@jupiterrise.com> wrote:
> Whoah. So my assumption in
> <CACBZZX78oKU5HuBEqb9qLy7--wcwhC_mW6x7Q+tB4suxohSCsQ@mail.gmail.com>
> that nobody was compiling this & thus not reporting failures was
> false. Rather there's an entire community & distribution mechanism
> around patching git for older EL versions, but the patches aren't
> making it upstream.
>

The community as I know it consists of me and EPEL5 (now dead and archived).
The packages that I build are probably only used by me and the company I 
work for as they are not exactly easy to find via search engines.
EPEL5 supported git 1.8.3.2 but the Fedora git specfile still contains 
all the infrastructure though I cannot know if it actually got used with 
anything later than 1.8.3.2.

I don't know of anyone that actually *needs* to use the latest git on 
RHEL < 5, myself included. I kept the support for RHEL < 5 because I 
could and it was good fun to tinker with.

Also I should say that testresults are good, no problems there except a 
few small nits as revealed in the specfile:
%if %{?el4:1}0
# These tests fail with subversion 1.1.4
export GIT_SKIP_TESTS="t9140.4"
%ifarch x86_64
# These tests fail with subversion 1.1.4 but only on x86_64
export GIT_SKIP_TESTS="$GIT_SKIP_TESTS t9106.7 t9106.8 t9106.9 t9106.10 
t9137.4 t9164.5 t9164.6 t9164.7 t9164.8"
%endif
%endif
%if %{?el3:1}%{?el4:1}0
# not ok 6 - url high-bit escapes
export GIT_SKIP_TESTS="$GIT_SKIP_TESTS t0110.6"
# not ok 32 - ref name 'heads/foo' is invalid
export GIT_SKIP_TESTS="$GIT_SKIP_TESTS t1402.32"
%endif
%if %{?el3:1}0
# t7800 failed 17 among 56 test(s)
export GIT_SKIP_TESTS="$GIT_SKIP_TESTS t7800"
%endif

It's been a little while since I did a build without those exclusions 
but I doubt much has changed.

> $ grep -h -e ^Subject -e ^Date *patch
> Date: Tue, 13 Oct 2009 11:34:11 +0200
> Subject: [PATCH 1/7] Make NO_PERL_MAKEMAKER behave more like
> Date: Fri, 13 Jun 2014 11:02:02 +0200
> Subject: [PATCH 2/7] Install man pages when NO_PERL_MAKEMAKER is used
> Date: Mon, 22 Sep 2014 13:42:50 +0200
> Subject: [PATCH 3/7] Allow svnrdump_sim.py to be used with Python 2.2
> Date: Tue, 8 Mar 2016 09:31:56 +0100
> Subject: [PATCH 4/7] Handle missing HTTP_CONNECTCODE in curl < 7.10.7
> Date: Tue, 23 Aug 2016 10:32:51 +0200
> Subject: [PATCH 5/7] Add support for gnupg < 1.4
> Date: Tue, 23 Aug 2016 18:15:13 +0200
> Subject: [PATCH 6/7] Handle missing CURLINFO_SSL_DATA_{IN,OUT}
> Date: Tue, 23 Aug 2016 18:26:54 +0200
> Subject: [PATCH 7/7] Do not use curl_easy_strerror with curl < 7.12.0

All original work done by me.

> Date: Wed, 2 Feb 2011 21:24:44 -0500
> Subject: [PATCH] Restore vc-git.el for basic compatibility on EL-5
> Date: Mon, 23 Mar 2009 00:03:36 -0400
> Subject: [PATCH] git-cvsimport: Ignore cvsps-2.2b1 Branches: output
>

These two I can't claim credit for. They are lifted verbatim from 
Fedora/EPEL and as the headers reveal they were created by Todd Zullinger.
I won't submit them for inclusion since I am not familiar with nor a 
user of the parts they touch.

>> Patches can be found in the src.rpm, though I can also post them here as
>> patch series, they cover more than just curl.
>>
>> I don't use the el3 and el4 versions much any more and el5 use will also
>> drop of now as I'm busy converting machines from el5 to el7.
>
> It would be great to have them on-list, as far as I can tell they were
> never submitted? Is there some time/administrative reason for why
> you're not submitting them?

Well I recently took the time to clean them up with the intention of 
maybe finally submitting them but I never got that far.

The first one in the series I actually submitted many years ago but it 
was ultimately rejected.

I've submitted a few patches over the years to support older RHEL 
releases and some of them ended up being included.

> Some of these are many years old, it would
> be great to have them on-list for wider review & included so vanilla
> git works on these platforms.
>

I just posted them now. The series was made against the v2.12.2 tag.

-tgc

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

* Re: [PATCH 3/7] Allow svnrdump_sim.py to be used with Python 2.2
  2017-04-05 13:04       ` [PATCH 3/7] Allow svnrdump_sim.py to be used with Python 2.2 Tom G. Christensen
@ 2017-04-05 13:40         ` Ævar Arnfjörð Bjarmason
  2017-04-05 14:36           ` Tom G. Christensen
  0 siblings, 1 reply; 48+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-04-05 13:40 UTC (permalink / raw)
  To: Tom G. Christensen; +Cc: Git Mailing List, Eric S. Raymond

On Wed, Apr 5, 2017 at 3:04 PM, Tom G. Christensen <tgc@jupiterrise.com> wrote:
> This allows running the git-svn testsuite with Python 2.2.

+CC-ing Eric S. Raymond who added these version limitations in a33faf2827.

Also, in his patch contrib/svn-fe/svnrdump_sim.py,
git_remote_helpers/git/__init__.py & git-p4.py is set to >=2.4 or
later.

Are you skipping those tests & they don't work under 2.2?

> Signed-off-by: Tom G. Christensen <tgc@jupiterrise.com>
> ---
>  contrib/svn-fe/svnrdump_sim.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/svn-fe/svnrdump_sim.py b/contrib/svn-fe/svnrdump_sim.py
> index 11ac6f692..86bf4a742 100755
> --- a/contrib/svn-fe/svnrdump_sim.py
> +++ b/contrib/svn-fe/svnrdump_sim.py
> @@ -8,9 +8,9 @@ to the highest revision that should be available.
>  import sys
>  import os
>
> -if sys.hexversion < 0x02040000:
> +if sys.hexversion < 0x02020000:
>      # The limiter is the ValueError() calls. This may be too conservative
> -    sys.stderr.write("svnrdump-sim.py: requires Python 2.4 or later.\n")
> +    sys.stderr.write("svnrdump-sim.py: requires Python 2.2 or later.\n")
>      sys.exit(1)
>
>
> --
> 2.12.2
>

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

* Re: [PATCH 5/7] Add support for gnupg < 1.4
  2017-04-05 13:04       ` [PATCH 5/7] Add support for gnupg < 1.4 Tom G. Christensen
@ 2017-04-05 13:45         ` Ævar Arnfjörð Bjarmason
  2017-04-13  6:31           ` Junio C Hamano
  2017-04-13 15:17           ` Ævar Arnfjörð Bjarmason
  0 siblings, 2 replies; 48+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-04-05 13:45 UTC (permalink / raw)
  To: Tom G. Christensen; +Cc: Git Mailing List, Linus Torvalds

On Wed, Apr 5, 2017 at 3:04 PM, Tom G. Christensen <tgc@jupiterrise.com> wrote:
> This adds an OLD_GNUPG define to the Makefile which when activated will
> ensure git does not use the --keyid-format argument when calling the
> 'gpg' program.
> This is consistent with how 'gpg' was used in git < 2.10.0 and slightly
> decreases security.

This changes the code Linus Torvalds added in b624a3e67f to mitigate
the evil32 project generating keys which looked the same for 32 bit
signatures.

I think this change makes sense, but the Makefile should have a
slightly scarier warning, something like:

"Define OLD_GNUPG if you need support for gnupg <1.4. Note that this
will cause git to only show the first 32 bits of PGP keys instead of
64, and there's a wide variety of brute-forced 32 bit keys in the wild
thanks to the evil32 project (https://evil32.com). Enabling this will
make GPG work old versions, but you might be fooled into accepting
malicious keys as a result".

> Signed-off-by: Tom G. Christensen <tgc@jupiterrise.com>
> ---
>  Makefile        | 6 ++++++
>  gpg-interface.c | 2 ++
>  2 files changed, 8 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index ca9f16d19..f8f585d21 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -386,6 +386,8 @@ all::
>  #
>  # to say "export LESS=FRX (and LV=-c) if the environment variable
>  # LESS (and LV) is not set, respectively".
> +#
> +# Define OLD_GNUPG if you need support for gnupg < 1.4.
>
>  GIT-VERSION-FILE: FORCE
>         @$(SHELL_PATH) ./GIT-VERSION-GEN
> @@ -1529,6 +1531,10 @@ ifndef PAGER_ENV
>  PAGER_ENV = LESS=FRX LV=-c
>  endif
>
> +ifdef OLD_GNUPG
> +       BASIC_CFLAGS += -DOLD_GNUPG
> +endif
> +
>  QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
>  QUIET_SUBDIR1  =
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index e44cc27da..57f1ea792 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -224,7 +224,9 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
>         argv_array_pushl(&gpg.args,
>                          gpg_program,
>                          "--status-fd=1",
> +#ifndef OLD_GNUPG
>                          "--keyid-format=long",
> +#endif
>                          "--verify", temp.filename.buf, "-",
>                          NULL);
>
> --
> 2.12.2
>

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

* Re: [PATCH 4/7] Handle missing HTTP_CONNECTCODE in curl < 7.10.7
  2017-04-05 13:04       ` [PATCH 4/7] Handle missing HTTP_CONNECTCODE in curl < 7.10.7 Tom G. Christensen
@ 2017-04-05 13:50         ` Ævar Arnfjörð Bjarmason
  2017-04-05 15:58           ` Franke, Knut
  0 siblings, 1 reply; 48+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-04-05 13:50 UTC (permalink / raw)
  To: Tom G. Christensen; +Cc: Git Mailing List, Knut Franke

On Wed, Apr 5, 2017 at 3:04 PM, Tom G. Christensen <tgc@jupiterrise.com> wrote:
> With curl < 7.10.7 we cannot get the proxy CONNECT response code.
> As a workaround set it to zero which means no response code available.

CC-ing Knut Franke <k.franke@science-computing.de> which added this
code in 372370f167.

This effectively disables that code & this later check:

+               if (results->http_connectcode == 407)
+                       credential_reject(&proxy_auth);

What's the impact of not taking that branch when the proxy returns a 407?

> Signed-off-by: Tom G. Christensen <tgc@jupiterrise.com>
> ---
>  http.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/http.c b/http.c
> index 96d84bbed..ce618bdca 100644
> --- a/http.c
> +++ b/http.c
> @@ -214,8 +214,12 @@ static void finish_active_slot(struct active_request_slot *slot)
>                 slot->results->auth_avail = 0;
>  #endif
>
> +#if LIBCURL_VERSION_NUM >= 0x070a07
>                 curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
>                         &slot->results->http_connectcode);
> +#else
> +               slot->results->http_connectcode = 0;
> +#endif
>         }
>
>         /* Run callback if appropriate */
> --
> 2.12.2
>

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

* Re: [PATCH 6/7] Handle missing CURLINFO_SSL_DATA_{IN,OUT}
  2017-04-05 13:04       ` [PATCH 6/7] Handle missing CURLINFO_SSL_DATA_{IN,OUT} Tom G. Christensen
@ 2017-04-05 13:52         ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 48+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-04-05 13:52 UTC (permalink / raw)
  To: Tom G. Christensen; +Cc: Git Mailing List, Elia Pinto

On Wed, Apr 5, 2017 at 3:04 PM, Tom G. Christensen <tgc@jupiterrise.com> wrote:
> Do not try and use CURLINFO_SSL_DATA_{IN,OUT} for curl < 7.12.1.

Disables code added by Elia Pinto in 74c682d3c6. Looks harmless to me
since it's just omitting some information when GIT_TRACE_CURL is set.

> Signed-off-by: Tom G. Christensen <tgc@jupiterrise.com>
> ---
>  http.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/http.c b/http.c
> index ce618bdca..a46ab23af 100644
> --- a/http.c
> +++ b/http.c
> @@ -649,10 +649,12 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,
>                 text = "=> Send data";
>                 curl_dump_data(text, (unsigned char *)data, size);
>                 break;
> +#if LIBCURL_VERSION_NUM >= 0x070c01
>         case CURLINFO_SSL_DATA_OUT:
>                 text = "=> Send SSL data";
>                 curl_dump_data(text, (unsigned char *)data, size);
>                 break;
> +#endif
>         case CURLINFO_HEADER_IN:
>                 text = "<= Recv header";
>                 curl_dump_header(text, (unsigned char *)data, size, NO_FILTER);
> @@ -661,10 +663,12 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size,
>                 text = "<= Recv data";
>                 curl_dump_data(text, (unsigned char *)data, size);
>                 break;
> +#if LIBCURL_VERSION_NUM >= 0x070c01
>         case CURLINFO_SSL_DATA_IN:
>                 text = "<= Recv SSL data";
>                 curl_dump_data(text, (unsigned char *)data, size);
>                 break;
> +#endif
>         }
>         return 0;
>  }
> --
> 2.12.2
>

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

* Re: [PATCH 7/7] Do not use curl_easy_strerror with curl < 7.12.0
  2017-04-05 13:04       ` [PATCH 7/7] Do not use curl_easy_strerror with curl < 7.12.0 Tom G. Christensen
@ 2017-04-05 13:53         ` Ævar Arnfjörð Bjarmason
  2017-04-06  9:18         ` Jeff King
  1 sibling, 0 replies; 48+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-04-05 13:53 UTC (permalink / raw)
  To: Tom G. Christensen; +Cc: Git Mailing List, Eric Wong

On Wed, Apr 5, 2017 at 3:04 PM, Tom G. Christensen <tgc@jupiterrise.com> wrote:
> Commit 17966c0a added an unguarded use of curl_easy_strerror.
> This adds a guard so it is not used with curl < 7.12.0.

Looks good to me. Eric Wong added this in 17966c0a63.

> Signed-off-by: Tom G. Christensen <tgc@jupiterrise.com>
> ---
>  http.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/http.c b/http.c
> index a46ab23af..104caaa75 100644
> --- a/http.c
> +++ b/http.c
> @@ -2116,8 +2116,12 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,
>                 CURLcode c = curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE,
>                                                 &slot->http_code);
>                 if (c != CURLE_OK)
> +#if LIBCURL_VERSION_NUM >= 0x070c00
>                         die("BUG: curl_easy_getinfo for HTTP code failed: %s",
>                                 curl_easy_strerror(c));
> +#else
> +                       die("BUG: curl_easy_getinfo for HTTP code failed");
> +#endif
>                 if (slot->http_code >= 300)
>                         return size;
>         }
> --
> 2.12.2
>

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

* Re: [PATCH 3/7] Allow svnrdump_sim.py to be used with Python 2.2
  2017-04-05 13:40         ` Ævar Arnfjörð Bjarmason
@ 2017-04-05 14:36           ` Tom G. Christensen
  0 siblings, 0 replies; 48+ messages in thread
From: Tom G. Christensen @ 2017-04-05 14:36 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Git Mailing List, Eric S. Raymond

On 05/04/17 15:40, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Apr 5, 2017 at 3:04 PM, Tom G. Christensen <tgc@jupiterrise.com> wrote:
>> This allows running the git-svn testsuite with Python 2.2.
>
> +CC-ing Eric S. Raymond who added these version limitations in a33faf2827.
>
> Also, in his patch contrib/svn-fe/svnrdump_sim.py,
> git_remote_helpers/git/__init__.py & git-p4.py is set to >=2.4 or
> later.
>
> Are you skipping those tests & they don't work under 2.2?
>

I don't know what git_remote_helpers/git/__init__.py does or which tests 
would run it.
I know that git-p4 tests require p4 which I don't have hence the tests 
are automatically skipped.

-tgc

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

* Re: [PATCH 4/7] Handle missing HTTP_CONNECTCODE in curl < 7.10.7
  2017-04-05 13:50         ` Ævar Arnfjörð Bjarmason
@ 2017-04-05 15:58           ` Franke, Knut
  0 siblings, 0 replies; 48+ messages in thread
From: Franke, Knut @ 2017-04-05 15:58 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Tom G. Christensen, Git Mailing List

On 2017-04-05 15:50, Ævar Arnfjörð Bjarmason wrote:
> This effectively disables that code & this later check:
> 
> +               if (results->http_connectcode == 407)
> +                       credential_reject(&proxy_auth);
> 
> What's the impact of not taking that branch when the proxy returns a 407?

We might be storing incorrect proxy credentials via the credential helper. If we can't get the
proxy's connect code, we should probably invalidate proxy credentials whenever http_code indicates
failure, since that's the only effect we'll see of a mistyped proxy password.

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-05 10:51   ` Ævar Arnfjörð Bjarmason
  2017-04-05 13:04     ` [PATCH 0/7] Patches to support older RHEL releases Tom G. Christensen
  2017-04-05 13:04     ` [RFC] dropping support for ancient versions of curl Tom G. Christensen
@ 2017-04-06  0:53     ` brian m. carlson
  2017-04-06  1:16       ` Todd Zullinger
  2017-04-06  9:29       ` Jeff King
  2 siblings, 2 replies; 48+ messages in thread
From: brian m. carlson @ 2017-04-06  0:53 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Tom G. Christensen, Jeff King, Git Mailing List, Jonathan Nieder,
	Todd Zullinger

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

On Wed, Apr 05, 2017 at 12:51:38PM +0200, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Apr 5, 2017 at 11:33 AM, Tom G. Christensen <tgc@jupiterrise.com> wrote:
> Whoah. So my assumption in
> <CACBZZX78oKU5HuBEqb9qLy7--wcwhC_mW6x7Q+tB4suxohSCsQ@mail.gmail.com>
> that nobody was compiling this & thus not reporting failures was
> false. Rather there's an entire community & distribution mechanism
> around patching git for older EL versions, but the patches aren't
> making it upstream.
> 
> $ grep -h -e ^Subject -e ^Date *patch
> Date: Tue, 13 Oct 2009 11:34:11 +0200
> Subject: [PATCH 1/7] Make NO_PERL_MAKEMAKER behave more like
> Date: Fri, 13 Jun 2014 11:02:02 +0200
> Subject: [PATCH 2/7] Install man pages when NO_PERL_MAKEMAKER is used
> Date: Mon, 22 Sep 2014 13:42:50 +0200
> Subject: [PATCH 3/7] Allow svnrdump_sim.py to be used with Python 2.2
> Date: Tue, 8 Mar 2016 09:31:56 +0100
> Subject: [PATCH 4/7] Handle missing HTTP_CONNECTCODE in curl < 7.10.7
> Date: Tue, 23 Aug 2016 10:32:51 +0200
> Subject: [PATCH 5/7] Add support for gnupg < 1.4
> Date: Tue, 23 Aug 2016 18:15:13 +0200
> Subject: [PATCH 6/7] Handle missing CURLINFO_SSL_DATA_{IN,OUT}
> Date: Tue, 23 Aug 2016 18:26:54 +0200
> Subject: [PATCH 7/7] Do not use curl_easy_strerror with curl < 7.12.0
> Date: Wed, 2 Feb 2011 21:24:44 -0500
> Subject: [PATCH] Restore vc-git.el for basic compatibility on EL-5
> Date: Mon, 23 Mar 2009 00:03:36 -0400
> Subject: [PATCH] git-cvsimport: Ignore cvsps-2.2b1 Branches: output
> 
> > Patches can be found in the src.rpm, though I can also post them here as
> > patch series, they cover more than just curl.
> >
> > I don't use the el3 and el4 versions much any more and el5 use will also
> > drop of now as I'm busy converting machines from el5 to el7.
> 
> It would be great to have them on-list, as far as I can tell they were
> never submitted? Is there some time/administrative reason for why
> you're not submitting them? Some of these are many years old, it would
> be great to have them on-list for wider review & included so vanilla
> git works on these platforms.

I'm very opposed to accepting patches for operating systems that are no
longer security supported.  Having insecure systems directly or
indirectly connected to the Internet is a very bad thing, and we
shouldn't make it easier for people who want to do that.

CentOS 6 and 7 are still in use and security supported, and I think we
should support them.  CentOS 5 is EOL, although RHEL 5 is still
supported in some cases.  RHEL 4 and earlier are definitely out of
support and we shouldn't support or consider them further.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

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

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-06  0:53     ` brian m. carlson
@ 2017-04-06  1:16       ` Todd Zullinger
  2017-04-06  9:29       ` Jeff King
  1 sibling, 0 replies; 48+ messages in thread
From: Todd Zullinger @ 2017-04-06  1:16 UTC (permalink / raw)
  To: brian m. carlson, Ævar Arnfjörð Bjarmason,
	Tom G. Christensen, Jeff King, Git Mailing List, Jonathan Nieder

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

brian m. carlson wrote:
> On Wed, Apr 05, 2017 at 12:51:38PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> On Wed, Apr 5, 2017 at 11:33 AM, Tom G. Christensen <tgc@jupiterrise.com> wrote:
>> Whoah. So my assumption in 
>> <CACBZZX78oKU5HuBEqb9qLy7--wcwhC_mW6x7Q+tB4suxohSCsQ@mail.gmail.com>
>> that nobody was compiling this & thus not reporting failures was 
>> false. Rather there's an entire community & distribution mechanism 
>> around patching git for older EL versions, but the patches aren't 
>> making it upstream.
>>
>> $ grep -h -e ^Subject -e ^Date *patch 
>> Date: Tue, 13 Oct 2009 11:34:11 +0200 
>> Subject: [PATCH 1/7] Make NO_PERL_MAKEMAKER behave more like 
>> Date: Fri, 13 Jun 2014 11:02:02 +0200 
>> Subject: [PATCH 2/7] Install man pages when NO_PERL_MAKEMAKER is used 
>> Date: Mon, 22 Sep 2014 13:42:50 +0200 
>> Subject: [PATCH 3/7] Allow svnrdump_sim.py to be used with Python 2.2 
>> Date: Tue, 8 Mar 2016 09:31:56 +0100 
>> Subject: [PATCH 4/7] Handle missing HTTP_CONNECTCODE in curl < 7.10.7 
>> Date: Tue, 23 Aug 2016 10:32:51 +0200 
>> Subject: [PATCH 5/7] Add support for gnupg < 1.4 
>> Date: Tue, 23 Aug 2016 18:15:13 +0200 
>> Subject: [PATCH 6/7] Handle missing CURLINFO_SSL_DATA_{IN,OUT} 
>> Date: Tue, 23 Aug 2016 18:26:54 +0200 
>> Subject: [PATCH 7/7] Do not use curl_easy_strerror with curl < 7.12.0 
>> Date: Wed, 2 Feb 2011 21:24:44 -0500 
>> Subject: [PATCH] Restore vc-git.el for basic compatibility on EL-5 
>> Date: Mon, 23 Mar 2009 00:03:36 -0400 
>> Subject: [PATCH] git-cvsimport: Ignore cvsps-2.2b1 Branches: output
>>
>>> Patches can be found in the src.rpm, though I can also post them here as 
>>> patch series, they cover more than just curl.
>>>
>>> I don't use the el3 and el4 versions much any more and el5 use will also 
>>> drop of now as I'm busy converting machines from el5 to el7.
>>
>> It would be great to have them on-list, as far as I can tell they were 
>> never submitted? Is there some time/administrative reason for why 
>> you're not submitting them? Some of these are many years old, it would 
>> be great to have them on-list for wider review & included so vanilla 
>> git works on these platforms.

The vc-git.el patch which I think came from the Fedora/EPEL package 
for EL-5 was only intended to restore functionality to support the 
older emacs on EL-5.  Now that EL-5 is EOL, it can (thankfully, IMO) 
die off.

The git-csvimport patch looks to be in the same category (assuming it 
was similar to what we included in the EPEL packages for EL-5 
support).  It was only needed (and applied) to support older systems 
and wasn't something worth carrying upstream.

> I'm very opposed to accepting patches for operating systems that are no 
> longer security supported.  Having insecure systems directly or 
> indirectly connected to the Internet is a very bad thing, and we 
> shouldn't make it easier for people who want to do that.

I concur.  I (or we, the Fedora/EPEL package maintainers for git) have 
tried to ensure that the packages for the latest Fedora release build 
cleanly for all supported Fedora and EL releases.  I think that is 
useful for those who use CentOS/RHEL and still want to have a recent 
git build.

I don't see any benefit in making it easier for folks to run systems 
that are no longer receiving updates for critical security issues.    
It's surely hard enough to support EL-6 as it ages. ;)

> CentOS 6 and 7 are still in use and security supported, and I think 
> we should support them.  CentOS 5 is EOL, although RHEL 5 is still 
> supported in some cases.  RHEL 4 and earlier are definitely out of 
> support and we shouldn't support or consider them further.

I know RHEL 5 has some extended support from Red Hat, but EPEL-5 is 
dead.  So from my perspective, supporting EL-5 isn't worth the effort.

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Statistics are like a lamp-post to a drunken man - more for leaning on
than illumination.


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

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

* Re: [PATCH 7/7] Do not use curl_easy_strerror with curl < 7.12.0
  2017-04-05 13:04       ` [PATCH 7/7] Do not use curl_easy_strerror with curl < 7.12.0 Tom G. Christensen
  2017-04-05 13:53         ` Ævar Arnfjörð Bjarmason
@ 2017-04-06  9:18         ` Jeff King
  2017-04-13  6:28           ` Junio C Hamano
  1 sibling, 1 reply; 48+ messages in thread
From: Jeff King @ 2017-04-06  9:18 UTC (permalink / raw)
  To: Tom G. Christensen; +Cc: git

On Wed, Apr 05, 2017 at 03:04:24PM +0200, Tom G. Christensen wrote:

> Commit 17966c0a added an unguarded use of curl_easy_strerror.
> This adds a guard so it is not used with curl < 7.12.0.
> 
> Signed-off-by: Tom G. Christensen <tgc@jupiterrise.com>
> ---
>  http.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/http.c b/http.c
> index a46ab23af..104caaa75 100644
> --- a/http.c
> +++ b/http.c
> @@ -2116,8 +2116,12 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,
>  		CURLcode c = curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE,
>  						&slot->http_code);
>  		if (c != CURLE_OK)
> +#if LIBCURL_VERSION_NUM >= 0x070c00
>  			die("BUG: curl_easy_getinfo for HTTP code failed: %s",
>  				curl_easy_strerror(c));
> +#else
> +			die("BUG: curl_easy_getinfo for HTTP code failed");
> +#endif

These kinds of interleaved conditionals make me nervous that we'll get
something wrong (especially without braces, it's not immediately clear
that both sides are a single statement).

I wonder if it would be more readable to do something like:

  #if LIBCURL_VERSION_NUM < 0x070c00
  static const char *curl_easy_strerror(CURL *curl)
  {
	return "[error code unavailable; curl version too old]";
  }
  #endif

Then callers don't have to individually deal with the ifdef. It does
mean that the user sees that kind-of ugly message, but maybe that is a
good thing. They know they need to upgrade curl to see more details.

-Peff

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-05  9:33 ` Tom G. Christensen
  2017-04-05 10:51   ` Ævar Arnfjörð Bjarmason
@ 2017-04-06  9:21   ` Jeff King
  2017-04-06 16:43     ` Tom G. Christensen
  1 sibling, 1 reply; 48+ messages in thread
From: Jeff King @ 2017-04-06  9:21 UTC (permalink / raw)
  To: Tom G. Christensen; +Cc: git, Jonathan Nieder

On Wed, Apr 05, 2017 at 11:33:37AM +0200, Tom G. Christensen wrote:

> FWIW I maintain freely available updated git packages for RHEL 3, 4, 5, 6
> and 7.
> 
> They can be found here:
> https://jupiterrise.com/blog/jrpms/
> 
> And direct access here:
> https://jupiterrise.com/jrpms/ (for el3,el4,el5)
> https://jupiterrise.com/jrpmsplus/ (for el6, el7)
> 
> They are built against system versions of curl though a few patches are
> required for 7.10.6 (el3) and 7.12.1 (el4) support.
> Patches can be found in the src.rpm, though I can also post them here as
> patch series, they cover more than just curl.
> 
> I don't use the el3 and el4 versions much any more and el5 use will also
> drop of now as I'm busy converting machines from el5 to el7.

Thanks for sharing, that's a really interesting data point.

I'm not quite sure what to take away from it, though. Either "yes,
somebody really is using Git with antique versions of curl". Or "even
the antique people are giving up el4, and it might be reasonable to
start requiring curl >= 7.11.0".

-Peff

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-06  0:53     ` brian m. carlson
  2017-04-06  1:16       ` Todd Zullinger
@ 2017-04-06  9:29       ` Jeff King
  2017-04-07 11:18         ` Johannes Schindelin
  1 sibling, 1 reply; 48+ messages in thread
From: Jeff King @ 2017-04-06  9:29 UTC (permalink / raw)
  To: brian m. carlson, Ævar Arnfjörð Bjarmason,
	Tom G. Christensen, Git Mailing List, Jonathan Nieder,
	Todd Zullinger

On Thu, Apr 06, 2017 at 12:53:01AM +0000, brian m. carlson wrote:

> > It would be great to have them on-list, as far as I can tell they were
> > never submitted? Is there some time/administrative reason for why
> > you're not submitting them? Some of these are many years old, it would
> > be great to have them on-list for wider review & included so vanilla
> > git works on these platforms.
> 
> I'm very opposed to accepting patches for operating systems that are no
> longer security supported.  Having insecure systems directly or
> indirectly connected to the Internet is a very bad thing, and we
> shouldn't make it easier for people who want to do that.

Hmm. I'm not sure whether I agree with that or not. I certainly wouldn't
want to _encourage_ people to use ancient unpatched systems. But I'm
also not entirely comfortable passing judgements on people's OS choices.
Security isn't a discrete variable, and there are lots of situations
where it makes sense to stick with an old, unpatched system because the
risk of changing it outweighs the risk of it being attacked (think
mission-critical systems sitting behind firewalls).

That said, I don't mind the argument "even the people who made this OS
are no longer supporting it; why should we?". And the response from Todd
seems to reinforce that.

And it's not like people on ancient mission-critical systems get cut
off. They can still run the version of Git they were running when their
OS went out of support.

-Peff

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-06  9:21   ` Jeff King
@ 2017-04-06 16:43     ` Tom G. Christensen
  2017-04-07  4:54       ` Jeff King
  0 siblings, 1 reply; 48+ messages in thread
From: Tom G. Christensen @ 2017-04-06 16:43 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Jonathan Nieder

On 06/04/17 11:21, Jeff King wrote:
> On Wed, Apr 05, 2017 at 11:33:37AM +0200, Tom G. Christensen wrote:
>> I don't use the el3 and el4 versions much any more and el5 use will also
>> drop of now as I'm busy converting machines from el5 to el7.
>
> Thanks for sharing, that's a really interesting data point.
>
> I'm not quite sure what to take away from it, though. Either "yes,
> somebody really is using Git with antique versions of curl". Or "even
> the antique people are giving up el4, and it might be reasonable to
> start requiring curl >= 7.11.0".
>

I do not know of anyone who actually need to have the latest git on 
their el3 or el4 system, myself included.
I'm not here to champion the inclusion of support for el3 and el4, there 
is no point.
I kept git buildable on those platforms since I could do so with minimal 
effort and good testresults, but this was entirely for my own satisfaction.

I know of no users of the packages that I make available other than 
myself and my work place (only el5 and later, soon just el6 and later).

There is not currently any patches needed to use git on el5 with curl 
7.15.5. The only thing not working out of the box in 2.12.2 would be the 
emacs integration.

If you must drop support for old curl releases then from my perspective 
the cutoff should be 7.19.7 at the latest since that is what ships with 
RHEL 6 and that is still supported by Red Hat.
Other long term supported Linux releases may have different ideas, I 
wouldn't know.

-tgc

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-06 16:43     ` Tom G. Christensen
@ 2017-04-07  4:54       ` Jeff King
  2017-04-14 11:12         ` Junio C Hamano
  0 siblings, 1 reply; 48+ messages in thread
From: Jeff King @ 2017-04-07  4:54 UTC (permalink / raw)
  To: Tom G. Christensen; +Cc: git, Jonathan Nieder

On Thu, Apr 06, 2017 at 06:43:06PM +0200, Tom G. Christensen wrote:

> On 06/04/17 11:21, Jeff King wrote:
> > On Wed, Apr 05, 2017 at 11:33:37AM +0200, Tom G. Christensen wrote:
> > > I don't use the el3 and el4 versions much any more and el5 use will also
> > > drop of now as I'm busy converting machines from el5 to el7.
> > 
> > Thanks for sharing, that's a really interesting data point.
> > 
> > I'm not quite sure what to take away from it, though. Either "yes,
> > somebody really is using Git with antique versions of curl". Or "even
> > the antique people are giving up el4, and it might be reasonable to
> > start requiring curl >= 7.11.0".

[Aside: re-reading my second paragraph above, it sounds like I am
 complaining that your comment wasn't clear. But you were perfectly
 clear. It's just that with your data point I am more conflicted than
 ever].

> I know of no users of the packages that I make available other than myself
> and my work place (only el5 and later, soon just el6 and later).
> 
> There is not currently any patches needed to use git on el5 with curl
> 7.15.5. The only thing not working out of the box in 2.12.2 would be the
> emacs integration.

I think I'm leaning towards the very first patch I posted, that assumes
7.11.0 and later. And then hold off on the others for a few years. In
terms of "number of ifdefs removed" we could go further, but I think it
was the first patch that removes a lot of the really questionable bits
(like silently ignoring security-related features).

> If you must drop support for old curl releases then from my perspective the
> cutoff should be 7.19.7 at the latest since that is what ships with RHEL 6
> and that is still supported by Red Hat.

Yeah, I think 7.19.7 would be the next reasonable cutoff (it shipped in
RHEL6, which is a reasonable standard for long-term support; and it
catches quite a few ifdefs in our code). But I think we can give RHEL5 a
bit more time. It just left its support window a few days ago.

-Peff

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-06  9:29       ` Jeff King
@ 2017-04-07 11:18         ` Johannes Schindelin
  2017-04-10 18:22           ` Jeff King
  0 siblings, 1 reply; 48+ messages in thread
From: Johannes Schindelin @ 2017-04-07 11:18 UTC (permalink / raw)
  To: Jeff King
  Cc: brian m. carlson, Ævar Arnfjörð Bjarmason,
	Tom G. Christensen, Git Mailing List, Jonathan Nieder,
	Todd Zullinger

Hi Peff,

On Thu, 6 Apr 2017, Jeff King wrote:

> And it's not like people on ancient mission-critical systems get cut
> off. They can still run the version of Git they were running when their
> OS went out of support.

You keep baiting me, so I'll bite, after resisting the urge for so long.

Let me share a little story of my own. So that this is not some academic,
theoretical wanking off, but something very tangible, very concrete, and
something very hard to handwave away.

As you know, in my previous job I was working as a scientist. Part of my
job was to support other scientists e.g. when they were using that really
big, really expensive, really advanced and super cool microscope. The
"really advanced" part only applied to the hardware, though, they were
running a super old CentOS, and everything was clamped down pretty good
(as many microscope vendors are wont to do), so there was no way to
upgrade the OS (I do not recall whether the drivers were only available
for this particular version of CentOS, but it would make sense, wouldn't
it, now, as microscope vendors are much more of experts in microscope
hardware than in anything vaguely related to software, even if they like
to pretend to be experts in both).

I did have access to gcc, though. And I did have to get Git working to be
able to install and develop additional software on that machine. It did
take me a while to get things to compile because of ancient libraries
being installed on that machine. Since only trusted people were allowed to
use that machine, security on that box was not really an issue. It did
make work for my colleagues substantially easier once I could develop on
that machine, using Git.

Just imagine how thankful I was that support for these old library
versions was almost working, still, and the one bigger problem I had was
easily solved by a patch that a quick web search found!

This. This is the impact of supporting older library versions, even if
they are long EOL.

Do not get me wrong: I am all for dropping support that is a huge
maintenance burden. You probably do not know the full back story, but let
me tell you what a relief it was to drop Windows XP support in Git for
Windows (and the Cygwin developers can sing several epic, Lord of the
Rings sized war songs about that, too).

At the same time, I want us to do better than all those maintainers out
there who drop backwards-compatibility just for the heck of it, not
because it would be a huge maintenance burden. "Everybody should upgrade,
anyway" is usually the naive excuse for not knowing why users are often
unable to upgrade.

Git is incredibly popular, and part of that is due to us being inclusive
and open and supporting a wide range of platforms. Yes, we can always do a
better job, that is true. We are also doing pretty well, though, and I
think that part of the reason is that we weigh carefully the maintenance
cost against the benefit of our users. Tiny "cleanup" patches that fail to
improve the maintenance burden substantially, and that also make users'
lives harder, are rightfully rejected. A little harder work from us
maintainers goes a long way to make a lot of users a lot happier.

My former self in my former life as a scientist is very thankful for the
consideration that goes into each and every "should we drop supporting
XYZ?" decision.

Very, very thankful.

Ciao,
Dscho

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-07 11:18         ` Johannes Schindelin
@ 2017-04-10 18:22           ` Jeff King
  0 siblings, 0 replies; 48+ messages in thread
From: Jeff King @ 2017-04-10 18:22 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: brian m. carlson, Ævar Arnfjörð Bjarmason,
	Tom G. Christensen, Git Mailing List, Jonathan Nieder,
	Todd Zullinger

On Fri, Apr 07, 2017 at 01:18:30PM +0200, Johannes Schindelin wrote:

> On Thu, 6 Apr 2017, Jeff King wrote:
> 
> > And it's not like people on ancient mission-critical systems get cut
> > off. They can still run the version of Git they were running when their
> > OS went out of support.
> 
> You keep baiting me, so I'll bite, after resisting the urge for so long.

I wasn't going to respond to this, because I didn't feel like the
discussion was going anywhere. But I ran across yet another issue
related to this today that hadn't been mentioned yet.

Your story shows that yes, it's convenient when old libraries are
supported. I don't dispute that. But one of my earlier points is that
this isn't just about maintenance burden (which I agree is not huge);
it's about whether we do a disservice to users to pretend that Git is
even remotely tested with older versions of curl.

For instance, did you know that versions of curl prior to v7.17 rely on
any strings fed via curl_easy_setopt() remaining valid for the lifetime
of the curl handle[1]?

We have some workarounds for this in old code (for example, see the
handling of CURLOPT_PASSWORD in http.c), but a lot of calls have been
added since then. I think there's a very good chance there are
use-after-free bugs when Git is compiled against an older curl.

I'm concerned that we're giving users a false sense of what is
reasonable to compile against.  You can reframe that as a maintenance
question (we _could_ find and fix those bugs), but that changes the
cost/benefit analysis.

[1] http://public-inbox.org/git/alpine.DEB.2.00.1306180825460.24456@tvnag.unkk.fr/

-Peff

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

* Re: [PATCH 7/7] Do not use curl_easy_strerror with curl < 7.12.0
  2017-04-06  9:18         ` Jeff King
@ 2017-04-13  6:28           ` Junio C Hamano
  2017-04-13 10:52             ` Jacob Keller
  0 siblings, 1 reply; 48+ messages in thread
From: Junio C Hamano @ 2017-04-13  6:28 UTC (permalink / raw)
  To: Jeff King; +Cc: Tom G. Christensen, git

Jeff King <peff@peff.net> writes:

> On Wed, Apr 05, 2017 at 03:04:24PM +0200, Tom G. Christensen wrote:
> ...
> These kinds of interleaved conditionals make me nervous that we'll get
> something wrong (especially without braces, it's not immediately clear
> that both sides are a single statement).
>
> I wonder if it would be more readable to do something like:
>
>   #if LIBCURL_VERSION_NUM < 0x070c00
>   static const char *curl_easy_strerror(CURL *curl)
>   {
> 	return "[error code unavailable; curl version too old]";
>   }
>   #endif
>
> Then callers don't have to individually deal with the ifdef. It does
> mean that the user sees that kind-of ugly message, but maybe that is a
> good thing. They know they need to upgrade curl to see more details.

Yup, thanks for a very good suggestion.

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

* Re: [PATCH 5/7] Add support for gnupg < 1.4
  2017-04-05 13:45         ` Ævar Arnfjörð Bjarmason
@ 2017-04-13  6:31           ` Junio C Hamano
  2017-04-13 15:17           ` Ævar Arnfjörð Bjarmason
  1 sibling, 0 replies; 48+ messages in thread
From: Junio C Hamano @ 2017-04-13  6:31 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Tom G. Christensen, Git Mailing List, Linus Torvalds

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

> On Wed, Apr 5, 2017 at 3:04 PM, Tom G. Christensen <tgc@jupiterrise.com> wrote:
>> This adds an OLD_GNUPG define to the Makefile which when activated will
>> ensure git does not use the --keyid-format argument when calling the
>> 'gpg' program.
>> This is consistent with how 'gpg' was used in git < 2.10.0 and slightly
>> decreases security.
>
> This changes the code Linus Torvalds added in b624a3e67f to mitigate
> the evil32 project generating keys which looked the same for 32 bit
> signatures.
>
> I think this change makes sense, but the Makefile should have a
> slightly scarier warning, something like:
>
> "Define OLD_GNUPG if you need support for gnupg <1.4. Note that this
> will cause git to only show the first 32 bits of PGP keys instead of
> 64, and there's a wide variety of brute-forced 32 bit keys in the wild
> thanks to the evil32 project (https://evil32.com). Enabling this will
> make GPG work old versions, but you might be fooled into accepting
> malicious keys as a result".

Very good point.  It surprised me somewhat to see that this is the
only change necessary (iow, there is no need to tweak anything in t/
directory).  Perhaps we would need a test or two (and need to figure
out a way to make them work with both old and more recent GnuPG)?

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

* Re: [PATCH 7/7] Do not use curl_easy_strerror with curl < 7.12.0
  2017-04-13  6:28           ` Junio C Hamano
@ 2017-04-13 10:52             ` Jacob Keller
  0 siblings, 0 replies; 48+ messages in thread
From: Jacob Keller @ 2017-04-13 10:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Tom G. Christensen, Git mailing list

On Wed, Apr 12, 2017 at 11:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Wed, Apr 05, 2017 at 03:04:24PM +0200, Tom G. Christensen wrote:
>> ...
>> These kinds of interleaved conditionals make me nervous that we'll get
>> something wrong (especially without braces, it's not immediately clear
>> that both sides are a single statement).
>>
>> I wonder if it would be more readable to do something like:
>>
>>   #if LIBCURL_VERSION_NUM < 0x070c00
>>   static const char *curl_easy_strerror(CURL *curl)
>>   {
>>       return "[error code unavailable; curl version too old]";
>>   }
>>   #endif
>>
>> Then callers don't have to individually deal with the ifdef. It does
>> mean that the user sees that kind-of ugly message, but maybe that is a
>> good thing. They know they need to upgrade curl to see more details.
>
> Yup, thanks for a very good suggestion.

I also think this is a good solution..

Thanks,
Jake

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

* Re: [PATCH 5/7] Add support for gnupg < 1.4
  2017-04-05 13:45         ` Ævar Arnfjörð Bjarmason
  2017-04-13  6:31           ` Junio C Hamano
@ 2017-04-13 15:17           ` Ævar Arnfjörð Bjarmason
  1 sibling, 0 replies; 48+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-04-13 15:17 UTC (permalink / raw)
  To: Tom G. Christensen; +Cc: Git Mailing List, Linus Torvalds

On Wed, Apr 5, 2017 at 3:45 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Wed, Apr 5, 2017 at 3:04 PM, Tom G. Christensen <tgc@jupiterrise.com> wrote:
>> This adds an OLD_GNUPG define to the Makefile which when activated will
>> ensure git does not use the --keyid-format argument when calling the
>> 'gpg' program.
>> This is consistent with how 'gpg' was used in git < 2.10.0 and slightly
>> decreases security.
>
> This changes the code Linus Torvalds added in b624a3e67f to mitigate
> the evil32 project generating keys which looked the same for 32 bit
> signatures.
>
> I think this change makes sense, but the Makefile should have a
> slightly scarier warning, something like:
>
> "Define OLD_GNUPG if you need support for gnupg <1.4. Note that this
> will cause git to only show the first 32 bits of PGP keys instead of
> 64, and there's a wide variety of brute-forced 32 bit keys in the wild
> thanks to the evil32 project (https://evil32.com). Enabling this will
> make GPG work old versions, but you might be fooled into accepting

grammar fix: "work on older versions"....

> malicious keys as a result".
>
>> Signed-off-by: Tom G. Christensen <tgc@jupiterrise.com>
>> ---
>>  Makefile        | 6 ++++++
>>  gpg-interface.c | 2 ++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/Makefile b/Makefile
>> index ca9f16d19..f8f585d21 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -386,6 +386,8 @@ all::
>>  #
>>  # to say "export LESS=FRX (and LV=-c) if the environment variable
>>  # LESS (and LV) is not set, respectively".
>> +#
>> +# Define OLD_GNUPG if you need support for gnupg < 1.4.
>>
>>  GIT-VERSION-FILE: FORCE
>>         @$(SHELL_PATH) ./GIT-VERSION-GEN
>> @@ -1529,6 +1531,10 @@ ifndef PAGER_ENV
>>  PAGER_ENV = LESS=FRX LV=-c
>>  endif
>>
>> +ifdef OLD_GNUPG
>> +       BASIC_CFLAGS += -DOLD_GNUPG
>> +endif
>> +
>>  QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
>>  QUIET_SUBDIR1  =
>>
>> diff --git a/gpg-interface.c b/gpg-interface.c
>> index e44cc27da..57f1ea792 100644
>> --- a/gpg-interface.c
>> +++ b/gpg-interface.c
>> @@ -224,7 +224,9 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
>>         argv_array_pushl(&gpg.args,
>>                          gpg_program,
>>                          "--status-fd=1",
>> +#ifndef OLD_GNUPG
>>                          "--keyid-format=long",
>> +#endif
>>                          "--verify", temp.filename.buf, "-",
>>                          NULL);
>>
>> --
>> 2.12.2
>>

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

* Re: [RFC] dropping support for ancient versions of curl
  2017-04-07  4:54       ` Jeff King
@ 2017-04-14 11:12         ` Junio C Hamano
  0 siblings, 0 replies; 48+ messages in thread
From: Junio C Hamano @ 2017-04-14 11:12 UTC (permalink / raw)
  To: Jeff King; +Cc: Tom G. Christensen, git, Jonathan Nieder

Jeff King <peff@peff.net> writes:

> I think I'm leaning towards the very first patch I posted, that assumes
> 7.11.0 and later. And then hold off on the others for a few years. In
> terms of "number of ifdefs removed" we could go further, but I think it
> was the first patch that removes a lot of the really questionable bits
> (like silently ignoring security-related features).

I do not have problems with setting cut-off at 7.11.0; I do not
foresee anybody who actively maintains a port for a platform whose
cURL is older than that version would step up ;-)

Thanks.

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

end of thread, other threads:[~2017-04-14 11:12 UTC | newest]

Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-04  2:54 [RFC] dropping support for ancient versions of curl Jeff King
2017-04-04  3:08 ` Jeff King
2017-04-04  5:44   ` Jessie Hernandez
2017-04-04  8:17 ` Ævar Arnfjörð Bjarmason
2017-04-04  8:33   ` Jeff King
2017-04-04 10:44     ` Ævar Arnfjörð Bjarmason
2017-04-04 11:54       ` Johannes Schindelin
2017-04-04 14:06         ` Ævar Arnfjörð Bjarmason
2017-04-04 16:53           ` Brandon Williams
2017-04-04 22:46             ` Johannes Schindelin
2017-04-04 23:03               ` Brandon Williams
2017-04-04 23:03               ` Stefan Beller
2017-04-05  8:49                 ` Johannes Schindelin
2017-04-05  9:29                   ` Jeff King
2017-04-04 20:16           ` Jeff King
2017-04-04 13:32 ` Frank Gevaerts
2017-04-05  9:33 ` Tom G. Christensen
2017-04-05 10:51   ` Ævar Arnfjörð Bjarmason
2017-04-05 13:04     ` [PATCH 0/7] Patches to support older RHEL releases Tom G. Christensen
2017-04-05 13:04       ` [PATCH 1/7] Make NO_PERL_MAKEMAKER behave more like ExtUtils::MakeMaker Tom G. Christensen
2017-04-05 13:04       ` [PATCH 2/7] Install man pages when NO_PERL_MAKEMAKER is used Tom G. Christensen
2017-04-05 13:04       ` [PATCH 3/7] Allow svnrdump_sim.py to be used with Python 2.2 Tom G. Christensen
2017-04-05 13:40         ` Ævar Arnfjörð Bjarmason
2017-04-05 14:36           ` Tom G. Christensen
2017-04-05 13:04       ` [PATCH 4/7] Handle missing HTTP_CONNECTCODE in curl < 7.10.7 Tom G. Christensen
2017-04-05 13:50         ` Ævar Arnfjörð Bjarmason
2017-04-05 15:58           ` Franke, Knut
2017-04-05 13:04       ` [PATCH 5/7] Add support for gnupg < 1.4 Tom G. Christensen
2017-04-05 13:45         ` Ævar Arnfjörð Bjarmason
2017-04-13  6:31           ` Junio C Hamano
2017-04-13 15:17           ` Ævar Arnfjörð Bjarmason
2017-04-05 13:04       ` [PATCH 6/7] Handle missing CURLINFO_SSL_DATA_{IN,OUT} Tom G. Christensen
2017-04-05 13:52         ` Ævar Arnfjörð Bjarmason
2017-04-05 13:04       ` [PATCH 7/7] Do not use curl_easy_strerror with curl < 7.12.0 Tom G. Christensen
2017-04-05 13:53         ` Ævar Arnfjörð Bjarmason
2017-04-06  9:18         ` Jeff King
2017-04-13  6:28           ` Junio C Hamano
2017-04-13 10:52             ` Jacob Keller
2017-04-05 13:04     ` [RFC] dropping support for ancient versions of curl Tom G. Christensen
2017-04-06  0:53     ` brian m. carlson
2017-04-06  1:16       ` Todd Zullinger
2017-04-06  9:29       ` Jeff King
2017-04-07 11:18         ` Johannes Schindelin
2017-04-10 18:22           ` Jeff King
2017-04-06  9:21   ` Jeff King
2017-04-06 16:43     ` Tom G. Christensen
2017-04-07  4:54       ` Jeff King
2017-04-14 11:12         ` Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.