{"thread":{"id":"46528","subject":"[PATCH 2/4] imap_send: setup_curl: prompt user for username/password if not set in config file","startedAt":"2017-08-07T14:04:12Z","lastAt":"2017-09-12T08:35:04Z","messageCount":13,"participants":["Nicolas Morey-Chaisemartin","Martin Ågren","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"325680","messageId":"38d3ae5b-4020-63cc-edfa-0a77e42798b8@morey-chaisemartin.com","threadId":"46528","inReplyTo":"c74c8c386f2c2d8b6cebd4addf925d0121986067.1502114584.git.nicolas@morey-chaisemartin.com","subject":"[PATCH 2/4] imap_send: setup_curl: prompt user for username/password if not set in config file","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-08-07T14:03:55Z","receivedAt":"2017-08-07T14:04:12Z","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 | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 38b3c817e..682a06551 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1415,6 +1415,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);\n \tcurl_easy_setopt(curl, CURLOPT_USERNAME, server.user);\n \tcurl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);\n \n-- \n2.14.0.rc1.16.g87fcec1e8\n\n\n"},{"id":"325681","messageId":"4a1f931c-9813-36a5-9c37-63b8abc4fecd@morey-chaisemartin.com","threadId":"46528","inReplyTo":"c74c8c386f2c2d8b6cebd4addf925d0121986067.1502114584.git.nicolas@morey-chaisemartin.com","subject":"[PATCH 3/4] imap_send: setup_curl: use server_conf parameter instead of the global variable","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-08-07T14:04:01Z","receivedAt":"2017-08-07T14:04:17Z","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 | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 682a06551..90b8683ed 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1415,37 +1415,37 @@ 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);\n-\tcurl_easy_setopt(curl, CURLOPT_USERNAME, server.user);\n-\tcurl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);\n+\tserver_fill_credential(srvc);\n+\tcurl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);\n+\tcurl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);\n \n-\tstrbuf_addstr(&path, server.use_ssl ? \"imaps://\" : \"imap://\");\n-\tstrbuf_addstr(&path, server.host);\n+\tstrbuf_addstr(&path, srvc->use_ssl ? \"imaps://\" : \"imap://\");\n+\tstrbuf_addstr(&path, srvc->host);\n \tif (!path.len || path.buf[path.len - 1] != '/')\n \t\tstrbuf_addch(&path, '/');\n-\tstrbuf_addstr(&path, server.folder);\n+\tstrbuf_addstr(&path, srvc->folder);\n \n \tcurl_easy_setopt(curl, CURLOPT_URL, path.buf);\n \tstrbuf_release(&path);\n-\tcurl_easy_setopt(curl, CURLOPT_PORT, server.port);\n+\tcurl_easy_setopt(curl, CURLOPT_PORT, srvc->port);\n \n-\tif (server.auth_method) {\n+\tif (srvc->auth_method) {\n #if LIBCURL_VERSION_NUM < 0x072200\n \t\twarning(\"No LOGIN_OPTIONS support in this cURL version\");\n #else\n \t\tstruct strbuf auth = STRBUF_INIT;\n \t\tstrbuf_addstr(&auth, \"AUTH=\");\n-\t\tstrbuf_addstr(&auth, server.auth_method);\n+\t\tstrbuf_addstr(&auth, srvc->auth_method);\n \t\tcurl_easy_setopt(curl, CURLOPT_LOGIN_OPTIONS, auth.buf);\n \t\tstrbuf_release(&auth);\n #endif\n \t}\n \n-\tif (!server.use_ssl)\n+\tif (!srvc->use_ssl)\n \t\tcurl_easy_setopt(curl, CURLOPT_USE_SSL, (long)CURLUSESSL_TRY);\n \n-\tcurl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, server.ssl_verify);\n-\tcurl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, server.ssl_verify);\n+\tcurl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, srvc->ssl_verify);\n+\tcurl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, srvc->ssl_verify);\n \n \tcurl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);\n \n-- \n2.14.0.rc1.16.g87fcec1e8\n\n\n"},{"id":"325682","messageId":"71479602-fabc-1861-124b-90b26b952344@morey-chaisemartin.com","threadId":"46528","inReplyTo":"c74c8c386f2c2d8b6cebd4addf925d0121986067.1502114584.git.nicolas@morey-chaisemartin.com","subject":"[PATCH 4/4] imap-send: use curl by default","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-08-07T14:04:05Z","receivedAt":"2017-08-07T14:23:38Z","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 | 6 ------\n 1 file changed, 6 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 90b8683ed..4ebc16437 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -35,13 +35,7 @@ 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 #define USE_CURL_DEFAULT 1\n-#else\n-/* strictly opt in */\n-#define USE_CURL_DEFAULT 0\n-#endif\n \n static int verbosity;\n static int use_curl = USE_CURL_DEFAULT;\n-- \n2.14.0.rc1.16.g87fcec1e8\n\n"},{"id":"325693","messageId":"CAN0heSqyiisjWyDBXLJcyx=Hh3NA562DoFqP8rpFyi_zuGKLEA@mail.gmail.com","threadId":"46528","inReplyTo":"4a1f931c-9813-36a5-9c37-63b8abc4fecd@morey-chaisemartin.com","subject":"Re: [PATCH 3/4] imap_send: setup_curl: use server_conf parameter instead of the global variable","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-07T16:34:42Z","receivedAt":"2017-08-07T16:34:55Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin\n wrote:\n> Signed-off-by: Nicolas Morey-Chaisemartin \n> ---\n> imap-send.c | 24 ++++++++++++------------\n> 1 file changed, 12 insertions(+), 12 deletions(-)\n>\n> diff --git a/imap-send.c b/imap-send.c\n> index 682a06551..90b8683ed 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -1415,37 +1415,37 @@ static CURL *setup_curl(struct imap_server_conf *srvc)\n> if (!curl)\n> die(\"curl_easy_init failed\");\n>\n> - server_fill_credential(&server);\n> - curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);\n> - curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);\n> + server_fill_credential(srvc);\n> + curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);\n> + curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);\n\nHere you change the server_fill_credential-call that you just added.\nMaybe do this patch earlier, perhaps even as patch 1?\n\nI'm snipping lots of s/server/srvc/-changes... There's a less noisy\nway of addressing the fact that srvc is unused: dropping it. I'm not\nsaying that's a good idea, but it could be considered, then explained\nwhy this approach is better. There are some other functions which\naccess \"server\" directly, and some which take (and use!) a \"srvc\".\nMaybe make the whole file consistent?\n\nMartin\n"},{"id":"325695","messageId":"CAN0heSoRXX_9-yzFf=2Obj_5WosLwK4UtrFrLsw8asdUea1+Yg@mail.gmail.com","threadId":"46528","inReplyTo":"71479602-fabc-1861-124b-90b26b952344@morey-chaisemartin.com","subject":"Re: [PATCH 4/4] imap-send: use curl by default","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-07T16:37:21Z","receivedAt":"2017-08-07T16:37:30Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin\n wrote:\n> Signed-off-by: Nicolas Morey-Chaisemartin \n> ---\n> imap-send.c | 6 ------\n> 1 file changed, 6 deletions(-)\n>\n> diff --git a/imap-send.c b/imap-send.c\n> index 90b8683ed..4ebc16437 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -35,13 +35,7 @@ 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> #define USE_CURL_DEFAULT 1\n> -#else\n> -/* strictly opt in */\n> -#define USE_CURL_DEFAULT 0\n> -#endif\n>\n> static int verbosity;\n> static int use_curl = USE_CURL_DEFAULT;\n\nSo this is now basically \"static int use_curl = 1;\".\n\nDo we need a compile-time escape-hatch in case someone really needs\nto avoid curl, e.g., because they have a too old version? I suppose\nthere is a conceptual difference between the \"default\", i.e., the value\nof USE_CURL_DEFAULT that is assigned to \"use_curl\", and the \"default\ndefault\", i.e., the value that is normally assigned to USE_CURL_DEFAULT.\n\nMartin\n"},{"id":"325696","messageId":"71817815-4e52-4623-b226-bbb57ca08047@morey-chaisemartin.com","threadId":"46528","inReplyTo":"CAN0heSqyiisjWyDBXLJcyx=Hh3NA562DoFqP8rpFyi_zuGKLEA@mail.gmail.com","subject":"Re: [PATCH 3/4] imap_send: setup_curl: use server_conf parameter instead of the global variable","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-08-07T17:06:07Z","receivedAt":"2017-08-07T17:06:17Z","isPatch":true,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"\n\nLe 07/08/2017 à 18:34, Martin Ågren a écrit :\n> On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin\n> wrote:\n>> Signed-off-by: Nicolas Morey-Chaisemartin \n>> ---\n>> imap-send.c | 24 ++++++++++++------------\n>> 1 file changed, 12 insertions(+), 12 deletions(-)\n>>\n>> diff --git a/imap-send.c b/imap-send.c\n>> index 682a06551..90b8683ed 100644\n>> --- a/imap-send.c\n>> +++ b/imap-send.c\n>> @@ -1415,37 +1415,37 @@ static CURL *setup_curl(struct imap_server_conf *srvc)\n>> if (!curl)\n>> die(\"curl_easy_init failed\");\n>>\n>> - server_fill_credential(&server);\n>> - curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);\n>> - curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);\n>> + server_fill_credential(srvc);\n>> + curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);\n>> + curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);\n> Here you change the server_fill_credential-call that you just added.\n> Maybe do this patch earlier, perhaps even as patch 1?\n>\n> I'm snipping lots of s/server/srvc/-changes... There's a less noisy\n> way of addressing the fact that srvc is unused: dropping it. I'm not\n> saying that's a good idea, but it could be considered, then explained\n> why this approach is better. There are some other functions which\n> access \"server\" directly, and some which take (and use!) a \"srvc\".\n> Maybe make the whole file consistent?\n>\n> Martin\nThat's why I applied it after #2. I was not sure if this one made sense or not. And it  can be dropped with the rest of the series still applying.\nI don't know what is the right approach here. Someone with more knowledge of why there is a mix of global variable and local can maybe help ?\n\nNicolas\n"},{"id":"325698","messageId":"890d4028-1351-a7b9-1797-cf419d5f01e8@morey-chaisemartin.com","threadId":"46528","inReplyTo":"CAN0heSoRXX_9-yzFf=2Obj_5WosLwK4UtrFrLsw8asdUea1+Yg@mail.gmail.com","subject":"Re: [PATCH 4/4] imap-send: use curl by default","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-08-07T17:10:46Z","receivedAt":"2017-08-07T17:10:56Z","isPatch":true,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"\n\nLe 07/08/2017 à 18:37, Martin Ågren a écrit :\n> On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin\n> wrote:\n>> Signed-off-by: Nicolas Morey-Chaisemartin \n>> ---\n>> imap-send.c | 6 ------\n>> 1 file changed, 6 deletions(-)\n>>\n>> diff --git a/imap-send.c b/imap-send.c\n>> index 90b8683ed..4ebc16437 100644\n>> --- a/imap-send.c\n>> +++ b/imap-send.c\n>> @@ -35,13 +35,7 @@ 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>> #define USE_CURL_DEFAULT 1\n>> -#else\n>> -/* strictly opt in */\n>> -#define USE_CURL_DEFAULT 0\n>> -#endif\n>>\n>> static int verbosity;\n>> static int use_curl = USE_CURL_DEFAULT;\n> So this is now basically \"static int use_curl = 1;\".\n>\n> Do we need a compile-time escape-hatch in case someone really needs\n> to avoid curl, e.g., because they have a too old version? I suppose\n> there is a conceptual difference between the \"default\", i.e., the value\n> of USE_CURL_DEFAULT that is assigned to \"use_curl\", and the \"default\n> default\", i.e., the value that is normally assigned to USE_CURL_DEFAULT.\n>\n> Martin\n\nThe curl code depends on USE_CURL_FOR_IMAP_SEND so even with use_curl == 1, it won't be an issue for people without curl (or old one).\nI wasn't sure whether to drop the define or not and figure it might be worth keeping in case in change in the future for some reason.\nI don't mind dropping it and hardcofing the default to 1\n\nAlso on a side note, I have a case where authentication works with --no-curl but fails withs --curl. Still trying to figure out why.\n\nNicolas\n"},{"id":"325705","messageId":"CAN0heSpSXRNgGeJ7TKOwPZJ3Puc1EvVUv1u84ghwDHhtE7=VHQ@mail.gmail.com","threadId":"46528","inReplyTo":"890d4028-1351-a7b9-1797-cf419d5f01e8@morey-chaisemartin.com","subject":"Re: [PATCH 4/4] imap-send: use curl by default","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-07T17:37:37Z","receivedAt":"2017-08-07T17:37:47Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 7 August 2017 at 19:10, Nicolas Morey-Chaisemartin\n wrote:\n>\n>\n> Le 07/08/2017 à 18:37, Martin Ågren a écrit :\n>> On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin\n>> wrote:\n>>> Signed-off-by: Nicolas Morey-Chaisemartin \n>>> ---\n>>> imap-send.c | 6 ------\n>>> 1 file changed, 6 deletions(-)\n>>>\n>>> diff --git a/imap-send.c b/imap-send.c\n>>> index 90b8683ed..4ebc16437 100644\n>>> --- a/imap-send.c\n>>> +++ b/imap-send.c\n>>> @@ -35,13 +35,7 @@ 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>>> #define USE_CURL_DEFAULT 1\n>>> -#else\n>>> -/* strictly opt in */\n>>> -#define USE_CURL_DEFAULT 0\n>>> -#endif\n>>>\n>>> static int verbosity;\n>>> static int use_curl = USE_CURL_DEFAULT;\n>> So this is now basically \"static int use_curl = 1;\".\n>>\n>> Do we need a compile-time escape-hatch in case someone really needs\n>> to avoid curl, e.g., because they have a too old version? I suppose\n>> there is a conceptual difference between the \"default\", i.e., the value\n>> of USE_CURL_DEFAULT that is assigned to \"use_curl\", and the \"default\n>> default\", i.e., the value that is normally assigned to USE_CURL_DEFAULT.\n>>\n>> Martin\n>\n> The curl code depends on USE_CURL_FOR_IMAP_SEND so even with use_curl == 1, it won't be an issue for people without curl (or old one).\n\nI have just looked at the sources and haven't thought too hard about it,\nbut doesn't it mean that compiling without USE_CURL_FOR_IMAP_SEND\nresults in a binary such that you must use --no-curl or get used to seeing\n\"warning: --curl not supported in this build\"?\n\n> I wasn't sure whether to drop the define or not and figure it might be worth keeping in case in change in the future for some reason.\n> I don't mind dropping it and hardcofing the default to 1\n\nI did not intend to suggest that. Just to be clear, I am very unfamiliar\nwith most of the Git codebase. Please don't take anything I say as\nadvice. :) As a question about something you have or haven't already\nthought about, sure. :)\n\nMartin\n"},{"id":"325735","messageId":"20170807195003.clo2vmnmufmfalb6@sigill.intra.peff.net","threadId":"46528","inReplyTo":"71817815-4e52-4623-b226-bbb57ca08047@morey-chaisemartin.com","subject":"Re: [PATCH 3/4] imap_send: setup_curl: use server_conf parameter instead of the global variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-07T19:50:03Z","receivedAt":"2017-08-07T19:50:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 07, 2017 at 07:06:07PM +0200, Nicolas Morey-Chaisemartin wrote:\n\n> >> - server_fill_credential(&server);\n> >> - curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);\n> >> - curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);\n> >> + server_fill_credential(srvc);\n> >> + curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);\n> >> + curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);\n> > Here you change the server_fill_credential-call that you just added.\n> > Maybe do this patch earlier, perhaps even as patch 1?\n> >\n> > I'm snipping lots of s/server/srvc/-changes... There's a less noisy\n> > way of addressing the fact that srvc is unused: dropping it. I'm not\n> > saying that's a good idea, but it could be considered, then explained\n> > why this approach is better. There are some other functions which\n> > access \"server\" directly, and some which take (and use!) a \"srvc\".\n> > Maybe make the whole file consistent?\n> >\n> That's why I applied it after #2. I was not sure if this one made\n> sense or not. And it  can be dropped with the rest of the series still\n> applying.\n> I don't know what is the right approach here. Someone with more\n> knowledge of why there is a mix of global variable and local can maybe\n> help ?\n\nI suspect it's just code in need of a cleanup. But let's cc the original\nauthor of 1e16b255b (git-imap-send: use libcurl for implementation,\n2014-11-09) to see if he has any comments[1].\n\n-Peff\n\n[1] Bernhard, the whole series is at:\n\n https://public-inbox.org/git/38d3ae5b-4020-63cc-edfa-0a77e42798b8@morey-chaisemartin.com/\n\n The general idea is to make sure the original and curl imap-send\n implementations have feature parity, make the curl version the\n default, and then hopefully eventually drop the non-curl one\n entirely.\n"},{"id":"325738","messageId":"20170807200126.fnbz57qd44vj43c6@sigill.intra.peff.net","threadId":"46528","inReplyTo":"71479602-fabc-1861-124b-90b26b952344@morey-chaisemartin.com","subject":"Re: [PATCH 4/4] imap-send: use curl by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-07T20:01:26Z","receivedAt":"2017-08-07T20:01:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 07, 2017 at 04:04:05PM +0200, Nicolas Morey-Chaisemartin wrote:\n\n> Signed-off-by: Nicolas Morey-Chaisemartin \n\nThanks for moving forward with this.\n\nCan you please flesh out your commit messages with some of the reasoning\nand related discussion? I know from a nearby thread why we want to flip\nthe default, but people reading `git log` much later will not have that\ncontext.\n\n> diff --git a/imap-send.c b/imap-send.c\n> index 90b8683ed..4ebc16437 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -35,13 +35,7 @@ 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> #define USE_CURL_DEFAULT 1\n> -#else\n> -/* strictly opt in */\n> -#define USE_CURL_DEFAULT 0\n> -#endif\n\nI agree with the comments Martin made here. I think there are really two\nlevels of \"default\" we need to care about:\n\n 1. Build-time: do we default to requiring curl if you want imap-send\n at all?\n\n 2. Run-time: if build with both implementations, which do we use by\n default? Related, if there is only one implementation, what should\n the default be?\n\nI think the answer to (1) is that we still want to build imap-send\nwithout USE_CURL_FOR_IMAP_SEND if the user doesn't have curl installed.\nAnd your patch leaves that, which is good. Though if we are deprecating\nit, we may want to issue a deprecation warning (eventually; we can still\nswitch the run-time default now, get more data on whether a\ndeprecation/switch is a good idea, and then later decide to deprecate).\n\nFor (2), you're trying to switch the default when both are built. But I\nthink it's important to continue to default to the old-style\nimplementation if that's the only thing that was built. Otherwise it\neffectively becomes the build-time deprecation warning, and we're not\nquite ready for that.\n\nSo I think this maybe needs to be:\n\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 /* We don't have curl, so continue to use the historical implementation */\n #define USE_CURL_DEFAULT 0\n #endif\n\n-Peff\n"},{"id":"327874","messageId":"xmqqy3pk1kjn.fsf@gitster.mtv.corp.google.com","threadId":"46528","inReplyTo":"20170807200126.fnbz57qd44vj43c6@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] imap-send: use curl by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-12T06:46:20Z","receivedAt":"2017-09-12T06:46:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 07, 2017 at 04:04:05PM +0200, Nicolas Morey-Chaisemartin wrote:\n>\n>> Signed-off-by: Nicolas Morey-Chaisemartin \n>\n> Thanks for moving forward with this.\n>\n> Can you please flesh out your commit messages with some of the reasoning\n> and related discussion? I know from a nearby thread why we want to flip\n> the default, but people reading `git log` much later will not have that\n> context.\n> ...\n\nI was sweeping my mailbox to collect loose ends that haven't been\ntied down, and noticed that this topic does not seem to reach a\nconclusion. Do we want to reboot the effort? Or should we just\nthrow it in the #leftoverbits bin for now?\n\nThanks.\n\n"},{"id":"327879","messageId":"xmqqk2141jst.fsf@gitster.mtv.corp.google.com","threadId":"46528","inReplyTo":"xmqqy3pk1kjn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/4] imap-send: use curl by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-12T07:02:26Z","receivedAt":"2017-09-12T07:02:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I was sweeping my mailbox to collect loose ends that haven't been\n> tied down, and noticed that this topic does not seem to have reached\n> a conclusion. Do we want to reboot the effort? Or should we just\n> throw it in the #leftoverbits bin for now?\n\nAhh, I failed to find newer incarnations of this topic (there was v3 that\nstarts at <087f5907-6558-ce32-2f5c-2e418522c030@morey-chaisemartin.com>)\nwhen I wrote it. Sorry about pinging a wrong/old article.\n\nI just gave a cursory re-read of the review thread over the v3\nseries; it appears that we were very close to the finishing line\nalready?\n\nOr am I yet missing/forgetting some later developments that made\nthis topic obsolete?\n\nThanks.\n\n\n"},{"id":"327880","messageId":"52583345-ada8-3f57-0bc1-04c92c8d0903@morey-chaisemartin.com","threadId":"46528","inReplyTo":"xmqqk2141jst.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/4] imap-send: use curl by default","fromName":"Nicolas Morey-Chaisemartin","fromEmail":"nicolas@morey-chaisemartin.com","sentAt":"2017-09-12T08:24:49Z","receivedAt":"2017-09-12T08:35:04Z","isPatch":true,"sender":{"key":"devel-git@morey-chaisemartin.com","avatar":"https://avatars.githubusercontent.com/u/108326?v=4"},"body":"\n\nLe 12/09/2017 à 09:02, Junio C Hamano a écrit :\n> Junio C Hamano writes:\n>\n>> I was sweeping my mailbox to collect loose ends that haven't been\n>> tied down, and noticed that this topic does not seem to have reached\n>> a conclusion. Do we want to reboot the effort? Or should we just\n>> throw it in the #leftoverbits bin for now?\n> Ahh, I failed to find newer incarnations of this topic (there was v3 that\n> starts at <087f5907-6558-ce32-2f5c-2e418522c030@morey-chaisemartin.com>)\n> when I wrote it. Sorry about pinging a wrong/old article.\n>\n> I just gave a cursory re-read of the review thread over the v3\n> series; it appears that we were very close to the finishing line\n> already?\n>\n> Or am I yet missing/forgetting some later developments that made\n> this topic obsolete?\n>\n> Thanks.\n>\nv3 needs just a few bits fixed:\n- Fix the bads return code in patch #1.\n- Reword patch #4 was you found confusing. I sent a proposal fix that you probably missed:\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\nIf you're happy with this log message, I've v4 ready to post.\n\nNicolas\n\n"}]}