diff mbox series

imap-send: replace auto-probe libcurl with hard dependency

Message ID patch-1.1-3bea1312322-20230201T225915Z-avarab@gmail.com (mailing list archive)
State Superseded
Headers show
Series imap-send: replace auto-probe libcurl with hard dependency | expand

Commit Message

Ævar Arnfjörð Bjarmason Feb. 1, 2023, 11:04 p.m. UTC 
Change the "imap-send" command to have a hard dependency on libcurl,
before this it had an optional dependency on both libcurl and OpenSSL,
now only the OpenSSL dependency is optional.

This simplifies our dependency matrix my getting rid of yet another
special-case. Given the prevalence of libcurl and portability of
libcurl it seems reasonable to say that "git imap-send" cannot be used
without libcurl, almost everyone building git needs to be able to push
or pull over http(s), so they'll be building with libcurl already.

So let's remove the previous "USE_CURL_FOR_IMAP_SEND" knob. Whether we
build git-imap-send or not is now controlled by the "NO_CURL"
knob. Let's also hide the old --curl and --no-curl options, and die if
"--no-curl" is provided.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Wed, Feb 01 2023, Jiang Xin wrote:

> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> [...]

I don't have any issue per-se with your proposed change, but perhaps
we can go a step further here. I've had this as part of my local build
for about a year, but never got around to submitting it.

As argued above I think we should just make curl a hard dependency for
"git-imap-send", and that furthermore it's OK to require a newer
version of curl for that utility than we do in general (as the http(s)
transports are more widely required, git-imap-send is more obscure).

 Documentation/git-imap-send.txt | 10 ---------
 INSTALL                         |  8 ++++----
 Makefile                        | 18 +++++------------
 imap-send.c                     | 36 +++++----------------------------
 4 files changed, 14 insertions(+), 58 deletions(-)

Comments

Junio C Hamano Feb. 1, 2023, 11:22 p.m. UTC | #1 
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the "imap-send" command to have a hard dependency on libcurl,
> before this it had an optional dependency on both libcurl and OpenSSL,
> now only the OpenSSL dependency is optional.
>
> This simplifies our dependency matrix my getting rid of yet another

"my" -> "by", I think.

> special-case. Given the prevalence of libcurl and portability of
> libcurl it seems reasonable to say that "git imap-send" cannot be used
> without libcurl, almost everyone building git needs to be able to push
> or pull over http(s), so they'll be building with libcurl already.

OK.

> So let's remove the previous "USE_CURL_FOR_IMAP_SEND" knob. Whether we
> build git-imap-send or not is now controlled by the "NO_CURL"
> knob.

OK.

> Let's also hide the old --curl and --no-curl options, and die if
> "--no-curl" is provided.

In other words, if we are building imap-send, we sure know cURL is
there, and there is no need to tell a running imap-send not to use
cURL to talk to the IMAP service?  I am not sure the linkage of this
change with the rest of the patch.  Isn't that a totally orthogonal
issue?  Your imap-send might be cURL enabled, but unless we stop to
ship with our own IMAP routines compiled into imap-send, --no-curl
does have a purpose.

Or did you just forget to document that we stop to ship with our own
IMAP routines in the above?  If so, as long as it is made a bit more
prominent in the proposed log message in a reroll, I would be happy
with such a change rolled into the same patch.
Ævar Arnfjörð Bjarmason Feb. 1, 2023, 11:56 p.m. UTC | #2 
On Wed, Feb 01 2023, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change the "imap-send" command to have a hard dependency on libcurl,
>> before this it had an optional dependency on both libcurl and OpenSSL,
>> now only the OpenSSL dependency is optional.
>>
>> This simplifies our dependency matrix my getting rid of yet another
>
> "my" -> "by", I think.

Thanks, I'll fix that in a re-roll, pending the below...

>> special-case. Given the prevalence of libcurl and portability of
>> libcurl it seems reasonable to say that "git imap-send" cannot be used
>> without libcurl, almost everyone building git needs to be able to push
>> or pull over http(s), so they'll be building with libcurl already.
>
> OK.
>
>> So let's remove the previous "USE_CURL_FOR_IMAP_SEND" knob. Whether we
>> build git-imap-send or not is now controlled by the "NO_CURL"
>> knob.
>
> OK.
>
>> Let's also hide the old --curl and --no-curl options, and die if
>> "--no-curl" is provided.
>
> In other words, if we are building imap-send, we sure know cURL is
> there, and there is no need to tell a running imap-send not to use
> cURL to talk to the IMAP service?  I am not sure the linkage of this
> change with the rest of the patch.  Isn't that a totally orthogonal
> issue?  Your imap-send might be cURL enabled, but unless we stop to
> ship with our own IMAP routines compiled into imap-send, --no-curl
> does have a purpose.

The equivalent of USE_CURL_FOR_IMAP_SEND is now always true, and that's
what "--curl" would enable.

The "--no-curl" option would then have us use the OpenSSL codepath, but
that'll no longer be supported, we'll always use curl.

The link to the rest of the patch is then that "USE_CURL_FOR_IMAP_SEND"
and "curl_check" etc. was needed to check if we had curl with imap-send,
now we declare that we'll always need it.

And the link to the thread-at-large is that Jiang Xin's upthread version
moves those checks from the Makefile into the code itself, I agree that
wolud be an improvement, but if we're happy to just make it a hard
dependency we won't need it there either...

> Or did you just forget to document that we stop to ship with our own
> IMAP routines in the above?  If so, as long as it is made a bit more
> prominent in the proposed log message in a reroll, I would be happy
> with such a change rolled into the same patch.

I'm not sure what you mean here, we still ship with the same routines,
we just always take the "curl" codepath for the non-tunnel codepath now.

Is this perhaps confusion because while we do make curl mandatory, we're
not dropping the OpenSSL code? That's because we're dropping its use for
the non-tunnel codepath, but unfortunately for the tunnel case we'll
still need it.

So we only make curl mandatory, but OpenSSL remains optional.
Jeff King Feb. 1, 2023, 11:59 p.m. UTC | #3 
On Wed, Feb 01, 2023 at 03:22:24PM -0800, Junio C Hamano wrote:

> > Let's also hide the old --curl and --no-curl options, and die if
> > "--no-curl" is provided.
> 
> In other words, if we are building imap-send, we sure know cURL is
> there, and there is no need to tell a running imap-send not to use
> cURL to talk to the IMAP service?  I am not sure the linkage of this
> change with the rest of the patch.  Isn't that a totally orthogonal
> issue?  Your imap-send might be cURL enabled, but unless we stop to
> ship with our own IMAP routines compiled into imap-send, --no-curl
> does have a purpose.
> 
> Or did you just forget to document that we stop to ship with our own
> IMAP routines in the above?  If so, as long as it is made a bit more
> prominent in the proposed log message in a reroll, I would be happy
> with such a change rolled into the same patch.

FWIW, I had the same urge as Ævar, to drop the non-curl support
completely, and was puzzled that his patch did not have a big code
deletion. ;)

The problem is that the tunnel mode still relies on the non-curl code.
There was a series to address that a while ago:

  https://lore.kernel.org/git/ab866314-608b-eaca-b335-12cffe165526@morey-chaisemartin.com/

but it ran into the problem that curl did not support PREAUTH
connections (which is one of the main points of tunneling). It looks
like that got added to curl via their befaa7b14f, which is in curl
7.56.0 from 2017. That's not old enough for us to require for http, but
might be OK for a marginal component like the tunneling mode of
imap-send.

I think there was also some question of how you even get the tunnel
going. Curl really wants to have a single socket descriptor, not two
pipe descriptors, so there may have to be some trickery with
socketpair(). There's more discussion in the linked thread.

So I think there's a path forward here for getting rid of the legacy
code (and I'd be really happy to see it gone; it's imported code that
does not seem super well maintained by us). But until we do that,
disabling --no-curl doesn't seem like that big a win, if that code can
all still be triggered for tunnel mode.

-Peff
Ævar Arnfjörð Bjarmason Feb. 2, 2023, 12:20 a.m. UTC | #4 
On Wed, Feb 01 2023, Jeff King wrote:

> On Wed, Feb 01, 2023 at 03:22:24PM -0800, Junio C Hamano wrote:
>
>> > Let's also hide the old --curl and --no-curl options, and die if
>> > "--no-curl" is provided.
>> 
>> In other words, if we are building imap-send, we sure know cURL is
>> there, and there is no need to tell a running imap-send not to use
>> cURL to talk to the IMAP service?  I am not sure the linkage of this
>> change with the rest of the patch.  Isn't that a totally orthogonal
>> issue?  Your imap-send might be cURL enabled, but unless we stop to
>> ship with our own IMAP routines compiled into imap-send, --no-curl
>> does have a purpose.
>> 
>> Or did you just forget to document that we stop to ship with our own
>> IMAP routines in the above?  If so, as long as it is made a bit more
>> prominent in the proposed log message in a reroll, I would be happy
>> with such a change rolled into the same patch.
>
> FWIW, I had the same urge as Ævar, to drop the non-curl support
> completely, and was puzzled that his patch did not have a big code
> deletion. ;)

FWIW I arrived at this from looking at the mandatory $(shell)-outs in
the Makefile, and wasn't looking to drop the OpenSSL code.

Then in looking at that, I found that we could probably make the curl
dependency mandatory.

> The problem is that the tunnel mode still relies on the non-curl code.
> There was a series to address that a while ago:
>
>   https://lore.kernel.org/git/ab866314-608b-eaca-b335-12cffe165526@morey-chaisemartin.com/
>
> but it ran into the problem that curl did not support PREAUTH
> connections (which is one of the main points of tunneling). It looks
> like that got added to curl via their befaa7b14f, which is in curl
> 7.56.0 from 2017. That's not old enough for us to require for http, but
> might be OK for a marginal component like the tunneling mode of
> imap-send.
>
> I think there was also some question of how you even get the tunnel
> going. Curl really wants to have a single socket descriptor, not two
> pipe descriptors, so there may have to be some trickery with
> socketpair(). There's more discussion in the linked thread.

That's neat, I didn't know about that attempt.

> So I think there's a path forward here for getting rid of the legacy
> code (and I'd be really happy to see it gone; it's imported code that
> does not seem super well maintained by us). But until we do that,
> disabling --no-curl doesn't seem like that big a win, if that code can
> all still be triggered for tunnel mode.

I think the biggest win is that we're dropping the dual curl/OpenSSL
codepath for everything except the "tunnel" mode, which is really
obscure compared to the already-obscure functionality of the main
"imap-send" tool.

It would also get us part of the way to e.g. depending on 7.56.0, as a
hard dependency on curl (and a newer version than we usually depend on)
would reveal if anyone's got an issue with that stepping stone.
Junio C Hamano Feb. 2, 2023, 1:32 a.m. UTC | #5 
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> The equivalent of USE_CURL_FOR_IMAP_SEND is now always true, and that's
> what "--curl" would enable.
>
> The "--no-curl" option would then have us use the OpenSSL codepath, but
> that'll no longer be supported, we'll always use curl.
> ...
>> Or did you just forget to document that we stop to ship with our own
>> IMAP routines in the above?  If so, as long as it is made a bit more
>> prominent in the proposed log message in a reroll, I would be happy
>> with such a change rolled into the same patch.
>
> I'm not sure what you mean here, we still ship with the same routines,
> we just always take the "curl" codepath for the non-tunnel codepath now.

I am referring to this part of the documentation:

    --no-curl::
            Talk to the IMAP server using git's own IMAP routines instead of
            using libcurl.  Ignored if Git was built with the NO_OPENSSL option
            set.

So when built with openssl and libcURL, we used to have a feature
that allowed to bypass cURL by passing --no-curl for whatever reason
the user chooses to avoid cURL.  This patch discards that option,
doesn't it?

Maybe such an optional feature may not be very useful, but it should
be explained and defended in the proposed log message, and it also
sounds like an orthogonal change to always require libcURL.
diff mbox series

Patch

diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
index f7b18515141..dcd29f011ce 100644
--- a/Documentation/git-imap-send.txt
+++ b/Documentation/git-imap-send.txt
@@ -37,16 +37,6 @@  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.  Ignored if Git was built with the NO_OPENSSL option
-	set.
-
 
 CONFIGURATION
 -------------
diff --git a/INSTALL b/INSTALL
index d5694f8c470..d9538bbcb45 100644
--- a/INSTALL
+++ b/INSTALL
@@ -129,13 +129,13 @@  Issues of note:
 	  itself, e.g. Digest::MD5, File::Spec, File::Temp, Net::Domain,
 	  Net::SMTP, and Time::HiRes.
 
-	- 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.
+	- git-imap-send needs libcurl 7.34.0 or newer, in addition
+	  OpenSSL is needed if using the "imap.tunnel" open to tunnel
+	  over SSL. Define NO_OPENSSL to omit the OpenSSL prerequisite.
 
 	- "libcurl" library is used for fetching and pushing
 	  repositories over http:// or https://, as well as by
-	  git-imap-send if the curl version is >= 7.34.0. If you do
+	  git-imap-send. If you do
 	  not need that functionality, use NO_CURL to build without
 	  it.
 
diff --git a/Makefile b/Makefile
index 45bd6ac9c3e..b08a855198c 100644
--- a/Makefile
+++ b/Makefile
@@ -773,7 +773,9 @@  PROGRAMS += $(EXTRA_PROGRAMS)
 
 PROGRAM_OBJS += daemon.o
 PROGRAM_OBJS += http-backend.o
+ifndef NO_CURL
 PROGRAM_OBJS += imap-send.o
+endif
 PROGRAM_OBJS += sh-i18n--envsubst.o
 PROGRAM_OBJS += shell.o
 .PHONY: program-objs
@@ -1583,7 +1585,6 @@  ifdef HAVE_ALLOCA_H
 	BASIC_CFLAGS += -DHAVE_ALLOCA_H
 endif
 
-IMAP_SEND_BUILDDEPS =
 IMAP_SEND_LDFLAGS =
 
 ifdef NO_CURL
@@ -1592,6 +1593,7 @@  ifdef NO_CURL
 	REMOTE_CURL_ALIASES =
 	REMOTE_CURL_NAMES =
 	EXCLUDED_PROGRAMS += git-http-fetch git-http-push
+	EXCLUDED_PROGRAMS += git-imap-send
 else
 	ifdef CURLDIR
 		# Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case.
@@ -1617,19 +1619,9 @@  else
 	REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)
 	PROGRAM_OBJS += http-fetch.o
 	PROGRAMS += $(REMOTE_CURL_NAMES)
+	IMAP_SEND_LDFLAGS += $(CURL_LIBCURL)
 	ifndef NO_EXPAT
 		PROGRAM_OBJS += http-push.o
-	endif
-	curl_check := $(shell (echo 072200; $(CURL_CONFIG) --vernum | sed -e '/^70[BC]/s/^/0/') 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
 			EXPAT_LIBEXPAT = -L$(EXPATDIR)/$(lib) $(CC_LD_DYNPATH)$(EXPATDIR)/$(lib) -lexpat
@@ -2786,7 +2778,7 @@  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 $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)
+git-imap-send$X: imap-send.o http.o GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(IMAP_SEND_LDFLAGS) $(LIBS)
 
diff --git a/imap-send.c b/imap-send.c
index a50af56b827..0e36ed4f854 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -30,26 +30,16 @@ 
 #if defined(NO_OPENSSL) && !defined(HAVE_OPENSSL_CSPRNG)
 typedef void *SSL;
 #endif
-#ifdef USE_CURL_FOR_IMAP_SEND
 #include "http.h"
-#endif
-
-#if defined(USE_CURL_FOR_IMAP_SEND)
-/* Always default to curl if it's available. */
-#define USE_CURL_DEFAULT 1
-#else
-/* We don't have curl, so continue to use the historical implementation */
-#define USE_CURL_DEFAULT 0
-#endif
 
 static int verbosity;
-static int use_curl = USE_CURL_DEFAULT;
+static int use_curl = 1;
 
 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_HIDDEN_BOOL(0, "curl", &use_curl, "use libcurl to communicate with the IMAP server"),
 	OPT_END()
 };
 
@@ -1396,7 +1386,6 @@  static int append_msgs_to_imap(struct imap_server_conf *server,
 	return 0;
 }
 
-#ifdef USE_CURL_FOR_IMAP_SEND
 static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
 {
 	CURL *curl;
@@ -1515,7 +1504,6 @@  static int curl_append_msgs_to_imap(struct imap_server_conf *server,
 
 	return res != CURLE_OK;
 }
-#endif
 
 int cmd_main(int argc, const char **argv)
 {
@@ -1531,17 +1519,8 @@  int cmd_main(int argc, const char **argv)
 	if (argc)
 		usage_with_options(imap_send_usage, imap_send_options);
 
-#ifndef USE_CURL_FOR_IMAP_SEND
-	if (use_curl) {
-		warning("--curl not supported in this build");
-		use_curl = 0;
-	}
-#elif defined(NO_OPENSSL)
-	if (!use_curl) {
-		warning("--no-curl not supported in this build");
-		use_curl = 1;
-	}
-#endif
+	if (!use_curl)
+		die(_("the --no-curl option to imap-send has been deprecated"));
 
 	if (!server.port)
 		server.port = server.use_ssl ? 993 : 143;
@@ -1580,10 +1559,5 @@  int cmd_main(int argc, const char **argv)
 	if (server.tunnel)
 		return append_msgs_to_imap(&server, &all_msgs, total);
 
-#ifdef USE_CURL_FOR_IMAP_SEND
-	if (use_curl)
-		return curl_append_msgs_to_imap(&server, &all_msgs, total);
-#endif
-
-	return append_msgs_to_imap(&server, &all_msgs, total);
+	return curl_append_msgs_to_imap(&server, &all_msgs, total);
 }