git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 27, 2014, 17:20 UTC
Message-ID
<xmqqoav5etnb.fsf@gitster.dls.corp.google.com>
In-Reply-To
<53FD0CD1.7030801@raz.or.at>
Bernhard Reiter <ockham@raz.or.at> writes:
Show 10 quoted lines
> 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.

https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions says that this was introduced as of 7.34.0, though.

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

Perhaps CC'ing those who have touched git-imap-send code over the years and asking for their help testing might help?

> Signed-off-by: Bernhard Reiter <ockham@raz.or.at>
> ---
> I rebased the patch on the pu branch, hope that was the right thing to do.

Usually I would appreciate a patch for a new feature not meant for the maintenance tracks to be based on 'master', so that it can go to the next release without having to wait other changes that may conflict with it and that may not yet be ready.

I will try to apply this one to 'pu', rebase it on 'master' to make sure the result does not depend on the other topics in flight, and then merge it back to 'pu'.

Thanks; some comments below.
Show 19 quoted lines
>  Documentation/git-imap-send.txt |   3 +-
>  INSTALL                         |  15 ++--
>  Makefile                        |  16 +++-
>  git.spec.in                     |   5 +-
>  imap-send.c                     | 165 +++++++++++++++++++++++++++++++++-------
>  5 files changed, 167 insertions(+), 37 deletions(-)
>
> diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
> index 7d991d9..9d244c4 100644
> --- a/Documentation/git-imap-send.txt
> +++ b/Documentation/git-imap-send.txt
> @@ -75,7 +75,8 @@ 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 you compiled git with the NO_CURL option or if your curl version is
> +	< 7.35.0, the only supported method is 'CRAM-MD5'. If this is not set
>  	then 'git imap-send' uses the basic IMAP plaintext LOGIN command.

Hmph, so there is no option that lets me say "I know my libcurl is new enough but I have some reason not to want to use the new code to interact with my imap server", at compile time or (more preferrably) at runtime?

Show 13 quoted lines
> diff --git a/INSTALL b/INSTALL
> index 6ec7a24..e2770a0 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.
> +	- "openssl" library is used by git-imap-send to use IMAP over SSL,
> +	  unless you're using curl >= 7.35.0, in which case that will be
> +	  used. If you don't need git-imap-send, you can use NO_OPENSSL.
The last sentence makes it unclear which of the following is true:
 - I have sufficiently new libcurl.  I cannot say NO_OPENSSL because
   I do need git-imap-send.
 - I have sufficiently new libcurl, so "openssl" is not used by
   git-imap send for me.  I can say NO_OPENSSL.
Perhaps
 - git-imap-send needs the OpenSSL library to talk IMAP over SSL if
   you are using libCurl older than 7.35.0.  Otherwise you can use
   NO_OPENSSL without losing git-imap-send.
Show 10 quoted lines
> diff --git a/git.spec.in b/git.spec.in
> index d61d537..9535cc3 100644
> --- a/git.spec.in
> +++ b/git.spec.in
> @@ -8,7 +8,7 @@ License: 	GPL
>  Group: 		Development/Tools
>  URL: 		http://kernel.org/pub/software/scm/git/
>  Source: 	http://kernel.org/pub/software/scm/git/%{name}-%{version}.tar.gz
> -BuildRequires:	zlib-devel >= 1.2, openssl-devel, curl-devel, expat-devel, gettext  %{!?_without_docs:, xmlto, asciidoc > 6.0.3}
> +BuildRequires:	zlib-devel >= 1.2, openssl-devel, curl-devel >= 7.35.0, expat-devel, gettext  %{!?_without_docs:, xmlto, asciidoc > 6.0.3}

This is very iffy. It incompatible with the body of the patch where you allow older curl library and because you depend on openssl-devel you wouldn't lose imap-send.

Show 37 quoted lines
> @@ -1391,29 +1518,13 @@ 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");
>  
> -	imap_close_store(ctx);
> +	if (server.tunnel)
> +		return append_msgs_to_imap(&server, &all_msgs, total);
>  
> -	return 0;
> +#ifndef USE_CURL_FOR_IMAP_SEND
> +	return append_msgs_to_imap(&server, &all_msgs, total);
> +#else
> +	return curl_append_msgs_to_imap(&server, &all_msgs, total);
> +#endif
>  }

Much more nicely done. It appears that you could already turn these #ifndef/#else/#endif into a runtime conditional, allowing:

 - At compile-time, can be built with the four combinations
   (1) USE_CURL_FOR_IMAP_SEND=Yes    NO_OPENSSL=No
   (2) USE_CURL_FOR_IMAP_SEND=Yes    NO_OPENSSL=Yes
   (3) USE_CURL_FOR_IMAP_SEND=No     NO_OPENSSL=No
   (4) USE_CURL_FOR_IMAP_SEND=No     NO_OPENSSL=Yes
 - The first two variants can support --with-curl/--without-curl and
   choose between curl_append/append.  When run --without-curl, it
   may lose some auth-methods and for variant (1) SSL is not
   supported.
or am I mis-reading the patch?
Previous: Bernhard ReiterNext: Bernhard Reiter
Message 2 of 12 in “git-imap-send: use libcurl for implementation”
  1. git-imap-send: use libcurl for implementationBernhard Reiter, Aug 26, 2014
  2. Junio C HamanoAug 27, 2014
  3. Bernhard ReiterOct 12, 2014
  4. Fwd: Re: [PATCH] git-imap-send: use libcurl for implementationBernhard Reiter, Oct 22, 2014
  5. Junio C HamanoOct 22, 2014
  6. git-imap-send: use libcurl for implementationBernhard Reiter, Oct 29, 2014
  7. Junio C HamanoOct 29, 2014
  8. Torsten BögershausenNov 9, 2014
  9. Bernhard ReiterNov 9, 2014
  10. Ramsay JonesNov 9, 2014
  11. Junio C HamanoNov 9, 2014
  12. Junio C HamanoNov 10, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.