{"thread":{"id":"46747","subject":"[PATCH v4 2/4] imap-send: add wrapper to get server credentials if needed","startedAt":"2017-09-14T07:52:10Z","lastAt":"2017-09-15T04:50:34Z","messageCount":7,"participants":["Nicolas Morey-Chaisemartin","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":4},"messages":[{"id":"328030","messageId":"3453a8a7-a390-6c93-1460-44d2ac88034e@morey-chaisemartin.com","threadId":"46747","inReplyTo":"828c6333-0ba0-2a01-324e-f910a8042ca1@morey-chaisemartin.com","subject":"[PATCH v4 2/4] imap-send: add wrapper to get server credentials if needed","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-09-14T07:52:02Z","receivedAt":"2017-09-14T07:52:10Z","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 b5e332420a..1b8fbbd545 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.1.461.g503560879\n\n\n"},{"id":"328031","messageId":"accffa40-3559-5f65-3149-aaa86a2278fc@morey-chaisemartin.com","threadId":"46747","inReplyTo":"828c6333-0ba0-2a01-324e-f910a8042ca1@morey-chaisemartin.com","subject":"[PATCH v4 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-09-14T07:52:06Z","receivedAt":"2017-09-14T07:52:15Z","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 1b8fbbd545..7e39993d95 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.1.461.g503560879\n\n\n"},{"id":"328032","messageId":"ee4cb3a1-3219-411f-cc05-6874da202b32@morey-chaisemartin.com","threadId":"46747","inReplyTo":"828c6333-0ba0-2a01-324e-f910a8042ca1@morey-chaisemartin.com","subject":"[PATCH v4 4/4] imap-send: use curl by default when possible","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-09-14T07:52:11Z","receivedAt":"2017-09-14T07:52:19Z","isPatch":true,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"Set curl as the runtime default when it is available.\nWhen linked against older curl versions (< 7_34_0) or without curl,\nuse the legacy imap implementation.\n\nThe goal is to validate feature parity between the legacy and\nthe curl implementation, deprecate the 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 7e39993d95..af1e1576bd 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.1.461.g503560879\n\n"},{"id":"328033","messageId":"ad4274f6-4a11-f722-9df8-f38bfdde5e76@morey-chaisemartin.com","threadId":"46747","inReplyTo":"828c6333-0ba0-2a01-324e-f910a8042ca1@morey-chaisemartin.com","subject":"[PATCH v4 1/4] imap-send: return with error if curl failed","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-09-14T07:51:57Z","receivedAt":"2017-09-14T08:00:05Z","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 something 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 b2d0b849bb..b5e332420a 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.1.461.g503560879\n\n\n"},{"id":"328034","messageId":"828c6333-0ba0-2a01-324e-f910a8042ca1@morey-chaisemartin.com","threadId":"46747","inReplyTo":null,"subject":"[PATCH v4 0/4] imap-send: Fix and enable curl by default","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-09-14T07:50:52Z","receivedAt":"2017-09-14T08:09:16Z","isPatch":true,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"Changes since v3:\n- Fix return code in patch #1\n- Reword patch#4\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 when possible\n\n imap-send.c | 60 ++++++++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 40 insertions(+), 20 deletions(-)\n\n-- \n2.14.1.461.g503560879\n\n"},{"id":"328098","messageId":"xmqqwp50y3j6.fsf@gitster.mtv.corp.google.com","threadId":"46747","inReplyTo":"accffa40-3559-5f65-3149-aaa86a2278fc@morey-chaisemartin.com","subject":"Re: [PATCH v4 3/4] imap_send: setup_curl: retreive credentials if not set in config file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-15T04:44:13Z","receivedAt":"2017-09-15T04:44:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\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\nA slight tangent. This is in line with the way in which we do\nconditional compilation to work with different versions of libCurl,\nbut we recently had discussion on modernizing these version based\nconditional compilation to use feature based one in another topic.\nWe may want to switch to\n\n\t#if defined(CURLE_LOGIN_DENIED)\n\t\t...\n\n(cf.\nhttps://public-inbox.org/git/cover.1502462884.git.tgc@jupiterrise.com/\nthe entire thread).\n\nNo need to change _this_ patch in this series, but something to keep\nin mind planning for a future follow-up work to clean things up.\n\nThanks.\n"},{"id":"328099","messageId":"xmqqshfoy38u.fsf@gitster.mtv.corp.google.com","threadId":"46747","inReplyTo":"accffa40-3559-5f65-3149-aaa86a2278fc@morey-chaisemartin.com","subject":"Re: [PATCH v4 3/4] imap_send: setup_curl: retreive credentials if not set in config file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-15T04:50:25Z","receivedAt":"2017-09-15T04:50:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Morey-Chaisemartin writes:\n\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\nAs my copy of GCC seemed to be worried about readers getting\nconfused by the if/else cascade, I'd place an extra pair of braces\naround this, i.e.\n\n\tif (cred.username) {\n\t\tif (res == CURLE_OK)\n\t\t\tcredential_approve(&cred);\n\t\telse /* or \"else if DENIED\" */\n\t\t\tcredential_reject(&cred);\n\t}\n\tcredential_clear(&cred);\n\nwhile queuing this patch.\n\n"}]}