Re: [PATCH] imap-send: use libcurl for implementation
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 6, 2014, 18:25 UTC
- Message-ID
- <xmqqzjc4gpa2.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <545B5D5D.5040904@raz.or.at>
Bernhard Reiter <ockham@raz.or.at> writes:
Show 6 quoted lines
> @@ -25,7 +25,6 @@ Typical usage is something like: > > git format-patch --signoff --stdout --attach origin | git imap-send > > - > OPTIONS
Why?
Show 15 quoted lines
> @@ -37,6 +36,17 @@ OPTIONS > --quiet:: > Be quiet. > > +--curl:: > + Use libcurl to communicate with the IMAP server, unless tunneling > + into it. Only available if git was built with the > + USE_CURL_FOR_IMAP_SEND option set, in which case this is the > + default behavior. > + > +--no-curl:: > + Talk to the IMAP server using git's own IMAP routines instead of > + using libcurl. Only available git was built with the > + USE_CURL_FOR_IMAP_SEND option set; implicitly assumed otherwise. > +
I think we tend to spell "Git" not "git" when we refer to the software suite as a whole.
More importantly, the description on these two items are no longer in line with the implementation, aren't they? We accept these options but warn and a build without libcurl ignores --curl with a warning, and --curl is not default in any build.
Show 7 quoted lines
> @@ -87,7 +97,9 @@ imap.preformattedHTML:: > > imap.authMethod:: > Specify authenticate method for authentication with IMAP server. > - Current supported method is 'CRAM-MD5' only. If this is not set > + If git was built with the NO_CURL option, or if your curl version is > + < 7.34.0, or if you're running git-imap-send with the --no-curl
s/< /older than /;
Also quote --no-curl inside bq-pair, i.e. `--no-curl`, as that is something the user will type as-is.
Show 17 quoted lines
> + option, the only supported method is 'CRAM-MD5'. If this is not set > then 'git imap-send' uses the basic IMAP plaintext LOGIN command. > > Examples > diff --git a/INSTALL b/INSTALL > index 6ec7a24..ffb071e 100644 > --- a/INSTALL > +++ b/INSTALL > @@ -108,18 +108,21 @@ Issues of note: > so you might need to install additional packages other than Perl > itself, e.g. Time::HiRes. > > - - "openssl" library is used by git-imap-send to use IMAP over SSL. > - If you don't need it, use NO_OPENSSL. > + - git-imap-send needs the OpenSSL library to talk IMAP over SSL if > + you are using libcurl older than 7.34.0. Otherwise you can use > + NO_OPENSSL without losing git-imap-send.
OK.
Show 6 quoted lines
> + - "libcurl" library is used by git-http-fetch, git-fetch, and, if > + the curl version >= 7.34.0, for git-imap-send. You might also > + want the "curl" executable for debugging purposes. If you do not > + use http:// or https:// repositories, and do not want to put > + patches into an IMAP mailbox, you do not have to have them > + (use NO_CURL).
OK.
Show 32 quoted lines
> diff --git a/imap-send.c b/imap-send.c
> index 7f9d30e..01ce175 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -30,13 +30,18 @@
> #ifdef NO_OPENSSL
> typedef void *SSL;
> #endif
> +#ifdef USE_CURL_FOR_IMAP_SEND
> +#include "http.h"
> +#endif
>
> static int verbosity;
> +static int use_curl; /* strictly opt in */
>
> -static const char * const imap_send_usage[] = { "git imap-send [-v] [-q] < <mbox>", NULL };
> +static const char * const imap_send_usage[] = { "git imap-send [-v] [-q] [--[no-]curl] < <mbox>", NULL };
>
> static struct option imap_send_options[] = {
> OPT__VERBOSITY(&verbosity),
> + OPT_BOOL(0, "curl", &use_curl, "use libcurl to communicate with the IMAP server"),
> OPT_END()
> };
>
> @@ -1344,14 +1349,138 @@ static void git_imap_config(void)
> git_config_get_string("imap.authmethod", &server.auth_method);
> }
>
> -int main(int argc, char **argv)
> -{
> - struct strbuf all_msgs = STRBUF_INIT;
> +static int append_msgs_to_imap(struct imap_server_conf *server, struct strbuf* all_msgs, int total) {The opening brace sits on its own line by itself, so
Show 31 quoted lines
> +#ifdef USE_CURL_FOR_IMAP_SEND
> +static CURL *setup_curl(struct imap_server_conf *srvc)
> +{
> + CURL *curl;
> + struct strbuf path = STRBUF_INIT;
> + struct strbuf auth = STRBUF_INIT;
> +
> + if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK)
> + die("curl_global_init failed");
> +
> + curl = curl_easy_init();
> +
> + if (!curl)
> + die("curl_easy_init failed");
> +
> + curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
> + curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
> +
> + strbuf_addstr(&path, server.host);
> + if (!path.len || path.buf[path.len - 1] != '/')
> + strbuf_addch(&path, '/');
> + strbuf_addstr(&path, server.folder);
> +
> + curl_easy_setopt(curl, CURLOPT_URL, path.buf);
> + curl_easy_setopt(curl, CURLOPT_PORT, server.port);
> +
> + if (server.auth_method) {
> + strbuf_addstr(&auth, "AUTH=");
> + strbuf_addstr(&auth, server.auth_method);
> + curl_easy_setopt(curl, CURLOPT_LOGIN_OPTIONS, auth.buf);
> + }Are path.buf and auth.buf leaked here, or does CURL *curl take possession of them by curl_easy_setopt() and not freeing them ourselves is the right thing to do? Assuming that is the case, perhaps we would want to use strbuf_detach() on &path and &auth to make it clear that is what is going on?
Show 13 quoted lines
> +int main(int argc, char **argv)
> +{
> + struct strbuf all_msgs = STRBUF_INIT;
> + int total;
> int nongit_ok;
>
> git_extract_argv0_path(argv[0]);
> @@ -1360,12 +1489,18 @@ int main(int argc, char **argv)
>
> setup_git_directory_gently(&nongit_ok);
> git_imap_config();
> -
> argc = parse_options(argc, (const char **)argv, "", imap_send_options, imap_send_usage, 0);Why?
Thanks.