threads / patch / 46528

patch, 4 partsimap_send: setup_curl: prompt user for username/password if not set in config file

Subject: [PATCH 2/4] imap_send: setup_curl: prompt user for username/password if not set in config file

## tl;dr

13 messages between Aug 7, 2017 and Sep 12, 2017. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Nicolas Morey-Chaisemartin· Aug 7, 2017, 14:03 UTC · lore
Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 imap-send.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to imap-send.c +1 −0
diff --git a/imap-send.c b/imap-send.c
index 38b3c817e..682a06551 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1415,6 +1415,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
 	if (!curl)
 		die("curl_easy_init failed");
 
+	server_fill_credential(&server);
 	curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
 	curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
 

-- 
2.14.0.rc1.16.g87fcec1e8
Nicolas Morey-Chaisemartin· Aug 7, 2017, 14:04 UTC · lore

[PATCH 3/4] imap_send: setup_curl: use server_conf parameter instead of the global variable

Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 imap-send.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)
Show changes to imap-send.c +12 −12
diff --git a/imap-send.c b/imap-send.c
index 682a06551..90b8683ed 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1415,37 +1415,37 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
 	if (!curl)
 		die("curl_easy_init failed");
 
-	server_fill_credential(&server);
-	curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
-	curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
+	server_fill_credential(srvc);
+	curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);
+	curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
 
-	strbuf_addstr(&path, server.use_ssl ? "imaps://" : "imap://");
-	strbuf_addstr(&path, server.host);
+	strbuf_addstr(&path, srvc->use_ssl ? "imaps://" : "imap://");
+	strbuf_addstr(&path, srvc->host);
 	if (!path.len || path.buf[path.len - 1] != '/')
 		strbuf_addch(&path, '/');
-	strbuf_addstr(&path, server.folder);
+	strbuf_addstr(&path, srvc->folder);
 
 	curl_easy_setopt(curl, CURLOPT_URL, path.buf);
 	strbuf_release(&path);
-	curl_easy_setopt(curl, CURLOPT_PORT, server.port);
+	curl_easy_setopt(curl, CURLOPT_PORT, srvc->port);
 
-	if (server.auth_method) {
+	if (srvc->auth_method) {
 #if LIBCURL_VERSION_NUM < 0x072200
 		warning("No LOGIN_OPTIONS support in this cURL version");
 #else
 		struct strbuf auth = STRBUF_INIT;
 		strbuf_addstr(&auth, "AUTH=");
-		strbuf_addstr(&auth, server.auth_method);
+		strbuf_addstr(&auth, srvc->auth_method);
 		curl_easy_setopt(curl, CURLOPT_LOGIN_OPTIONS, auth.buf);
 		strbuf_release(&auth);
 #endif
 	}
 
-	if (!server.use_ssl)
+	if (!srvc->use_ssl)
 		curl_easy_setopt(curl, CURLOPT_USE_SSL, (long)CURLUSESSL_TRY);
 
-	curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, server.ssl_verify);
-	curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, server.ssl_verify);
+	curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, srvc->ssl_verify);
+	curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, srvc->ssl_verify);
 
 	curl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);
 

-- 
2.14.0.rc1.16.g87fcec1e8
Martin Ågren· Aug 7, 2017, 16:34 UTC · re: Nicolas Morey-Chaisemartin · lore

Re: [PATCH 3/4] imap_send: setup_curl: use server_conf parameter instead of the global variable

On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com> wrote:

Show 19 quoted lines
> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
> ---
>  imap-send.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index 682a06551..90b8683ed 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1415,37 +1415,37 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
>         if (!curl)
>                 die("curl_easy_init failed");
>
> -       server_fill_credential(&server);
> -       curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
> -       curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
> +       server_fill_credential(srvc);
> +       curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);
> +       curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);

Here you change the server_fill_credential-call that you just added. Maybe do this patch earlier, perhaps even as patch 1?

I'm snipping lots of s/server/srvc/-changes... There's a less noisy way of addressing the fact that srvc is unused: dropping it. I'm not saying that's a good idea, but it could be considered, then explained why this approach is better. There are some other functions which access "server" directly, and some which take (and use!) a "srvc". Maybe make the whole file consistent?

Martin
Nicolas Morey-Chaisemartin· Aug 7, 2017, 17:06 UTC · re: Martin Ågren · lore

Re: [PATCH 3/4] imap_send: setup_curl: use server_conf parameter instead of the global variable

Le 07/08/2017 à 18:34, Martin Ågren a écrit :
Show 32 quoted lines
> On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin
> <nicolas@morey-chaisemartin.com> wrote:
>> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
>> ---
>>  imap-send.c | 24 ++++++++++++------------
>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 682a06551..90b8683ed 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -1415,37 +1415,37 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
>>         if (!curl)
>>                 die("curl_easy_init failed");
>>
>> -       server_fill_credential(&server);
>> -       curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
>> -       curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
>> +       server_fill_credential(srvc);
>> +       curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);
>> +       curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
> Here you change the server_fill_credential-call that you just added.
> Maybe do this patch earlier, perhaps even as patch 1?
>
> I'm snipping lots of s/server/srvc/-changes... There's a less noisy
> way of addressing the fact that srvc is unused: dropping it. I'm not
> saying that's a good idea, but it could be considered, then explained
> why this approach is better. There are some other functions which
> access "server" directly, and some which take (and use!) a "srvc".
> Maybe make the whole file consistent?
>
> Martin

That'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. I 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 ?

Nicolas
Jeff King· Aug 7, 2017, 19:50 UTC · re: Nicolas Morey-Chaisemartin · lore

Re: [PATCH 3/4] imap_send: setup_curl: use server_conf parameter instead of the global variable

On Mon, Aug 07, 2017 at 07:06:07PM +0200, Nicolas Morey-Chaisemartin wrote:
Show 22 quoted lines
> >> -       server_fill_credential(&server);
> >> -       curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
> >> -       curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
> >> +       server_fill_credential(srvc);
> >> +       curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);
> >> +       curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
> > Here you change the server_fill_credential-call that you just added.
> > Maybe do this patch earlier, perhaps even as patch 1?
> >
> > I'm snipping lots of s/server/srvc/-changes... There's a less noisy
> > way of addressing the fact that srvc is unused: dropping it. I'm not
> > saying that's a good idea, but it could be considered, then explained
> > why this approach is better. There are some other functions which
> > access "server" directly, and some which take (and use!) a "srvc".
> > Maybe make the whole file consistent?
> >
> That'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.
> I 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 ?

I suspect it's just code in need of a cleanup. But let's cc the original author of 1e16b255b (git-imap-send: use libcurl for implementation, 2014-11-09) to see if he has any comments[1].

-Peff
[1] Bernhard, the whole series is at:
      https://public-inbox.org/git/38d3ae5b-4020-63cc-edfa-0a77e42798b8@morey-chaisemartin.com/
    The general idea is to make sure the original and curl imap-send
    implementations have feature parity, make the curl version the
    default, and then hopefully eventually drop the non-curl one
    entirely.
Nicolas Morey-Chaisemartin· Aug 7, 2017, 14:04 UTC · lore

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

Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 imap-send.c | 6 ------
 1 file changed, 6 deletions(-)
Show changes to imap-send.c +0 −6
diff --git a/imap-send.c b/imap-send.c
index 90b8683ed..4ebc16437 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -35,13 +35,7 @@ typedef void *SSL;
 #include "http.h"
 #endif
 
-#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
-/* only available option */
 #define USE_CURL_DEFAULT 1
-#else
-/* strictly opt in */
-#define USE_CURL_DEFAULT 0
-#endif
 
 static int verbosity;
 static int use_curl = USE_CURL_DEFAULT;

-- 
2.14.0.rc1.16.g87fcec1e8
Martin Ågren· Aug 7, 2017, 16:37 UTC · re: Nicolas Morey-Chaisemartin · lore

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

On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com> wrote:

Show 23 quoted lines
> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
> ---
>  imap-send.c | 6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index 90b8683ed..4ebc16437 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -35,13 +35,7 @@ typedef void *SSL;
>  #include "http.h"
>  #endif
>
> -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
> -/* only available option */
>  #define USE_CURL_DEFAULT 1
> -#else
> -/* strictly opt in */
> -#define USE_CURL_DEFAULT 0
> -#endif
>
>  static int verbosity;
>  static int use_curl = USE_CURL_DEFAULT;
So this is now basically "static int use_curl = 1;".

Do we need a compile-time escape-hatch in case someone really needs to avoid curl, e.g., because they have a too old version? I suppose there is a conceptual difference between the "default", i.e., the value of USE_CURL_DEFAULT that is assigned to "use_curl", and the "default default", i.e., the value that is normally assigned to USE_CURL_DEFAULT.

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

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

Le 07/08/2017 à 18:37, Martin Ågren a écrit :
Show 34 quoted lines
> On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin
> <nicolas@morey-chaisemartin.com> wrote:
>> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
>> ---
>>  imap-send.c | 6 ------
>>  1 file changed, 6 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 90b8683ed..4ebc16437 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -35,13 +35,7 @@ typedef void *SSL;
>>  #include "http.h"
>>  #endif
>>
>> -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
>> -/* only available option */
>>  #define USE_CURL_DEFAULT 1
>> -#else
>> -/* strictly opt in */
>> -#define USE_CURL_DEFAULT 0
>> -#endif
>>
>>  static int verbosity;
>>  static int use_curl = USE_CURL_DEFAULT;
> So this is now basically "static int use_curl = 1;".
>
> Do we need a compile-time escape-hatch in case someone really needs
> to avoid curl, e.g., because they have a too old version? I suppose
> there is a conceptual difference between the "default", i.e., the value
> of USE_CURL_DEFAULT that is assigned to "use_curl", and the "default
> default", i.e., the value that is normally assigned to USE_CURL_DEFAULT.
>
> Martin

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). 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. I don't mind dropping it and hardcofing the default to 1

Also on a side note, I have a case where authentication works with --no-curl but fails withs --curl. Still trying to figure out why.
Nicolas
Martin Ågren· Aug 7, 2017, 17:37 UTC · re: Nicolas Morey-Chaisemartin · lore

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

On 7 August 2017 at 19:10, Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com> wrote:

Show 39 quoted lines
>
>
> Le 07/08/2017 à 18:37, Martin Ågren a écrit :
>> On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin
>> <nicolas@morey-chaisemartin.com> wrote:
>>> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
>>> ---
>>>  imap-send.c | 6 ------
>>>  1 file changed, 6 deletions(-)
>>>
>>> diff --git a/imap-send.c b/imap-send.c
>>> index 90b8683ed..4ebc16437 100644
>>> --- a/imap-send.c
>>> +++ b/imap-send.c
>>> @@ -35,13 +35,7 @@ typedef void *SSL;
>>>  #include "http.h"
>>>  #endif
>>>
>>> -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
>>> -/* only available option */
>>>  #define USE_CURL_DEFAULT 1
>>> -#else
>>> -/* strictly opt in */
>>> -#define USE_CURL_DEFAULT 0
>>> -#endif
>>>
>>>  static int verbosity;
>>>  static int use_curl = USE_CURL_DEFAULT;
>> So this is now basically "static int use_curl = 1;".
>>
>> Do we need a compile-time escape-hatch in case someone really needs
>> to avoid curl, e.g., because they have a too old version? I suppose
>> there is a conceptual difference between the "default", i.e., the value
>> of USE_CURL_DEFAULT that is assigned to "use_curl", and the "default
>> default", i.e., the value that is normally assigned to USE_CURL_DEFAULT.
>>
>> Martin
>
> 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).

I have just looked at the sources and haven't thought too hard about it, but doesn't it mean that compiling without USE_CURL_FOR_IMAP_SEND results in a binary such that you must use --no-curl or get used to seeing "warning: --curl not supported in this build"?

> 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.
> I don't mind dropping it and hardcofing the default to 1

I did not intend to suggest that. Just to be clear, I am very unfamiliar with most of the Git codebase. Please don't take anything I say as advice. :) As a question about something you have or haven't already thought about, sure. :)

Martin
Jeff King· Aug 7, 2017, 20:01 UTC · re: Nicolas Morey-Chaisemartin · lore

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

On Mon, Aug 07, 2017 at 04:04:05PM +0200, Nicolas Morey-Chaisemartin wrote:
> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
Thanks for moving forward with this.

Can you please flesh out your commit messages with some of the reasoning and related discussion? I know from a nearby thread why we want to flip the default, but people reading `git log` much later will not have that context.

Show 15 quoted lines
> diff --git a/imap-send.c b/imap-send.c
> index 90b8683ed..4ebc16437 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -35,13 +35,7 @@ typedef void *SSL;
>  #include "http.h"
>  #endif
>  
> -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
> -/* only available option */
>  #define USE_CURL_DEFAULT 1
> -#else
> -/* strictly opt in */
> -#define USE_CURL_DEFAULT 0
> -#endif

I agree with the comments Martin made here. I think there are really two levels of "default" we need to care about:

  1. Build-time: do we default to requiring curl if you want imap-send
     at all?
  2. Run-time: if build with both implementations, which do we use by
     default? Related, if there is only one implementation, what should
     the default be?

I think the answer to (1) is that we still want to build imap-send without USE_CURL_FOR_IMAP_SEND if the user doesn't have curl installed. And your patch leaves that, which is good. Though if we are deprecating it, we may want to issue a deprecation warning (eventually; we can still switch the run-time default now, get more data on whether a deprecation/switch is a good idea, and then later decide to deprecate).

For (2), you're trying to switch the default when both are built. But I think it's important to continue to default to the old-style implementation if that's the only thing that was built. Otherwise it effectively becomes the build-time deprecation warning, and we're not quite ready for that.

So I think this maybe needs to be:
  #if defined(USE_CURL_FOR_IMAP_SEND)
  /* Always default to curl if it's available. */
  #define USE_CURL_DEFAULT 1
  #else
  /* We don't have curl, so continue to use the historical implementation */
  #define USE_CURL_DEFAULT 0
  #endif
-Peff
Junio C Hamano· Sep 12, 2017, 06:46 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> On Mon, Aug 07, 2017 at 04:04:05PM +0200, Nicolas Morey-Chaisemartin wrote:
>
>> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
>
> Thanks for moving forward with this.
>
> Can you please flesh out your commit messages with some of the reasoning
> and related discussion? I know from a nearby thread why we want to flip
> the default, but people reading `git log` much later will not have that
> context.
> ...

I was sweeping my mailbox to collect loose ends that haven't been tied down, and noticed that this topic does not seem to reach a conclusion. Do we want to reboot the effort? Or should we just throw it in the #leftoverbits bin for now?

Thanks.
Junio C Hamano· Sep 12, 2017, 07:02 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
> I was sweeping my mailbox to collect loose ends that haven't been
> tied down, and noticed that this topic does not seem to have reached
> a conclusion.  Do we want to reboot the effort?  Or should we just
> throw it in the #leftoverbits bin for now?

Ahh, I failed to find newer incarnations of this topic (there was v3 that starts at <087f5907-6558-ce32-2f5c-2e418522c030@morey-chaisemartin.com>) when I wrote it. Sorry about pinging a wrong/old article.

I just gave a cursory re-read of the review thread over the v3 series; it appears that we were very close to the finishing line already?

Or am I yet missing/forgetting some later developments that made this topic obsolete?

Thanks.
Nicolas Morey-Chaisemartin· Sep 12, 2017, 08:24 UTC · re: Junio C Hamano · lore

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

Le 12/09/2017 à 09:02, Junio C Hamano a écrit :
Show 19 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I was sweeping my mailbox to collect loose ends that haven't been
>> tied down, and noticed that this topic does not seem to have reached
>> a conclusion.  Do we want to reboot the effort?  Or should we just
>> throw it in the #leftoverbits bin for now?
> Ahh, I failed to find newer incarnations of this topic (there was v3 that
> starts at <087f5907-6558-ce32-2f5c-2e418522c030@morey-chaisemartin.com>)
> when I wrote it.  Sorry about pinging a wrong/old article.
>
> I just gave a cursory re-read of the review thread over the v3
> series; it appears that we were very close to the finishing line
> already?
>
> Or am I yet missing/forgetting some later developments that made
> this topic obsolete?
>
> Thanks.
>
v3 needs just a few bits fixed:
- Fix the bads return code in patch #1.
- Reword patch #4 was you found confusing. I sent a proposal fix that you probably missed:
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>
If you're happy with this log message, I've v4 ready to post.
Nicolas

← back to recent threads