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

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

[Nicolas Morey-Chaisemartin]

Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
---
 imap-send.c | 1 +
 1 file changed, 1 insertion(+)

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

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

[Nicolas Morey-Chaisemartin]

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);
 
-	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

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

[Martin Ågren]

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

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

[Nicolas Morey-Chaisemartin]

Le 07/08/2017 à 18:34, 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 | 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

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

[Jeff King]

On Mon, Aug 07, 2017 at 07:06:07PM +0200, Nicolas Morey-Chaisemartin wrote:

> >> -       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.

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

[Nicolas Morey-Chaisemartin]

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;
-- 
2.14.0.rc1.16.g87fcec1e8

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

[Martin Ågren]

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

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

[Nicolas Morey-Chaisemartin]

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 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

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

[Martin Ågren]

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

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

[Jeff King]

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.

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Junio C Hamano]

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.

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

[Nicolas Morey-Chaisemartin]

Le 12/09/2017 à 09:02, Junio C Hamano a écrit :
> 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