[PATCH/RFC 0/3] port upload-archive to windows

[PATCH/RFC 0/3] port upload-archive to windows

[Erik Faye-Lund]

Upload-archive, while compiled and installed on Windows, never
worked due to it's dependence on fork. This is a patch-series to
remedy that.

While doing this, I found a bug in gnulib's poll-emulation where
it could return 0 even when timeout was -1. This is neither
something POSIX allows, nor something our code is expecting.

So I've submitted an upstream patch for gnulib and upgraded our
copy.

Erik Faye-Lund (3):
  compat/win32/sys/poll.c: upgrade from upstream
  mingw: fix compilation of poll-emulation
  upload-archive: use start_command instead of fork

 builtin/upload-archive.c |   62 ++++++++++++----------------------------------
 compat/mingw.h           |    2 -
 compat/win32/sys/poll.c  |   17 +++++++++---
 3 files changed, 28 insertions(+), 53 deletions(-)

-- 
1.7.6.135.ge14e3f

[PATCH/RFC 1/3] compat/win32/sys/poll.c: upgrade from upstream

[Erik Faye-Lund]

poll.c is updated from revision adc3a5b in
git://git.savannah.gnu.org/gnulib.git

The changes are applied with --whitespace=fix to reduce noise.

poll.h is not upgraded, because the most recent version now
contains template-stuff that breaks compilation for us.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 compat/win32/sys/poll.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/compat/win32/sys/poll.c b/compat/win32/sys/poll.c
index 708a6c9..403eaa7 100644
--- a/compat/win32/sys/poll.c
+++ b/compat/win32/sys/poll.c
@@ -1,7 +1,7 @@
 /* Emulation for poll(2)
    Contributed by Paolo Bonzini.
 
-   Copyright 2001-2003, 2006-2010 Free Software Foundation, Inc.
+   Copyright 2001-2003, 2006-2011 Free Software Foundation, Inc.
 
    This file is part of gnulib.
 
@@ -27,7 +27,10 @@
 #include <malloc.h>
 
 #include <sys/types.h>
-#include "poll.h"
+
+/* Specification.  */
+#include <poll.h>
+
 #include <errno.h>
 #include <limits.h>
 #include <assert.h>
@@ -314,10 +317,7 @@ compute_revents (int fd, int sought, fd_set *rfds, fd_set *wfds, fd_set *efds)
 #endif /* !MinGW */
 
 int
-poll (pfd, nfd, timeout)
-     struct pollfd *pfd;
-     nfds_t nfd;
-     int timeout;
+poll (struct pollfd *pfd, nfds_t nfd, int timeout)
 {
 #ifndef WIN32_NATIVE
   fd_set rfds, wfds, efds;
@@ -454,6 +454,7 @@ poll (pfd, nfd, timeout)
   if (!hEvent)
     hEvent = CreateEvent (NULL, FALSE, FALSE, NULL);
 
+restart:
   handle_array[0] = hEvent;
   nhandles = 1;
   FD_ZERO (&rfds);
@@ -594,6 +595,12 @@ poll (pfd, nfd, timeout)
 	rc++;
     }
 
+  if (!rc && timeout == INFTIM)
+    {
+      SwitchToThread();
+      goto restart;
+    }
+
   return rc;
 #endif
 }
-- 
1.7.6.135.ge14e3f

[PATCH/RFC 2/3] mingw: fix compilation of poll-emulation

[Erik Faye-Lund]

gnulib has changed the inclusion of poll.h from double quotes
to single-quotes. But because compat/win32/sys/ isn't in our
include-path, this breaks compilation. Change it back the way
it was.

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

Ugh, I don't like this so much; this means that 1/3 is left in a
broken state, which is a bit nasty for bisection. But this is the
only way I can think of to show what are upstream-changes and what
are git-specific.

An alternative is to move Windows' poll emulation out of the 'sys'
folder, and enable the NO_SYS_POLL_H-switch for MinGW and MSVC
builds.

I'm sure what best approach is. Input, anyone? :)

 compat/win32/sys/poll.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/compat/win32/sys/poll.c b/compat/win32/sys/poll.c
index 403eaa7..225ddce 100644
--- a/compat/win32/sys/poll.c
+++ b/compat/win32/sys/poll.c
@@ -29,7 +29,7 @@
 #include <sys/types.h>
 
 /* Specification.  */
-#include <poll.h>
+#include "poll.h"
 
 #include <errno.h>
 #include <limits.h>
-- 
1.7.6.135.ge14e3f

[PATCH/RFC 3/3] upload-archive: use start_command instead of fork

[Erik Faye-Lund]

The POSIX-function fork is not supported on Windows. Use our
start_command API instead.

As this is the last call-site that depends on the fork-stub in
compat/mingw.h, remove that as well.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 builtin/upload-archive.c |   62 ++++++++++++----------------------------------
 compat/mingw.h           |    2 -
 2 files changed, 16 insertions(+), 48 deletions(-)

diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 73f788e..e05172a 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -6,6 +6,7 @@
 #include "archive.h"
 #include "pkt-line.h"
 #include "sideband.h"
+#include "run-command.h"
 
 static const char upload_archive_usage[] =
 	"git upload-archive <repo>";
@@ -18,28 +19,16 @@ static const char lostchild[] =
 
 #define MAX_ARGS (64)
 
-static int run_upload_archive(int argc, const char **argv, const char *prefix)
+static void prepare_argv(const char **sent_argv, const char **argv)
 {
-	const char *sent_argv[MAX_ARGS];
 	const char *arg_cmd = "argument ";
 	char *p, buf[4096];
 	int sent_argc;
 	int len;
 
-	if (argc != 2)
-		usage(upload_archive_usage);
-
-	if (strlen(argv[1]) + 1 > sizeof(buf))
-		die("insanely long repository name");
-
-	strcpy(buf, argv[1]); /* enter-repo smudges its argument */
-
-	if (!enter_repo(buf, 0))
-		die("'%s' does not appear to be a git repository", buf);
-
 	/* put received options in sent_argv[] */
 	sent_argc = 1;
-	sent_argv[0] = "git-upload-archive";
+	sent_argv[0] = "archive";
 	for (p = buf;;) {
 		/* This will die if not enough free space in buf */
 		len = packet_read_line(0, p, (buf + sizeof buf) - p);
@@ -62,9 +51,6 @@ static int run_upload_archive(int argc, const char **argv, const char *prefix)
 		*p++ = 0;
 	}
 	sent_argv[sent_argc] = NULL;
-
-	/* parse all options sent by the client */
-	return write_archive(sent_argc, sent_argv, prefix, 0);
 }
 
 __attribute__((format (printf, 1, 2)))
@@ -96,38 +82,22 @@ static ssize_t process_input(int child_fd, int band)
 
 int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 {
-	pid_t writer;
-	int fd1[2], fd2[2];
-	/*
-	 * Set up sideband subprocess.
-	 *
-	 * We (parent) monitor and read from child, sending its fd#1 and fd#2
-	 * multiplexed out to our fd#1.  If the child dies, we tell the other
-	 * end over channel #3.
-	 */
-	if (pipe(fd1) < 0 || pipe(fd2) < 0) {
-		int err = errno;
-		packet_write(1, "NACK pipe failed on the remote side\n");
-		die("upload-archive: %s", strerror(err));
-	}
-	writer = fork();
-	if (writer < 0) {
+	const char *sent_argv[MAX_ARGS];
+	struct child_process cld = { sent_argv };
+	cld.out = cld.err = -1;
+	cld.git_cmd = 1;
+
+	if (argc != 2)
+		usage(upload_archive_usage);
+
+	prepare_argv(sent_argv, argv);
+	if (start_command(&cld)) {
 		int err = errno;
 		packet_write(1, "NACK fork failed on the remote side\n");
 		die("upload-archive: %s", strerror(err));
 	}
-	if (!writer) {
-		/* child - connect fd#1 and fd#2 to the pipe */
-		dup2(fd1[1], 1);
-		dup2(fd2[1], 2);
-		close(fd1[1]); close(fd2[1]);
-		close(fd1[0]); close(fd2[0]); /* we do not read from pipe */
-
-		exit(run_upload_archive(argc, argv, prefix));
-	}
 
 	/* parent - read from child, multiplex and send out to fd#1 */
-	close(fd1[1]); close(fd2[1]); /* we do not write to pipe */
 	packet_write(1, "ACK\n");
 	packet_flush(1);
 
@@ -135,9 +105,9 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 		struct pollfd pfd[2];
 		int status;
 
-		pfd[0].fd = fd1[0];
+		pfd[0].fd = cld.out;
 		pfd[0].events = POLLIN;
-		pfd[1].fd = fd2[0];
+		pfd[1].fd = cld.err;
 		pfd[1].events = POLLIN;
 		if (poll(pfd, 2, -1) < 0) {
 			if (errno != EINTR) {
@@ -156,7 +126,7 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 			if (process_input(pfd[0].fd, 1))
 				continue;
 
-		if (waitpid(writer, &status, 0) < 0)
+		if (waitpid(cld.pid, &status, 0) < 0)
 			error_clnt("%s", lostchild);
 		else if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)
 			error_clnt("%s", deadchild);
diff --git a/compat/mingw.h b/compat/mingw.h
index ce9dd98..9cb6eb9 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -85,8 +85,6 @@ static inline int symlink(const char *oldpath, const char *newpath)
 { errno = ENOSYS; return -1; }
 static inline int fchmod(int fildes, mode_t mode)
 { errno = ENOSYS; return -1; }
-static inline pid_t fork(void)
-{ errno = ENOSYS; return -1; }
 static inline unsigned int alarm(unsigned int seconds)
 { return 0; }
 static inline int fsync(int fd)
-- 
1.7.6.135.ge14e3f

Re: [PATCH/RFC 3/3] upload-archive: use start_command instead of fork

[Johannes Sixt]

Am 07.07.2011 13:43, schrieb Erik Faye-Lund:
> -	if (!enter_repo(buf, 0))
> -		die("'%s' does not appear to be a git repository", buf);
> -
>  	/* put received options in sent_argv[] */
>  	sent_argc = 1;
> -	sent_argv[0] = "git-upload-archive";
> +	sent_argv[0] = "archive";

Is git-archive a valid replacement of enter_repo() followed by
git-upload-archive?

-- Hannes

Re: [PATCH/RFC 3/3] upload-archive: use start_command instead of fork

[Erik Faye-Lund]

On Thu, Jul 7, 2011 at 8:21 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 07.07.2011 13:43, schrieb Erik Faye-Lund:
>> -     if (!enter_repo(buf, 0))
>> -             die("'%s' does not appear to be a git repository", buf);
>> -
>>       /* put received options in sent_argv[] */
>>       sent_argc = 1;
>> -     sent_argv[0] = "git-upload-archive";
>> +     sent_argv[0] = "archive";
>
> Is git-archive a valid replacement of enter_repo() followed by
> git-upload-archive?
>

What do you mean "enter_repo() followed by git-upload-archive"? Don't
you mean "enter_repo() followed by write_archive"? It does work for
me, and the test-suite didn't reveal any breakages either.

write_archive gets called by both versions, but my version doesn't
call enter_repo directly. It is however called by git-daemon before
starting git-upload-archive, so perhaps that's why it works for me.
And that's a regression indeed; I don't respect the path-argument. So
expect a new version over the week-end ;)

Re: [PATCH/RFC 3/3] upload-archive: use start_command instead of fork

[Jeff King]

On Thu, Jul 07, 2011 at 01:43:09PM +0200, Erik Faye-Lund wrote:

> The POSIX-function fork is not supported on Windows. Use our
> start_command API instead.

Is start_command the right solution? From my reading, the fork is
actually because we want to set up a sideband multiplexer. Should we not
just be using start_async() to start a thread, as we do in receive-pack?

-Peff

Re: [PATCH/RFC 3/3] upload-archive: use start_command instead of fork

[Erik Faye-Lund]

On Thu, Jul 7, 2011 at 9:15 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Jul 07, 2011 at 01:43:09PM +0200, Erik Faye-Lund wrote:
>
>> The POSIX-function fork is not supported on Windows. Use our
>> start_command API instead.
>
> Is start_command the right solution? From my reading, the fork is
> actually because we want to set up a sideband multiplexer. Should we not
> just be using start_async() to start a thread, as we do in receive-pack?

I considered that, but discarded it because I figured it required me
to plug through a file descriptor all the way through the code. But
perhaps I was wrong, and dup2 will make that job a lot easier?

Re: [PATCH/RFC 3/3] upload-archive: use start_command instead of fork

[Jeff King]

On Fri, Jul 08, 2011 at 12:25:00AM +0200, Erik Faye-Lund wrote:

> On Thu, Jul 7, 2011 at 9:15 PM, Jeff King <peff@peff.net> wrote:
> > On Thu, Jul 07, 2011 at 01:43:09PM +0200, Erik Faye-Lund wrote:
> >
> >> The POSIX-function fork is not supported on Windows. Use our
> >> start_command API instead.
> >
> > Is start_command the right solution? From my reading, the fork is
> > actually because we want to set up a sideband multiplexer. Should we not
> > just be using start_async() to start a thread, as we do in receive-pack?
> 
> I considered that, but discarded it because I figured it required me
> to plug through a file descriptor all the way through the code. But
> perhaps I was wrong, and dup2 will make that job a lot easier?

Yeah, exactly. The current code is already using dup2 in the same way.

-Peff

Re: [PATCH/RFC 3/3] upload-archive: use start_command instead of fork

[Erik Faye-Lund]

On Fri, Jul 8, 2011 at 12:27 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Jul 08, 2011 at 12:25:00AM +0200, Erik Faye-Lund wrote:
>
>> On Thu, Jul 7, 2011 at 9:15 PM, Jeff King <peff@peff.net> wrote:
>> > On Thu, Jul 07, 2011 at 01:43:09PM +0200, Erik Faye-Lund wrote:
>> >
>> >> The POSIX-function fork is not supported on Windows. Use our
>> >> start_command API instead.
>> >
>> > Is start_command the right solution? From my reading, the fork is
>> > actually because we want to set up a sideband multiplexer. Should we not
>> > just be using start_async() to start a thread, as we do in receive-pack?
>>
>> I considered that, but discarded it because I figured it required me
>> to plug through a file descriptor all the way through the code. But
>> perhaps I was wrong, and dup2 will make that job a lot easier?
>
> Yeah, exactly. The current code is already using dup2 in the same way.
>

It does, but I'm not entirely sure how dup2 works when start_async is
implemented with threads. Won't cause the multiplexer to fail (the
multiplexer and the archive-thread needs different stdouts), because
file descriptors are process-resources, and not thread-resources?

I guess I could dup stdout/stderr before dup2'ing, and have the
multiplexer write to explicitly to the duped fds. It's starting to
sound very confusing to my ears, but perhaps it's the best option
still ;)

Re: [PATCH/RFC 3/3] upload-archive: use start_command instead of fork

[Jeff King]

On Fri, Jul 08, 2011 at 12:37:48AM +0200, Erik Faye-Lund wrote:

> > Yeah, exactly. The current code is already using dup2 in the same way.
> 
> It does, but I'm not entirely sure how dup2 works when start_async is
> implemented with threads. Won't cause the multiplexer to fail (the
> multiplexer and the archive-thread needs different stdouts), because
> file descriptors are process-resources, and not thread-resources?

Oh, right. I'm being a moron. Sorry.

> I guess I could dup stdout/stderr before dup2'ing, and have the
> multiplexer write to explicitly to the duped fds. It's starting to
> sound very confusing to my ears, but perhaps it's the best option
> still ;)

Yes, you would have to dup stdout in the muxer to a new fd, then write
to that explicitly. And then you would be free to dup the write half of
the async pipe to stdout. It's not all that complicated, but maybe it is
simpler and more obvious (especially to a later reader of the code) to
just run a separate command.

-Peff