Re: [PATCH] imap-send: use libcurl for implementation

[Junio C Hamano <gitster@pobox.com>]

Bernhard Reiter <ockham@raz.or.at> writes:

> @@ -25,7 +25,6 @@ Typical usage is something like:
>  
>  git format-patch --signoff --stdout --attach origin | git imap-send
>  
> -
>  OPTIONS

Why?

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

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

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

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

> 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

> +#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?

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