Re: [PATCH v3 4/4] imap-send: use curl by default

[Nicolas Morey-Chaisemartin]

Le 23/08/2017 à 22:28, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin  writes:
>
>> Now that curl is enable by default,
> s/enable//; 
>
> But it is unclear what the above really means.  You certainly do not
> mean that [PATCH 1-3/4] somewhere tweaked our Makefile to always use
> libcurl and makes Git fail to build without it, but the above sounds
> as if that were the case.
>
>> use the curl implementation
>> for imap too.
> The Makefile for a long time by default set USE_CURL_FOR_IMAP_SEND
> to YesPlease when the version of cURL we have is recent enough, I
> think.  So I am not sure what you want to add with this change.
It did but apart from allowing the compilation of the code and enabling the 
--curl option to do something, it had no impact on the default runtime.
>> The goal is to validate feature parity between the legacy and
>> the curl implementation, deprecate thee legacy implementation
> s/thee/the/;

Yes this is confusing.
In the current state, even if build against a curl version supporting imap, 
curl is not used by default at runtime (unless OpenSSL was not available).
This patch changes this behavior.

>
>> later on and in the long term, hopefully drop it altogether.
>>
>> Signed-off-by: Nicolas Morey-Chaisemartin 
>> ---
>>  imap-send.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
> Hmph, the patch itself is also confusing.
>
>> diff --git a/imap-send.c b/imap-send.c
>> index a74d011a9..58c191704 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -35,11 +35,11 @@ typedef void *SSL;
>>  #include "http.h"
>>  #endif
>>  
>> -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
>> -/* only available option */
>> +#if defined(USE_CURL_FOR_IMAP_SEND)
>> +/* Always default to curl if it's available. */
>>  #define USE_CURL_DEFAULT 1
> The original says "we want to use CURL, if Makefile tells us to
> *AND* if Makefile tells us not to use OpenSSL", which does not make
> much sense to me.  I wonder if the original is buggy and should have
> been using "|| defined(NO_OPENSSL)" there instead.  
I think the idea for this was that curl should be used when curl is available 
and OpenSSL is not (curl being the only solution for secured authentication in 
this case)

>
> Perhaps that is the bug you are fixing with this patch, and all the
> talk about curl is default we saw above is a red herring?  If that
> is the case, then instead of removing, turning "&&" into "||" may be
> a better fix.  I dunno.

As said before, the goal of this patch is to enable curl by default at runtime 
when it has been "enabled" at compile time.
I'll reword

Is something like this clearer ?
Author: Nicolas Morey-Chaisemartin 
Date:   Sun Aug 6 21:30:15 2017 +0200

    imap-send: use curl by default when possible
   
    Set curl as the runtime default when it is available.
    When linked against older curl versions (< 7_34_0) or without curl,
    use the legacy imap implementation.
   
    The goal is to validate feature parity between the legacy and
    the curl implementation, deprecate the legacy implementation
    later on and in the long term, hopefully drop it altogether.
   
    Signed-off-by: Nicolas Morey-Chaisemartin 

Re: [PATCH v3 4/4] imap-send: use curl by default

[Junio C Hamano]

Nicolas Morey-Chaisemartin  writes:

> Now that curl is enable by default,

s/enable//; 

But it is unclear what the above really means.  You certainly do not
mean that [PATCH 1-3/4] somewhere tweaked our Makefile to always use
libcurl and makes Git fail to build without it, but the above sounds
as if that were the case.

> use the curl implementation
> for imap too.

The Makefile for a long time by default set USE_CURL_FOR_IMAP_SEND
to YesPlease when the version of cURL we have is recent enough, I
think.  So I am not sure what you want to add with this change.

> The goal is to validate feature parity between the legacy and
> the curl implementation, deprecate thee legacy implementation

s/thee/the/;

> later on and in the long term, hopefully drop it altogether.
>
> Signed-off-by: Nicolas Morey-Chaisemartin 
> ---
>  imap-send.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Hmph, the patch itself is also confusing.

> diff --git a/imap-send.c b/imap-send.c
> index a74d011a9..58c191704 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -35,11 +35,11 @@ typedef void *SSL;
>  #include "http.h"
>  #endif
>  
> -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
> -/* only available option */
> +#if defined(USE_CURL_FOR_IMAP_SEND)
> +/* Always default to curl if it's available. */
>  #define USE_CURL_DEFAULT 1

The original says "we want to use CURL, if Makefile tells us to
*AND* if Makefile tells us not to use OpenSSL", which does not make
much sense to me.  I wonder if the original is buggy and should have
been using "|| defined(NO_OPENSSL)" there instead.  

Perhaps that is the bug you are fixing with this patch, and all the
talk about curl is default we saw above is a red herring?  If that
is the case, then instead of removing, turning "&&" into "||" may be
a better fix.  I dunno.

>  #else
> -/* strictly opt in */
> +/* We don't have curl, so continue to use the historical implementation */
>  #define USE_CURL_DEFAULT 0
>  #endif

[PATCH v3 4/4] imap-send: use curl by default

[Nicolas Morey-Chaisemartin]

Now that curl is enable by default, use the curl implementation
for imap too.
The goal is to validate feature parity between the legacy and
the curl implementation, deprecate thee legacy implementation
later on and in the long term, hopefully drop it altogether.

Signed-off-by: Nicolas Morey-Chaisemartin 
---
 imap-send.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index a74d011a9..58c191704 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -35,11 +35,11 @@ typedef void *SSL;
 #include "http.h"
 #endif
 
-#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
-/* only available option */
+#if defined(USE_CURL_FOR_IMAP_SEND)
+/* Always default to curl if it's available. */
 #define USE_CURL_DEFAULT 1
 #else
-/* strictly opt in */
+/* We don't have curl, so continue to use the historical implementation */
 #define USE_CURL_DEFAULT 0
 #endif
 
-- 
2.14.0.1.gd9597ce13