Re: [PATCH 3/4] http: drop support for curl < 7.19.4

2017-08-10 Thread Jeff King
On Thu, Aug 10, 2017 at 02:36:41PM +0200, Mischa POSLAWSKY wrote:

> Jeff King wrote:
> > -#if LIBCURL_VERSION_NUM >= 0x071301
> > curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
> > -#elif LIBCURL_VERSION_NUM >= 0x071101
> > curl_easy_setopt(result, CURLOPT_POST301, 1);
> > -#endif
> 
> This seems to be an unintended behavioural change: the second condition
> wouldn't have applied previously and overrides the first option
> (equivalent to CURLOPT_POSTREDIR = CURL_REDIR_POST_301).

Thanks, you're right. I'll fix it in my re-roll.

-Peff


Re: [PATCH 3/4] http: drop support for curl < 7.19.4

2017-08-10 Thread Mischa POSLAWSKY
Jeff King wrote:
> -#if LIBCURL_VERSION_NUM >= 0x071301
>   curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
> -#elif LIBCURL_VERSION_NUM >= 0x071101
>   curl_easy_setopt(result, CURLOPT_POST301, 1);
> -#endif

This seems to be an unintended behavioural change: the second condition
wouldn't have applied previously and overrides the first option
(equivalent to CURLOPT_POSTREDIR = CURL_REDIR_POST_301).

-- 
Mischa


Re: [PATCH 3/4] http: drop support for curl < 7.19.4

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

> On Wed, Aug 9, 2017 at 5:02 AM, Jeff King  wrote:
> > Since v2.12.0, Git does not compile with versions of curl
> > older than 7.19.4. That version of curl is about 8 years
> > old. This means it may still be used in some distributions
> > with long-running support periods. But the fact that we
> > haven't received a single bug report about the compile-time
> > breakage implies that nobody cares about building recent
> > releases on such platforms.
> 
> I would not state it as bland, see how
> https://public-inbox.org/git/20170806233850.14711-1-ava...@gmail.com/
> came here to the mailing list.

Heh, I almost added "Or they are happy patching Git themselves". This
_does_ make patching Git harder for them, because now there are a lot
more spots to patch.

> > As discussed in the previous two commits, this cleans up the
> > code and gives a more realistic signal to users about which
> > versions of Git are actually tested (in particular, this
> > moves us past the potential use-after-free issues with curl
> > older than 7.17.0).
> 
> This is a good reason for this patch, though, so maybe just elide
> the "nobody cares" part?

I think I'd rather elaborate than elide. One of the reasons to split
this into multiple patches is that it's a ready-made patch for a
distributor to apply (in reverse) if they really want to.

-Peff


Re: [PATCH 3/4] http: drop support for curl < 7.19.4

2017-08-09 Thread Stefan Beller
On Wed, Aug 9, 2017 at 5:02 AM, Jeff King  wrote:
> Since v2.12.0, Git does not compile with versions of curl
> older than 7.19.4. That version of curl is about 8 years
> old. This means it may still be used in some distributions
> with long-running support periods. But the fact that we
> haven't received a single bug report about the compile-time
> breakage implies that nobody cares about building recent
> releases on such platforms.

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


> As discussed in the previous two commits, this cleans up the
> code and gives a more realistic signal to users about which
> versions of Git are actually tested (in particular, this
> moves us past the potential use-after-free issues with curl
> older than 7.17.0).

This is a good reason for this patch, though, so maybe just elide
the "nobody cares" part?

Thanks for these cleanups!


Re: [PATCH 3/4] http: drop support for curl < 7.19.4

2017-08-09 Thread Jeff King
On Wed, Aug 09, 2017 at 03:14:22PM +0200, Ævar Arnfjörð Bjarmason wrote:

> This whole series looks good to me. As I commented on in the thread you
> referenced in 0/4 I think this is the right trade-off, and people like
> me who occasionally compile git on older systems can just easily package
> a newer curl as well if we need it.
> 
> My reading of the curl history/docs is that you should squash this into
> this last patch. It's code that's now dead since we require
> 7.19.4.
> 
> CURLAUTH_DIGEST_IE was added in 7.19.3, and as a comment this squash
> removes indicates CURLOPT_USE_SSL hasn't been needed since 7.16.4:
> https://curl.haxx.se/libcurl/c/CURLOPT_USE_SSL.html

Thanks. Do you mind formatting this as a patch on top instead of a
squash? I think it's sufficiently subtle that it should be separate from
the main cleanup, which is just dropping our own internal #ifdefs.

I guess that would make reverting harder, though.

-Peff


Re: [PATCH 3/4] http: drop support for curl < 7.19.4

2017-08-09 Thread Ævar Arnfjörð Bjarmason

On Wed, Aug 09 2017, Jeff King jotted:

> Since v2.12.0, Git does not compile with versions of curl
> older than 7.19.4. That version of curl is about 8 years
> old. This means it may still be used in some distributions
> with long-running support periods. But the fact that we
> haven't received a single bug report about the compile-time
> breakage implies that nobody cares about building recent
> releases on such platforms.

This whole series looks good to me. As I commented on in the thread you
referenced in 0/4 I think this is the right trade-off, and people like
me who occasionally compile git on older systems can just easily package
a newer curl as well if we need it.

My reading of the curl history/docs is that you should squash this into
this last patch. It's code that's now dead since we require
7.19.4.

CURLAUTH_DIGEST_IE was added in 7.19.3, and as a comment this squash
removes indicates CURLOPT_USE_SSL hasn't been needed since 7.16.4:
https://curl.haxx.se/libcurl/c/CURLOPT_USE_SSL.html

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

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

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

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

 #define DEFAULT_MAX_REQUESTS 5

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