Re: [PATCH] imap-send: replace auto-probe libcurl with hard dependency

From: Jeff King <peff@peff.net>
Date: Feb 1, 2023, 23:59 UTC
Message-ID: <Y9r84bezJ0scapwC@coredump.intra.peff.net>
In-Reply-To: <xmqqlelhx973.fsf@gitster.g>

On Wed, Feb 01, 2023 at 03:22:24PM -0800, Junio C Hamano wrote:

Show 15 quoted lines

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