[PATCH v3 0/4] imap-send: Fix and enable curl by default

[PATCH v3 0/4] imap-send: Fix and enable curl by default

[Nicolas Morey-Chaisemartin]

Changes since v2:
Patch 3 reject credit when curl failed:
 - when a login error  is returned (curl >= 7.13.1)
 - for any curl error in older versions

Nicolas Morey-Chaisemartin (4):
  imap-send: return with error if curl failed
  imap-send: add wrapper to get server credentials if needed
  imap_send: setup_curl: retreive credentials if not set in config file
  imap-send: use curl by default

 imap-send.c | 60 ++++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 40 insertions(+), 20 deletions(-)

-- 
2.14.0.1.gd9597ce13

[PATCH v3 1/4] imap-send: return with error if curl failed

[Nicolas Morey-Chaisemartin]

curl_append_msgs_to_imap always returned 0, whether curl failed or not.
Return a proper status so git imap-send will exit with an error code
if womething wrong happened.

Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 imap-send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/imap-send.c b/imap-send.c
index b2d0b849b..09f29ea95 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
 	curl_easy_cleanup(curl);
 	curl_global_cleanup();
 
-	return 0;
+	return res == CURLE_OK;
 }
 #endif
 
-- 
2.14.0.1.gd9597ce13

Re: [PATCH v3 1/4] imap-send: return with error if curl failed

[Junio C Hamano]

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

> curl_append_msgs_to_imap always returned 0, whether curl failed or not.
> Return a proper status so git imap-send will exit with an error code
> if womething wrong happened.

womething?  No need to resend only to fix if this is typo s/wome/some/.

>
> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
> ---
>  imap-send.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index b2d0b849b..09f29ea95 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
>  	curl_easy_cleanup(curl);
>  	curl_global_cleanup();
>  
> -	return 0;
> +	return res == CURLE_OK;
>  }
>  #endif

Re: [PATCH v3 1/4] imap-send: return with error if curl failed

[Junio C Hamano]

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

> curl_append_msgs_to_imap always returned 0, whether curl failed or not.
> Return a proper status so git imap-send will exit with an error code
> if womething wrong happened.
>
> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
> ---
>  imap-send.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index b2d0b849b..09f29ea95 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
>  	curl_easy_cleanup(curl);
>  	curl_global_cleanup();
>  
> -	return 0;
> +	return res == CURLE_OK;
>  }
>  #endif

Wait a bit.  Did you mean "res != CURLE_OK"?  If we got an OK, we
want to return 0 as success, because the value we return from here
is returned by cmd_main() as-is to main() and to the outside world,
no?

Re: [PATCH v3 1/4] imap-send: return with error if curl failed

[Nicolas Morey-Chaisemartin]

Le 23/08/2017 à 22:12, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com> writes:
>
>> curl_append_msgs_to_imap always returned 0, whether curl failed or not.
>> Return a proper status so git imap-send will exit with an error code
>> if womething wrong happened.
>>
>> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
>> ---
>>  imap-send.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index b2d0b849b..09f29ea95 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
>>  	curl_easy_cleanup(curl);
>>  	curl_global_cleanup();
>>  
>> -	return 0;
>> +	return res == CURLE_OK;
>>  }
>>  #endif
> Wait a bit.  Did you mean "res != CURLE_OK"?  If we got an OK, we
> want to return 0 as success, because the value we return from here
> is returned by cmd_main() as-is to main() and to the outside world,
> no?
>
>


Good catch. I remember testing this out but I messed up somewhere along the line.

Nicolas

[PATCH v3 2/4] imap-send: add wrapper to get server credentials if needed

[Nicolas Morey-Chaisemartin]

Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 imap-send.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 09f29ea95..448a4a0b3 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
 	return 0;
 }
 
+static void server_fill_credential(struct imap_server_conf *srvc, struct credential *cred)
+{
+	if (srvc->user && srvc->pass)
+		return;
+
+	cred->protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
+	cred->host = xstrdup(srvc->host);
+
+	cred->username = xstrdup_or_null(srvc->user);
+	cred->password = xstrdup_or_null(srvc->pass);
+
+	credential_fill(cred);
+
+	if (!srvc->user)
+		srvc->user = xstrdup(cred->username);
+	if (!srvc->pass)
+		srvc->pass = xstrdup(cred->password);
+}
+
 static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *folder)
 {
 	struct credential cred = CREDENTIAL_INIT;
@@ -1078,20 +1097,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *f
 		}
 #endif
 		imap_info("Logging in...\n");
-		if (!srvc->user || !srvc->pass) {
-			cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
-			cred.host = xstrdup(srvc->host);
-
-			cred.username = xstrdup_or_null(srvc->user);
-			cred.password = xstrdup_or_null(srvc->pass);
-
-			credential_fill(&cred);
-
-			if (!srvc->user)
-				srvc->user = xstrdup(cred.username);
-			if (!srvc->pass)
-				srvc->pass = xstrdup(cred.password);
-		}
+		server_fill_credential(srvc, &cred);
 
 		if (srvc->auth_method) {
 			struct imap_cmd_cb cb;
-- 
2.14.0.1.gd9597ce13

Re: [PATCH v3 2/4] imap-send: add wrapper to get server credentials if needed

[Junio C Hamano]

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

> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
> ---
>  imap-send.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index 09f29ea95..448a4a0b3 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
>  	return 0;
>  }
>  
> +static void server_fill_credential(struct imap_server_conf *srvc, struct credential *cred)
> +{
> +	if (srvc->user && srvc->pass)
> +		return;
> +
> +	cred->protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
> +	cred->host = xstrdup(srvc->host);
> +
> +	cred->username = xstrdup_or_null(srvc->user);
> +	cred->password = xstrdup_or_null(srvc->pass);
> +
> +	credential_fill(cred);
> +
> +	if (!srvc->user)
> +		srvc->user = xstrdup(cred->username);
> +	if (!srvc->pass)
> +		srvc->pass = xstrdup(cred->password);
> +}
> +

This looks straight-forward code movement.  The only thing that
makes me wonder is if this is "server".  The existing naming of the
variables screams at me that this is not "server" but is "service".

>  static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *folder)
>  {
>  	struct credential cred = CREDENTIAL_INIT;
> @@ -1078,20 +1097,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *f
>  		}
>  #endif
>  		imap_info("Logging in...\n");
> -		if (!srvc->user || !srvc->pass) {
> -			cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
> -			cred.host = xstrdup(srvc->host);
> -
> -			cred.username = xstrdup_or_null(srvc->user);
> -			cred.password = xstrdup_or_null(srvc->pass);
> -
> -			credential_fill(&cred);
> -
> -			if (!srvc->user)
> -				srvc->user = xstrdup(cred.username);
> -			if (!srvc->pass)
> -				srvc->pass = xstrdup(cred.password);
> -		}
> +		server_fill_credential(srvc, &cred);
>  
>  		if (srvc->auth_method) {
>  			struct imap_cmd_cb cb;

Re: [PATCH v3 2/4] imap-send: add wrapper to get server credentials if needed

[Nicolas Morey-Chaisemartin]

Le 23/08/2017 à 22:13, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com> writes:
>
>> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
>> ---
>>  imap-send.c | 34 ++++++++++++++++++++--------------
>>  1 file changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 09f29ea95..448a4a0b3 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
>>  	return 0;
>>  }
>>  
>> +static void server_fill_credential(struct imap_server_conf *srvc, struct credential *cred)
>> +{
>> +	if (srvc->user && srvc->pass)
>> +		return;
>> +
>> +	cred->protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
>> +	cred->host = xstrdup(srvc->host);
>> +
>> +	cred->username = xstrdup_or_null(srvc->user);
>> +	cred->password = xstrdup_or_null(srvc->pass);
>> +
>> +	credential_fill(cred);
>> +
>> +	if (!srvc->user)
>> +		srvc->user = xstrdup(cred->username);
>> +	if (!srvc->pass)
>> +		srvc->pass = xstrdup(cred->password);
>> +}
>> +
> This looks straight-forward code movement.  The only thing that
> makes me wonder is if this is "server".  The existing naming of the
> variables screams at me that this is not "server" but is "service".

I read srvc as server conf not service.
But I can change the name if you prefer

Nicolas

Re: [PATCH v3 2/4] imap-send: add wrapper to get server credentials if needed

[Junio C Hamano]

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

> Le 23/08/2017 à 22:13, Junio C Hamano a écrit :
>> Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com> writes:
>>
>>> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
>>> ---
>>>  imap-send.c | 34 ++++++++++++++++++++--------------
>>>  1 file changed, 20 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/imap-send.c b/imap-send.c
>>> index 09f29ea95..448a4a0b3 100644
>>> --- a/imap-send.c
>>> +++ b/imap-send.c
>>> @@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha
>>>  	return 0;
>>>  }
>>>  
>>> +static void server_fill_credential(struct imap_server_conf *srvc, struct credential *cred)
>>> +{
>>> +	if (srvc->user && srvc->pass)
>>> +		return;
>>> +
>>> +	cred->protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
>>> +	cred->host = xstrdup(srvc->host);
>>> +
>>> +	cred->username = xstrdup_or_null(srvc->user);
>>> +	cred->password = xstrdup_or_null(srvc->pass);
>>> +
>>> +	credential_fill(cred);
>>> +
>>> +	if (!srvc->user)
>>> +		srvc->user = xstrdup(cred->username);
>>> +	if (!srvc->pass)
>>> +		srvc->pass = xstrdup(cred->password);
>>> +}
>>> +
>> This looks straight-forward code movement.  The only thing that
>> makes me wonder is if this is "server".  The existing naming of the
>> variables screams at me that this is not "server" but is "service".
>
> I read srvc as server conf not service.

Oh, yeah, "server conf" is OK (I didn't think of it).

> But I can change the name if you prefer

Nah.  Please keep it.

[PATCH v3 3/4] imap_send: setup_curl: retreive credentials if not set in config file

[Nicolas Morey-Chaisemartin]

Up to this point, the curl mode only supported getting the username
and password from the gitconfig file while the legacy mode could also
fetch them using the credential API.

Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 imap-send.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 448a4a0b3..a74d011a9 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1398,7 +1398,7 @@ static int append_msgs_to_imap(struct imap_server_conf *server,
 }
 
 #ifdef USE_CURL_FOR_IMAP_SEND
-static CURL *setup_curl(struct imap_server_conf *srvc)
+static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
 {
 	CURL *curl;
 	struct strbuf path = STRBUF_INIT;
@@ -1411,6 +1411,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
 	if (!curl)
 		die("curl_easy_init failed");
 
+	server_fill_credential(&server, cred);
 	curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
 	curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
 
@@ -1460,8 +1461,9 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
 	struct buffer msgbuf = { STRBUF_INIT, 0 };
 	CURL *curl;
 	CURLcode res = CURLE_OK;
+	struct credential cred = CREDENTIAL_INIT;
 
-	curl = setup_curl(server);
+	curl = setup_curl(server, &cred);
 	curl_easy_setopt(curl, CURLOPT_READDATA, &msgbuf);
 
 	fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : "");
@@ -1496,6 +1498,18 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
 	curl_easy_cleanup(curl);
 	curl_global_cleanup();
 
+	if (cred.username)
+		if (res == CURLE_OK)
+			credential_approve(&cred);
+#if LIBCURL_VERSION_NUM >= 0x070d01
+		else if (res == CURLE_LOGIN_DENIED)
+#else
+		else
+#endif
+			credential_reject(&cred);
+
+	credential_clear(&cred);
+
 	return res == CURLE_OK;
 }
 #endif
-- 
2.14.0.1.gd9597ce13

[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 <nicolas@morey-chaisemartin.com>
---
 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

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

[Junio C Hamano]

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

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

> 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

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 <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.
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 <nicolas@morey-chaisemartin.com>
>> ---
>>  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 <nicolas@morey-chaisemartin.com>
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 <nicolas@morey-chaisemartin.com>