[PATCH/RFC 1/7] imap-send: use separate read and write fds

[PATCH/RFC 1/7] imap-send: use separate read and write fds

[Erik Faye-Lund]

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 imap-send.c |   37 +++++++++++++++++++++++--------------
 1 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 3847fd1..ba50961 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -154,7 +154,7 @@ struct imap_list {
 };
 
 struct imap_socket {
-	int fd;
+	int fd[2];
 	SSL *ssl;
 };
 
@@ -304,8 +304,12 @@ static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int ve
 		ssl_socket_perror("SSL_new");
 		return -1;
 	}
-	if (!SSL_set_fd(sock->ssl, sock->fd)) {
-		ssl_socket_perror("SSL_set_fd");
+	if (!SSL_set_rfd(sock->ssl, sock->fd[0])) {
+		ssl_socket_perror("SSL_set_rfd");
+		return -1;
+	}
+	if (!SSL_set_wfd(sock->ssl, sock->fd[1])) {
+		ssl_socket_perror("SSL_set_wfd");
 		return -1;
 	}
 
@@ -327,11 +331,12 @@ static int socket_read(struct imap_socket *sock, char *buf, int len)
 		n = SSL_read(sock->ssl, buf, len);
 	else
 #endif
-		n = xread(sock->fd, buf, len);
+		n = xread(sock->fd[0], buf, len);
 	if (n <= 0) {
 		socket_perror("read", sock, n);
-		close(sock->fd);
-		sock->fd = -1;
+		close(sock->fd[0]);
+		close(sock->fd[1]);
+		sock->fd[0] = sock->fd[1] = -1;
 	}
 	return n;
 }
@@ -344,11 +349,12 @@ static int socket_write(struct imap_socket *sock, const char *buf, int len)
 		n = SSL_write(sock->ssl, buf, len);
 	else
 #endif
-		n = write_in_full(sock->fd, buf, len);
+		n = write_in_full(sock->fd[1], buf, len);
 	if (n != len) {
 		socket_perror("write", sock, n);
-		close(sock->fd);
-		sock->fd = -1;
+		close(sock->fd[0]);
+		close(sock->fd[1]);
+		sock->fd[0] = sock->fd[1] = -1;
 	}
 	return n;
 }
@@ -361,7 +367,8 @@ static void socket_shutdown(struct imap_socket *sock)
 		SSL_free(sock->ssl);
 	}
 #endif
-	close(sock->fd);
+	close(sock->fd[0]);
+	close(sock->fd[1]);
 }
 
 /* simple line buffering */
@@ -960,7 +967,7 @@ static void imap_close_server(struct imap_store *ictx)
 {
 	struct imap *imap = ictx->imap;
 
-	if (imap->buf.sock.fd != -1) {
+	if (imap->buf.sock.fd[0] != -1) {
 		imap_exec(ictx, NULL, "LOGOUT");
 		socket_shutdown(&imap->buf.sock);
 	}
@@ -988,7 +995,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 	ctx = xcalloc(sizeof(*ctx), 1);
 
 	ctx->imap = imap = xcalloc(sizeof(*imap), 1);
-	imap->buf.sock.fd = -1;
+	imap->buf.sock.fd[0] = imap->buf.sock.fd[1] = -1;
 	imap->in_progress_append = &imap->in_progress;
 
 	/* open connection to IMAP server */
@@ -1015,7 +1022,8 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 
 		close(a[0]);
 
-		imap->buf.sock.fd = a[1];
+		imap->buf.sock.fd[0] = a[1];
+		imap->buf.sock.fd[1] = dup(a[1]);
 
 		imap_info("ok\n");
 	} else {
@@ -1092,7 +1100,8 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 			goto bail;
 		}
 
-		imap->buf.sock.fd = s;
+		imap->buf.sock.fd[0] = s;
+		imap->buf.sock.fd[1] = dup(s);
 
 		if (srvc->use_ssl &&
 		    ssl_socket_connect(&imap->buf.sock, 0, srvc->ssl_verify)) {
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 2/7] imap-send: use run-command API for tunneling

[Erik Faye-Lund]

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 imap-send.c |   37 ++++++++++++++++---------------------
 1 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index ba50961..55a663a 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -24,6 +24,7 @@
 
 #include "cache.h"
 #include "exec_cmd.h"
+#include "run-command.h"
 #ifdef NO_OPENSSL
 typedef void *SSL;
 #endif
@@ -989,8 +990,7 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 	struct imap_store *ctx;
 	struct imap *imap;
 	char *arg, *rsp;
-	int s = -1, a[2], preauth;
-	pid_t pid;
+	int s = -1, preauth;
 
 	ctx = xcalloc(sizeof(*ctx), 1);
 
@@ -1001,29 +1001,24 @@ static struct store *imap_open_store(struct imap_server_conf *srvc)
 	/* open connection to IMAP server */
 
 	if (srvc->tunnel) {
-		imap_info("Starting tunnel '%s'... ", srvc->tunnel);
+		const char *argv[4];
+		struct child_process tunnel = {0};
 
-		if (socketpair(PF_UNIX, SOCK_STREAM, 0, a)) {
-			perror("socketpair");
-			exit(1);
-		}
+		imap_info("Starting tunnel '%s'... ", srvc->tunnel);
 
-		pid = fork();
-		if (pid < 0)
-			_exit(127);
-		if (!pid) {
-			if (dup2(a[0], 0) == -1 || dup2(a[0], 1) == -1)
-				_exit(127);
-			close(a[0]);
-			close(a[1]);
-			execl("/bin/sh", "sh", "-c", srvc->tunnel, NULL);
-			_exit(127);
-		}
+		argv[0] = "/bin/sh";
+		argv[1] = "-c";
+		argv[2] = srvc->tunnel;
+		argv[3] = NULL;
 
-		close(a[0]);
+		tunnel.argv = argv;
+		tunnel.in = -1;
+		tunnel.out = -1;
+		if (start_command(&tunnel))
+			die("cannot start proxy %s", argv[0]);
 
-		imap->buf.sock.fd[0] = a[1];
-		imap->buf.sock.fd[1] = dup(a[1]);
+		imap->buf.sock.fd[0] = tunnel.out;
+		imap->buf.sock.fd[1] = tunnel.in;
 
 		imap_info("ok\n");
 	} else {
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 3/7] imap-send: fix compilation-error on Windows

[Erik Faye-Lund]

mmsystem.h (included from windows.h) defines DRV_OK to 1. To avoid
an error due to DRV_OK redefenition, this patch undefines the old
definition (i.e the one from mmsystem.h) before defining DRV_OK.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 imap-send.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 55a663a..8338717 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -94,6 +94,10 @@ struct msg_data {
 	unsigned int crlf:1;
 };
 
+#ifdef DRV_OK
+#undef DRV_OK
+#endif
+
 #define DRV_OK          0
 #define DRV_MSG_BAD     -1
 #define DRV_BOX_BAD     -2
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 4/7] imap-send: build imap-send on Windows

[Erik Faye-Lund]

Since the POSIX-specific tunneling code has been replaced
by the run-command API (and a compile-error has been
cleaned away), we can now enable imap-send on Windows
builds.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 12defd4..8ba789a 100644
--- a/Makefile
+++ b/Makefile
@@ -361,6 +361,7 @@ PROGRAMS += git-show-index$X
 PROGRAMS += git-unpack-file$X
 PROGRAMS += git-upload-pack$X
 PROGRAMS += git-var$X
+PROGRAMS += git-imap-send$X
 
 # List built-in command $C whose implementation cmd_$C() is not in
 # builtin-$C.o but is linked in as part of some other command.
@@ -1056,7 +1057,6 @@ EXTLIBS += -lz
 
 ifndef NO_POSIX_ONLY_PROGRAMS
 	PROGRAMS += git-daemon$X
-	PROGRAMS += git-imap-send$X
 endif
 ifndef NO_OPENSSL
 	OPENSSL_LIBSSL = -lssl
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 5/7] imap-send: provide fall-back random-source

[Erik Faye-Lund]

Since some systems (at least Windows) does not have
/dev/random nor friends, we need another random-source.

This patch uses the C-runtime's rand()-function as a
poor-mans random-source.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 imap-send.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 8338717..dda7b7f 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -511,14 +511,17 @@ static void arc4_init(void)
 	unsigned char j, si, dat[128];
 
 	if ((fd = open("/dev/urandom", O_RDONLY)) < 0 && (fd = open("/dev/random", O_RDONLY)) < 0) {
-		fprintf(stderr, "Fatal: no random number source available.\n");
-		exit(3);
-	}
-	if (read_in_full(fd, dat, 128) != 128) {
-		fprintf(stderr, "Fatal: cannot read random number source.\n");
-		exit(3);
+		/* poor-mans random-source */
+		srand(clock());
+		for (i = 0; i < 128; ++i)
+			dat[i] = rand() & 0xFF;
+	} else {
+		if (read_in_full(fd, dat, 128) != 128) {
+			fprintf(stderr, "Fatal: cannot read random number source.\n");
+			exit(3);
+		}
+		close(fd);
 	}
-	close(fd);
 
 	for (i = 0; i < 256; i++)
 		rs.s[i] = i;
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 6/7] mingw: wrap SSL_set_(w|r)fd to call _get_osfhandle

[Erik Faye-Lund]

SSL_set_fd (and friends) expects a OS file handle on Windows, not
a file descriptor as on UNIX(-ish).

This patch makes the Windows version of SSL_set_fd behave like the
UNIX versions, by calling _get_osfhandle on it's input.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 compat/mingw.h |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/compat/mingw.h b/compat/mingw.h
index 5b5258b..6907345 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -124,6 +124,27 @@ static inline int waitpid(pid_t pid, int *status, unsigned options)
 	return -1;
 }
 
+#ifndef NO_OPENSSL
+#include <openssl/ssl.h>
+static inline int mingw_SSL_set_fd(SSL *ssl, int fd)
+{
+	return SSL_set_fd(ssl, _get_osfhandle(fd));
+}
+#define SSL_set_fd mingw_SSL_set_fd
+
+static inline int mingw_SSL_set_rfd(SSL *ssl, int fd)
+{
+	return SSL_set_rfd(ssl, _get_osfhandle(fd));
+}
+#define SSL_set_rfd mingw_SSL_set_rfd
+
+static inline int mingw_SSL_set_wfd(SSL *ssl, int fd)
+{
+	return SSL_set_wfd(ssl, _get_osfhandle(fd));
+}
+#define SSL_set_wfd mingw_SSL_set_wfd
+#endif
+
 /*
  * implementations of missing functions
  */
-- 
1.6.5.rc2.7.g4f8d3

[PATCH/RFC 7/7] mingw: enable OpenSSL

[Erik Faye-Lund]

Since we have OpenSSL in msysgit now, enable it to support SSL
encryption for imap-send.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 Makefile |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 8ba789a..8818f0f 100644
--- a/Makefile
+++ b/Makefile
@@ -933,7 +933,6 @@ else
 ifneq (,$(findstring MINGW,$(uname_S)))
 	pathsep = ;
 	NO_PREAD = YesPlease
-	NO_OPENSSL = YesPlease
 	NO_LIBGEN_H = YesPlease
 	NO_SYMLINK_HEAD = YesPlease
 	NO_IPV6 = YesPlease
-- 
1.6.5.rc2.7.g4f8d3

Re: [PATCH/RFC 5/7] imap-send: provide fall-back random-source

[Jeff King]

On Sat, Oct 03, 2009 at 12:39:43AM +0000, Erik Faye-Lund wrote:

> Since some systems (at least Windows) does not have
> /dev/random nor friends, we need another random-source.
> 
> This patch uses the C-runtime's rand()-function as a
> poor-mans random-source.

Hmm. It looks like this arc4 RNG is used just for generating a unique
"X-TUID" header. Which seems to be used in isync (from which imap-send
is derived) to be to avoid duplicates in synchronization. But imap-send
doesn't actually use it for anything, as it just blindly pushes the
messages.

In other words, should all of this TUID code (and the arc4 code) simply
be ripped out?

-Peff

Re: [PATCH/RFC 5/7] imap-send: provide fall-back random-source

[Erik Faye-Lund]

On Sat, Oct 3, 2009 at 2:58 AM, Jeff King <peff@peff.net> wrote:
>
> Hmm. It looks like this arc4 RNG is used just for generating a unique
> "X-TUID" header. Which seems to be used in isync (from which imap-send
> is derived) to be to avoid duplicates in synchronization. But imap-send
> doesn't actually use it for anything, as it just blindly pushes the
> messages.
>
> In other words, should all of this TUID code (and the arc4 code) simply
> be ripped out?

Possibly. I must admit that I didn't dig far on this one - I just
added some randomness, and saw that things worked for me.

I tried to trace this a little bit, but I got lost a bit in the
callback-stuff. However, it looks to me like it might get sent to the
server: it gets injected into cb.data in imap_store_msg(), and in
v_issue_imap_cmd() it gets sent to the server if the LITERALPLUS
capability is supported. I might be wrong though, as I find this code
quite confusing.

I CC'ed Mike McCormack, who initially added imap-send (including the
arc4 code), as he *might* have more insight on this. It's a long-shot
though, considering that this appears to be code adapted around 3.5
years ago.

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Re: [PATCH/RFC 5/7] imap-send: provide fall-back random-source

[Erik Faye-Lund]

On Sat, Oct 3, 2009 at 11:45 AM, Erik Faye-Lund
<kusmabite@googlemail.com> wrote:
> I CC'ed Mike McCormack, who initially added imap-send (including the
> arc4 code), as he *might* have more insight on this. It's a long-shot
> though, considering that this appears to be code adapted around 3.5
> years ago.

OK, it seems the e-mail address he used for the commit isn't valid any
more, and I can't easily find another address for him. So I guess
we're out of luck on that front.

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Re: [PATCH/RFC 5/7] imap-send: provide fall-back random-source

[Jeff King]

On Sat, Oct 03, 2009 at 11:45:55AM -0700, Erik Faye-Lund wrote:

> I tried to trace this a little bit, but I got lost a bit in the
> callback-stuff. However, it looks to me like it might get sent to the
> server: it gets injected into cb.data in imap_store_msg(), and in
> v_issue_imap_cmd() it gets sent to the server if the LITERALPLUS
> capability is supported. I might be wrong though, as I find this code
> quite confusing.

It does get stored on the server either way (LITERALPLUS is just an imap
server extension that gives us different options for how we send). The
code actually munges an extra "X-TUID" rfc822 header into your message,
which has a randomly generated value. But we never _use_ the value for
anything. I think it is just inherited via cut-and-paste from the
original isync, which I guess actually does use it for synchronization.

The patch below rips it (and the arc4 code) out completely. It still
works in my simple test case, but I am not really an imap-send user, so
caveat emptor.

The other confusing bit is that the code carefully tracks the "uid"
(deep within the call chain it munges cb.ctx, which is a pointer to uid)
which is assigned to the newly created message by the server. This could
be used by a client to later refer to the same message unambiguously.
But we never do that, and just throw away the uid value that the server
gives us.  Again, I suspect this is a holdover from isync wanting to do
repeated synchronization (and it looks like this x-tuid stuff may be
about working around servers which don't support certain uid
operations).

So that could probably be ripped out, too, with no ill effect.

That's just from looking at the code for a few minutes. I would not be
surprised if there is more useless cruft, nor would I be surprised to
find that I am totally wrong about something above.

Anyway, here is the patch to rip out the arc4 stuff. It has a very
pleasant diff-stat. :)

---
 imap-send.c |  130 ++--------------------------------------------------------
 1 files changed, 5 insertions(+), 125 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 3847fd1..d60a0bd 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -123,9 +123,6 @@ static int nfvasprintf(char **strp, const char *fmt, va_list ap)
 	return len;
 }
 
-static void arc4_init(void);
-static unsigned char arc4_getbyte(void);
-
 struct imap_server_conf {
 	char *name;
 	char *tunnel;
@@ -489,52 +486,6 @@ static int nfsnprintf(char *buf, int blen, const char *fmt, ...)
 	return ret;
 }
 
-static struct {
-	unsigned char i, j, s[256];
-} rs;
-
-static void arc4_init(void)
-{
-	int i, fd;
-	unsigned char j, si, dat[128];
-
-	if ((fd = open("/dev/urandom", O_RDONLY)) < 0 && (fd = open("/dev/random", O_RDONLY)) < 0) {
-		fprintf(stderr, "Fatal: no random number source available.\n");
-		exit(3);
-	}
-	if (read_in_full(fd, dat, 128) != 128) {
-		fprintf(stderr, "Fatal: cannot read random number source.\n");
-		exit(3);
-	}
-	close(fd);
-
-	for (i = 0; i < 256; i++)
-		rs.s[i] = i;
-	for (i = j = 0; i < 256; i++) {
-		si = rs.s[i];
-		j += si + dat[i & 127];
-		rs.s[i] = rs.s[j];
-		rs.s[j] = si;
-	}
-	rs.i = rs.j = 0;
-
-	for (i = 0; i < 256; i++)
-		arc4_getbyte();
-}
-
-static unsigned char arc4_getbyte(void)
-{
-	unsigned char si, sj;
-
-	rs.i++;
-	si = rs.s[rs.i];
-	rs.j += si;
-	sj = rs.s[rs.j];
-	rs.s[rs.i] = sj;
-	rs.s[rs.j] = si;
-	return rs.s[(si + sj) & 0xff];
-}
-
 static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,
 					 struct imap_cmd_cb *cb,
 					 const char *fmt, va_list ap)
@@ -1198,88 +1149,20 @@ static int imap_make_flags(int flags, char *buf)
 	return d;
 }
 
-#define TUIDL 8
-
 static int imap_store_msg(struct store *gctx, struct msg_data *data, int *uid)
 {
 	struct imap_store *ctx = (struct imap_store *)gctx;
 	struct imap *imap = ctx->imap;
 	struct imap_cmd_cb cb;
-	char *fmap, *buf;
 	const char *prefix, *box;
-	int ret, i, j, d, len, extra, nocr;
-	int start, sbreak = 0, ebreak = 0;
-	char flagstr[128], tuid[TUIDL * 2 + 1];
+	int ret, d;
+	char flagstr[128];
 
 	memset(&cb, 0, sizeof(cb));
 
-	fmap = data->data;
-	len = data->len;
-	nocr = !data->crlf;
-	extra = 0, i = 0;
-	if (!CAP(UIDPLUS) && uid) {
-	nloop:
-		start = i;
-		while (i < len)
-			if (fmap[i++] == '\n') {
-				extra += nocr;
-				if (i - 2 + nocr == start) {
-					sbreak = ebreak = i - 2 + nocr;
-					goto mktid;
-				}
-				if (!memcmp(fmap + start, "X-TUID: ", 8)) {
-					extra -= (ebreak = i) - (sbreak = start) + nocr;
-					goto mktid;
-				}
-				goto nloop;
-			}
-		/* invalid message */
-		free(fmap);
-		return DRV_MSG_BAD;
-	mktid:
-		for (j = 0; j < TUIDL; j++)
-			sprintf(tuid + j * 2, "%02x", arc4_getbyte());
-		extra += 8 + TUIDL * 2 + 2;
-	}
-	if (nocr)
-		for (; i < len; i++)
-			if (fmap[i] == '\n')
-				extra++;
-
-	cb.dlen = len + extra;
-	buf = cb.data = xmalloc(cb.dlen);
-	i = 0;
-	if (!CAP(UIDPLUS) && uid) {
-		if (nocr) {
-			for (; i < sbreak; i++)
-				if (fmap[i] == '\n') {
-					*buf++ = '\r';
-					*buf++ = '\n';
-				} else
-					*buf++ = fmap[i];
-		} else {
-			memcpy(buf, fmap, sbreak);
-			buf += sbreak;
-		}
-		memcpy(buf, "X-TUID: ", 8);
-		buf += 8;
-		memcpy(buf, tuid, TUIDL * 2);
-		buf += TUIDL * 2;
-		*buf++ = '\r';
-		*buf++ = '\n';
-		i = ebreak;
-	}
-	if (nocr) {
-		for (; i < len; i++)
-			if (fmap[i] == '\n') {
-				*buf++ = '\r';
-				*buf++ = '\n';
-			} else
-				*buf++ = fmap[i];
-	} else
-		memcpy(buf, fmap + i, len - i);
-
-	free(fmap);
+	cb.dlen = data->len;
+	cb.data = xmalloc(cb.dlen);
+	memcpy(cb.data, data->data, data->len);
 
 	d = 0;
 	if (data->flags) {
@@ -1491,9 +1374,6 @@ int main(int argc, char **argv)
 
 	git_extract_argv0_path(argv[0]);
 
-	/* init the random number generator */
-	arc4_init();
-
 	setup_git_directory_gently(&nongit_ok);
 	git_config(git_imap_config, NULL);
 

Re: [PATCH/RFC 5/7] imap-send: provide fall-back random-source

[Jeff King]

On Sat, Oct 03, 2009 at 04:43:17PM -0400, Jeff King wrote:

> The other confusing bit is that the code carefully tracks the "uid"
> (deep within the call chain it munges cb.ctx, which is a pointer to uid)
> which is assigned to the newly created message by the server. This could
> be used by a client to later refer to the same message unambiguously.
> But we never do that, and just throw away the uid value that the server
> gives us.  Again, I suspect this is a holdover from isync wanting to do
> repeated synchronization (and it looks like this x-tuid stuff may be
> about working around servers which don't support certain uid
> operations).
> 
> So that could probably be ripped out, too, with no ill effect.

And here is a patch (on top of the earlier one) to do that.

Even more can be ripped out from the lower levels, too, I'm not sure if
it is worth it. Ripping out the arc4 code is worthwhile, because it
solves a portability problem. Ripping out more isn't really helping us
much. Less code makes it easier to read, but given our lack of tests and
my relatively small knowledge of this code, it is entirely possible I am
introducing new bugs.

---

 imap-send.c |   25 ++++++-------------------
 1 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index d60a0bd..8da7a94 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1149,7 +1149,7 @@ static int imap_make_flags(int flags, char *buf)
 	return d;
 }
 
-static int imap_store_msg(struct store *gctx, struct msg_data *data, int *uid)
+static int imap_store_msg(struct store *gctx, struct msg_data *data)
 {
 	struct imap_store *ctx = (struct imap_store *)gctx;
 	struct imap *imap = ctx->imap;
@@ -1171,26 +1171,14 @@ static int imap_store_msg(struct store *gctx, struct msg_data *data, int *uid)
 	}
 	flagstr[d] = 0;
 
-	if (!uid) {
-		box = gctx->conf->trash;
-		prefix = ctx->prefix;
-		cb.create = 1;
-		if (ctx->trashnc)
-			imap->caps = imap->rcaps & ~(1 << LITERALPLUS);
-	} else {
-		box = gctx->name;
-		prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
-		cb.create = 0;
-	}
-	cb.ctx = uid;
+	box = gctx->name;
+	prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
+	cb.create = 0;
 	ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" %s", prefix, box, flagstr);
 	imap->caps = imap->rcaps;
 	if (ret != DRV_OK)
 		return ret;
-	if (!uid)
-		ctx->trashnc = 0;
-	else
-		gctx->count++;
+	gctx->count++;
 
 	return DRV_OK;
 }
@@ -1366,7 +1354,6 @@ int main(int argc, char **argv)
 {
 	struct msg_data all_msgs, msg;
 	struct store *ctx = NULL;
-	int uid = 0;
 	int ofs = 0;
 	int r;
 	int total, n = 0;
@@ -1420,7 +1407,7 @@ int main(int argc, char **argv)
 			break;
 		if (server.use_html)
 			wrap_in_html(&msg);
-		r = imap_store_msg(ctx, &msg, &uid);
+		r = imap_store_msg(ctx, &msg);
 		if (r != DRV_OK)
 			break;
 		n++;

Re: [PATCH/RFC 5/7] imap-send: provide fall-back random-source

[Erik Faye-Lund]

On Sat, Oct 3, 2009 at 10:52 PM, Jeff King <peff@peff.net> wrote:
>> So that could probably be ripped out, too, with no ill effect.
>
> And here is a patch (on top of the earlier one) to do that.

Alright, so I'm spinning a new version of this series, and I'm
wondering a bit how to include patches like these, where there's no
commit message (because it was a sketch or something, I guess) or
sign-off. Should I send the commit-messages to the author and have the
him/her sign off on them, or should I set me as author and credit the
real author for the actual work in the commit message? I see the
latter have been done quite a bit in git.git already. The benefit of
the first one is of course that authorship is retained, but the
backside is that it incorrectly looks like the author wrote the commit
message.

Are there any preferences?

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Re: [PATCH/RFC 5/7] imap-send: provide fall-back random-source

[Jeff King]

On Fri, Oct 09, 2009 at 01:16:50AM +0200, Erik Faye-Lund wrote:

> > And here is a patch (on top of the earlier one) to do that.
> 
> Alright, so I'm spinning a new version of this series, and I'm
> wondering a bit how to include patches like these, where there's no
> commit message (because it was a sketch or something, I guess) or
> sign-off. Should I send the commit-messages to the author and have the
> him/her sign off on them, or should I set me as author and credit the
> real author for the actual work in the commit message? I see the
> latter have been done quite a bit in git.git already. The benefit of
> the first one is of course that authorship is retained, but the
> backside is that it incorrectly looks like the author wrote the commit
> message.
> 
> Are there any preferences?

Usually a "something like this" patch means it is not very well tested,
and that was certainly the case here (I don't even used imap-send). But
if somebody else (like you) concurs that it is the sane thing to do and
has tested or run with it for a while, then maybe it is time to promote
it.

As for how to do that, both of your proposed solutions happen in
practice. Usually I would only mark myself as author if I picked up
somebody's "how about this" and tweaked it significantly or added
something to it. If you write the commit message, or add tests, or
whatever, then usually it is a good idea to just say so in the commit
message.

But when in doubt, ask the original author what they want to do. In this
case, I think it is best for me to write the message, as I did a fair
bit of looking into the reasons for this code.

So here goes. It's still only lightly tested by me, but we have no test
scripts at all for imap-send. I suspect getting it into 'next' is the
best way to actually get it tested.

-- >8 --
Subject: [PATCH] imap-send: remove useless uid code

The imap-send code is based on code from isync, a program
for syncing imap mailboxes. Because of this, it has
inherited some code that makes sense for isync, but not for
imap-send.

In particular, when storing a message, it does one of:

  - if the server supports it, note the server-assigned
    unique identifier (UID) given to each message

  - otherwise, assigned a random UID and store it in the
    message header as X-TUID

Presumably this is used in isync to be able to synchronize
mailstores multiple times without duplication. But for
imap-send, it the values are useless; we never do anything
with them and simply forget them at the end of the program.

This patch removes the useless code. Not only is it nice for
maintainability to get rid of dead code, but the removed
code relied on the existence of /dev/urandom, which made it
a portability problem for non-Unix platforms.

Signed-off-by: Jeff King <peff@peff.net>
---
As you may be able to tell from the commit message, this is both of the
patches rolled into one. I could probably rip out even more, but it's
not really worth the time. This one solves the portability issue.

 imap-send.c |  155 ++++------------------------------------------------------
 1 files changed, 11 insertions(+), 144 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 3847fd1..8da7a94 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -123,9 +123,6 @@ static int nfvasprintf(char **strp, const char *fmt, va_list ap)
 	return len;
 }
 
-static void arc4_init(void);
-static unsigned char arc4_getbyte(void);
-
 struct imap_server_conf {
 	char *name;
 	char *tunnel;
@@ -489,52 +486,6 @@ static int nfsnprintf(char *buf, int blen, const char *fmt, ...)
 	return ret;
 }
 
-static struct {
-	unsigned char i, j, s[256];
-} rs;
-
-static void arc4_init(void)
-{
-	int i, fd;
-	unsigned char j, si, dat[128];
-
-	if ((fd = open("/dev/urandom", O_RDONLY)) < 0 && (fd = open("/dev/random", O_RDONLY)) < 0) {
-		fprintf(stderr, "Fatal: no random number source available.\n");
-		exit(3);
-	}
-	if (read_in_full(fd, dat, 128) != 128) {
-		fprintf(stderr, "Fatal: cannot read random number source.\n");
-		exit(3);
-	}
-	close(fd);
-
-	for (i = 0; i < 256; i++)
-		rs.s[i] = i;
-	for (i = j = 0; i < 256; i++) {
-		si = rs.s[i];
-		j += si + dat[i & 127];
-		rs.s[i] = rs.s[j];
-		rs.s[j] = si;
-	}
-	rs.i = rs.j = 0;
-
-	for (i = 0; i < 256; i++)
-		arc4_getbyte();
-}
-
-static unsigned char arc4_getbyte(void)
-{
-	unsigned char si, sj;
-
-	rs.i++;
-	si = rs.s[rs.i];
-	rs.j += si;
-	sj = rs.s[rs.j];
-	rs.s[rs.i] = sj;
-	rs.s[rs.j] = si;
-	return rs.s[(si + sj) & 0xff];
-}
-
 static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,
 					 struct imap_cmd_cb *cb,
 					 const char *fmt, va_list ap)
@@ -1198,88 +1149,20 @@ static int imap_make_flags(int flags, char *buf)
 	return d;
 }
 
-#define TUIDL 8
-
-static int imap_store_msg(struct store *gctx, struct msg_data *data, int *uid)
+static int imap_store_msg(struct store *gctx, struct msg_data *data)
 {
 	struct imap_store *ctx = (struct imap_store *)gctx;
 	struct imap *imap = ctx->imap;
 	struct imap_cmd_cb cb;
-	char *fmap, *buf;
 	const char *prefix, *box;
-	int ret, i, j, d, len, extra, nocr;
-	int start, sbreak = 0, ebreak = 0;
-	char flagstr[128], tuid[TUIDL * 2 + 1];
+	int ret, d;
+	char flagstr[128];
 
 	memset(&cb, 0, sizeof(cb));
 
-	fmap = data->data;
-	len = data->len;
-	nocr = !data->crlf;
-	extra = 0, i = 0;
-	if (!CAP(UIDPLUS) && uid) {
-	nloop:
-		start = i;
-		while (i < len)
-			if (fmap[i++] == '\n') {
-				extra += nocr;
-				if (i - 2 + nocr == start) {
-					sbreak = ebreak = i - 2 + nocr;
-					goto mktid;
-				}
-				if (!memcmp(fmap + start, "X-TUID: ", 8)) {
-					extra -= (ebreak = i) - (sbreak = start) + nocr;
-					goto mktid;
-				}
-				goto nloop;
-			}
-		/* invalid message */
-		free(fmap);
-		return DRV_MSG_BAD;
-	mktid:
-		for (j = 0; j < TUIDL; j++)
-			sprintf(tuid + j * 2, "%02x", arc4_getbyte());
-		extra += 8 + TUIDL * 2 + 2;
-	}
-	if (nocr)
-		for (; i < len; i++)
-			if (fmap[i] == '\n')
-				extra++;
-
-	cb.dlen = len + extra;
-	buf = cb.data = xmalloc(cb.dlen);
-	i = 0;
-	if (!CAP(UIDPLUS) && uid) {
-		if (nocr) {
-			for (; i < sbreak; i++)
-				if (fmap[i] == '\n') {
-					*buf++ = '\r';
-					*buf++ = '\n';
-				} else
-					*buf++ = fmap[i];
-		} else {
-			memcpy(buf, fmap, sbreak);
-			buf += sbreak;
-		}
-		memcpy(buf, "X-TUID: ", 8);
-		buf += 8;
-		memcpy(buf, tuid, TUIDL * 2);
-		buf += TUIDL * 2;
-		*buf++ = '\r';
-		*buf++ = '\n';
-		i = ebreak;
-	}
-	if (nocr) {
-		for (; i < len; i++)
-			if (fmap[i] == '\n') {
-				*buf++ = '\r';
-				*buf++ = '\n';
-			} else
-				*buf++ = fmap[i];
-	} else
-		memcpy(buf, fmap + i, len - i);
-
-	free(fmap);
+	cb.dlen = data->len;
+	cb.data = xmalloc(cb.dlen);
+	memcpy(cb.data, data->data, data->len);
 
 	d = 0;
 	if (data->flags) {
@@ -1288,26 +1171,14 @@ static int imap_store_msg(struct store *gctx, struct msg_data *data, int *uid)
 	}
 	flagstr[d] = 0;
 
-	if (!uid) {
-		box = gctx->conf->trash;
-		prefix = ctx->prefix;
-		cb.create = 1;
-		if (ctx->trashnc)
-			imap->caps = imap->rcaps & ~(1 << LITERALPLUS);
-	} else {
-		box = gctx->name;
-		prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
-		cb.create = 0;
-	}
-	cb.ctx = uid;
+	box = gctx->name;
+	prefix = !strcmp(box, "INBOX") ? "" : ctx->prefix;
+	cb.create = 0;
 	ret = imap_exec_m(ctx, &cb, "APPEND \"%s%s\" %s", prefix, box, flagstr);
 	imap->caps = imap->rcaps;
 	if (ret != DRV_OK)
 		return ret;
-	if (!uid)
-		ctx->trashnc = 0;
-	else
-		gctx->count++;
+	gctx->count++;
 
 	return DRV_OK;
 }
@@ -1483,7 +1354,6 @@ int main(int argc, char **argv)
 {
 	struct msg_data all_msgs, msg;
 	struct store *ctx = NULL;
-	int uid = 0;
 	int ofs = 0;
 	int r;
 	int total, n = 0;
@@ -1491,9 +1361,6 @@ int main(int argc, char **argv)
 
 	git_extract_argv0_path(argv[0]);
 
-	/* init the random number generator */
-	arc4_init();
-
 	setup_git_directory_gently(&nongit_ok);
 	git_config(git_imap_config, NULL);
 
@@ -1540,7 +1407,7 @@ int main(int argc, char **argv)
 			break;
 		if (server.use_html)
 			wrap_in_html(&msg);
-		r = imap_store_msg(ctx, &msg, &uid);
+		r = imap_store_msg(ctx, &msg);
 		if (r != DRV_OK)
 			break;
 		n++;
-- 
1.6.5.rc3.206.g63d9c

Re: [PATCH/RFC 1/7] imap-send: use separate read and write fds

[Jeff King]

On Sat, Oct 03, 2009 at 12:39:39AM +0000, Erik Faye-Lund wrote:

> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>

Why? Given its presence in this series, I can only assume it has to do
with windows portability, but it would be helpful to give a little bit
of the reasoning in the commit message.

-Peff

Re: [PATCH/RFC 1/7] imap-send: use separate read and write fds

[Erik Faye-Lund]

On Sat, Oct 3, 2009 at 2:40 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Oct 03, 2009 at 12:39:39AM +0000, Erik Faye-Lund wrote:
>
>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>
> Why? Given its presence in this series, I can only assume it has to do
> with windows portability, but it would be helpful to give a little bit
> of the reasoning in the commit message.
>
> -Peff
>

Yeah, this is about Windows portability.

I'll add something like "This is a patch that enables us to use the
run-command API, which is supported on Windows." to the commit-message
in the next round. Is that enough?

I also guess I should have made a cover letter for this series, making
it apparent to reviewers that:
- This patch series is about supporting imap-send on Windows
- It needs some additional patching to get tunnelling support working
on Windows, because we can't exec "/bin/sh" there. Changing it to
"c:\\msysgit\\bin\\sh.exe" makes tunneling work for me, but isn't
exactly portable across installations.

I'll write one up for the next round.

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Re: [PATCH/RFC 1/7] imap-send: use separate read and write fds

[Jeff King]

On Sat, Oct 03, 2009 at 11:44:50AM -0700, Erik Faye-Lund wrote:

> Yeah, this is about Windows portability.
> 
> I'll add something like "This is a patch that enables us to use the
> run-command API, which is supported on Windows." to the commit-message
> in the next round. Is that enough?

Yeah, that would be fine. I was just left scratching my head wondering
what subtle portability difference the two descriptors could have. But
if it really is just a cleanup for the next patch, that's OK; just say
so.

-Peff

Re: [PATCH/RFC 1/7] imap-send: use separate read and write fds

[Johannes Sixt]

On Samstag, 3. Oktober 2009, Erik Faye-Lund wrote:
> - It needs some additional patching to get tunnelling support working
> on Windows, because we can't exec "/bin/sh" there. Changing it to
> "c:\\msysgit\\bin\\sh.exe" makes tunneling work for me, but isn't
> exactly portable across installations.

It should be fine to just exec "sh" (without a path).

-- Hannes