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

[Ramsay Jones <ramsay@ramsay1.demon.co.uk>]

On 09/11/14 14:55, Bernhard Reiter wrote:
> Am 2014-11-09 um 14:00 schrieb Torsten Bögershausen:
>> On 2014-08-27 00.40, Bernhard Reiter wrote:
>>> Use libcurl's high-level API functions to implement git-imap-send
>>> instead of the previous low-level OpenSSL-based functions.
>>>
>> This doesn't seem to fully work under Debian 7:
>> /home/tb/projects/git/git.pu/imap-send.c:1546: undefined reference to `curl_append_msgs_to_imap'
> 
> Thx for the notice. I forgot to guard that with an #ifdef.
> 
> The new patch below includes that, and the fix sent by Ramsay; hopefully the squashed/edited commit message is fine.

Sorry for not responding earlier, I've been somewhat busy for the last
couple of days. :(

> Bernhard
> 
> -- >8 --
> 
> Use libcurl's high-level API functions to implement git-imap-send
> instead of the previous low-level OpenSSL-based functions.
> 
> Since version 7.30.0, libcurl's API has been able to communicate with
> IMAP servers. Using those high-level functions instead of the current
> ones would reduce imap-send.c by some 1200 lines of code. For now,
> the old ones are wrapped in #ifdefs, and the new functions are enabled
> by make if curl's version is >= 7.34.0, from which version on curl's
> CURLOPT_LOGIN_OPTIONS (enabling IMAP authentication) parameter has been
> available. The low-level functions will still be used for tunneling
> into the server for now.
> 
> As I don't have access to that many IMAP servers, I haven't been able to
> test the new code with a wide variety of parameter combinations. I did
> test both secure and insecure (imaps:// and imap://) connections and
> values of "PLAIN" and "LOGIN" for the authMethod.
> 
> In order to suppress a sparse warning about "using sizeof on a
> function", we use the same solution used in commit 9371322a6
> ("sparse: suppress some "using sizeof on a function" warnings",
> 06-10-2013) which solved exactly this problem for the other commands
> using libcurl.

Although it doesn't hurt, I don't think this 'problem' deserves
so many (or any) inches in the commit message. ;-)

> 
> Signed-off-by: Bernhard Reiter <ockham@raz.or.at>
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>

Since I didn't actually review this patch, or make any significant
contribution to the code (it's not even enough to be copyright-able!),
then this 'Signed-off-by:' should not be included. At *most* you might
want to put a 'Helped-by:' _prior_ to your sign-off.

Thanks.

ATB,
Ramsay Jones


> ---
>  Documentation/git-imap-send.txt |  15 +++-
>  INSTALL                         |  15 ++--
>  Makefile                        |  18 +++-
>  imap-send.c                     | 176 ++++++++++++++++++++++++++++++++++------
>  4 files changed, 187 insertions(+), 37 deletions(-)
> 
> diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
> index 0897131..77aacf1 100644
> --- a/Documentation/git-imap-send.txt
> +++ b/Documentation/git-imap-send.txt
> @@ -9,7 +9,7 @@ git-imap-send - Send a collection of patches from stdin to an IMAP folder
>  SYNOPSIS
>  --------
>  [verse]
> -'git imap-send' [-v] [-q]
> +'git imap-send' [-v] [-q] [--[no-]curl]
>  
>  
>  DESCRIPTION
> @@ -37,6 +37,15 @@ OPTIONS
>  --quiet::
>  	Be quiet.
>  
> +--curl::
> +	Use libcurl to communicate with the IMAP server, unless tunneling
> +	into it.  Ignored if Git was built without the USE_CURL_FOR_IMAP_SEND
> +	option set.
> +
> +--no-curl::
> +	Talk to the IMAP server using git's own IMAP routines instead of
> +	using libcurl.
> +
>  
>  CONFIGURATION
>  -------------
> @@ -87,7 +96,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 older
> +	than 7.34.0, or if you're running git-imap-send with the `--no-curl`
> +	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.
>  
>  	  By default, git uses OpenSSL for SHA1 but it will use its own
>  	  library (inspired by Mozilla's) with either NO_OPENSSL or
>  	  BLK_SHA1.  Also included is a version optimized for PowerPC
>  	  (PPC_SHA1).
>  
> -	- "libcurl" library is used by git-http-fetch and git-fetch.  You
> -	  might also want the "curl" executable for debugging purposes.
> -	  If you do not use http:// or https:// repositories, you do not
> -	  have to have them (use NO_CURL).
> +	- "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).
>  
>  	- "expat" library; git-http-push uses it for remote lock
>  	  management over DAV.  Similar to "curl" above, this is optional
> diff --git a/Makefile b/Makefile
> index 827006b..14d5ac1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -995,6 +995,9 @@ ifdef HAVE_ALLOCA_H
>  	BASIC_CFLAGS += -DHAVE_ALLOCA_H
>  endif
>  
> +IMAP_SEND_BUILDDEPS =
> +IMAP_SEND_LDFLAGS = $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
> +
>  ifdef NO_CURL
>  	BASIC_CFLAGS += -DNO_CURL
>  	REMOTE_CURL_PRIMARY =
> @@ -1029,6 +1032,15 @@ else
>  			PROGRAM_OBJS += http-push.o
>  		endif
>  	endif
> +	curl_check := $(shell (echo 072200; curl-config --vernum) 2>/dev/null | sort -r | sed -ne 2p)
> +	ifeq "$(curl_check)" "072200"
> +		USE_CURL_FOR_IMAP_SEND = YesPlease
> +	endif
> +	ifdef USE_CURL_FOR_IMAP_SEND
> +		BASIC_CFLAGS += -DUSE_CURL_FOR_IMAP_SEND
> +		IMAP_SEND_BUILDDEPS = http.o
> +		IMAP_SEND_LDFLAGS += $(CURL_LIBCURL)
> +	endif
>  	ifndef NO_EXPAT
>  		ifdef EXPATDIR
>  			BASIC_CFLAGS += -I$(EXPATDIR)/include
> @@ -1874,7 +1886,7 @@ gettext.sp gettext.s gettext.o: GIT-PREFIX
>  gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \
>  	-DGIT_LOCALE_PATH='"$(localedir_SQ)"'
>  
> -http-push.sp http.sp http-walker.sp remote-curl.sp: SPARSE_FLAGS += \
> +http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SPARSE_FLAGS += \
>  	-DCURL_DISABLE_TYPECHECK
>  
>  ifdef NO_EXPAT
> @@ -1895,9 +1907,9 @@ endif
>  git-%$X: %.o GIT-LDFLAGS $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
>  
> -git-imap-send$X: imap-send.o GIT-LDFLAGS $(GITLIBS)
> +git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> -		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
> +		$(LIBS) $(IMAP_SEND_LDFLAGS)
>  
>  git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
> diff --git a/imap-send.c b/imap-send.c
> index 7f9d30e..08271d9 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,141 @@ static void git_imap_config(void)
>  	git_config_get_string("imap.authmethod", &server.auth_method);
>  }
>  
> -int main(int argc, char **argv)
> +static int append_msgs_to_imap(struct imap_server_conf *server, struct strbuf* all_msgs, int total)
>  {
> -	struct strbuf all_msgs = STRBUF_INIT;
>  	struct strbuf msg = STRBUF_INIT;
>  	struct imap_store *ctx = NULL;
>  	int ofs = 0;
>  	int r;
> -	int total, n = 0;
> +	int n = 0;
> +
> +	ctx = imap_open_store(server, server->folder);
> +	if (!ctx) {
> +		fprintf(stderr, "failed to open store\n");
> +		return 1;
> +	}
> +	ctx->name = server->folder;
> +
> +	fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : "");
> +	while (1) {
> +		unsigned percent = n * 100 / total;
> +
> +		fprintf(stderr, "%4u%% (%d/%d) done\r", percent, n, total);
> +
> +		if (!split_msg(all_msgs, &msg, &ofs))
> +			break;
> +		if (server->use_html)
> +			wrap_in_html(&msg);
> +		r = imap_store_msg(ctx, &msg);
> +		if (r != DRV_OK)
> +			break;
> +		n++;
> +	}
> +	fprintf(stderr, "\n");
> +
> +	imap_close_store(ctx);
> +
> +	return 0;
> +}
> +
> +#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);
> +	strbuf_release(&path);
> +	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);
> +	}
> +	strbuf_release(&auth);
> +
> +	if (server.use_ssl)
> +		curl_easy_setopt(curl, CURLOPT_USE_SSL, (long)CURLUSESSL_ALL);
> +
> +	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_READFUNCTION, fread_buffer);
> +
> +	curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);
> +
> +	if (0 < verbosity)
> +		curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
> +
> +	return curl;
> +}
> +
> +static int curl_append_msgs_to_imap(struct imap_server_conf *server, struct strbuf* all_msgs, int total) {
> +	int ofs = 0;
> +	int n = 0;
> +	struct buffer msgbuf = { STRBUF_INIT, 0 };
> +	CURL *curl;
> +	CURLcode res = CURLE_OK;
> +
> +	curl = setup_curl(server);
> +	curl_easy_setopt(curl, CURLOPT_READDATA, &msgbuf);
> +
> +	fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : "");
> +	while (1) {
> +		unsigned percent = n * 100 / total;
> +
> +		fprintf(stderr, "%4u%% (%d/%d) done\r", percent, n, total);
> +
> +		int prev_len = msgbuf.buf.len;
> +		if (!split_msg(all_msgs, &msgbuf.buf, &ofs))
> +			break;
> +		if (server->use_html)
> +			wrap_in_html(&msgbuf.buf);
> +		lf_to_crlf(&msgbuf.buf);
> +
> +		curl_easy_setopt(curl, CURLOPT_INFILESIZE_LARGE, (curl_off_t)(msgbuf.buf.len-prev_len));
> +
> +		res = curl_easy_perform(curl);
> +
> +		if(res != CURLE_OK) {
> +			fprintf(stderr, "curl_easy_perform() failed: %s\n",
> +					curl_easy_strerror(res));
> +			break;
> +		}
> +
> +		n++;
> +	}
> +	fprintf(stderr, "\n");
> +
> +	curl_easy_cleanup(curl);
> +	curl_global_cleanup();
> +
> +	return 0;
> +}
> +#endif
> +
> +int main(int argc, char **argv)
> +{
> +	struct strbuf all_msgs = STRBUF_INIT;
> +	int total;
>  	int nongit_ok;
>  
>  	git_extract_argv0_path(argv[0]);
> @@ -1366,6 +1498,13 @@ int main(int argc, char **argv)
>  	if (argc)
>  		usage_with_options(imap_send_usage, imap_send_options);
>  
> +#ifndef USE_CURL_FOR_IMAP_SEND
> +	if (use_curl) {
> +		warning("--use-curl not supported in this build");
> +		use_curl = 0;
> +	}
> +#endif
> +
>  	if (!server.port)
>  		server.port = server.use_ssl ? 993 : 143;
>  
> @@ -1399,29 +1538,14 @@ int main(int argc, char **argv)
>  	}
>  
>  	/* write it to the imap server */
> -	ctx = imap_open_store(&server, server.folder);
> -	if (!ctx) {
> -		fprintf(stderr, "failed to open store\n");
> -		return 1;
> -	}
> -
> -	fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : "");
> -	while (1) {
> -		unsigned percent = n * 100 / total;
>  
> -		fprintf(stderr, "%4u%% (%d/%d) done\r", percent, n, total);
> -		if (!split_msg(&all_msgs, &msg, &ofs))
> -			break;
> -		if (server.use_html)
> -			wrap_in_html(&msg);
> -		r = imap_store_msg(ctx, &msg);
> -		if (r != DRV_OK)
> -			break;
> -		n++;
> -	}
> -	fprintf(stderr, "\n");
> +	if (server.tunnel)
> +		return append_msgs_to_imap(&server, &all_msgs, total);
>  
> -	imap_close_store(ctx);
> +#ifdef USE_CURL_FOR_IMAP_SEND
> +	if (use_curl)
> +		return curl_append_msgs_to_imap(&server, &all_msgs, total);
> +#endif
>  
> -	return 0;
> +	return append_msgs_to_imap(&server, &all_msgs, total);
>  }
>