Re: [PATCH 2/4] http: drop support for curl < 7.16.0

Jeff King [off-list ref] writes:

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

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

Le 09/08/2017 à 19:40, Junio C Hamano a écrit :

Jeff King [off-list ref] writes:

quoted

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

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

According to the documentation:

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


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

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

quoted

quoted

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

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

According to the documentation:

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


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

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

-Peff

Le 09/08/2017 à 23:17, Jeff King a écrit :

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

quoted

quoted

quoted

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

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

According to the documentation:

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


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

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

-Peff

It seems to be a decimal/hex issue:
docs/libcurl/symbols-in-versions:153:CURLOPT_KEYPASSWD               7.17.0

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

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

Nicolas

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

quoted

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

It seems to be a decimal/hex issue:
docs/libcurl/symbols-in-versions:153:CURLOPT_KEYPASSWD               7.17.0

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

If I'm not mistaken on cpp behaviour it means CURLOPT_KEYPASSWD is
evaluated to CURLOPT_SSLKEYPASSWD (git define) which is evaluated into
CURLOPT_KEYPASSWD (curl define).

It should stop here as CURLOPT_KEYPASSWD was not a defined macro when
the curl one was evaluated.  It might be worth cleaning though,
specially it wouldn't work anymore if the git macro is ever moved
before the curl include.

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

Thanks for digging up the curl history.

-Peff

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

Jeff King [off-list ref] writes:

quoted

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

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

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

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

-Peff