Bernhard Reiter <ockham@xxxxxxxxx> writes: > Use libcurl's high-level API functions to implement git-imap-send > instead of the previous low-level OpenSSL-based functions. > > Since version 7.30.0, libcurl's API has been able to communicate with > IMAP servers. Using those high-level functions instead of the current > ones would reduce imap-send.c by some 1200 lines of code. For now, > the old ones are wrapped in #ifdefs, and the new functions are enabled > by make if curl's version is >= 7.35.0, from which version on curl's > CURLOPT_LOGIN_OPTIONS (enabling IMAP authentication) parameter has been > available. https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions says that this was introduced as of 7.34.0, though. > As I don't have access to that many IMAP servers, I haven't been able to > test the new code with a wide variety of parameter combinations. I did > test both secure and insecure (imaps:// and imap://) connections and > values of "PLAIN" and "LOGIN" for the authMethod. Perhaps CC'ing those who have touched git-imap-send code over the years and asking for their help testing might help? > Signed-off-by: Bernhard Reiter <ockham@xxxxxxxxx> > --- > 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. > Documentation/git-imap-send.txt | 3 +- > INSTALL | 15 ++-- > Makefile | 16 +++- > git.spec.in | 5 +- > imap-send.c | 165 +++++++++++++++++++++++++++++++++------- > 5 files changed, 167 insertions(+), 37 deletions(-) > > diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt > index 7d991d9..9d244c4 100644 > --- a/Documentation/git-imap-send.txt > +++ b/Documentation/git-imap-send.txt > @@ -75,7 +75,8 @@ imap.preformattedHTML:: > > imap.authMethod:: > Specify authenticate method for authentication with IMAP server. > - Current supported method is 'CRAM-MD5' only. If this is not set > + If you compiled git with the NO_CURL option or if your curl version is > + < 7.35.0, the only supported method is 'CRAM-MD5'. If this is not set > then 'git imap-send' uses the basic IMAP plaintext LOGIN command. Hmph, so there is no option that lets me say "I know my libcurl is new enough but I have some reason not to want to use the new code to interact with my imap server", at compile time or (more preferrably) at runtime? > diff --git a/INSTALL b/INSTALL > index 6ec7a24..e2770a0 100644 > --- a/INSTALL > +++ b/INSTALL > @@ -108,18 +108,21 @@ Issues of note: > so you might need to install additional packages other than Perl > itself, e.g. Time::HiRes. > > - - "openssl" library is used by git-imap-send to use IMAP over SSL. > - If you don't need it, use NO_OPENSSL. > + - "openssl" library is used by git-imap-send to use IMAP over SSL, > + unless you're using curl >= 7.35.0, in which case that will be > + used. If you don't need git-imap-send, you can use NO_OPENSSL. The last sentence makes it unclear which of the following is true: - I have sufficiently new libcurl. I cannot say NO_OPENSSL because I do need git-imap-send. - I have sufficiently new libcurl, so "openssl" is not used by git-imap send for me. I can say NO_OPENSSL. Perhaps - git-imap-send needs the OpenSSL library to talk IMAP over SSL if you are using libCurl older than 7.35.0. Otherwise you can use NO_OPENSSL without losing git-imap-send. > diff --git a/git.spec.in b/git.spec.in > index d61d537..9535cc3 100644 > --- a/git.spec.in > +++ b/git.spec.in > @@ -8,7 +8,7 @@ License: GPL > Group: Development/Tools > URL: http://kernel.org/pub/software/scm/git/ > Source: http://kernel.org/pub/software/scm/git/%{name}-%{version}.tar.gz > -BuildRequires: zlib-devel >= 1.2, openssl-devel, curl-devel, expat-devel, gettext %{!?_without_docs:, xmlto, asciidoc > 6.0.3} > +BuildRequires: zlib-devel >= 1.2, openssl-devel, curl-devel >= 7.35.0, expat-devel, gettext %{!?_without_docs:, xmlto, asciidoc > 6.0.3} This is very iffy. It incompatible with the body of the patch where you allow older curl library and because you depend on openssl-devel you wouldn't lose imap-send. > @@ -1391,29 +1518,13 @@ int main(int argc, char **argv) > } > > /* write it to the imap server */ > - ctx = imap_open_store(&server, server.folder); > - if (!ctx) { > - fprintf(stderr, "failed to open store\n"); > - return 1; > - } > - > - fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : ""); > - while (1) { > - unsigned percent = n * 100 / total; > - > - fprintf(stderr, "%4u%% (%d/%d) done\r", percent, n, total); > - if (!split_msg(&all_msgs, &msg, &ofs)) > - break; > - if (server.use_html) > - wrap_in_html(&msg); > - r = imap_store_msg(ctx, &msg); > - if (r != DRV_OK) > - break; > - n++; > - } > - fprintf(stderr, "\n"); > > - imap_close_store(ctx); > + if (server.tunnel) > + return append_msgs_to_imap(&server, &all_msgs, total); > > - return 0; > +#ifndef USE_CURL_FOR_IMAP_SEND > + return append_msgs_to_imap(&server, &all_msgs, total); > +#else > + return curl_append_msgs_to_imap(&server, &all_msgs, total); > +#endif > } Much more nicely done. It appears that you could already turn these #ifndef/#else/#endif into a runtime conditional, allowing: - At compile-time, can be built with the four combinations (1) USE_CURL_FOR_IMAP_SEND=Yes NO_OPENSSL=No (2) USE_CURL_FOR_IMAP_SEND=Yes NO_OPENSSL=Yes (3) USE_CURL_FOR_IMAP_SEND=No NO_OPENSSL=No (4) USE_CURL_FOR_IMAP_SEND=No NO_OPENSSL=Yes - The first two variants can support --with-curl/--without-curl and choose between curl_append/append. When run --without-curl, it may lose some auth-methods and for variant (1) SSL is not supported. or am I mis-reading the patch? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html