git-imap-send: use libcurl for implementation

PATCH In review Bernhard Reiter 4 participants 12 messages Jun 15, 2016

Subsystems (2)

Documentation Kbuild

lore

Share & tools

lore

Bernhard Reiter

author

2016-06-15 23:02 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

---
I rebased the patch on the pu branch, hope that was the right thing to do.

Bernhard

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

Documentation/git-imap-send.txt +2 -1

index 7d991d9..9d244c4 100644
@@ -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.
 
 Examples

INSTALL +9 -6

index 6ec7a24..e2770a0 100644
@@ -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.
 
 	  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.35.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

Makefile +14 -2

index 237bc05..c08963c 100644
@@ -1133,6 +1133,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 =

@@ -1167,6 +1170,15 @@ else
 			PROGRAM_OBJS += http-push.o
 		endif
 	endif
+	curl_check := $(shell (echo 072300; curl-config --vernum) 2>/dev/null | sort -r | sed -ne 2p)
+	ifeq "$(curl_check)" "072300"
+		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

@@ -2084,9 +2096,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,$^) \

git.spec.in +4 -1

index d61d537..9535cc3 100644
@@ -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}
 BuildRoot:	%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 
 Requires:	perl-Git = %{version}-%{release}

@@ -214,6 +214,9 @@ rm -rf $RPM_BUILD_ROOT
 # No files for you!
 
 %changelog
+* Mon Aug 11 2014 Bernhard Reiter <[email protected]>
+- Require version >= 7.35.0 of curl-devel for IMAP functions.
+
 * Sun Sep 18 2011 Jakub Narebski <[email protected]>
 - Add gitweb manpages to 'gitweb' subpackage

imap-send.c +138 ~~-27~~

187 lines not on this page · raw message

Signature · 1 line

2.1.0.372.g32d59d1.dirty

Junio C Hamano

2016-06-15 23:02 UTC #1 lore raw

Quoted 10 lines · Bernhard Reiter

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.

Quoted 4 lines

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?

Quoted 3 lines

Signed-off-by Bernhard Reiter <[email protected]>
---
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.

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

Documentation/git-imap-send.txt +0 -0

index 7d991d9..9d244c4 100644

Documentation/git-imap-send.txt imap.preformattedHTML:: · line 80

Quoted 7 lines

Documentation/git-imap-send.txt +2 -1

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

INSTALL Issues of note: · line 113

Quoted 13 lines

INSTALL +3 -2

index 6ec7a24..e2770a0 100644
@@ -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.

git.spec.in License: GPL · line 11

Quoted 10 lines

git.spec.in +1 -1

index d61d537..9535cc3 100644
@@ -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.

Quoted 27 lines

git.spec.in +0 ~~-22~~

@@ -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);

git.spec.in int main(int argc, char **argv) · line 1530

Quoted 10 lines

git.spec.in +7 -1

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

Bernhard Reiter

author

2016-06-15 23:02 UTC #2 lore raw

Sorry for not getting back to this any sooner, I've been pretty busy recently with Other Projects(tm).

Am 2014-08-27 um 19:20 schrieb Junio C Hamano:

Quoted 10 lines

Quoted 5 lines · Bernhard Reiter
[...] 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.

Strange, I thought I recalled having seen that in
http://curl.haxx.se/libcurl/c/CURLOPT_LOGIN_OPTIONS.html but it clearly says 7.34.0 there too. I've now changed all occurrences of 7.35.0 to 7.34.0 (and the corresponding hex value in the Makefile).

Quoted 7 lines

Quoted 4 lines
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?

CC'ing them (going back about 2 years, which already makes the list quite long) and the people who have taken part in the initial discussion on this feature in August. And the related Debian bug.

Please test this, folks!

Quoted 4 lines

Quoted 3 lines
Signed-off-by Bernhard Reiter <[email protected]>
---
I rebased the patch on the pu branch, hope that was the right thing to do.

Quoted 8 lines

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

Okay, I'll stick to master. I've rebased on master now that the first couple related patches are there anyway.

Quoted 8 lines

[...]
Quoted 7 lines
Documentation/git-imap-send.txt +0 -0
index 7d991d9..9d244c4 100644
@@ -75,7 +75,8 @@ imap.preformattedHTML::
 

Quoted 11 lines

Documentation/git-imap-send.txt imap.preformattedHTML:: · line 80
Quoted 6 lines
Documentation/git-imap-send.txt +2 -1
 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?

Added a runtime option, see below.

Quoted 19 lines

INSTALL Issues of note: · line 113

Quoted 13 lines

INSTALL +3 -2

index 6ec7a24..e2770a0 100644
@@ -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.

Quoted 8 lines

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

Fixed.

Quoted 6 lines

git.spec.in +0 -0

index d61d537..9535cc3 100644
@@ -8,7 +8,7 @@ License: 	GPL
 Group: 		Development/Tools

Quoted 8 lines

git.spec.in License: GPL · line 11
Quoted 4 lines
git.spec.in +1 -1
 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.

Okay, removed the version requirement.

Quoted 13 lines

git.spec.in int main(int argc, char **argv)
Quoted 2 lines
git.spec.in +0 -0
@@ -1391,29 +1518,13 @@ int main(int argc, char **argv)
[...]
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

Quoted 6 lines

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

You're reading it correctly; thanks for the hint about the runtime switch.

I've now implemented it using parse_options, so the switch is called --[no-]curl. I hope parse_options isn't overkill, but as I wanted to keep other options -- namely verbose and quiet switches, useful for debugging curl -- I would've had to look through all argv[] items manually, so I figured it made sense to us an already existing API for that instead.

Bernhard

Quoted 1 line

From 218c47580330d5ac38875c30e14dc3049d1ce3c5 Mon Sep 17 00:00:00 2001

From: Bernhard Reiter <[email protected]>
Date: Wed, 13 Aug 2014 23:41:40 +0200
Subject: [PATCH] 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.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.

Signed-off-by Bernhard Reiter <[email protected]>

---
 Documentation/git-imap-send.txt |  26 +++++-
 INSTALL                         |  15 ++--
 Makefile                        |  16 +++-
 imap-send.c                     | 189 +++++++++++++++++++++++++++++++++-------
 4 files changed, 204 insertions(+), 42 deletions(-)

Documentation/git-imap-send.txt +24 -2

index 7d991d9..1c24e1f 100644
@@ -9,7 +9,7 @@ git-imap-send - Send a collection of patches from stdin to an IMAP folder
 SYNOPSIS
 --------
 [verse]
-'git imap-send'
+'git imap-send' [-v] [-q] --[no-]curl
 
 
 DESCRIPTION

@@ -25,6 +25,26 @@ Typical usage is something like:
 
 git format-patch --signoff --stdout --attach origin | git imap-send
 
+OPTIONS
+-------
+-v::
+--verbose::
+	Be verbose.
+
+-q::
+--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.
 
 CONFIGURATION
 -------------

@@ -75,7 +95,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
+	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

INSTALL +9 -6

index 6ec7a24..ffb071e 100644
@@ -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

Makefile +14 -2

index f34a2d4..69b2fbf 100644
@@ -992,6 +992,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 =

@@ -1026,6 +1029,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

@@ -1892,9 +1904,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,$^) \

imap-send.c +157 ~~-32~~

227 lines not on this page · raw message

Signature · 1 line

2.1.2.375.g218c475

Bernhard Reiter

author

2016-06-15 23:02 UTC #3 lore raw

*ping*
Hope I didn't mess up formatting again...
Or do I need to top-post, as the original thread is too old to keep posting to it?

Bernhard

-------- Weitergeleitete Nachricht --------
Betreff: Re: [PATCH] git-imap-send: use libcurl for implementation
Datum: Sun, 12 Oct 2014 17:22:20 +0200
Von: Bernhard Reiter <[email protected]>
An: Junio C Hamano <[email protected]>
Kopie (CC): [email protected], Jonathan Nieder <[email protected]>, Jeff King <[email protected]>, [email protected], René Scharfe <[email protected]>, Tony Finch <[email protected]>, Tanay Abhra <[email protected]>, Dan Albert <[email protected]>, Jeremy Huddleston <[email protected]>, David Aguilar <[email protected]>, Michael Haggerty <[email protected]>, Oswald Buddenhagen <[email protected]>

Sorry for not getting back to this any sooner, I've been pretty busy recently with Other Projects(tm).

Am 2014-08-27 um 19:20 schrieb Junio C Hamano:

Quoted 10 lines

Quoted 5 lines · Bernhard Reiter
[...] 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.

Quoted 7 lines

Quoted 4 lines
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?

CC'ing them (going back about 2 years, which already makes the list quite long) and the people who have taken part in the initial discussion on this feature in August. And the related Debian bug.

Please test this, folks!

Quoted 4 lines

Quoted 3 lines
Signed-off-by Bernhard Reiter <[email protected]>
---
I rebased the patch on the pu branch, hope that was the right thing to do.

Quoted 8 lines

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

Okay, I'll stick to master. I've rebased on master now that the first couple related patches are there anyway.

Quoted 8 lines

[...]
Quoted 7 lines
Documentation/git-imap-send.txt +0 -0
index 7d991d9..9d244c4 100644
@@ -75,7 +75,8 @@ imap.preformattedHTML::
 

Quoted 11 lines

Documentation/git-imap-send.txt imap.preformattedHTML:: · line 80
Quoted 6 lines
Documentation/git-imap-send.txt +2 -1
 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?

Added a runtime option, see below.

Quoted 19 lines

INSTALL Issues of note: · line 113

Quoted 13 lines

INSTALL +3 -2

index 6ec7a24..e2770a0 100644
@@ -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.

Quoted 8 lines

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

Fixed.

Quoted 6 lines

git.spec.in +0 -0

index d61d537..9535cc3 100644
@@ -8,7 +8,7 @@ License: 	GPL
 Group: 		Development/Tools

Quoted 8 lines

git.spec.in License: GPL · line 11
Quoted 4 lines
git.spec.in +1 -1
 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.

Okay, removed the version requirement.

Quoted 13 lines

git.spec.in int main(int argc, char **argv)
Quoted 2 lines
git.spec.in +0 -0
@@ -1391,29 +1518,13 @@ int main(int argc, char **argv)
[...]
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

Quoted 6 lines

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

You're reading it correctly; thanks for the hint about the runtime switch.

Bernhard

Quoted 1 line

From 218c47580330d5ac38875c30e14dc3049d1ce3c5 Mon Sep 17 00:00:00 2001

From: Bernhard Reiter <[email protected]>
Date: Wed, 13 Aug 2014 23:41:40 +0200
Subject: [PATCH] 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.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.

Signed-off-by Bernhard Reiter <[email protected]>

---
 Documentation/git-imap-send.txt |  26 +++++-
 INSTALL                         |  15 ++--
 Makefile                        |  16 +++-
 imap-send.c                     | 189 +++++++++++++++++++++++++++++++++-------
 4 files changed, 204 insertions(+), 42 deletions(-)

Documentation/git-imap-send.txt +24 -2

index 7d991d9..1c24e1f 100644
@@ -9,7 +9,7 @@ git-imap-send - Send a collection of patches from stdin to an IMAP folder
 SYNOPSIS
 --------
 [verse]
-'git imap-send'
+'git imap-send' [-v] [-q] --[no-]curl
 
 
 DESCRIPTION

@@ -25,6 +25,26 @@ Typical usage is something like:
 
 git format-patch --signoff --stdout --attach origin | git imap-send
 
+OPTIONS
+-------
+-v::
+--verbose::
+	Be verbose.
+
+-q::
+--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.
 
 CONFIGURATION
 -------------

@@ -75,7 +95,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
+	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

INSTALL +9 -6

index 6ec7a24..ffb071e 100644
@@ -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

Makefile +14 -2

index f34a2d4..69b2fbf 100644
@@ -992,6 +992,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 =

@@ -1026,6 +1029,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

@@ -1892,9 +1904,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,$^) \

imap-send.c +157 ~~-32~~

227 lines not on this page · raw message

Signature · 1 line

2.1.2.375.g218c475

Junio C Hamano

2016-06-15 23:02 UTC #4 lore raw

Quoted 3 lines · Bernhard Reiter

*ping*
Hope I didn't mess up formatting again...
Or do I need to top-post, as the original thread is too old to keep posting to it?

Please avoid top-posting on this list.

If you have some background material (e.g. summary of previous discussions, list of things that have been changed in response to previous review, etc.), you have three options:

* Use a cover letter, even for a one-patch topic. Use [PATCH 0/1] for the background material if you have tons of it and then make [PATCH 1/1] as a follow-up to that cover letter.

See http://thread.gmane.org/gmane.comp.version-control.git/256386
for how such an approach looks like.

* Use a single message, [PATCH] (aka [PATCH 1/1]), and add your background material _after_ three-dash lines as comment.

See http://thread.gmane.org/gmane.comp.version-control.git/258451
for how such an approach looks like.

* Use a single message, [PATCH] (aka [PATCH 1/1]), and add your background material _before_ a scissors line "-- >8 --", optionally followed by "From:" and/or "Subject:" (but in your case, you are sending your own patch as yourself with the same subject, so neither of them apply).

See http://thread.gmane.org/gmane.comp.version-control.git/258470/focus=258474
for how such an approach looks like.

Thanks.

Bernhard Reiter

author

2016-06-15 23:02 UTC #5 lore raw

PATCH 1/1 git-imap-send: use libcurl for implementation

In reply to Junio C Hamano

Resending this once more, as indicated by <[email protected]> Hope my formatting and posting style is now conformant. Sorry for the noise.

Am 2014-08-27 um 19:20 schrieb Junio C Hamano:

Quoted 10 lines

Quoted 5 lines · Bernhard Reiter
[...] 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.

Quoted 7 lines

Quoted 4 lines
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?

CC'ing them (going back about 2 years, which already makes the list quite long) and the people who have taken part in the initial discussion on this feature in August. And the related Debian bug.

Please test this, folks!

Quoted 4 lines

Quoted 3 lines
Signed-off-by Bernhard Reiter <[email protected]>
---
I rebased the patch on the pu branch, hope that was the right thing to do.

Quoted 8 lines

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

Okay, I'll stick to master. I've rebased on master now that the first couple related patches are there anyway.

Quoted 8 lines

[...]
Quoted 7 lines
Documentation/git-imap-send.txt +0 -0
index 7d991d9..9d244c4 100644
@@ -75,7 +75,8 @@ imap.preformattedHTML::
 

Quoted 11 lines

Documentation/git-imap-send.txt imap.preformattedHTML:: · line 80
Quoted 6 lines
Documentation/git-imap-send.txt +2 -1
 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?

Added a runtime option, see below.

Quoted 19 lines

INSTALL Issues of note: · line 113

Quoted 13 lines

INSTALL +3 -2

index 6ec7a24..e2770a0 100644
@@ -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.

Quoted 8 lines

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

Fixed.

Quoted 6 lines

git.spec.in +0 -0

index d61d537..9535cc3 100644
@@ -8,7 +8,7 @@ License: 	GPL
 Group: 		Development/Tools

Quoted 8 lines

git.spec.in License: GPL · line 11
Quoted 4 lines
git.spec.in +1 -1
 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.

Okay, removed the version requirement.

Quoted 13 lines

git.spec.in int main(int argc, char **argv)
Quoted 2 lines
git.spec.in +0 -0
@@ -1391,29 +1518,13 @@ int main(int argc, char **argv)
[...]
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

Quoted 6 lines

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

You're reading it correctly; thanks for the hint about the runtime switch.

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.

Signed-off-by Bernhard Reiter <[email protected]>

---
 Documentation/git-imap-send.txt |  26 +++++-
 INSTALL                         |  15 ++--
 Makefile                        |  16 +++-
 imap-send.c                     | 189 +++++++++++++++++++++++++++++++++-------
 4 files changed, 204 insertions(+), 42 deletions(-)

Documentation/git-imap-send.txt +23 -2

index 7d991d9..1c24e1f 100644
@@ -9,7 +9,7 @@ git-imap-send - Send a collection of patches from stdin to an IMAP folder
 SYNOPSIS
 --------
 [verse]
-'git imap-send'
+'git imap-send' [-v] [-q] --[no-]curl
   DESCRIPTION

@@ -25,6 +25,26 @@ Typical usage is something like:
  git format-patch --signoff --stdout --attach origin | git imap-send
 +OPTIONS
+-------
+-v::
+--verbose::
+	Be verbose.
+
+-q::
+--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.
  CONFIGURATION
 -------------

@@ -75,7 +95,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
+	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

INSTALL +9 -4

index 6ec7a24..ffb071e 100644
@@ -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

Makefile +13 -1

index f34a2d4..69b2fbf 100644
@@ -992,6 +992,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 =

@@ -1026,6 +1029,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

@@ -1892,9 +1904,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,$^) \

imap-send.c +157 ~~-25~~

216 lines not on this page · raw message

Signature · 1 line

2.1.2.375.g218c475

Junio C Hamano

2016-06-15 23:02 UTC #6 lore raw

Quoted 2 lines · Bernhard Reiter

Resending this once more, as indicated by <[email protected]> Hope my formatting and posting style is now conformant. Sorry for the noise.

Thanks.

The patch does not apply for me (please send a trial message to yourself and then try to apply it out of your mailbox to avoid such an incident in the future), unfortunately. I'll comment on the patch from just code inspection without applying it, though.

Quoted 11 lines

Documentation/git-imap-send.txt +1 -1

index 7d991d9..1c24e1f 100644
@@ -9,7 +9,7 @@ git-imap-send - Send a collection of patches from stdin to an IMAP folder
 SYNOPSIS
 --------
 [verse]
-'git imap-send'
+'git imap-send' [-v] [-q] --[no-]curl
   DESCRIPTION

Quoted 1 line

@@ ...

The hunk header says that the original and the updated both starts at line #9 and they both should have 7 lines, but I count only 5 lines. Where did you lose two lines of the post-context?

Makefile ifdef HAVE_ALLOCA_H · line 996

Quoted 10 lines

Makefile +2 -0

index f34a2d4..69b2fbf 100644
@@ -992,6 +992,9 @@ ifdef HAVE_ALLOCA_H
 	BASIC_CFLAGS += -DHAVE_ALLOCA_H
 endif
 +IMAP_SEND_BUILDDEPS =
+IMAP_SEND_LDFLAGS = $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
+

Here the patch looks funny as well. I guess "IMAP_SEND_BUILDDEPS =" is an added line prefixed with "+", but where does the SP before the plus sign come from?

I won't point out other patch breakages in the remainder of this message.

Quoted 7 lines

imap-send.c +0 -0

index 70bcc7a..9cc80ae 100644
@@ -26,11 +26,31 @@
 #include "credential.h"
 #include "exec_cmd.h"

imap-send.c line 37

Quoted 10 lines

imap-send.c +5 -0

 #include "run-command.h"
+#include "parse-options.h"
 #ifdef NO_OPENSSL
 typedef void *SSL;
 #endif
+#ifdef USE_CURL_FOR_IMAP_SEND
+#include "http.h"
+#endif
 -static const char imap_send_usage[] = "git imap-send < <mbox>";
+static int Verbose, Quiet;

Yikes. I know this is not a new problem, but please do not name variables Capitalized. Also what would it mean to set both Verbose and Quiet at the same time?

I think we would want to use OPT__VERBOSITY with a single variable "verbosity" (see hits from "git grep OPT__VERBOSITY" for examples). Such a change should probably come before this change to use libcurl as a separate preparatory clean-up patch.

imap-send.c line 42

Quoted 5 lines

imap-send.c +5 -0

+#ifdef USE_CURL_FOR_IMAP_SEND
+static int use_curl = 1; // on by default; set later by parse_options
+#else
+static int use_curl = 0; // not available
+#endif

Please don't use // comments. Besides, I think this should be initialized to -1 to mean "unspecified by the user" in both cases (i.e. no need for #ifdef/#endif).

imap-send.c line 49

Quoted 4 lines

imap-send.c +4 -0

+static struct option imap_send_options[] = {
+#ifdef USE_CURL_FOR_IMAP_SEND
+	OPT_BOOL(0, "curl", &use_curl, "use libcurl to communicate with the IMAP server"),
+#endif

And lose the ifdef here (i.e. take "--use-curl" even on a build that does not support it).

imap-send.c static void git_imap_config(void) · line 1498

Quoted 13 lines

imap-send.c +6 -1

+int main(int argc, char **argv)
+{
+	struct strbuf all_msgs = STRBUF_INIT;
+	int total;
 	int nongit_ok;
  	git_extract_argv0_path(argv[0]);
 -	git_setup_gettext();
+	argc = parse_options(argc, (const char **)argv, "", imap_send_options, imap_send_usage, 0);
 -	if (argc != 1)
-		usage(imap_send_usage);
+	git_setup_gettext();
  	setup_git_directory_gently(&nongit_ok);
 	git_imap_config();

Usually we read config and then parse options, so that people can set configuration variables for their usual use pattern and override what is configured from the command line option as needed.

For example, you may want to add

	[imap]
        	useCurl = true

in the configuration file and run "git imap-send" without "--curl" option on the command line almost always, but in some specific occasion, "git imap-send --no-curl" to countermand the configuration.

Was there a specific reason why you had to add parse_options() before git_imap_config()?

With "use_curl" initialized to "-1 (unspecified)", the final code structure may look more like this:

		...
                git_imap_config();
                parse_options();

        #ifndef USE_CURL_FOR_IMAP_SEND
                if (use_curl < 0)
                        use_curl = 0;
                if (use_curl) {
                        warning("--use-curl not supported in this build");
                        use_curl = 0;
                }
        #else
                if (use_curl < 0) /* default to enable libcurl */
                        use_curl = 1;
        #endif

although I think it is also OK to make this feature strictly optional by defaulting to "use_curl = 0" in the #else part above. Initializing the static variable to 0 would make the result even shorter if we were to go that route, i.e.

	static int use_curl; /* strictly opt in */
        ...
	main()
        {
		...
		git_imap_config();
	        parse_options();
	#ifndef USE_CURL_FOR_IMAP_SEND
        	if (use_curl) {
			warning(...);
			use_curl = 0;
		}
	#endif

Torsten Bögershausen

2016-06-15 23:02 UTC #7 lore raw

In reply to Bernhard Reiter

Quoted 3 lines · Bernhard Reiter

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'

Some more info:
curl-config --vernum
071a00

(echo 072200; curl-config --vernum) 2>/dev/null | sort -r
072200
071a00

(echo 072200; curl-config --vernum) 2>/dev/null | sort -r | sed -ne 2p 071a00

Bernhard Reiter

author

2016-06-15 23:02 UTC #8 lore raw

Am 2014-11-09 um 14:00 schrieb Torsten Bögershausen:

Quoted 6 lines · Bernhard Reiter

Quoted 3 lines
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.

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.

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.

Signed-off-by Bernhard Reiter <[email protected]>

Signed-off-by Ramsay Jones <[email protected]>

---
 Documentation/git-imap-send.txt |  15 +++-
 INSTALL                         |  15 ++--
 Makefile                        |  18 +++-
 imap-send.c                     | 176 ++++++++++++++++++++++++++++++++++------
 4 files changed, 187 insertions(+), 37 deletions(-)

Documentation/git-imap-send.txt +13 -2

index 0897131..77aacf1 100644
@@ -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

INSTALL +9 -6

index 6ec7a24..ffb071e 100644
@@ -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

Makefile +15 -3

index 827006b..14d5ac1 100644
@@ -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,$^) \

imap-send.c +150 ~~-26~~

212 lines not on this page · raw message

Signature · 1 line

2.2.0.rc0.4.g1aeb70f.dirty

Ramsay Jones

2016-06-15 23:02 UTC #9 lore raw

Quoted 11 lines · Bernhard Reiter

Am 2014-11-09 um 14:00 schrieb Torsten Bögershausen:
Quoted 6 lines · Bernhard Reiter
Quoted 3 lines
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. :(

Quoted 21 lines

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.

Quoted 5 lines

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

Quoted 3 lines

Signed-off-by Bernhard Reiter <[email protected]>
Signed-off-by Ramsay Jones <[email protected]>

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

Quoted 351 lines

---
 Documentation/git-imap-send.txt |  15 +++-
 INSTALL                         |  15 ++--
 Makefile                        |  18 +++-
 imap-send.c                     | 176 ++++++++++++++++++++++++++++++++++------
 4 files changed, 187 insertions(+), 37 deletions(-)

Documentation/git-imap-send.txt +13 -2

index 0897131..77aacf1 100644
@@ -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

INSTALL +9 -6

index 6ec7a24..ffb071e 100644
@@ -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

Makefile +15 -3

index 827006b..14d5ac1 100644
@@ -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,$^) \

imap-send.c +150 ~~-26~~

index 7f9d30e..08271d9 100644
@@ -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);
 }

Junio C Hamano

2016-06-15 23:02 UTC #10 lore raw

Quoted 8 lines · Ramsay Jones

Quoted 5 lines
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. ;-)

Use smaller line pitch, then? ;-) I am sure myself 6 months down the road as a "git show" reader of this patch would appreciate these five lines when I scratch my head looking at the Makefile changes.

Quoted 7 lines

Quoted 2 lines
Signed-off-by Bernhard Reiter <[email protected]>
Signed-off-by Ramsay Jones <[email protected]>
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.

Yes, that is the appropriate thing to do, I would think.

Thanks.

Junio C Hamano

2016-06-15 23:02 UTC #11 lore raw

In reply to Bernhard Reiter

Quoted 5 lines · Bernhard Reiter

Am 2014-11-09 um 14:00 schrieb Torsten Bögershausen:
Quoted 4 lines · Bernhard Reiter
Quoted 3 lines
Use libcurl's high-level API functions to implement git-imap-send instead of the previous low-level OpenSSL-based functions.

Quoted 8 lines

Quoted 3 lines
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.

Queued with a small fix-ups, including

- line-fold a couple of overlong lines;

- avoid decl-after-stmt of "int prev_len";

- reduce the scope of "struct struct auth" down to only the block it is used;

- the footer of the log message now reads "helped-by ramsay", your sign-off and then mine.

Thanks.

imap-send.c +10 -6

index 08271d9..4dfe4c2 100644
@@ -1349,7 +1349,8 @@ static void git_imap_config(void)
 	git_config_get_string("imap.authmethod", &server.auth_method);
 }
 
-static int append_msgs_to_imap(struct imap_server_conf *server, struct strbuf* all_msgs, int total)
+static int append_msgs_to_imap(struct imap_server_conf *server,
+			       struct strbuf* all_msgs, int total)
 {
 	struct strbuf msg = STRBUF_INIT;
 	struct imap_store *ctx = NULL;

@@ -1391,7 +1392,6 @@ 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");

@@ -1414,11 +1414,12 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
 	curl_easy_setopt(curl, CURLOPT_PORT, server.port);
 
 	if (server.auth_method) {
+		struct strbuf auth = STRBUF_INIT;
 		strbuf_addstr(&auth, "AUTH=");
 		strbuf_addstr(&auth, server.auth_method);
 		curl_easy_setopt(curl, CURLOPT_LOGIN_OPTIONS, auth.buf);
+		strbuf_release(&auth);
 	}
-	strbuf_release(&auth);
 
 	if (server.use_ssl)
 		curl_easy_setopt(curl, CURLOPT_USE_SSL, (long)CURLUSESSL_ALL);

@@ -1436,7 +1437,8 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
 	return curl;
 }
 
-static int curl_append_msgs_to_imap(struct imap_server_conf *server, struct strbuf* all_msgs, int total) {
+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 };

@@ -1449,17 +1451,19 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server, struct strb
 	fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : "");
 	while (1) {
 		unsigned percent = n * 100 / total;
+		int prev_len;
 
 		fprintf(stderr, "%4u%% (%d/%d) done\r", percent, n, total);
 
-		int prev_len = msgbuf.buf.len;
+		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));
+		curl_easy_setopt(curl, CURLOPT_INFILESIZE_LARGE,
+				 (curl_off_t)(msgbuf.buf.len-prev_len));
 
 		res = curl_easy_perform(curl);