{"thread":{"id":"7214","subject":"[PATCH 4/4] Replace fork_with_pipe in bundle with run_command","startedAt":"2007-03-12T18:38:15Z","lastAt":"2007-03-13T00:56:21Z","messageCount":5,"participants":["Shawn O. Pearce","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"36935","messageId":"20070312183815.GD15996@spearce.org","threadId":"7214","inReplyTo":null,"subject":"[PATCH 4/4] Replace fork_with_pipe in bundle with run_command","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-03-12T18:38:15Z","receivedAt":"2007-03-12T18:38:15Z","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 builtin-bundle.c |  126 ++++++++++++-----------------------------------------\n 1 files changed, 29 insertions(+), 97 deletions(-)\n\ndiff --git a/builtin-bundle.c b/builtin-bundle.c\nindex 7868080..42d7176 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, status, 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,23 +291,21 @@ 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@@ -379,22 +316,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":"36974","messageId":"7vslcaow13.fsf@assigned-by-dhcp.cox.net","threadId":"7214","inReplyTo":"20070312183815.GD15996@spearce.org","subject":"Re: [PATCH 4/4] Replace fork_with_pipe in bundle with run_command","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-12T23:57:12Z","receivedAt":"2007-03-12T23:57:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\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> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>\n> ...\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, status, 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> ...\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> +\tif (finish_command(&rls))\n>  \t\treturn error (\"pack-objects died\");\n>  \n>  \treturn status;\n\nAs the variable 'status' is not used anymore, I think this should return 0.\n"},{"id":"36977","messageId":"20070313003646.GA17169@spearce.org","threadId":"7214","inReplyTo":"7vslcaow13.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 4/4] Replace fork_with_pipe in bundle with run_command","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-03-13T00:36:47Z","receivedAt":"2007-03-13T00:36:47Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <junkio@cox.net> wrote:\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n> >  \treturn status;\n> \n> As the variable 'status' is not used anymore, I think this should return 0.\n\nIndeed.  I replaced that patch with another (same subject) in my\nlatest series.  Because leaving that 'status' variable in caused\na failure in the test suite when trying to create a bundle.\n\n  fixed:  http://article.gmane.org/gmane.comp.version-control.git/42087\n  borked: http://article.gmane.org/gmane.comp.version-control.git/42056\n\nYou quoted the borked one.  Please drop it and apply the fixed one.\n\n-- \nShawn.\n"},{"id":"36979","messageId":"7v3b4aotd9.fsf@assigned-by-dhcp.cox.net","threadId":"7214","inReplyTo":"20070313003646.GA17169@spearce.org","subject":"Re: [PATCH 4/4] Replace fork_with_pipe in bundle with run_command","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-13T00:54:42Z","receivedAt":"2007-03-13T00:54:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> Junio C Hamano <junkio@cox.net> wrote:\n>> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n>> >  \treturn status;\n>> \n>> As the variable 'status' is not used anymore, I think this should return 0.\n>\n> Indeed.  I replaced that patch with another (same subject) in my\n> latest series.  Because leaving that 'status' variable in caused\n> a failure in the test suite when trying to create a bundle.\n>\n>   fixed:  http://article.gmane.org/gmane.comp.version-control.git/42087\n>   borked: http://article.gmane.org/gmane.comp.version-control.git/42056\n>\n> You quoted the borked one.  Please drop it and apply the fixed one.\n\nOk, diffing these two the only differences I see are identical\nto what I locally hand-fixed, so we are Ok it seems.\n"},{"id":"36981","messageId":"20070313005621.GA17243@spearce.org","threadId":"7214","inReplyTo":"7v3b4aotd9.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 4/4] Replace fork_with_pipe in bundle with run_command","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-03-13T00:56:21Z","receivedAt":"2007-03-13T00:56:21Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <junkio@cox.net> wrote:\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n> \n> > Junio C Hamano <junkio@cox.net> wrote:\n> >> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n> >> >  \treturn status;\n> >> \n> >> As the variable 'status' is not used anymore, I think this should return 0.\n> >\n> > Indeed.  I replaced that patch with another (same subject) in my\n> > latest series.  Because leaving that 'status' variable in caused\n> > a failure in the test suite when trying to create a bundle.\n> >\n> >   fixed:  http://article.gmane.org/gmane.comp.version-control.git/42087\n> >   borked: http://article.gmane.org/gmane.comp.version-control.git/42056\n> >\n> > You quoted the borked one.  Please drop it and apply the fixed one.\n> \n> Ok, diffing these two the only differences I see are identical\n> to what I locally hand-fixed, so we are Ok it seems.\n\nYes, the only difference between the two patches was the unused\n'status' variable, and returning 0 when we did actually make\nthe bundle successfully.  ;-)\n\nSorry for the pain today.  I usually try harder to avoid problems\nlike this...\n\n-- \nShawn.\n"}]}