{"thread":{"id":"27768","subject":"[PATCH/RFC 0/3] port upload-archive to windows","startedAt":"2011-07-07T11:43:06Z","lastAt":"2011-07-07T22:44:27Z","messageCount":11,"participants":["Erik Faye-Lund","Johannes Sixt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"170970","messageId":"1310038989-5724-1-git-send-email-kusmabite@gmail.com","threadId":"27768","inReplyTo":null,"subject":"[PATCH/RFC 0/3] port upload-archive to windows","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-07T11:43:06Z","receivedAt":"2011-07-07T11:43:06Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Upload-archive, while compiled and installed on Windows, never\nworked due to it's dependence on fork. This is a patch-series to\nremedy that.\n\nWhile doing this, I found a bug in gnulib's poll-emulation where\nit could return 0 even when timeout was -1. This is neither\nsomething POSIX allows, nor something our code is expecting.\n\nSo I've submitted an upstream patch for gnulib and upgraded our\ncopy.\n\nErik Faye-Lund (3):\n compat/win32/sys/poll.c: upgrade from upstream\n mingw: fix compilation of poll-emulation\n upload-archive: use start_command instead of fork\n\n builtin/upload-archive.c | 62 ++++++++++++----------------------------------\n compat/mingw.h | 2 -\n compat/win32/sys/poll.c | 17 +++++++++---\n 3 files changed, 28 insertions(+), 53 deletions(-)\n\n-- \n1.7.6.135.ge14e3f\n"},{"id":"170971","messageId":"1310038989-5724-2-git-send-email-kusmabite@gmail.com","threadId":"27768","inReplyTo":"1310038989-5724-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH/RFC 1/3] compat/win32/sys/poll.c: upgrade from upstream","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-07T11:43:07Z","receivedAt":"2011-07-07T11:43:07Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"poll.c is updated from revision adc3a5b in\ngit://git.savannah.gnu.org/gnulib.git\n\nThe changes are applied with --whitespace=fix to reduce noise.\n\npoll.h is not upgraded, because the most recent version now\ncontains template-stuff that breaks compilation for us.\n\nSigned-off-by: Erik Faye-Lund \n---\n compat/win32/sys/poll.c | 19 +++++++++++++------\n 1 files changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/compat/win32/sys/poll.c b/compat/win32/sys/poll.c\nindex 708a6c9..403eaa7 100644\n--- a/compat/win32/sys/poll.c\n+++ b/compat/win32/sys/poll.c\n@@ -1,7 +1,7 @@\n /* Emulation for poll(2)\n Contributed by Paolo Bonzini.\n \n- Copyright 2001-2003, 2006-2010 Free Software Foundation, Inc.\n+ Copyright 2001-2003, 2006-2011 Free Software Foundation, Inc.\n \n This file is part of gnulib.\n \n@@ -27,7 +27,10 @@\n #include \n \n #include \n-#include \"poll.h\"\n+\n+/* Specification. */\n+#include \n+\n #include \n #include \n #include \n@@ -314,10 +317,7 @@ compute_revents (int fd, int sought, fd_set *rfds, fd_set *wfds, fd_set *efds)\n #endif /* !MinGW */\n \n int\n-poll (pfd, nfd, timeout)\n- struct pollfd *pfd;\n- nfds_t nfd;\n- int timeout;\n+poll (struct pollfd *pfd, nfds_t nfd, int timeout)\n {\n #ifndef WIN32_NATIVE\n fd_set rfds, wfds, efds;\n@@ -454,6 +454,7 @@ poll (pfd, nfd, timeout)\n if (!hEvent)\n hEvent = CreateEvent (NULL, FALSE, FALSE, NULL);\n \n+restart:\n handle_array[0] = hEvent;\n nhandles = 1;\n FD_ZERO (&rfds);\n@@ -594,6 +595,12 @@ poll (pfd, nfd, timeout)\n \trc++;\n }\n \n+ if (!rc && timeout == INFTIM)\n+ {\n+ SwitchToThread();\n+ goto restart;\n+ }\n+\n return rc;\n #endif\n }\n-- \n1.7.6.135.ge14e3f\n"},{"id":"170972","messageId":"1310038989-5724-3-git-send-email-kusmabite@gmail.com","threadId":"27768","inReplyTo":"1310038989-5724-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH/RFC 2/3] mingw: fix compilation of poll-emulation","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-07T11:43:08Z","receivedAt":"2011-07-07T11:43:08Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"gnulib has changed the inclusion of poll.h from double quotes\nto single-quotes. But because compat/win32/sys/ isn't in our\ninclude-path, this breaks compilation. Change it back the way\nit was.\n\nSigned-off-by: Erik Faye-Lund \n---\n\nUgh, I don't like this so much; this means that 1/3 is left in a\nbroken state, which is a bit nasty for bisection. But this is the\nonly way I can think of to show what are upstream-changes and what\nare git-specific.\n\nAn alternative is to move Windows' poll emulation out of the 'sys'\nfolder, and enable the NO_SYS_POLL_H-switch for MinGW and MSVC\nbuilds.\n\nI'm sure what best approach is. Input, anyone? :)\n\n compat/win32/sys/poll.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/compat/win32/sys/poll.c b/compat/win32/sys/poll.c\nindex 403eaa7..225ddce 100644\n--- a/compat/win32/sys/poll.c\n+++ b/compat/win32/sys/poll.c\n@@ -29,7 +29,7 @@\n #include \n \n /* Specification. */\n-#include \n+#include \"poll.h\"\n \n #include \n #include \n-- \n1.7.6.135.ge14e3f\n"},{"id":"170973","messageId":"1310038989-5724-4-git-send-email-kusmabite@gmail.com","threadId":"27768","inReplyTo":"1310038989-5724-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH/RFC 3/3] upload-archive: use start_command instead of fork","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-07T11:43:09Z","receivedAt":"2011-07-07T11:43:09Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"The POSIX-function fork is not supported on Windows. Use our\nstart_command API instead.\n\nAs this is the last call-site that depends on the fork-stub in\ncompat/mingw.h, remove that as well.\n\nSigned-off-by: Erik Faye-Lund \n---\n builtin/upload-archive.c | 62 ++++++++++++----------------------------------\n compat/mingw.h | 2 -\n 2 files changed, 16 insertions(+), 48 deletions(-)\n\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 73f788e..e05172a 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -6,6 +6,7 @@\n #include \"archive.h\"\n #include \"pkt-line.h\"\n #include \"sideband.h\"\n+#include \"run-command.h\"\n \n static const char upload_archive_usage[] =\n \t\"git upload-archive \";\n@@ -18,28 +19,16 @@ static const char lostchild[] =\n \n #define MAX_ARGS (64)\n \n-static int run_upload_archive(int argc, const char **argv, const char *prefix)\n+static void prepare_argv(const char **sent_argv, const char **argv)\n {\n-\tconst char *sent_argv[MAX_ARGS];\n \tconst char *arg_cmd = \"argument \";\n \tchar *p, buf[4096];\n \tint sent_argc;\n \tint len;\n \n-\tif (argc != 2)\n-\t\tusage(upload_archive_usage);\n-\n-\tif (strlen(argv[1]) + 1 > sizeof(buf))\n-\t\tdie(\"insanely long repository name\");\n-\n-\tstrcpy(buf, argv[1]); /* enter-repo smudges its argument */\n-\n-\tif (!enter_repo(buf, 0))\n-\t\tdie(\"'%s' does not appear to be a git repository\", buf);\n-\n \t/* put received options in sent_argv[] */\n \tsent_argc = 1;\n-\tsent_argv[0] = \"git-upload-archive\";\n+\tsent_argv[0] = \"archive\";\n \tfor (p = buf;;) {\n \t\t/* This will die if not enough free space in buf */\n \t\tlen = packet_read_line(0, p, (buf + sizeof buf) - p);\n@@ -62,9 +51,6 @@ static int run_upload_archive(int argc, const char **argv, const char *prefix)\n \t\t*p++ = 0;\n \t}\n \tsent_argv[sent_argc] = NULL;\n-\n-\t/* parse all options sent by the client */\n-\treturn write_archive(sent_argc, sent_argv, prefix, 0);\n }\n \n __attribute__((format (printf, 1, 2)))\n@@ -96,38 +82,22 @@ static ssize_t process_input(int child_fd, int band)\n \n int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n {\n-\tpid_t writer;\n-\tint fd1[2], fd2[2];\n-\t/*\n-\t * Set up sideband subprocess.\n-\t *\n-\t * We (parent) monitor and read from child, sending its fd#1 and fd#2\n-\t * multiplexed out to our fd#1. If the child dies, we tell the other\n-\t * end over channel #3.\n-\t */\n-\tif (pipe(fd1) < 0 || pipe(fd2) < 0) {\n-\t\tint err = errno;\n-\t\tpacket_write(1, \"NACK pipe failed on the remote side\\n\");\n-\t\tdie(\"upload-archive: %s\", strerror(err));\n-\t}\n-\twriter = fork();\n-\tif (writer < 0) {\n+\tconst char *sent_argv[MAX_ARGS];\n+\tstruct child_process cld = { sent_argv };\n+\tcld.out = cld.err = -1;\n+\tcld.git_cmd = 1;\n+\n+\tif (argc != 2)\n+\t\tusage(upload_archive_usage);\n+\n+\tprepare_argv(sent_argv, argv);\n+\tif (start_command(&cld)) {\n \t\tint err = errno;\n \t\tpacket_write(1, \"NACK fork failed on the remote side\\n\");\n \t\tdie(\"upload-archive: %s\", strerror(err));\n \t}\n-\tif (!writer) {\n-\t\t/* child - connect fd#1 and fd#2 to the pipe */\n-\t\tdup2(fd1[1], 1);\n-\t\tdup2(fd2[1], 2);\n-\t\tclose(fd1[1]); close(fd2[1]);\n-\t\tclose(fd1[0]); close(fd2[0]); /* we do not read from pipe */\n-\n-\t\texit(run_upload_archive(argc, argv, prefix));\n-\t}\n \n \t/* parent - read from child, multiplex and send out to fd#1 */\n-\tclose(fd1[1]); close(fd2[1]); /* we do not write to pipe */\n \tpacket_write(1, \"ACK\\n\");\n \tpacket_flush(1);\n \n@@ -135,9 +105,9 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n \t\tstruct pollfd pfd[2];\n \t\tint status;\n \n-\t\tpfd[0].fd = fd1[0];\n+\t\tpfd[0].fd = cld.out;\n \t\tpfd[0].events = POLLIN;\n-\t\tpfd[1].fd = fd2[0];\n+\t\tpfd[1].fd = cld.err;\n \t\tpfd[1].events = POLLIN;\n \t\tif (poll(pfd, 2, -1) < 0) {\n \t\t\tif (errno != EINTR) {\n@@ -156,7 +126,7 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n \t\t\tif (process_input(pfd[0].fd, 1))\n \t\t\t\tcontinue;\n \n-\t\tif (waitpid(writer, &status, 0) < 0)\n+\t\tif (waitpid(cld.pid, &status, 0) < 0)\n \t\t\terror_clnt(\"%s\", lostchild);\n \t\telse if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n \t\t\terror_clnt(\"%s\", deadchild);\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex ce9dd98..9cb6eb9 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -85,8 +85,6 @@ static inline int symlink(const char *oldpath, const char *newpath)\n { errno = ENOSYS; return -1; }\n static inline int fchmod(int fildes, mode_t mode)\n { errno = ENOSYS; return -1; }\n-static inline pid_t fork(void)\n-{ errno = ENOSYS; return -1; }\n static inline unsigned int alarm(unsigned int seconds)\n { return 0; }\n static inline int fsync(int fd)\n-- \n1.7.6.135.ge14e3f\n"},{"id":"170981","messageId":"4E15F90F.8090300@kdbg.org","threadId":"27768","inReplyTo":"1310038989-5724-4-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH/RFC 3/3] upload-archive: use start_command instead of fork","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-07-07T18:21:03Z","receivedAt":"2011-07-07T18:21:03Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 07.07.2011 13:43, schrieb Erik Faye-Lund:\n> -\tif (!enter_repo(buf, 0))\n> -\t\tdie(\"'%s' does not appear to be a git repository\", buf);\n> -\n> \t/* put received options in sent_argv[] */\n> \tsent_argc = 1;\n> -\tsent_argv[0] = \"git-upload-archive\";\n> +\tsent_argv[0] = \"archive\";\n\nIs git-archive a valid replacement of enter_repo() followed by\ngit-upload-archive?\n\n-- Hannes\n"},{"id":"170982","messageId":"CABPQNSYsBjtyGAinh2c6YBXHezpADirhr9_hOknSah-o1oy=0Q@mail.gmail.com","threadId":"27768","inReplyTo":"4E15F90F.8090300@kdbg.org","subject":"Re: [PATCH/RFC 3/3] upload-archive: use start_command instead of fork","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-07T18:39:51Z","receivedAt":"2011-07-07T18:39:51Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Jul 7, 2011 at 8:21 PM, Johannes Sixt wrote:\n> Am 07.07.2011 13:43, schrieb Erik Faye-Lund:\n>> -     if (!enter_repo(buf, 0))\n>> -             die(\"'%s' does not appear to be a git repository\", buf);\n>> -\n>>       /* put received options in sent_argv[] */\n>>       sent_argc = 1;\n>> -     sent_argv[0] = \"git-upload-archive\";\n>> +     sent_argv[0] = \"archive\";\n>\n> Is git-archive a valid replacement of enter_repo() followed by\n> git-upload-archive?\n>\n\nWhat do you mean \"enter_repo() followed by git-upload-archive\"? Don't\nyou mean \"enter_repo() followed by write_archive\"? It does work for\nme, and the test-suite didn't reveal any breakages either.\n\nwrite_archive gets called by both versions, but my version doesn't\ncall enter_repo directly. It is however called by git-daemon before\nstarting git-upload-archive, so perhaps that's why it works for me.\nAnd that's a regression indeed; I don't respect the path-argument. So\nexpect a new version over the week-end ;)\n"},{"id":"170988","messageId":"20110707191528.GD12044@sigill.intra.peff.net","threadId":"27768","inReplyTo":"1310038989-5724-4-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH/RFC 3/3] upload-archive: use start_command instead of fork","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-07T19:15:28Z","receivedAt":"2011-07-07T19:15:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 07, 2011 at 01:43:09PM +0200, Erik Faye-Lund wrote:\n\n> The POSIX-function fork is not supported on Windows. Use our\n> start_command API instead.\n\nIs start_command the right solution? From my reading, the fork is\nactually because we want to set up a sideband multiplexer. Should we not\njust be using start_async() to start a thread, as we do in receive-pack?\n\n-Peff\n"},{"id":"170998","messageId":"CABPQNSYZqpfGwgQXqk4YFBXhSHTen1w7m2hwaF7YDsZmsaYY5Q@mail.gmail.com","threadId":"27768","inReplyTo":"20110707191528.GD12044@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 3/3] upload-archive: use start_command instead of fork","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-07T22:25:00Z","receivedAt":"2011-07-07T22:25:00Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Jul 7, 2011 at 9:15 PM, Jeff King wrote:\n> On Thu, Jul 07, 2011 at 01:43:09PM +0200, Erik Faye-Lund wrote:\n>\n>> The POSIX-function fork is not supported on Windows. Use our\n>> start_command API instead.\n>\n> Is start_command the right solution? From my reading, the fork is\n> actually because we want to set up a sideband multiplexer. Should we not\n> just be using start_async() to start a thread, as we do in receive-pack?\n\nI considered that, but discarded it because I figured it required me\nto plug through a file descriptor all the way through the code. But\nperhaps I was wrong, and dup2 will make that job a lot easier?\n"},{"id":"170999","messageId":"20110707222726.GA23424@sigill.intra.peff.net","threadId":"27768","inReplyTo":"CABPQNSYZqpfGwgQXqk4YFBXhSHTen1w7m2hwaF7YDsZmsaYY5Q@mail.gmail.com","subject":"Re: [PATCH/RFC 3/3] upload-archive: use start_command instead of fork","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-07T22:27:26Z","receivedAt":"2011-07-07T22:27:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 08, 2011 at 12:25:00AM +0200, Erik Faye-Lund wrote:\n\n> On Thu, Jul 7, 2011 at 9:15 PM, Jeff King wrote:\n> > On Thu, Jul 07, 2011 at 01:43:09PM +0200, Erik Faye-Lund wrote:\n> >\n> >> The POSIX-function fork is not supported on Windows. Use our\n> >> start_command API instead.\n> >\n> > Is start_command the right solution? From my reading, the fork is\n> > actually because we want to set up a sideband multiplexer. Should we not\n> > just be using start_async() to start a thread, as we do in receive-pack?\n> \n> I considered that, but discarded it because I figured it required me\n> to plug through a file descriptor all the way through the code. But\n> perhaps I was wrong, and dup2 will make that job a lot easier?\n\nYeah, exactly. The current code is already using dup2 in the same way.\n\n-Peff\n"},{"id":"171000","messageId":"CABPQNSbUx3Z-JCsr1rpjqEvKHSoOLnRP3f7pN6cMfNEiNoZBhg@mail.gmail.com","threadId":"27768","inReplyTo":"20110707222726.GA23424@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 3/3] upload-archive: use start_command instead of fork","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-07-07T22:37:48Z","receivedAt":"2011-07-07T22:37:48Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Jul 8, 2011 at 12:27 AM, Jeff King wrote:\n> On Fri, Jul 08, 2011 at 12:25:00AM +0200, Erik Faye-Lund wrote:\n>\n>> On Thu, Jul 7, 2011 at 9:15 PM, Jeff King wrote:\n>> > On Thu, Jul 07, 2011 at 01:43:09PM +0200, Erik Faye-Lund wrote:\n>> >\n>> >> The POSIX-function fork is not supported on Windows. Use our\n>> >> start_command API instead.\n>> >\n>> > Is start_command the right solution? From my reading, the fork is\n>> > actually because we want to set up a sideband multiplexer. Should we not\n>> > just be using start_async() to start a thread, as we do in receive-pack?\n>>\n>> I considered that, but discarded it because I figured it required me\n>> to plug through a file descriptor all the way through the code. But\n>> perhaps I was wrong, and dup2 will make that job a lot easier?\n>\n> Yeah, exactly. The current code is already using dup2 in the same way.\n>\n\nIt does, but I'm not entirely sure how dup2 works when start_async is\nimplemented with threads. Won't cause the multiplexer to fail (the\nmultiplexer and the archive-thread needs different stdouts), because\nfile descriptors are process-resources, and not thread-resources?\n\nI guess I could dup stdout/stderr before dup2'ing, and have the\nmultiplexer write to explicitly to the duped fds. It's starting to\nsound very confusing to my ears, but perhaps it's the best option\nstill ;)\n"},{"id":"171002","messageId":"20110707224427.GB23273@sigill.intra.peff.net","threadId":"27768","inReplyTo":"CABPQNSbUx3Z-JCsr1rpjqEvKHSoOLnRP3f7pN6cMfNEiNoZBhg@mail.gmail.com","subject":"Re: [PATCH/RFC 3/3] upload-archive: use start_command instead of fork","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-07T22:44:27Z","receivedAt":"2011-07-07T22:44:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 08, 2011 at 12:37:48AM +0200, Erik Faye-Lund wrote:\n\n> > Yeah, exactly. The current code is already using dup2 in the same way.\n> \n> It does, but I'm not entirely sure how dup2 works when start_async is\n> implemented with threads. Won't cause the multiplexer to fail (the\n> multiplexer and the archive-thread needs different stdouts), because\n> file descriptors are process-resources, and not thread-resources?\n\nOh, right. I'm being a moron. Sorry.\n\n> I guess I could dup stdout/stderr before dup2'ing, and have the\n> multiplexer write to explicitly to the duped fds. It's starting to\n> sound very confusing to my ears, but perhaps it's the best option\n> still ;)\n\nYes, you would have to dup stdout in the muxer to a new fd, then write\nto that explicitly. And then you would be free to dup the write half of\nthe async pipe to stdout. It's not all that complicated, but maybe it is\nsimpler and more obvious (especially to a later reader of the code) to\njust run a separate command.\n\n-Peff\n"}]}