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 */
> - 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(server.host, "/"))
> + pathlen = snprintf (path, sizeof(path), "%s%s", server.host, imap_folder);
> + else
> + pathlen = snprintf (path, sizeof(path), "%s/%s", server.host, 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
strbuf_addstr(&path, server.host);
if (!path.len || path.buf[path.len - 1] != '/')
strbuf_addch(&path, '/');
strbuf_addstr(&path, imap_folder);
or
if (ends_with(...))
strbuf_addf(&path, "%s%s", ...);
else
strbuf_addf(...);
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
argv_array:
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 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.
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
http://fedoraproject.org/wiki/FedoraCryptoConsolidation 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,
Jonathan
--
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