{"thread":{"id":"46645","subject":"[PATCH v3 0/4] imap-send: Fix and enable curl by default","startedAt":"2017-08-22T15:55:29Z","lastAt":"2017-08-24T18:54:49Z","messageCount":13,"participants":["Nicolas Morey-Chaisemartin","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"326936","messageId":"087f5907-6558-ce32-2f5c-2e418522c030@morey-chaisemartin.com","threadId":"46645","inReplyTo":null,"subject":"[PATCH v3 0/4] imap-send: Fix and enable curl by default","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-08-22T15:55:20Z","receivedAt":"2017-08-22T15:55:29Z","isPatch":true,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"Changes since v2:\nPatch 3 reject credit when curl failed:\n - when a login error is returned (curl >= 7.13.1)\n - for any curl error in older versions\n\nNicolas Morey-Chaisemartin (4):\n imap-send: return with error if curl failed\n imap-send: add wrapper to get server credentials if needed\n imap_send: setup_curl: retreive credentials if not set in config file\n imap-send: use curl by default\n\n imap-send.c | 60 ++++++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 40 insertions(+), 20 deletions(-)\n\n-- \n2.14.0.1.gd9597ce13\n\n"},{"id":"326937","messageId":"da29995c-3c56-ac43-9f8f-12bf381711f3@morey-chaisemartin.com","threadId":"46645","inReplyTo":"087f5907-6558-ce32-2f5c-2e418522c030@morey-chaisemartin.com","subject":"[PATCH v3 1/4] imap-send: return with error if curl failed","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-08-22T15:56:16Z","receivedAt":"2017-08-22T15:56:23Z","isPatch":true,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"curl_append_msgs_to_imap always returned 0, whether curl failed or not.\nReturn a proper status so git imap-send will exit with an error code\nif womething wrong happened.\n\nSigned-off-by: Nicolas Morey-Chaisemartin \n---\n imap-send.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex b2d0b849b..09f29ea95 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,\n \tcurl_easy_cleanup(curl);\n \tcurl_global_cleanup();\n \n-\treturn 0;\n+\treturn res == CURLE_OK;\n }\n #endif\n \n-- \n2.14.0.1.gd9597ce13\n\n\n"},{"id":"326938","messageId":"9df045f4-71bd-92a1-20bb-4931e25b22c3@morey-chaisemartin.com","threadId":"46645","inReplyTo":"087f5907-6558-ce32-2f5c-2e418522c030@morey-chaisemartin.com","subject":"[PATCH v3 2/4] imap-send: add wrapper to get server credentials if needed","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-08-22T15:56:24Z","receivedAt":"2017-08-22T15:56:31Z","isPatch":true,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"Signed-off-by: Nicolas Morey-Chaisemartin \n---\n imap-send.c | 34 ++++++++++++++++++++--------------\n 1 file changed, 20 insertions(+), 14 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 09f29ea95..448a4a0b3 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha\n \treturn 0;\n }\n \n+static void server_fill_credential(struct imap_server_conf *srvc, struct credential *cred)\n+{\n+\tif (srvc->user && srvc->pass)\n+\t\treturn;\n+\n+\tcred->protocol = xstrdup(srvc->use_ssl ? \"imaps\" : \"imap\");\n+\tcred->host = xstrdup(srvc->host);\n+\n+\tcred->username = xstrdup_or_null(srvc->user);\n+\tcred->password = xstrdup_or_null(srvc->pass);\n+\n+\tcredential_fill(cred);\n+\n+\tif (!srvc->user)\n+\t\tsrvc->user = xstrdup(cred->username);\n+\tif (!srvc->pass)\n+\t\tsrvc->pass = xstrdup(cred->password);\n+}\n+\n static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *folder)\n {\n \tstruct credential cred = CREDENTIAL_INIT;\n@@ -1078,20 +1097,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *f\n \t\t}\n #endif\n \t\timap_info(\"Logging in...\\n\");\n-\t\tif (!srvc->user || !srvc->pass) {\n-\t\t\tcred.protocol = xstrdup(srvc->use_ssl ? \"imaps\" : \"imap\");\n-\t\t\tcred.host = xstrdup(srvc->host);\n-\n-\t\t\tcred.username = xstrdup_or_null(srvc->user);\n-\t\t\tcred.password = xstrdup_or_null(srvc->pass);\n-\n-\t\t\tcredential_fill(&cred);\n-\n-\t\t\tif (!srvc->user)\n-\t\t\t\tsrvc->user = xstrdup(cred.username);\n-\t\t\tif (!srvc->pass)\n-\t\t\t\tsrvc->pass = xstrdup(cred.password);\n-\t\t}\n+\t\tserver_fill_credential(srvc, &cred);\n \n \t\tif (srvc->auth_method) {\n \t\t\tstruct imap_cmd_cb cb;\n-- \n2.14.0.1.gd9597ce13\n\n\n"},{"id":"326939","messageId":"60becdb6-6b08-e3db-a217-62933f924eae@morey-chaisemartin.com","threadId":"46645","inReplyTo":"087f5907-6558-ce32-2f5c-2e418522c030@morey-chaisemartin.com","subject":"[PATCH v3 3/4] imap_send: setup_curl: retreive credentials if not set in config file","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-08-22T15:56:28Z","receivedAt":"2017-08-22T15:56:34Z","isPatch":true,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"Up to this point, the curl mode only supported getting the username\nand password from the gitconfig file while the legacy mode could also\nfetch them using the credential API.\n\nSigned-off-by: Nicolas Morey-Chaisemartin \n---\n imap-send.c | 18 ++++++++++++++++--\n 1 file changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 448a4a0b3..a74d011a9 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1398,7 +1398,7 @@ static int append_msgs_to_imap(struct imap_server_conf *server,\n }\n \n #ifdef USE_CURL_FOR_IMAP_SEND\n-static CURL *setup_curl(struct imap_server_conf *srvc)\n+static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)\n {\n \tCURL *curl;\n \tstruct strbuf path = STRBUF_INIT;\n@@ -1411,6 +1411,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)\n \tif (!curl)\n \t\tdie(\"curl_easy_init failed\");\n \n+\tserver_fill_credential(&server, cred);\n \tcurl_easy_setopt(curl, CURLOPT_USERNAME, server.user);\n \tcurl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);\n \n@@ -1460,8 +1461,9 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,\n \tstruct buffer msgbuf = { STRBUF_INIT, 0 };\n \tCURL *curl;\n \tCURLcode res = CURLE_OK;\n+\tstruct credential cred = CREDENTIAL_INIT;\n \n-\tcurl = setup_curl(server);\n+\tcurl = setup_curl(server, &cred);\n \tcurl_easy_setopt(curl, CURLOPT_READDATA, &msgbuf);\n \n \tfprintf(stderr, \"sending %d message%s\\n\", total, (total != 1) ? \"s\" : \"\");\n@@ -1496,6 +1498,18 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,\n \tcurl_easy_cleanup(curl);\n \tcurl_global_cleanup();\n \n+\tif (cred.username)\n+\t\tif (res == CURLE_OK)\n+\t\t\tcredential_approve(&cred);\n+#if LIBCURL_VERSION_NUM >= 0x070d01\n+\t\telse if (res == CURLE_LOGIN_DENIED)\n+#else\n+\t\telse\n+#endif\n+\t\t\tcredential_reject(&cred);\n+\n+\tcredential_clear(&cred);\n+\n \treturn res == CURLE_OK;\n }\n #endif\n-- \n2.14.0.1.gd9597ce13\n\n\n"},{"id":"326941","messageId":"bb94cace-6bc5-2009-7c9d-a6965e3b84c6@morey-chaisemartin.com","threadId":"46645","inReplyTo":"087f5907-6558-ce32-2f5c-2e418522c030@morey-chaisemartin.com","subject":"[PATCH v3 4/4] imap-send: use curl by default","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-08-22T15:56:32Z","receivedAt":"2017-08-22T16:02:55Z","isPatch":true,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"Now that curl is enable by default, use the curl implementation\nfor imap too.\nThe goal is to validate feature parity between the legacy and\nthe curl implementation, deprecate thee legacy implementation\nlater on and in the long term, hopefully drop it altogether.\n\nSigned-off-by: Nicolas Morey-Chaisemartin \n---\n imap-send.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex a74d011a9..58c191704 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -35,11 +35,11 @@ typedef void *SSL;\n #include \"http.h\"\n #endif\n \n-#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)\n-/* only available option */\n+#if defined(USE_CURL_FOR_IMAP_SEND)\n+/* Always default to curl if it's available. */\n #define USE_CURL_DEFAULT 1\n #else\n-/* strictly opt in */\n+/* We don't have curl, so continue to use the historical implementation */\n #define USE_CURL_DEFAULT 0\n #endif\n \n-- \n2.14.0.1.gd9597ce13\n\n"},{"id":"327077","messageId":"xmqq378iyrj9.fsf@gitster.mtv.corp.google.com","threadId":"46645","inReplyTo":"da29995c-3c56-ac43-9f8f-12bf381711f3@morey-chaisemartin.com","subject":"Re: [PATCH v3 1/4] imap-send: return with error if curl failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-23T20:10:02Z","receivedAt":"2017-08-23T20:11:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n> curl_append_msgs_to_imap always returned 0, whether curl failed or not.\n> Return a proper status so git imap-send will exit with an error code\n> if womething wrong happened.\n\nwomething? No need to resend only to fix if this is typo s/wome/some/.\n\n>\n> Signed-off-by: Nicolas Morey-Chaisemartin \n> ---\n> imap-send.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/imap-send.c b/imap-send.c\n> index b2d0b849b..09f29ea95 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,\n> \tcurl_easy_cleanup(curl);\n> \tcurl_global_cleanup();\n> \n> -\treturn 0;\n> +\treturn res == CURLE_OK;\n> }\n> #endif\n"},{"id":"327078","messageId":"xmqqy3qaxcur.fsf@gitster.mtv.corp.google.com","threadId":"46645","inReplyTo":"da29995c-3c56-ac43-9f8f-12bf381711f3@morey-chaisemartin.com","subject":"Re: [PATCH v3 1/4] imap-send: return with error if curl failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-23T20:12:28Z","receivedAt":"2017-08-23T20:12:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n> curl_append_msgs_to_imap always returned 0, whether curl failed or not.\n> Return a proper status so git imap-send will exit with an error code\n> if womething wrong happened.\n>\n> Signed-off-by: Nicolas Morey-Chaisemartin \n> ---\n> imap-send.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/imap-send.c b/imap-send.c\n> index b2d0b849b..09f29ea95 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,\n> \tcurl_easy_cleanup(curl);\n> \tcurl_global_cleanup();\n> \n> -\treturn 0;\n> +\treturn res == CURLE_OK;\n> }\n> #endif\n\nWait a bit. Did you mean \"res != CURLE_OK\"? If we got an OK, we\nwant to return 0 as success, because the value we return from here\nis returned by cmd_main() as-is to main() and to the outside world,\nno?\n\n\n"},{"id":"327080","messageId":"xmqqtw0yxcsk.fsf@gitster.mtv.corp.google.com","threadId":"46645","inReplyTo":"9df045f4-71bd-92a1-20bb-4931e25b22c3@morey-chaisemartin.com","subject":"Re: [PATCH v3 2/4] imap-send: add wrapper to get server credentials if needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-23T20:13:47Z","receivedAt":"2017-08-23T20:13:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n> Signed-off-by: Nicolas Morey-Chaisemartin \n> ---\n> imap-send.c | 34 ++++++++++++++++++++--------------\n> 1 file changed, 20 insertions(+), 14 deletions(-)\n>\n> diff --git a/imap-send.c b/imap-send.c\n> index 09f29ea95..448a4a0b3 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha\n> \treturn 0;\n> }\n> \n> +static void server_fill_credential(struct imap_server_conf *srvc, struct credential *cred)\n> +{\n> +\tif (srvc->user && srvc->pass)\n> +\t\treturn;\n> +\n> +\tcred->protocol = xstrdup(srvc->use_ssl ? \"imaps\" : \"imap\");\n> +\tcred->host = xstrdup(srvc->host);\n> +\n> +\tcred->username = xstrdup_or_null(srvc->user);\n> +\tcred->password = xstrdup_or_null(srvc->pass);\n> +\n> +\tcredential_fill(cred);\n> +\n> +\tif (!srvc->user)\n> +\t\tsrvc->user = xstrdup(cred->username);\n> +\tif (!srvc->pass)\n> +\t\tsrvc->pass = xstrdup(cred->password);\n> +}\n> +\n\nThis looks straight-forward code movement. The only thing that\nmakes me wonder is if this is \"server\". The existing naming of the\nvariables screams at me that this is not \"server\" but is \"service\".\n\n> static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *folder)\n> {\n> \tstruct credential cred = CREDENTIAL_INIT;\n> @@ -1078,20 +1097,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char *f\n> \t\t}\n> #endif\n> \t\timap_info(\"Logging in...\\n\");\n> -\t\tif (!srvc->user || !srvc->pass) {\n> -\t\t\tcred.protocol = xstrdup(srvc->use_ssl ? \"imaps\" : \"imap\");\n> -\t\t\tcred.host = xstrdup(srvc->host);\n> -\n> -\t\t\tcred.username = xstrdup_or_null(srvc->user);\n> -\t\t\tcred.password = xstrdup_or_null(srvc->pass);\n> -\n> -\t\t\tcredential_fill(&cred);\n> -\n> -\t\t\tif (!srvc->user)\n> -\t\t\t\tsrvc->user = xstrdup(cred.username);\n> -\t\t\tif (!srvc->pass)\n> -\t\t\t\tsrvc->pass = xstrdup(cred.password);\n> -\t\t}\n> +\t\tserver_fill_credential(srvc, &cred);\n> \n> \t\tif (srvc->auth_method) {\n> \t\t\tstruct imap_cmd_cb cb;\n"},{"id":"327081","messageId":"xmqqpobmxc3a.fsf@gitster.mtv.corp.google.com","threadId":"46645","inReplyTo":"bb94cace-6bc5-2009-7c9d-a6965e3b84c6@morey-chaisemartin.com","subject":"Re: [PATCH v3 4/4] imap-send: use curl by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-23T20:28:57Z","receivedAt":"2017-08-23T20:29:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n> Now that curl is enable by default,\n\ns/enable/&d/; \n\nBut it is unclear what the above really means. You certainly do not\nmean that [PATCH 1-3/4] somewhere tweaked our Makefile to always use\nlibcurl and makes Git fail to build without it, but the above sounds\nas if that were the case.\n\n> use the curl implementation\n> for imap too.\n\nThe Makefile for a long time by default set USE_CURL_FOR_IMAP_SEND\nto YesPlease when the version of cURL we have is recent enough, I\nthink. So I am not sure what you want to add with this change.\n\n> The goal is to validate feature parity between the legacy and\n> the curl implementation, deprecate thee legacy implementation\n\ns/thee/the/;\n\n> later on and in the long term, hopefully drop it altogether.\n>\n> Signed-off-by: Nicolas Morey-Chaisemartin \n> ---\n> imap-send.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n\nHmph, the patch itself is also confusing.\n\n> diff --git a/imap-send.c b/imap-send.c\n> index a74d011a9..58c191704 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -35,11 +35,11 @@ typedef void *SSL;\n> #include \"http.h\"\n> #endif\n> \n> -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)\n> -/* only available option */\n> +#if defined(USE_CURL_FOR_IMAP_SEND)\n> +/* Always default to curl if it's available. */\n> #define USE_CURL_DEFAULT 1\n\nThe original says \"we want to use CURL, if Makefile tells us to\n*AND* if Makefile tells us not to use OpenSSL\", which does not make\nmuch sense to me. I wonder if the original is buggy and should have\nbeen using \"|| defined(NO_OPENSSL)\" there instead. \n\nPerhaps that is the bug you are fixing with this patch, and all the\ntalk about curl is default we saw above is a red herring? If that\nis the case, then instead of removing, turning \"&&\" into \"||\" may be\na better fix. I dunno.\n\n> #else\n> -/* strictly opt in */\n> +/* We don't have curl, so continue to use the historical implementation */\n> #define USE_CURL_DEFAULT 0\n> #endif\n"},{"id":"327107","messageId":"d1c39912-a847-1831-60ae-18b15359092f@morey-chaisemartin.com","threadId":"46645","inReplyTo":"xmqqtw0yxcsk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/4] imap-send: add wrapper to get server credentials if needed","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-08-24T06:08:31Z","receivedAt":"2017-08-24T06:08:44Z","isPatch":true,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"\n\nLe 23/08/2017 à 22:13, Junio C Hamano a écrit :\n> Nicolas Morey-Chaisemartin writes:\n>\n>> Signed-off-by: Nicolas Morey-Chaisemartin \n>> ---\n>> imap-send.c | 34 ++++++++++++++++++++--------------\n>> 1 file changed, 20 insertions(+), 14 deletions(-)\n>>\n>> diff --git a/imap-send.c b/imap-send.c\n>> index 09f29ea95..448a4a0b3 100644\n>> --- a/imap-send.c\n>> +++ b/imap-send.c\n>> @@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha\n>> \treturn 0;\n>> }\n>> \n>> +static void server_fill_credential(struct imap_server_conf *srvc, struct credential *cred)\n>> +{\n>> +\tif (srvc->user && srvc->pass)\n>> +\t\treturn;\n>> +\n>> +\tcred->protocol = xstrdup(srvc->use_ssl ? \"imaps\" : \"imap\");\n>> +\tcred->host = xstrdup(srvc->host);\n>> +\n>> +\tcred->username = xstrdup_or_null(srvc->user);\n>> +\tcred->password = xstrdup_or_null(srvc->pass);\n>> +\n>> +\tcredential_fill(cred);\n>> +\n>> +\tif (!srvc->user)\n>> +\t\tsrvc->user = xstrdup(cred->username);\n>> +\tif (!srvc->pass)\n>> +\t\tsrvc->pass = xstrdup(cred->password);\n>> +}\n>> +\n> This looks straight-forward code movement. The only thing that\n> makes me wonder is if this is \"server\". The existing naming of the\n> variables screams at me that this is not \"server\" but is \"service\".\n\nI read srvc as server conf not service.\nBut I can change the name if you prefer\n\nNicolas\n\n"},{"id":"327109","messageId":"59827003-6f84-712c-bbc8-33e3ac90f590@morey-chaisemartin.com","threadId":"46645","inReplyTo":"xmqqy3qaxcur.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/4] imap-send: return with error if curl failed","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-08-24T06:57:11Z","receivedAt":"2017-08-24T07:17:18Z","isPatch":true,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"\nLe 23/08/2017 à 22:12, Junio C Hamano a écrit :\n> Nicolas Morey-Chaisemartin writes:\n>\n>> curl_append_msgs_to_imap always returned 0, whether curl failed or not.\n>> Return a proper status so git imap-send will exit with an error code\n>> if womething wrong happened.\n>>\n>> Signed-off-by: Nicolas Morey-Chaisemartin \n>> ---\n>> imap-send.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/imap-send.c b/imap-send.c\n>> index b2d0b849b..09f29ea95 100644\n>> --- a/imap-send.c\n>> +++ b/imap-send.c\n>> @@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,\n>> \tcurl_easy_cleanup(curl);\n>> \tcurl_global_cleanup();\n>> \n>> -\treturn 0;\n>> +\treturn res == CURLE_OK;\n>> }\n>> #endif\n> Wait a bit. Did you mean \"res != CURLE_OK\"? If we got an OK, we\n> want to return 0 as success, because the value we return from here\n> is returned by cmd_main() as-is to main() and to the outside world,\n> no?\n>\n>\n\n\nGood catch. I remember testing this out but I messed up somewhere along the line.\n\nNicolas\n"},{"id":"327110","messageId":"6c1481b3-5025-a911-bbaf-aaa88343d859@morey-chaisemartin.com","threadId":"46645","inReplyTo":"xmqqpobmxc3a.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 4/4] imap-send: use curl by default","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-08-24T07:24:15Z","receivedAt":"2017-08-24T07:24:23Z","isPatch":true,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"\n\nLe 23/08/2017 à 22:28, Junio C Hamano a écrit :\n> Nicolas Morey-Chaisemartin writes:\n>\n>> Now that curl is enable by default,\n> s/enable/&d/; \n>\n> But it is unclear what the above really means. You certainly do not\n> mean that [PATCH 1-3/4] somewhere tweaked our Makefile to always use\n> libcurl and makes Git fail to build without it, but the above sounds\n> as if that were the case.\n>\n>> use the curl implementation\n>> for imap too.\n> The Makefile for a long time by default set USE_CURL_FOR_IMAP_SEND\n> to YesPlease when the version of cURL we have is recent enough, I\n> think. So I am not sure what you want to add with this change.\nIt 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.\n>> The goal is to validate feature parity between the legacy and\n>> the curl implementation, deprecate thee legacy implementation\n> s/thee/the/;\n\nYes this is confusing.\nIn 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).\nThis patch changes this behavior.\n\n>\n>> later on and in the long term, hopefully drop it altogether.\n>>\n>> Signed-off-by: Nicolas Morey-Chaisemartin \n>> ---\n>> imap-send.c | 6 +++---\n>> 1 file changed, 3 insertions(+), 3 deletions(-)\n> Hmph, the patch itself is also confusing.\n>\n>> diff --git a/imap-send.c b/imap-send.c\n>> index a74d011a9..58c191704 100644\n>> --- a/imap-send.c\n>> +++ b/imap-send.c\n>> @@ -35,11 +35,11 @@ typedef void *SSL;\n>> #include \"http.h\"\n>> #endif\n>> \n>> -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)\n>> -/* only available option */\n>> +#if defined(USE_CURL_FOR_IMAP_SEND)\n>> +/* Always default to curl if it's available. */\n>> #define USE_CURL_DEFAULT 1\n> The original says \"we want to use CURL, if Makefile tells us to\n> *AND* if Makefile tells us not to use OpenSSL\", which does not make\n> much sense to me. I wonder if the original is buggy and should have\n> been using \"|| defined(NO_OPENSSL)\" there instead. \nI 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)\n\n>\n> Perhaps that is the bug you are fixing with this patch, and all the\n> talk about curl is default we saw above is a red herring? If that\n> is the case, then instead of removing, turning \"&&\" into \"||\" may be\n> a better fix. I dunno.\n\nAs said before, the goal of this patch is to enable curl by default at runtime when it has been \"enabled\" at compile time.\nI'll reword\n\nIs something like this clearer ?\nAuthor: Nicolas Morey-Chaisemartin \nDate:   Sun Aug 6 21:30:15 2017 +0200\n\n    imap-send: use curl by default when possible\n   \n    Set curl as the runtime default when it is available.\n    When linked against older curl versions (< 7_34_0) or without curl,\n    use the legacy imap implementation.\n   \n    The goal is to validate feature parity between the legacy and\n    the curl implementation, deprecate the legacy implementation\n    later on and in the long term, hopefully drop it altogether.\n   \n    Signed-off-by: Nicolas Morey-Chaisemartin \n\n\n"},{"id":"327142","messageId":"xmqqh8wwx0ct.fsf@gitster.mtv.corp.google.com","threadId":"46645","inReplyTo":"d1c39912-a847-1831-60ae-18b15359092f@morey-chaisemartin.com","subject":"Re: [PATCH v3 2/4] imap-send: add wrapper to get server credentials if needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-24T18:54:42Z","receivedAt":"2017-08-24T18:54:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\n> Le 23/08/2017 à 22:13, Junio C Hamano a écrit :\n>> Nicolas Morey-Chaisemartin writes:\n>>\n>>> Signed-off-by: Nicolas Morey-Chaisemartin \n>>> ---\n>>> imap-send.c | 34 ++++++++++++++++++++--------------\n>>> 1 file changed, 20 insertions(+), 14 deletions(-)\n>>>\n>>> diff --git a/imap-send.c b/imap-send.c\n>>> index 09f29ea95..448a4a0b3 100644\n>>> --- a/imap-send.c\n>>> +++ b/imap-send.c\n>>> @@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct imap_cmd *cmd, const cha\n>>> \treturn 0;\n>>> }\n>>> \n>>> +static void server_fill_credential(struct imap_server_conf *srvc, struct credential *cred)\n>>> +{\n>>> +\tif (srvc->user && srvc->pass)\n>>> +\t\treturn;\n>>> +\n>>> +\tcred->protocol = xstrdup(srvc->use_ssl ? \"imaps\" : \"imap\");\n>>> +\tcred->host = xstrdup(srvc->host);\n>>> +\n>>> +\tcred->username = xstrdup_or_null(srvc->user);\n>>> +\tcred->password = xstrdup_or_null(srvc->pass);\n>>> +\n>>> +\tcredential_fill(cred);\n>>> +\n>>> +\tif (!srvc->user)\n>>> +\t\tsrvc->user = xstrdup(cred->username);\n>>> +\tif (!srvc->pass)\n>>> +\t\tsrvc->pass = xstrdup(cred->password);\n>>> +}\n>>> +\n>> This looks straight-forward code movement. The only thing that\n>> makes me wonder is if this is \"server\". The existing naming of the\n>> variables screams at me that this is not \"server\" but is \"service\".\n>\n> I read srvc as server conf not service.\n\nOh, yeah, \"server conf\" is OK (I didn't think of it).\n\n> But I can change the name if you prefer\n\nNah. Please keep it.\n\n"}]}