{"thread":{"id":"7220","subject":"[PATCH 2/7] Replace fork_with_pipe in bundle with run_command","startedAt":"2007-03-12T23:00:11Z","lastAt":"2007-03-12T23:59:05Z","messageCount":3,"participants":["Shawn O. Pearce","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"36966","messageId":"20070312230011.GB16840@spearce.org","threadId":"7220","inReplyTo":null,"subject":"[PATCH 2/7] Replace fork_with_pipe in bundle with run_command","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-03-12T23:00:11Z","receivedAt":"2007-03-12T23:00:11Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Now that the run_command family supports all of the redirection\nmodes needed by builtin-bundle, we can use those functions rather\nthan the underlying POSIX primitives.  This should help to make the\nbundle command slightly more portable to other systems, like Windows.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n\n Yes, this replaces my earlier version of the same patch.  I failed\n to test it before sending it out; the earlier version returned a\n bad status code in create_bundle.\n\n builtin-bundle.c |  129 +++++++++++++-----------------------------------------\n 1 files changed, 30 insertions(+), 99 deletions(-)\n\ndiff --git a/builtin-bundle.c b/builtin-bundle.c\nindex 7868080..0a9b738 100644\n--- a/builtin-bundle.c\n+++ b/builtin-bundle.c\n@@ -4,7 +4,7 @@\n #include \"diff.h\"\n #include \"revision.h\"\n #include \"list-objects.h\"\n-#include \"exec_cmd.h\"\n+#include \"run-command.h\"\n \n /*\n  * Basic handler for bundle files to connect repositories via sneakernet.\n@@ -99,67 +99,6 @@ static int read_header(const char *path, struct bundle_header *header) {\n \treturn fd;\n }\n \n-/* if in && *in >= 0, take that as input file descriptor instead */\n-static int fork_with_pipe(const char **argv, int *in, int *out)\n-{\n-\tint needs_in, needs_out;\n-\tint fdin[2], fdout[2], pid;\n-\n-\tneeds_in = in && *in < 0;\n-\tif (needs_in) {\n-\t\tif (pipe(fdin) < 0)\n-\t\t\treturn error(\"could not setup pipe\");\n-\t\t*in = fdin[1];\n-\t}\n-\n-\tneeds_out = out && *out < 0;\n-\tif (needs_out) {\n-\t\tif (pipe(fdout) < 0)\n-\t\t\treturn error(\"could not setup pipe\");\n-\t\t*out = fdout[0];\n-\t}\n-\n-\tif ((pid = fork()) < 0) {\n-\t\tif (needs_in) {\n-\t\t\tclose(fdin[0]);\n-\t\t\tclose(fdin[1]);\n-\t\t}\n-\t\tif (needs_out) {\n-\t\t\tclose(fdout[0]);\n-\t\t\tclose(fdout[1]);\n-\t\t}\n-\t\treturn error(\"could not fork\");\n-\t}\n-\tif (!pid) {\n-\t\tif (needs_in) {\n-\t\t\tdup2(fdin[0], 0);\n-\t\t\tclose(fdin[0]);\n-\t\t\tclose(fdin[1]);\n-\t\t} else if (in) {\n-\t\t\tdup2(*in, 0);\n-\t\t\tclose(*in);\n-\t\t}\n-\t\tif (needs_out) {\n-\t\t\tdup2(fdout[1], 1);\n-\t\t\tclose(fdout[0]);\n-\t\t\tclose(fdout[1]);\n-\t\t} else if (out) {\n-\t\t\tdup2(*out, 1);\n-\t\t\tclose(*out);\n-\t\t}\n-\t\texit(execv_git_cmd(argv));\n-\t}\n-\tif (needs_in)\n-\t\tclose(fdin[0]);\n-\telse if (in)\n-\t\tclose(*in);\n-\tif (needs_out)\n-\t\tclose(fdout[1]);\n-\telse if (out)\n-\t\tclose(*out);\n-\treturn pid;\n-}\n-\n static int list_refs(struct ref_list *r, int argc, const char **argv)\n {\n \tint i;\n@@ -263,9 +202,10 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \tint bundle_fd = -1;\n \tconst char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));\n \tconst char **argv_pack = xmalloc(5 * sizeof(const char *));\n-\tint pid, in, out, i, status, ref_count = 0;\n+\tint i, ref_count = 0;\n \tchar buffer[1024];\n \tstruct rev_info revs;\n+\tstruct child_process rls;\n \n \tbundle_fd = (!strcmp(path, \"-\") ? 1 :\n \t\t\topen(path, O_CREAT | O_EXCL | O_WRONLY, 0666));\n@@ -285,11 +225,13 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \targv_boundary[1] = \"--boundary\";\n \targv_boundary[2] = \"--pretty=oneline\";\n \targv_boundary[argc + 2] = NULL;\n-\tout = -1;\n-\tpid = fork_with_pipe(argv_boundary, NULL, &out);\n-\tif (pid < 0)\n+\tmemset(&rls, 0, sizeof(rls));\n+\trls.argv = argv_boundary;\n+\trls.out = -1;\n+\trls.git_cmd = 1;\n+\tif (start_command(&rls))\n \t\treturn -1;\n-\twhile ((i = read_string(out, buffer, sizeof(buffer))) > 0) {\n+\twhile ((i = read_string(rls.out, buffer, sizeof(buffer))) > 0) {\n \t\tunsigned char sha1[20];\n \t\tif (buffer[0] == '-') {\n \t\t\twrite_or_die(bundle_fd, buffer, i);\n@@ -303,11 +245,8 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \t\t\tobject->flags |= SHOWN;\n \t\t}\n \t}\n-\twhile ((i = waitpid(pid, &status, 0)) < 0)\n-\t\tif (errno != EINTR)\n-\t\t\treturn error(\"rev-list died\");\n-\tif (!WIFEXITED(status) || WEXITSTATUS(status))\n-\t\treturn error(\"rev-list died %d\", WEXITSTATUS(status));\n+\tif (finish_command(&rls))\n+\t\treturn error(\"rev-list died\");\n \n \t/* write references */\n \targc = setup_revisions(argc, argv, &revs, NULL);\n@@ -352,26 +291,23 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \targv_pack[2] = \"--stdout\";\n \targv_pack[3] = \"--thin\";\n \targv_pack[4] = NULL;\n-\tin = -1;\n-\tout = bundle_fd;\n-\tpid = fork_with_pipe(argv_pack, &in, &out);\n-\tif (pid < 0)\n+\tmemset(&rls, 0, sizeof(rls));\n+\trls.argv = argv_pack;\n+\trls.in = -1;\n+\trls.out = bundle_fd;\n+\trls.git_cmd = 1;\n+\tif (start_command(&rls))\n \t\treturn error(\"Could not spawn pack-objects\");\n \tfor (i = 0; i < revs.pending.nr; i++) {\n \t\tstruct object *object = revs.pending.objects[i].item;\n \t\tif (object->flags & UNINTERESTING)\n-\t\t\twrite(in, \"^\", 1);\n-\t\twrite(in, sha1_to_hex(object->sha1), 40);\n-\t\twrite(in, \"\\n\", 1);\n+\t\t\twrite(rls.in, \"^\", 1);\n+\t\twrite(rls.in, sha1_to_hex(object->sha1), 40);\n+\t\twrite(rls.in, \"\\n\", 1);\n \t}\n-\tclose(in);\n-\twhile (waitpid(pid, &status, 0) < 0)\n-\t\tif (errno != EINTR)\n-\t\t\treturn -1;\n-\tif (!WIFEXITED(status) || WEXITSTATUS(status))\n+\tif (finish_command(&rls))\n \t\treturn error (\"pack-objects died\");\n-\n-\treturn status;\n+\treturn 0;\n }\n \n static int unbundle(struct bundle_header *header, int bundle_fd,\n@@ -379,22 +315,17 @@ static int unbundle(struct bundle_header *header, int bundle_fd,\n {\n \tconst char *argv_index_pack[] = {\"index-pack\",\n \t\t\"--fix-thin\", \"--stdin\", NULL};\n-\tint pid, status, dev_null;\n+\tstruct child_process ip;\n \n \tif (verify_bundle(header, 0))\n \t\treturn -1;\n-\tdev_null = open(\"/dev/null\", O_WRONLY);\n-\tif (dev_null < 0)\n-\t\treturn error(\"Could not open /dev/null\");\n-\tpid = fork_with_pipe(argv_index_pack, &bundle_fd, &dev_null);\n-\tif (pid < 0)\n-\t\treturn error(\"Could not spawn index-pack\");\n-\twhile (waitpid(pid, &status, 0) < 0)\n-\t\tif (errno != EINTR)\n-\t\t\treturn error(\"index-pack died\");\n-\tif (!WIFEXITED(status) || WEXITSTATUS(status))\n-\t\treturn error(\"index-pack exited with status %d\",\n-\t\t\t\tWEXITSTATUS(status));\n+\tmemset(&ip, 0, sizeof(ip));\n+\tip.argv = argv_index_pack;\n+\tip.in = bundle_fd;\n+\tip.no_stdout = 1;\n+\tip.git_cmd = 1;\n+\tif (run_command(&ip))\n+\t\treturn error(\"index-pack died\");\n \treturn list_heads(header, argc, argv);\n }\n \n-- \n1.5.0.3.985.gcf0b4\n"},{"id":"36973","messageId":"Pine.LNX.4.63.0703130011190.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"7220","inReplyTo":"20070312230011.GB16840@spearce.org","subject":"Re: [PATCH 2/7] Replace fork_with_pipe in bundle with run_command","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-03-12T23:44:18Z","receivedAt":"2007-03-12T23:44:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 12 Mar 2007, Shawn O. Pearce wrote:\n\n> Now that the run_command family supports all of the redirection\n> modes needed by builtin-bundle, we can use those functions rather\n> than the underlying POSIX primitives.  This should help to make the\n> bundle command slightly more portable to other systems, like Windows.\n\nLooks fine to me (and since it passes the tests, it must be correct, \nright?)\n\nCiao,\nDscho\n"},{"id":"36975","messageId":"7vodmyovxy.fsf@assigned-by-dhcp.cox.net","threadId":"7220","inReplyTo":"Pine.LNX.4.63.0703130011190.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 2/7] Replace fork_with_pipe in bundle with run_command","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-12T23:59:05Z","receivedAt":"2007-03-12T23:59:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Mon, 12 Mar 2007, Shawn O. Pearce wrote:\n>\n>> Now that the run_command family supports all of the redirection\n>> modes needed by builtin-bundle, we can use those functions rather\n>> than the underlying POSIX primitives.  This should help to make the\n>> bundle command slightly more portable to other systems, like Windows.\n>\n> Looks fine to me (and since it passes the tests, it must be correct, \n> right?)\n\nFunny.  Maybe the tests need to be tightened up?\n"}]}