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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 23, 2017, 20:28 UTC
Message-ID: <xmqqpobmxc3a.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <bb94cace-6bc5-2009-7c9d-a6965e3b84c6@morey-chaisemartin.com>

Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com> writes:

> Now that curl is enable by default,

s/enable/&d/;

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

Show 6 quoted lines

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

Hmph, the patch itself is also confusing.

Show 13 quoted lines

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

Show 5 quoted lines

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