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

[Bernhard Reiter]

Am 2014-08-19 um 19:51 schrieb Junio C Hamano:
 This looks strange and stands out like a sore thumb.  Do any of our
 other sources do this kind of macro tweaking inside C source before
 including git-compat-util.h (or its equivalent like cache.h)?

I haven't checked, but I agree that it's desirable to avoid.

 I think what you are trying to do is to change the meaning of
 NO_OPENSSL, which merely means we do not have OpenSSL library and
 do not want to link with it, locally to we may or may not have and
 use OpenSSL library elsewhere in Git, but in the code below we do
 not want to use the code written to work on top of OpenSSL and
 instead use libcurl.  

...and we don't want to link to OpenSSL in that case. Yeah.

 Because of that, you define NO_OPENSSL before
 including any of our headers to cause us not to include the headers,
 and typedef away SSL, for example.

The SSL un-typedef'ing was there before, but it's true that I'm defining
NO_OPENSSL on the very top so the included headers don't require OpenSSL
(and so we don't have to link to it later).

  #include cache.h
  #include credential.h
  #include exec_cmd.h
 @@ -29,6 +33,9 @@
  #ifdef NO_OPENSSL
  typedef void *SSL;
  #endif
 +#ifdef USE_CURL_FOR_IMAP_SEND
 +#include http.h
 +#endif
 
 But does it have to be that way?  For one thing, doing it this way,
 the user has to make a compilation-time choice, but if you separate
 these compilation time macro into two, one for can we even link
 with and use OpenSSL? (which is what NO_OPENSSL is about) and
 another for do we want an ability to talk to imap via libcurl?,
 wouldn't it make it possible for you to switch between them at
 runtime (e.g. you might want to go over the direct connection when
 tunneling, while letting libcurl do the heavy lifting in
 non-tunneled operation)?

Yeah, but I still need to wrap the non-tunneled operation in #ifdefs in
case we don't USE_CURL_FOR_IMAP_SEND in which case we fall back to the
handwritten IMAP code, don't I?

Maybe I'm not getting you entirely right, but if I don't typedef
NO_OPENSSL if USE_CURL_FOR_IMAP_SEND is defined, I don't see any way to
not link to OpenSSL, even if it's not required. I'm including a slightly
modified patch which does that (hoping that I've finally managed to send
a usable patch). Sorry it's nothing breathtakingly better than before,
even after giving this some thought I didn't arrive at a very elegant
new solution... (see below for more on that)

 @@ -1417,18 +1476,48 @@ int main(int argc, char **argv)
  }
  
  /* write it to the imap server */
 +
 +#ifdef USE_CURL_FOR_IMAP_SEND
 +if (!server.tunnel) {
 +curl = setup_curl(server);
 +curl_easy_setopt(curl, CURLOPT_READDATA, msgbuf);
 +} else {
 +#endif
  ctx = imap_open_store(server);
  if (!ctx) {
  fprintf(stderr, failed to open store\n);
  return 1;
  }
 +ctx-name = server.folder;
 +#ifdef USE_CURL_FOR_IMAP_SEND
 +}
 +#endif
  
  fprintf(stderr, sending %d message%s\n, total, (total != 1) ? s : 
 );
 -ctx-name = imap_folder;
  while (1) {
  unsigned percent = n * 100 / total;
  
  fprintf(stderr, %4u%% (%d/%d) done\r, percent, n, total);
 +#ifdef USE_CURL_FOR_IMAP_SEND
 +if (!server.tunnel) {
 ...
 +}
 +} else {
 +#endif
  if (!split_msg(all_msgs, msg, ofs))
  break;
  if (server.use_html)
 @@ -1436,11 +1525,19 @@ int main(int argc, char **argv)
  r = imap_store_msg(ctx, msg);
  if (r != DRV_OK)
  break;
 +#ifdef USE_CURL_FOR_IMAP_SEND
 +}
 +#endif
  n++;
  }
  fprintf(stderr, \n);
  
 +#ifdef USE_CURL_FOR_IMAP_SEND
 +curl_easy_cleanup(curl);
 +curl_global_cleanup();
 +#else
  imap_close_store(ctx);
 +#endif
  
  return 0;
  }
 
 Ugly.  Can we do this with less #ifdef/#else/#endif in the primary
 code path?

It is ugly, but as much as I'd love to put e.g.

+#ifdef USE_CURL_FOR_IMAP_SEND
+   if (!server.tunnel) {
+   curl = setup_curl(server);

etc into imap_open_store (and similarly for imap_store_msg etc), I don't
see any easy way to do it; imap_open_store's return type would still be
struct imap_store* in the non-tunneling case, and CURL* otherwise.

So the best I can come up with here is merging some of the #ifdef
blocks, but that means duplicating the code that applies in both cases.
But that isn't any better, is it?

 If we were to keep these two modes as a choice the users have to
 make at the compilation time, that is.

As stated above, I'm not sure how to do entirely without at least those
two compile time switches (NO_OPENSSL and USE_CURL_FOR_IMAP_SEND).

Sorry, perhaps I missed something obvious; grateful for any hints on how
to do it better.

Bernhard

 Documentation/git-imap-send.txt |  3 +-
 INSTALL | 15 ---
 Makefile 

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

[Junio C Hamano]

Bernhard Reiter ock...@raz.or.at writes:

 Yeah, but I still need to wrap the non-tunneled operation in #ifdefs in
 case we don't USE_CURL_FOR_IMAP_SEND in which case we fall back to the
 handwritten IMAP code, don't I?

We do not mind multiple implementations of the same helper function
that are guarded with #ifdef/#endif, and we do use that style quite
a lot.  Would it help?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bernhard Reiter]

Am 2014-08-17 um 20:42 schrieb Jeff King:
 [...]
 
 I'm not sure I understand this comment. Even if SSL is not in use,
 wouldn't we be passing a regular pipe to curl, which would break?

 Yeah, we can't do that, and thus would have to keep the handwritten IMAP
 implementation just for the tunnel case (allowing to drop only the
 OpenSSL specific stuff), see my other email:
 http://www.mail-archive.com/git@vger.kernel.org/msg56791.html (the
 relevant part is pretty far down at the bottom).
 
 I'd really love it if we could make this work with tunnels and
 eventually get rid of the hand-written imap code entirely. I agree with
 Jonathan that we probably need to keep it around a bit for people on
 older curl, but dropping it is a good goal in the long run. That code
 was forked from the isync project, but mangled enough that we could not
 take bug fixes from upstream. As not many people use imap-send, I
 suspect it is largely unmaintained and the source of many lurking
 bugs[1]. Replacing it with curl's maintained implementation is probably
 a good step.

I'll work on this as soon as I find some time, but as that will include
changes to run-command.c (and possibly other files?), I'd like to cover
that in a commit of its own. Do you guys think the current patch [1] is
good enough for official submission already? If so, do I need some
sort of official review? Documentation/SubmittingPatches says I'm only
supposed to direct it to Junio after the list reaches consensus, so
I'm wondering how to get there... :-)

Bernhard
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Bernhard Reiter ock...@raz.or.at writes:

 Am 2014-08-17 um 20:42 schrieb Jeff King:
 [...]
 
 I'm not sure I understand this comment. Even if SSL is not in use,
 wouldn't we be passing a regular pipe to curl, which would break?

 Yeah, we can't do that, and thus would have to keep the handwritten IMAP
 implementation just for the tunnel case (allowing to drop only the
 OpenSSL specific stuff), see my other email:
 http://www.mail-archive.com/git@vger.kernel.org/msg56791.html (the
 relevant part is pretty far down at the bottom).
 
 I'd really love it if we could make this work with tunnels and
 eventually get rid of the hand-written imap code entirely. I agree with
 Jonathan that we probably need to keep it around a bit for people on
 older curl, but dropping it is a good goal in the long run. That code
 was forked from the isync project, but mangled enough that we could not
 take bug fixes from upstream. As not many people use imap-send, I
 suspect it is largely unmaintained and the source of many lurking
 bugs[1]. Replacing it with curl's maintained implementation is probably
 a good step.

I would agree with s/a good step/a good goal/ ;-)

 I'll work on this as soon as I find some time, but as that will include
 changes to run-command.c (and possibly other files?), I'd like to cover
 that in a commit of its own. Do you guys think the current patch [1] is
 good enough for official submission already?

My impression from reading the discussion in this thread has been
that the patch that started this thread would break the tunneling
code, i.e. is not there yet.  Or did you mean some other patch?

The other patch $gmane/255403 from you looked good and I think I
already have a copy queued on 'pu' as f9dc5d65 (git-imap-send:
simplify tunnel construction, 2014-08-13).

Thanks.


[References]

*$gmane/255403*
http://thread.gmane.org/gmane.comp.version-control.git/255220/focus=255403
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Bernhard Reiter ock...@raz.or.at 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.

 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 ock...@raz.or.at
 ---

I think people have missed this one, partly because it was hidden as
an attachment.

  Documentation/git-imap-send.txt |   3 +-
  INSTALL |  15 +++---
  Makefile|  16 +-
  git.spec.in |   5 +-
  imap-send.c | 109
 +---
  5 files changed, 132 insertions(+), 16 deletions(-)

I smell a line-wrapped patch but it probably is at my receiving end,
forcing the attachment into a flattened form inside my MUA.

Nice to see git.spec.in updated; I like it when I see that the
author paid attention to details.

 diff --git a/imap-send.c b/imap-send.c
 index fb01a9c..a45570d 100644
 --- a/imap-send.c
 +++ b/imap-send.c
 @@ -22,6 +22,10 @@
   *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
   */
  
 +#ifdef USE_CURL_FOR_IMAP_SEND
 +#define NO_OPENSSL
 +#endif
 +

This looks strange and stands out like a sore thumb.  Do any of our
other sources do this kind of macro tweaking inside C source before
including git-compat-util.h (or its equivalent like cache.h)?

I think what you are trying to do is to change the meaning of
NO_OPENSSL, which merely means we do not have OpenSSL library and
do not want to link with it, locally to we may or may not have and
use OpenSSL library elsewhere in Git, but in the code below we do
not want to use the code written to work on top of OpenSSL and
instead use libcurl.  Because of that, you define NO_OPENSSL before
including any of our headers to cause us not to include the headers,
and typedef away SSL, for example.

  #include cache.h
  #include credential.h
  #include exec_cmd.h
 @@ -29,6 +33,9 @@
  #ifdef NO_OPENSSL
  typedef void *SSL;
  #endif
 +#ifdef USE_CURL_FOR_IMAP_SEND
 +#include http.h
 +#endif

But does it have to be that way?  For one thing, doing it this way,
the user has to make a compilation-time choice, but if you separate
these compilation time macro into two, one for can we even link
with and use OpenSSL? (which is what NO_OPENSSL is about) and
another for do we want an ability to talk to imap via libcurl?,
wouldn't it make it possible for you to switch between them at
runtime (e.g. you might want to go over the direct connection when
tunneling, while letting libcurl do the heavy lifting in
non-tunneled operation)?

 @@ -44,9 +51,7 @@ __attribute__((format (printf, 1, 2)))
  static void imap_info(const char *, ...);
  __attribute__((format (printf, 1, 2)))
  static void imap_warn(const char *, ...);
 -
  static char *next_arg(char **);
 -
  __attribute__((format (printf, 3, 4)))
  static int nfsnprintf(char *buf, int blen, const char *fmt, ...);
  
 @@ -69,6 +74,7 @@ struct imap_server_conf {
   char *tunnel;
   char *host;
   int port;
 + char *folder;
   char *user;
   char *pass;
   int use_ssl;
 @@ -82,6 +88,7 @@ static struct imap_server_conf server = {
   NULL,   /* tunnel */
   NULL,   /* host */
   0,  /* port */
 + NULL,   /* folder */
   NULL,   /* user */
   NULL,   /* pass */
   0,  /* use_ssl */
 @@ -1323,7 +1330,54 @@ static int split_msg(struct strbuf *all_msgs, struct 
 strbuf *msg, int *ofs)
   return 1;
  }
  
 -static char *imap_folder;

All of the above seem to have meant well, but these changes are not
about talking with IMAP servers via libcurl.  We'd prefer to see
changes like these as preliminary clean-up before the main change as
separate patch(es).

 @@ -1417,18 +1476,48 @@ int main(int argc, char **argv)
   }
  
   /* write it to the imap server */
 +
 +#ifdef USE_CURL_FOR_IMAP_SEND
 + if (!server.tunnel) {
 + curl = setup_curl(server);
 + curl_easy_setopt(curl, CURLOPT_READDATA, msgbuf);
 + } else {
 +#endif
   ctx = imap_open_store(server);
   if (!ctx) {
   fprintf(stderr, failed to open store\n);
   return 1;
   }
 + ctx-name = server.folder;
 +#ifdef USE_CURL_FOR_IMAP_SEND
 + }
 +#endif
  
   fprintf(stderr, sending %d 

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

[Jeff King]

On Tue, Aug 12, 2014 at 06:59:17PM -0700, Jonathan Nieder wrote:

  +   curl_socket_t sockfd = tunnel.out; // what about tunnel.in ?
 
 Hmm.  curl expects to get a socket it can send(), recv(), setsockopt(),
 etc on instead of a pair of fds to read() and write().

I wonder if we could teach run_command to optionally use socketpair()
instead of pipe(). I'm not sure if that would cause problems on Windows,
though.

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

I'm not sure I understand this comment. Even if SSL is not in use,
wouldn't we be passing a regular pipe to curl, which would break?

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bernhard Reiter]

Am 2014-08-17 um 10:30 schrieb Jeff King:
 On Tue, Aug 12, 2014 at 06:59:17PM -0700, Jonathan Nieder wrote:
 
 +   curl_socket_t sockfd = tunnel.out; // what about tunnel.in ?

 Hmm.  curl expects to get a socket it can send(), recv(), setsockopt(),
 etc on instead of a pair of fds to read() and write().
 
 I wonder if we could teach run_command to optionally use socketpair()
 instead of pipe(). 

That sounds like a good idea to me.

 I'm not sure if that would cause problems on Windows,
 though.

Apparently socketpair is not available there. Googling socketpair
windows yields, among a lot of other useful resources, the following
relatively actively maintained ~150 LOC, BSD-3-clause-licensed,
implementation:

https://github.com/ncm/selectable-socketpair

That license is GPL compatible, so should we consider including that
implementation with git? That's the kind of stuff that goes to
compat/win32, right?

One thing to consider: seems like socketpair() gives AF_LOCAL sockets,
so I've asked [1] on the curl ML if that would work or if libcurl needs
an AF_INET one.

 I wonder why someone would want to use SSL through a tunnel, though.
 Currently it's impossible to get to the SSL codepath when a tunnel is
 active (it's in the 'else' block an 'if (srvc-tunnel)').  If that
 property is preserved, then we should be safe.
 
 I'm not sure I understand this comment. Even if SSL is not in use,
 wouldn't we be passing a regular pipe to curl, which would break?

Yeah, we can't do that, and thus would have to keep the handwritten IMAP
implementation just for the tunnel case (allowing to drop only the
OpenSSL specific stuff), see my other email:
http://www.mail-archive.com/git@vger.kernel.org/msg56791.html (the
relevant part is pretty far down at the bottom).

Bernhard

[1] http://curl.haxx.se/mail/lib-2014-08/0131.html
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

On Sun, Aug 17, 2014 at 02:56:10PM +0200, Bernhard Reiter wrote:

  I'm not sure if that would cause problems on Windows,
  though.
 
 Apparently socketpair is not available there. Googling socketpair
 windows yields, among a lot of other useful resources, the following
 relatively actively maintained ~150 LOC, BSD-3-clause-licensed,
 implementation:
 
 https://github.com/ncm/selectable-socketpair
 
 That license is GPL compatible, so should we consider including that
 implementation with git? That's the kind of stuff that goes to
 compat/win32, right?

Thanks for researching. Sticking that in compat/ would be our usual
strategy, yes.

  I'm not sure I understand this comment. Even if SSL is not in use,
  wouldn't we be passing a regular pipe to curl, which would break?
 
 Yeah, we can't do that, and thus would have to keep the handwritten IMAP
 implementation just for the tunnel case (allowing to drop only the
 OpenSSL specific stuff), see my other email:
 http://www.mail-archive.com/git@vger.kernel.org/msg56791.html (the
 relevant part is pretty far down at the bottom).

I'd really love it if we could make this work with tunnels and
eventually get rid of the hand-written imap code entirely. I agree with
Jonathan that we probably need to keep it around a bit for people on
older curl, but dropping it is a good goal in the long run. That code
was forked from the isync project, but mangled enough that we could not
take bug fixes from upstream. As not many people use imap-send, I
suspect it is largely unmaintained and the source of many lurking
bugs[1]. Replacing it with curl's maintained implementation is probably
a good step.

-Peff

[1] That's my somewhat subjective opinion, but I feel like I have seen
several bugs reported in imap-send that had literally been there for
years. And having worked on IMAP implementations in a past life, I
know there are many dark corners of the protocol that vary from
server to server.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

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 ock...@raz.or.at
---
Am 2014-08-13 um 03:59 schrieb Jonathan Nieder:
 Bernhard Reiter wrote:
 [...]

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

Well thanks for the friendly welcome and the helpful comments!

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

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

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

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

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

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

 +curl_global_init(CURL_GLOBAL_ALL);

 http.c seems to make the same mistake,

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

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

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

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

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

No comments on those patches yet, though.

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

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

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

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

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

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

 To summarize:
 [...]

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

Is this one good enough already?

Bernhard

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

 Documentation/git-imap-send.txt |   3 +-
 INSTALL |  15 +++---
 Makefile|  16 +-
 git.spec.in |   5 +-
 imap-send.c | 109
+---
 5 files 

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

[Bernhard Reiter]

Use libcurl's high-level API functions to implement git-imap-send
instead of the previous low-level OpenSSL-based functions.

Signed-off-by: Bernhard Reiter ock...@raz.or.at
---
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 reduces imap-send.c by some 1200 lines of code.

As I don't have access to that many IMAP servers, I haven't been able to
test a variety of parameter combinations. I did test both secure and
insecure (imaps:// and imap://) connections -- this e-mail was generated
that way -- but could e.g. neither test the authMethod nor the tunnel
parameter.

As git-imap-send is one of the two instances OpenSSL is currently used
by git -- the other one being SHA1 -- it might be worthwhile considering
dropping it altogether as there's already a SHA1 library built into git
available as an alternative.

Kind regards
Bernhard
PS: Please CC!

 INSTALL |   14 +-
 Makefile|4 +-
 git.spec.in |5 +-
 imap-send.c | 1288
+--
 4 files changed, 111 insertions(+), 1200 deletions(-)


diff --git a/INSTALL b/INSTALL
index 6ec7a24..2cd3a42 100644
--- a/INSTALL
+++ b/INSTALL
@@ -108,18 +108,16 @@ 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.
-
-	  By default, git uses OpenSSL for SHA1 but it will use its own
+	- 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
+	  git-impap-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
diff --git a/Makefile b/Makefile
index 2320de5..7805603 100644
--- a/Makefile
+++ b/Makefile
@@ -2067,9 +2067,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 http.o GIT-LDFLAGS $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
-		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
+		$(LIBS) $(CURL_LIBCURL)
 
 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,$^) \
diff --git a/git.spec.in b/git.spec.in
index d61d537..7d9230f 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.30.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 ock...@raz.or.at
+- Require version = 7.30.0 of curl-devel for IMAP functions.
+
 * Sun Sep 18 2011 Jakub Narebski jna...@gmail.com
 - Add gitweb manpages to 'gitweb' subpackage
 
diff --git a/imap-send.c b/imap-send.c
index 524fbab..0c4583f 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -22,47 +22,13 @@
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
-#include cache.h
-#include credential.h
+#include http.h
 #include exec_cmd.h
 #include run-command.h
-#ifdef NO_OPENSSL
-typedef void *SSL;
-#endif
 
-static const char imap_send_usage[] = git imap-send  mbox;
-
-#undef DRV_OK
-#define DRV_OK  0
-#define DRV_MSG_BAD -1
-#define DRV_BOX_BAD -2
-#define DRV_STORE_BAD   -3
-
-static int Verbose, Quiet;
-
-__attribute__((format (printf, 1, 2)))
-static void imap_info(const char *, ...);
-__attribute__((format (printf, 1, 2)))
-static void imap_warn(const char *, ...);
-
-static char *next_arg(char **);
-
-__attribute__((format (printf, 3, 4)))
-static int nfsnprintf(char *buf, int blen, const char *fmt, ...);
+#include curl/curl.h
 

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

[Jonathan Nieder]

Bernhard Reiter wrote:

 Use libcurl's high-level API functions to implement git-imap-send
 instead of the previous low-level OpenSSL-based functions.

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

[...]
 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 reduces imap-send.c by some 1200 lines of code.

 As I don't have access to that many IMAP servers, I haven't been able to
 test a variety of parameter combinations. I did test both secure and
 insecure (imaps:// and imap://) connections -- this e-mail was generated
 that way -- but could e.g. neither test the authMethod nor the tunnel
 parameter.

The above two paragraphs belong in the commit message, too, since
they'll be just as useful for someone looking back through the history
as for someone reading the patch on-list today.

[...]
 --- a/INSTALL
 +++ b/INSTALL
[...]
 - - libcurl library is used by git-http-fetch and git-fetch.  You
 + - libcurl library is used by git-http-fetch, git-fetch, and
 +   git-impap-send.  You might also want the curl executable for

Typo: s/impap-send/imap-send/

 --- a/Makefile
 +++ b/Makefile
 @@ -2067,9 +2067,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 http.o GIT-LDFLAGS $(GITLIBS)
   $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 - $(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
 + $(LIBS) $(CURL_LIBCURL)

7.30.0 is only ~1 year old.  Does this mean users would need to update
curl in order to build imap-send?

For example, Debian 7.6 ships curl 7.26.0 and CentOS 7 has curl 7.29.0.

Ideally this could be done as an optional feature:

 1. Introduce a USE_CURL_FOR_IMAP_SEND makefile variable to take
advantage of the nice new API.

 2. (optional) Use the curl_check makefile variable to turn on
USE_CURL_FOR_IMAP_SEND automatically when appropriate.

 3. In a few years, when everyone has upgraded, we could simplify by
getting rid of the USE_OPENSSL_FOR_IMAP_SEND code path.

What do you think?

[...]
 --- a/imap-send.c
 +++ b/imap-send.c
 @@ -22,47 +22,13 @@
   *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
   */
  
 -#include cache.h

Usual style is to start with a #include of cache.h or
git-compat-util.h.  http.h including cache.h for itself was an
old mistake.  (I'll reply with a patch to fix that.)

[...]
 +#include curl/curl.h

http.h already #includes this.  Do you use other helpers from
http.h/http.c or do you use libcurl directly?  (Just curious.)

Some style nits:

[...]
 +static curl_socket_t opensocket(void *clientp, curlsocktype purpose,
 + struct 
 curl_sockaddr *address)

Long line.  Do you have ts=4?  (Git uses 8-space tabs.  There's some
emacs magic in Documentation/CodingGuidelines.  It should be possible
to add similar hints there for other editors if they don't do the
right thing by default.)

 +{
 + curl_socket_t sockfd;
 + (void)purpose;
 + (void)address;

Elsewhere git lets unused parameters be.  The unused parameter warning
is too noisy in callback-heavy code (e.g., for_each_ref) so we don't
turn it on.

 + sockfd = *(curl_socket_t *)clientp;
 + /* the actual externally set socket is passed in via the OPENSOCKETDATA
 +option */

(style nit) Comments in git look like this:

/*
 * The actual, externally set socket is passed in via the
 * OPENSOCKETDATA option.
 */
return sockfd;

[...]
 +static int sockopt_callback(void *clientp, curl_socket_t curlfd,
 + curlsocktype purpose)
 +{
 + /* This return code was added in libcurl 7.21.5 */
 + return CURL_SOCKOPT_ALREADY_CONNECTED;

I'd drop the comment, unless there's some subtlety involved.  (E.g.,
is there some other return code that would be more appropriate but was
introudced later or something?)

[...]
 @@ -1368,12 +218,14 @@ static int git_imap_config(const char *key, const char 
 *val, void *cb)
  int main(int argc, char **argv)
  {
   struct strbuf all_msgs = STRBUF_INIT;
 - struct strbuf msg = STRBUF_INIT;
 + struct buffer msg = { STRBUF_INIT, 0 };

Ah, ok --- we do use http.c stuff.

[...]
 + char path[8192];
 + int pathlen;

I realize the old code only had 8192 for the IMAP command buffer,
but could this be a strbuf now, or is there some underlying limit
somewhere else?

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

http.c seems to make the same mistake, but should the return value
from this be checked?

 - /* write it to the imap server */
 -