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();
> >> -   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-0a77e4279...@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.

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
>  wrote:
>> Signed-off-by: Nicolas Morey-Chaisemartin 
>> ---
>>  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();
>> -   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

[Martin Ågren]

On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin
 wrote:
> Signed-off-by: Nicolas Morey-Chaisemartin 
> ---
>  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();
> -   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

[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 
---
 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();
-   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(, server.use_ssl ? "imaps://" : "imap://");
-   strbuf_addstr(, server.host);
+   strbuf_addstr(, srvc->use_ssl ? "imaps://" : "imap://");
+   strbuf_addstr(, srvc->host);
if (!path.len || path.buf[path.len - 1] != '/')
strbuf_addch(, '/');
-   strbuf_addstr(, server.folder);
+   strbuf_addstr(, srvc->folder);
 
curl_easy_setopt(curl, CURLOPT_URL, path.buf);
strbuf_release();
-   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=");
-   strbuf_addstr(, server.auth_method);
+   strbuf_addstr(, srvc->auth_method);
curl_easy_setopt(curl, CURLOPT_LOGIN_OPTIONS, auth.buf);
strbuf_release();
 #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