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) $(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 */

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 */
> -     ctx = imap_open_store(&server);
> -     if (!ctx) {
> -             fprintf(stderr, "failed to open store\n");
> +     curl = curl_easy_init();
> +
> +     if (!curl) {
> +             fprintf(stderr, "failed to init curl\n");
>               return 1;

Could do

                die("failed to init curl");

for more consistent message format and exit codes (128 for internal
errors, with an error message starting with 'fatal: ').

> +     if (ends_with(, "/"))
> +             pathlen = snprintf (path, sizeof(path), "%s%s",, 
> imap_folder);
> +     else
> +             pathlen = snprintf (path, sizeof(path), "%s/%s",, 
> imap_folder);
> +
> +     if (pathlen < 0)
> +             die("Fatal: Out of memory");
> +     if (pathlen >= sizeof(path))
> +             die("imap URL overflow!");

With a strbuf, this could be something like

        if (!path.len || path.buf[path.len - 1] != '/')
                strbuf_addch(&path, '/');
        strbuf_addstr(&path, imap_folder);


        if (ends_with(...))
                strbuf_addf(&path, "%s%s", ...);

Killing the unused ctx->prefix handling is nice. :)

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

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

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

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.

To summarize:

 * I like this idea a lot!  Using libcurl's imaps:// support directly
   means one less dependency to worry about, and using alternate SSL
   libraries like gnutls or nss becomes much easier (e.g., see for how
   that makes configuring certificate trust simpler).

 * This would be easier to take if guarded by an #ifdef, so people
   stuck on ancient libcurl would still be able to use git (and
   ideally still use imap over SSL).

 * This shouldn't have to touch the imap.tunnel support.  imap-send's
   imap.tunnel configuration expects the tunnel to take care of
   securing the channel (e.g. by using 'openssl s_client').

 * Any potential cleanups noticed along the way are very welcome,
   as separate patches.

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

Thanks and hope that helps,
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to
More majordomo info at

Reply via email to