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

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 <redacted>
---
Am 2014-08-13 um 03:59 schrieb Jonathan Nieder:

Bernhard Reiter wrote:

quoted

[...]

Wow!  This sounds lovely.  Thanks for working on this.

Well thanks for the friendly welcome and the helpful comments!

I'm attaching a patch where I've applied the fixes you suggested, plus:

* I added the lf_to_crlf conversion to the curl codepath as
communication with another IMAP server I tried was broken without it.

* I added STARTTLS. (That's just the
curl_easy_setopt(curl, CURLOPT_USE_SSL, (long)CURLUSESSL_ALL);
line)

* I tested (and fixed) authentication, i.e. the auth_method stuff. As
the corresponding CURLOPT_LOGIN_OPTIONS flag has only been available
starting with curl 7.35.0, I've bumped the required version to that.
(Apparently it was possible to achieve the same effect with a different
option in between versions 7.31.0 and 7.34.0 [1], but I haven't found
yet how. Is it worth the effort?)

* I made that file scope imap_folder a member of struct imap_server_conf
(named folder), which makes some things easier.

quoted

@@ -1417,31 +269,89 @@ int main(int argc, char **argv)
 		return 1;
 	}

+	curl_global_init(CURL_GLOBAL_ALL);

http.c seems to make the same mistake,

Patch at http://permalink.gmane.org/gmane.comp.version-control.git/255221

[...]

quoted

+	if (server.tunnel) {
+		const char *argv[] = { server.tunnel, NULL };
+		struct child_process tunnel = {NULL};

(not about this patch) Could use the child_proccess's internal
argv_array:

		struct child_process tunnel = {NULL};
		argv_array_push(&tunnel.args, server.tunnel);

Patch at
http://permalink.gmane.org/gmane.comp.version-control.git/255220 (The
patch attached to this mail depends on that one.)

No comments on those patches yet, though.

(about this patch) Would there be a way to make this part reuse the
existing code?  The only difference I see is that *srvc has been
renamed to server, which doesn't seem to be related to the change of
transport API from OpenSSL to libcurl.

[...]

quoted

+		curl_socket_t sockfd = tunnel.out; // what about tunnel.in ?

Hmm.  curl expects to get a socket it can send(), recv(), setsockopt(),
etc on instead of a pair of fds to read() and write().

I wonder why someone would want to use SSL through a tunnel, though.
Currently it's impossible to get to the SSL codepath when a tunnel
is active (it's in the 'else' block an 'if (srvc->tunnel)').  If that
property is preserved, then we should be safe.

Now this turns out to be the one major annoyance left, because we only
have those two fds (actually pipes, right?), and not a socket that we
could pass to curl, so we can't use it to talk to the IMAP server. So if
the tunnel parameter is set, we're stuck with the old hand-written IMAP
handling routines, even with USE_CURL_FOR_IMAP set, meaning I can't wrap
as much in #ifdef...#endif blocks as I'd like. :-( BTW, due to two of
the blocks that I do add I get a compiler warning about the curl handle
remaining possibly unitialized :-/
I've removed the curl specific socket handling routines, as we can't use
them anyway for now.

I've asked about passing two pipes instead of a socket to curl on their
ML [1] as this has even been discussed before [2], but unfortunately,
there doesn't seem to be a solution as of yet. I've also asked on SO
[3], but no answers yet.

To summarize:
[...]

 * As soon as you're ready to roll this out to a wider audience of
   testers, let me know, and we can try to get it into shape for
   Junio's "next" branch (and hence Debian experimental).

Is this one good enough already?

Bernhard

[1] http://sourceforge.net/p/curl/bugs/1372/
[2] http://curl.haxx.se/mail/lib-2014-08/0102.html
[3] http://curl.haxx.se/mail/lib-2011-05/0102.html
[4]
http://stackoverflow.com/questions/25306264/connect-in-and-out-pipes-to-network-socket

 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(-)