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: > >> - 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-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
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(&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
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(&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
[PATCH 3/4] imap_send: setup_curl: use server_conf parameter instead of the global variable
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(&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