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

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

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

> 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.35.0, from which version on curl's
> CURLOPT_LOGIN_OPTIONS (enabling IMAP authentication) parameter has been
> available.
>
> 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.
>
> Signed-off-by: Bernhard Reiter <ockham@raz.or.at>
> ---

I think people have missed this one, partly because it was hidden as
an attachment.

>  Documentation/git-imap-send.txt |   3 +-
>  INSTALL                         |  15 +++---
>  Makefile                        |  16 +++++-
>  git.spec.in                     |   5 +-
>  imap-send.c                     | 109
> +++++++++++++++++++++++++++++++++++++---
>  5 files changed, 132 insertions(+), 16 deletions(-)

I smell a line-wrapped patch but it probably is at my receiving end,
forcing the attachment into a flattened form inside my MUA.

Nice to see git.spec.in updated; I like it when I see that the
author paid attention to details.

> diff --git a/imap-send.c b/imap-send.c
> index fb01a9c..a45570d 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -22,6 +22,10 @@
>   *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>   */
>  
> +#ifdef USE_CURL_FOR_IMAP_SEND
> +#define NO_OPENSSL
> +#endif
> +

This looks strange and stands out like a sore thumb.  Do any of our
other sources do this kind of macro tweaking inside C source before
including git-compat-util.h (or its equivalent like cache.h)?

I think what you are trying to do is to change the meaning of
NO_OPENSSL, which merely means "we do not have OpenSSL library and
do not want to link with it", locally to "we may or may not have and
use OpenSSL library elsewhere in Git, but in the code below we do
not want to use the code written to work on top of OpenSSL and
instead use libcurl".  Because of that, you define NO_OPENSSL before
including any of our headers to cause us not to include the headers,
and typedef away SSL, for example.

>  #include "cache.h"
>  #include "credential.h"
>  #include "exec_cmd.h"
> @@ -29,6 +33,9 @@
>  #ifdef NO_OPENSSL
>  typedef void *SSL;
>  #endif
> +#ifdef USE_CURL_FOR_IMAP_SEND
> +#include "http.h"
> +#endif

But does it have to be that way?  For one thing, doing it this way,
the user has to make a compilation-time choice, but if you separate
these compilation time macro into two, one for "can we even link
with and use OpenSSL?" (which is what NO_OPENSSL is about) and
another for "do we want an ability to talk to imap via libcurl?",
wouldn't it make it possible for you to switch between them at
runtime (e.g. you might want to go over the direct connection when
tunneling, while letting libcurl do the heavy lifting in
non-tunneled operation)?

> @@ -44,9 +51,7 @@ __attribute__((format (printf, 1, 2)))
>  static void imap_info(const char *, ...);
>  __attribute__((format (printf, 1, 2)))
>  static void imap_warn(const char *, ...);
> -
>  static char *next_arg(char **);
> -
>  __attribute__((format (printf, 3, 4)))
>  static int nfsnprintf(char *buf, int blen, const char *fmt, ...);
>  
> @@ -69,6 +74,7 @@ struct imap_server_conf {
>  	char *tunnel;
>  	char *host;
>  	int port;
> +	char *folder;
>  	char *user;
>  	char *pass;
>  	int use_ssl;
> @@ -82,6 +88,7 @@ static struct imap_server_conf server = {
>  	NULL,	/* tunnel */
>  	NULL,	/* host */
>  	0,	/* port */
> +	NULL,	/* folder */
>  	NULL,	/* user */
>  	NULL,	/* pass */
>  	0,   	/* use_ssl */
> @@ -1323,7 +1330,54 @@ static int split_msg(struct strbuf *all_msgs, struct strbuf *msg, int *ofs)
>  	return 1;
>  }
>  
> -static char *imap_folder;

All of the above seem to have meant well, but these changes are not
about talking with IMAP servers via libcurl.  We'd prefer to see
changes like these as preliminary clean-up before the main change as
separate patch(es).

> @@ -1417,18 +1476,48 @@ int main(int argc, char **argv)
>  	}
>  
>  	/* write it to the imap server */
> +
> +#ifdef USE_CURL_FOR_IMAP_SEND
> +	if (!server.tunnel) {
> +		curl = setup_curl(&server);
> +		curl_easy_setopt(curl, CURLOPT_READDATA, &msgbuf);
> +	} else {
> +#endif
>  	ctx = imap_open_store(&server);
>  	if (!ctx) {
>  		fprintf(stderr, "failed to open store\n");
>  		return 1;
>  	}
> +	ctx->name = server.folder;
> +#ifdef USE_CURL_FOR_IMAP_SEND
> +	}
> +#endif
>  
>  	fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : "");
> -	ctx->name = imap_folder;
>  	while (1) {
>  		unsigned percent = n * 100 / total;
>  
>  		fprintf(stderr, "%4u%% (%d/%d) done\r", percent, n, total);
> +#ifdef USE_CURL_FOR_IMAP_SEND
> +		if (!server.tunnel) {
> ...
> +			}
> +		} else {
> +#endif
>  		if (!split_msg(&all_msgs, &msg, &ofs))
>  			break;
>  		if (server.use_html)
> @@ -1436,11 +1525,19 @@ int main(int argc, char **argv)
>  		r = imap_store_msg(ctx, &msg);
>  		if (r != DRV_OK)
>  			break;
> +#ifdef USE_CURL_FOR_IMAP_SEND
> +		}
> +#endif
>  		n++;
>  	}
>  	fprintf(stderr, "\n");
>  
> +#ifdef USE_CURL_FOR_IMAP_SEND
> +	curl_easy_cleanup(curl);
> +	curl_global_cleanup();
> +#else
>  	imap_close_store(ctx);
> +#endif
>  
>  	return 0;
>  }

Ugly.  Can we do this with less #ifdef/#else/#endif in the primary
code path?

If we were to keep these two modes as a choice the users have to
make at the compilation time, that is.

Thanks.