{"thread":{"id":"28979","subject":"[PATCH] upload-archive: use start_command instead of fork","startedAt":"2011-11-19T07:40:04Z","lastAt":"2011-11-19T07:40:04Z","messageCount":1,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"179730","messageId":"20111119074004.GA658@sigill.intra.peff.net","threadId":"28979","inReplyTo":null,"subject":"[PATCH] upload-archive: use start_command instead of fork","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-19T07:40:04Z","receivedAt":"2011-11-19T07:40:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The POSIX-function fork is not supported on Windows. Use our\nstart_command API instead, respawning ourselves in a special\n\"writer\" mode to follow the alternate code path.\n\nRemove the NOT_MINGW-prereq for t5000, as git-archive --remote\nnow works.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nNow that Junio has reverted the original implementation, I took a closer\nlook at how this ought to be done.\n\nI looked briefly into doing the writing in-process, or using\nstart_async.  But the code relies too heavily on being able to die(),\nand having the parent process grab stderr and the exit code to send to\nthe client. So going that direction would involve a lot of changes\neither to the async code, or to the archive code.\n\nWe really do want a separate process, as it fits in with our usual die()\nmentality for errors.  But fortunately looking into the async stuff made\nme realize another issue with the original patch. Originally we forked\nas the first thing. Thus any error messages or die()s (e.g., because we\ncouldn't enter_repo, or got bogus input from the client) would be in the\nchild process, and the parent sideband muxer would relay the errors\nproperly.  But in your original patch, we do quite a bit of processing\nbefore spawning; those errors would go to git-daemon's stderr, and the\nclient would just see an unexpected hangup.\n\nSo we really do want to just switch the \"fork immediately\" to \"spawn\nimmediately\". Which fortunately means the change ends up being even\nsmaller, and I'm way more confident that there won't be side effects;\nit's almost nothing but converting pipe/fork code into start_command\ncode.\n\nAnd as an added bonus, the diffstat hopefully speaks for itself. :)\n\n builtin.h                |    1 +\n builtin/upload-archive.c |   43 ++++++++++++-------------------------------\n git.c                    |    1 +\n t/t5000-tar-tree.sh      |   10 +++++-----\n 4 files changed, 19 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin.h b/builtin.h\nindex 0e9da90..f2357a9 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -133,6 +133,7 @@ int copy_note_for_rewrite(struct notes_rewrite_cfg *c,\n extern int cmd_update_ref(int argc, const char **argv, const char *prefix);\n extern int cmd_update_server_info(int argc, const char **argv, const char *prefix);\n extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);\n+extern int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix);\n extern int cmd_upload_tar(int argc, const char **argv, const char *prefix);\n extern int cmd_var(int argc, const char **argv, const char *prefix);\n extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 2d0b383..b928beb 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 <repo>\";\n@@ -13,12 +14,9 @@\n static const char deadchild[] =\n \"git upload-archive: archiver died with error\";\n \n-static const char lostchild[] =\n-\"git upload-archive: archiver process was lost\";\n-\n #define MAX_ARGS (64)\n \n-static int run_upload_archive(int argc, const char **argv, const char *prefix)\n+int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n {\n \tconst char *sent_argv[MAX_ARGS];\n \tconst char *arg_cmd = \"argument \";\n@@ -96,8 +94,8 @@ 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+\tstruct child_process writer = { argv };\n+\n \t/*\n \t * Set up sideband subprocess.\n \t *\n@@ -105,39 +103,24 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)\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+\targv[0] = \"upload-archive--writer\";\n+\twriter.out = writer.err = -1;\n+\twriter.git_cmd = 1;\n+\tif (start_command(&writer)) {\n \t\tint err = errno;\n-\t\tpacket_write(1, \"NACK fork failed on the remote side\\n\");\n+\t\tpacket_write(1, \"NACK unable to spawn subprocess\\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 \twhile (1) {\n \t\tstruct pollfd pfd[2];\n-\t\tint status;\n \n-\t\tpfd[0].fd = fd1[0];\n+\t\tpfd[0].fd = writer.out;\n \t\tpfd[0].events = POLLIN;\n-\t\tpfd[1].fd = fd2[0];\n+\t\tpfd[1].fd = writer.err;\n \t\tpfd[1].events = POLLIN;\n \t\tif (poll(pfd, 2, -1) < 0) {\n \t\t\tif (errno != EINTR) {\n@@ -156,9 +139,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\t\terror_clnt(\"%s\", lostchild);\n-\t\telse if (!WIFEXITED(status) || WEXITSTATUS(status) > 0)\n+\t\tif (finish_command(&writer))\n \t\t\terror_clnt(\"%s\", deadchild);\n \t\tpacket_flush(1);\n \t\tbreak;\ndiff --git a/git.c b/git.c\nindex 8e34903..250f448 100644\n--- a/git.c\n+++ b/git.c\n@@ -434,6 +434,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"update-ref\", cmd_update_ref, RUN_SETUP },\n \t\t{ \"update-server-info\", cmd_update_server_info, RUN_SETUP },\n \t\t{ \"upload-archive\", cmd_upload_archive },\n+\t\t{ \"upload-archive--writer\", cmd_upload_archive_writer },\n \t\t{ \"var\", cmd_var, RUN_SETUP_GENTLY },\n \t\t{ \"verify-pack\", cmd_verify_pack },\n \t\t{ \"verify-tag\", cmd_verify_tag, RUN_SETUP },\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex d906898..889842e 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -96,7 +96,7 @@ test_expect_success 'git archive with --output' \\\n     'git archive --output=b4.tar HEAD &&\n     test_cmp b.tar b4.tar'\n \n-test_expect_success NOT_MINGW 'git archive --remote' \\\n+test_expect_success 'git archive --remote' \\\n     'git archive --remote=. HEAD >b5.tar &&\n     test_cmp b.tar b5.tar'\n \n@@ -266,7 +266,7 @@ test_expect_success 'archive --list mentions user filter' '\n \tgrep \"^bar\\$\" output\n '\n \n-test_expect_success NOT_MINGW 'archive --list shows only enabled remote filters' '\n+test_expect_success 'archive --list shows only enabled remote filters' '\n \tgit archive --list --remote=. >output &&\n \t! grep \"^tar\\.foo\\$\" output &&\n \tgrep \"^bar\\$\" output\n@@ -298,7 +298,7 @@ test_expect_success 'extension matching requires dot' '\n \ttest_cmp b.tar config-implicittar.foo\n '\n \n-test_expect_success NOT_MINGW 'only enabled filters are available remotely' '\n+test_expect_success 'only enabled filters are available remotely' '\n \ttest_must_fail git archive --remote=. --format=tar.foo HEAD \\\n \t\t>remote.tar.foo &&\n \tgit archive --remote=. --format=bar >remote.bar HEAD &&\n@@ -341,12 +341,12 @@ test_expect_success GZIP,GUNZIP 'extract tgz file' '\n \ttest_cmp b.tar j.tar\n '\n \n-test_expect_success GZIP,NOT_MINGW 'remote tar.gz is allowed by default' '\n+test_expect_success GZIP 'remote tar.gz is allowed by default' '\n \tgit archive --remote=. --format=tar.gz HEAD >remote.tar.gz &&\n \ttest_cmp j.tgz remote.tar.gz\n '\n \n-test_expect_success GZIP,NOT_MINGW 'remote tar.gz can be disabled' '\n+test_expect_success GZIP 'remote tar.gz can be disabled' '\n \tgit config tar.tar.gz.remote false &&\n \ttest_must_fail git archive --remote=. --format=tar.gz HEAD \\\n \t\t>remote.tar.gz\n-- \n1.7.7.3.8.g38efa\n"}]}