threads / discuss / 46538

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

Subject: [PATCHv2 0/4] imap-send: Fix and enable curl by default

## tl;dr

7 messages between Aug 8, 2017 and Aug 8, 2017.

replies: 6people: 2as markdown or json

Nicolas Morey-Chaisemartin· Aug 8, 2017, 07:30 UTC · lore

Changes since v1:

- Add patch fo fix return value of the curl_append_msgs_to_imap
- Patch #2: server_fill_credentials takes a credential struct as a parameter so they can be approved later
- Dropped the s/server/srvc/ cleanup (previous patch #3)
- Patch #4: Only use curl as the default if it's available

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 | 52 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 20 deletions(-)

Nicolas Morey-Chaisemartin· Aug 8, 2017, 07:48 UTC · re: Nicolas Morey-Chaisemartin · lore

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

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.rc1.16.gcc208c97c

Nicolas Morey-Chaisemartin· Aug 8, 2017, 07:48 UTC · re: Nicolas Morey-Chaisemartin · lore

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

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 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 448a4a0b3..5e80edaff 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,10 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
 	curl_easy_cleanup(curl);
 	curl_global_cleanup();
 
+	if (res == CURLE_OK && cred.username)
+		credential_approve(&cred);
+	credential_clear(&cred);
+
 	return res == CURLE_OK;
 }
 #endif

-- 
2.14.0.rc1.16.gcc208c97c

Martin Ågren· Aug 8, 2017, 10:09 UTC · re: Nicolas Morey-Chaisemartin · lore

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

On 8 August 2017 at 09:48, Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com> wrote:

Show 19 quoted lines

> 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 | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index 448a4a0b3..5e80edaff 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)

Hmm, yeah, that really did pollute the interface. :)

Show 28 quoted lines

>  {
>         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,10 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
>         curl_easy_cleanup(curl);
>         curl_global_cleanup();
>
> +       if (res == CURLE_OK && cred.username)
> +               credential_approve(&cred);

Maybe a similar call to credential_reject(&cred) here? I guess all we'll know is that some sort of error happened, possibly credentials-related, possibly not. Just a thought.

Show 9 quoted lines

> +       credential_clear(&cred);
> +
>         return res == CURLE_OK;
>  }
>  #endif
> --
> 2.14.0.rc1.16.gcc208c97c
>
>

Nicolas Morey-Chaisemartin· Aug 8, 2017, 10:34 UTC · re: Martin Ågren · lore

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

Le 08/08/2017 à 12:09, Martin Ågren a écrit :

Show 54 quoted lines

> On 8 August 2017 at 09:48, Nicolas Morey-Chaisemartin
> <nicolas@morey-chaisemartin.com> wrote:
>> 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 | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 448a4a0b3..5e80edaff 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)
> Hmm, yeah, that really did pollute the interface. :)
>
>>  {
>>         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,10 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
>>         curl_easy_cleanup(curl);
>>         curl_global_cleanup();
>>
>> +       if (res == CURLE_OK && cred.username)
>> +               credential_approve(&cred);
> Maybe a similar call to credential_reject(&cred) here? I guess all we'll
> know is that some sort of error happened, possibly credentials-related,
> possibly not. Just a thought.

Checking the doc, there is actually a CURLE_LOGIN_DENIED return code which means authentication failed. I'll fix this in v3

Nicolas

Nicolas Morey-Chaisemartin· Aug 8, 2017, 07:48 UTC · re: Nicolas Morey-Chaisemartin · lore

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

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.rc1.16.gcc208c97c

Nicolas Morey-Chaisemartin· Aug 8, 2017, 07:48 UTC · re: Nicolas Morey-Chaisemartin · lore

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

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 5e80edaff..138d776f7 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.rc1.16.gcc208c97c