{"thread":{"id":"12086","subject":"[PATCH 2/6] Automatically close stderr pipes created by run_command","startedAt":"2008-02-14T06:22:29Z","lastAt":"2008-02-22T01:17:42Z","messageCount":15,"participants":["Shawn O. Pearce","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"68708","messageId":"20080214062229.GB30516@spearce.org","threadId":"12086","inReplyTo":null,"subject":"[PATCH 2/6] Automatically close stderr pipes created by run_command","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-02-14T06:22:29Z","receivedAt":"2008-02-14T06:22:29Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Like the out pipe and in pipe, we now automatically close the err\npipe if it was requested by the caller and it hasn't been closed\nby the caller.  This simplifies anyone who wants to get a pipe to\nthe stderr stream for another process.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n run-command.c |    3 +++\n run-command.h |    1 +\n 2 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 52f80be..7bf2cd7 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -51,6 +51,7 @@ int start_command(struct child_process *cmd)\n \t\t\treturn -ERR_RUN_COMMAND_PIPE;\n \t\t}\n \t\tcmd->err = fderr[0];\n+\t\tcmd->close_err = 1;\n \t}\n \n \tcmd->pid = fork();\n@@ -161,6 +162,8 @@ int finish_command(struct child_process *cmd)\n \t\tclose(cmd->in);\n \tif (cmd->close_out)\n \t\tclose(cmd->out);\n+\tif (cmd->close_err)\n+\t\tclose(cmd->err);\n \treturn wait_or_whine(cmd->pid);\n }\n \ndiff --git a/run-command.h b/run-command.h\nindex 1fc781d..705cf2f 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -21,6 +21,7 @@ struct child_process {\n \tconst char *const *env;\n \tunsigned close_in:1;\n \tunsigned close_out:1;\n+\tunsigned close_err:1;\n \tunsigned no_stdin:1;\n \tunsigned no_stdout:1;\n \tunsigned no_stderr:1;\n-- \n1.5.4.1.1309.g833c2\n"},{"id":"68721","messageId":"47B3F51C.3060002@viscovery.net","threadId":"12086","inReplyTo":"20080214062229.GB30516@spearce.org","subject":"Re: [PATCH 2/6] Automatically close stderr pipes created by run_command","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-02-14T08:00:28Z","receivedAt":"2008-02-14T08:00:28Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Shawn O. Pearce schrieb:\n> Like the out pipe and in pipe, we now automatically close the err\n> pipe if it was requested by the caller and it hasn't been closed\n> by the caller.  This simplifies anyone who wants to get a pipe to\n> the stderr stream for another process.\n\nIMHO, this is backwards.\n\nThe .in, .out, .err members of struct child_process serve two different\npurposes:\n\n1. Caller sets them to some fd > 0. This means:\n   \"Here is a readable (.in) or writable (.out, .err) fd; use it.\"\n\n2. Caller sets them to -1: This means:\n   \"Create a pipe and give me the writable (.in) or readable (.out,\n   .err) end of it back.\"\n\nNotice that in a clean implementation:\n\n- case 1. would imply that the fd is \"given away\", i.e.\n  start_command/finish_command take ownership and close it;\n\n- case 2. would imply that the caller takes ownership of the returned\n  fd and has to close it.\n\nThe current implementation of start_command/finish_command as well as its\ncallers don't follow these rules (because they are not documented\nanywhere). As a nasty side-effect we have double-closes in many places\nbecause some callers close fds even though they are not supposed to do it.\n(That's the reason why the close() calls in finish_command cannot check\nfor errors!)\n\nI've a patch cooking that cleans up this mess. IMHO, your patch makes\nthings messier.\n\n-- Hannes\n"},{"id":"68818","messageId":"7vfxvui1pz.fsf@gitster.siamese.dyndns.org","threadId":"12086","inReplyTo":"47B3F51C.3060002@viscovery.net","subject":"Re: [PATCH 2/6] Automatically close stderr pipes created by run_command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-15T17:11:36Z","receivedAt":"2008-02-15T17:11:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Shawn O. Pearce schrieb:\n>> Like the out pipe and in pipe, we now automatically close the err\n>> pipe if it was requested by the caller and it hasn't been closed\n>> by the caller.  This simplifies anyone who wants to get a pipe to\n>> the stderr stream for another process.\n>\n> IMHO, this is backwards.\n>\n> The .in, .out, .err members of struct child_process serve two different\n> purposes:\n>\n> 1. Caller sets them to some fd > 0. This means:\n>    \"Here is a readable (.in) or writable (.out, .err) fd; use it.\"\n>\n> 2. Caller sets them to -1: This means:\n>    \"Create a pipe and give me the writable (.in) or readable (.out,\n>    .err) end of it back.\"\n>\n> Notice that in a clean implementation:\n>\n> - case 1. would imply that the fd is \"given away\", i.e.\n>   start_command/finish_command take ownership and close it;\n>\n> - case 2. would imply that the caller takes ownership of the returned\n>   fd and has to close it.\n\nI am puzzled.  In a clean implementation perhaps the fds belong\nto the caller in both cases.  Or belong to the callee in both\ncases.  Or perhaps sometimes belong to the caller and someimes\nto the callee as you wrote above, but that seems arbitrary.\n\nI suspect from the caller's point of view, if we can always make\nthem owned by the caller, that would be the most consistent and\nconvenient.  \"Here is a fd --- I lend it to you so use it but do\nnot close --- I may have other uses for it later\".\n\nIn any case, I agree with you that the current code lacks\nconsistency either way and we need an order there.\n"},{"id":"68836","messageId":"200802152045.37755.johannes.sixt@telecom.at","threadId":"12086","inReplyTo":"7vfxvui1pz.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/6] Automatically close stderr pipes created by run_command","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-02-15T19:45:37Z","receivedAt":"2008-02-15T19:45:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Friday 15 February 2008 18:11, Junio C Hamano wrote:\n> Johannes Sixt <j.sixt@viscovery.net> writes:\n> > Shawn O. Pearce schrieb:\n> >> Like the out pipe and in pipe, we now automatically close the err\n> >> pipe if it was requested by the caller and it hasn't been closed\n> >> by the caller.  This simplifies anyone who wants to get a pipe to\n> >> the stderr stream for another process.\n> >\n> > IMHO, this is backwards.\n> >\n> > The .in, .out, .err members of struct child_process serve two different\n> > purposes:\n> >\n> > 1. Caller sets them to some fd > 0. This means:\n> >    \"Here is a readable (.in) or writable (.out, .err) fd; use it.\"\n> >\n> > 2. Caller sets them to -1: This means:\n> >    \"Create a pipe and give me the writable (.in) or readable (.out,\n> >    .err) end of it back.\"\n> >\n> > Notice that in a clean implementation:\n> >\n> > - case 1. would imply that the fd is \"given away\", i.e.\n> >   start_command/finish_command take ownership and close it;\n> >\n> > - case 2. would imply that the caller takes ownership of the returned\n> >   fd and has to close it.\n>\n> I am puzzled.  In a clean implementation perhaps the fds belong\n> to the caller in both cases.  Or belong to the callee in both\n> cases.  Or perhaps sometimes belong to the caller and someimes\n> to the callee as you wrote above, but that seems arbitrary.\n\nHeh. In a clean implementation you also don't use the same thing to do two \ncompletely different things. For example, note how in one case .in contains a \nreadable, in the other a writable fd, and how fds are used in completely \ndifferent ways.\n\n> I suspect from the caller's point of view, if we can always make\n> them owned by the caller, that would be the most consistent and\n> convenient.  \"Here is a fd --- I lend it to you so use it but do\n> not close --- I may have other uses for it later\".\n\nWell, I think we could do that, theoretically. But practically in the case \nwhere a fd > 0 is assigned to .in/.out/.err:\n\n- case .out, .err: the caller is required to close the fd early after \nstart_command() because (if this is a pipe) the child won't see EOF;\n\n- case .in: the caller must not read from the fd anyway, else the child gets \ninconsistent input.\n\nSo, while there *is* some inconsistency, the inconsistent cases can be clearly \nseparated into the cases fd > 0 and fd == -1.\n\n-- Hannes\n"},{"id":"68846","messageId":"7v1w7dhnov.fsf@gitster.siamese.dyndns.org","threadId":"12086","inReplyTo":"200802152045.37755.johannes.sixt@telecom.at","subject":"Re: [PATCH 2/6] Automatically close stderr pipes created by run_command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-15T22:14:40Z","receivedAt":"2008-02-15T22:14:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <johannes.sixt@telecom.at> writes:\n\n> ... But practically in the case \n> where a fd > 0 is assigned to .in/.out/.err:\n>\n> - case .out, .err: the caller is required to close the fd early after \n> start_command() because (if this is a pipe) the child won't see EOF;\n>\n> - case .in: the caller must not read from the fd anyway, else the child gets \n> inconsistent input.\n>\n> So, while there *is* some inconsistency, the inconsistent cases can be clearly \n> separated into the cases fd > 0 and fd == -1.\n\nOk, sold.\n"},{"id":"68888","messageId":"7vwsp5b951.fsf@gitster.siamese.dyndns.org","threadId":"12086","inReplyTo":"47B3F51C.3060002@viscovery.net","subject":"Re: [PATCH 2/6] Automatically close stderr pipes created by run_command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-16T08:25:30Z","receivedAt":"2008-02-16T08:25:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Shawn O. Pearce schrieb:\n>> Like the out pipe and in pipe, we now automatically close the err\n>> pipe if it was requested by the caller and it hasn't been closed\n>> by the caller.  This simplifies anyone who wants to get a pipe to\n>> the stderr stream for another process.\n>\n> IMHO, this is backwards.\n>\n> The .in, .out, .err members of struct child_process serve two different\n> purposes:\n> ...\n> Notice that in a clean implementation:\n> ...\n> The current implementation of start_command/finish_command as well as its\n> callers don't follow these rules (because they are not documented\n> anywhere).\n\nWe should define what the clean semantics of this API should be,\nand update Documentation/technical/api-run-command.txt, before\ngoing any further.\n\nAnd I happen to notice two of your names in the list of people\nwho have been involved in its evolution ;-).\n"},{"id":"68908","messageId":"1203183399-4813-1-git-send-email-johannes.sixt@telecom.at","threadId":"12086","inReplyTo":"7v1w7dhnov.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] start_command(), .in/.out/.err = -1: Callers must close the file descriptor","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-02-16T17:36:38Z","receivedAt":"2008-02-16T17:36:38Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"By setting .in, .out, or .err members of struct child_process to -1, the\ncallers of start_command() can request that a pipe is allocated that talks\nto the child process and one end is returned by replacing -1 with the\nfile descriptor.\n\nPreviously, a flag was set (for .in and .out, but not .err) to signal\nfinish_command() to close the pipe end that start_command() had handed out,\nso it was optional for callers to close the pipe, and many already do so.\nNow we make it mandatory to close the pipe.\n\nSigned-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n builtin-fetch-pack.c |    4 +++-\n builtin-send-pack.c  |    1 +\n builtin-tag.c        |    3 ++-\n builtin-verify-tag.c |    1 -\n bundle.c             |    1 +\n receive-pack.c       |    2 ++\n run-command.c        |    6 ------\n run-command.h        |    2 --\n 8 files changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex f401352..5ea48ca 100644\n--- a/builtin-fetch-pack.c\n+++ b/builtin-fetch-pack.c\n@@ -538,8 +538,10 @@ static int get_pack(int xd[2], char **pack_lockfile)\n \tcmd.git_cmd = 1;\n \tif (start_command(&cmd))\n \t\tdie(\"fetch-pack: unable to fork off %s\", argv[0]);\n-\tif (do_keep && pack_lockfile)\n+\tif (do_keep && pack_lockfile) {\n \t\t*pack_lockfile = index_pack_lockfile(cmd.out);\n+\t\tclose(cmd.out);\n+\t}\n \n \tif (finish_command(&cmd))\n \t\tdie(\"%s failed\", argv[0]);\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 8afb1d0..ba9bc91 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -71,6 +71,7 @@ static int pack_objects(int fd, struct ref *refs)\n \t\trefs = refs->next;\n \t}\n \n+\tclose(po.in);\n \tif (finish_command(&po))\n \t\treturn error(\"pack-objects died with strange error\");\n \treturn 0;\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex 4a4a88c..8f7936d 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -226,12 +226,13 @@ static int do_sign(struct strbuf *buffer)\n \n \tif (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {\n \t\tclose(gpg.in);\n+\t\tclose(gpg.out);\n \t\tfinish_command(&gpg);\n \t\treturn error(\"gpg did not accept the tag data\");\n \t}\n \tclose(gpg.in);\n-\tgpg.close_in = 0;\n \tlen = strbuf_read(buffer, gpg.out, 1024);\n+\tclose(gpg.out);\n \n \tif (finish_command(&gpg) || !len || len < 0)\n \t\treturn error(\"gpg failed to sign the tag\");\ndiff --git a/builtin-verify-tag.c b/builtin-verify-tag.c\nindex cc4c55d..b3010f9 100644\n--- a/builtin-verify-tag.c\n+++ b/builtin-verify-tag.c\n@@ -52,7 +52,6 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)\n \n \twrite_in_full(gpg.in, buf, len);\n \tclose(gpg.in);\n-\tgpg.close_in = 0;\n \tret = finish_command(&gpg);\n \n \tunlink(path);\ndiff --git a/bundle.c b/bundle.c\nindex 5c95eca..f150c19 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -332,6 +332,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t\twrite_or_die(rls.in, sha1_to_hex(object->sha1), 40);\n \t\twrite_or_die(rls.in, \"\\n\", 1);\n \t}\n+\tclose(rls.in);\n \tif (finish_command(&rls))\n \t\treturn error (\"pack-objects died\");\n \ndiff --git a/receive-pack.c b/receive-pack.c\nindex 3267495..a971433 100644\n--- a/receive-pack.c\n+++ b/receive-pack.c\n@@ -132,6 +132,7 @@ static int run_hook(const char *hook_name)\n \t\t\t\tbreak;\n \t\t}\n \t}\n+\tclose(proc.in);\n \treturn hook_status(finish_command(&proc), hook_name);\n }\n \n@@ -414,6 +415,7 @@ static const char *unpack(void)\n \t\tif (start_command(&ip))\n \t\t\treturn \"index-pack fork failed\";\n \t\tpack_lockfile = index_pack_lockfile(ip.out);\n+\t\tclose(ip.out);\n \t\tstatus = finish_command(&ip);\n \t\tif (!status) {\n \t\t\treprepare_packed_git();\ndiff --git a/run-command.c b/run-command.c\nindex 476d00c..2919330 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -25,7 +25,6 @@ int start_command(struct child_process *cmd)\n \t\tif (pipe(fdin) < 0)\n \t\t\treturn -ERR_RUN_COMMAND_PIPE;\n \t\tcmd->in = fdin[1];\n-\t\tcmd->close_in = 1;\n \t}\n \n \tneed_out = !cmd->no_stdout\n@@ -38,7 +37,6 @@ int start_command(struct child_process *cmd)\n \t\t\treturn -ERR_RUN_COMMAND_PIPE;\n \t\t}\n \t\tcmd->out = fdout[0];\n-\t\tcmd->close_out = 1;\n \t}\n \n \tneed_err = !cmd->no_stderr && cmd->err < 0;\n@@ -157,10 +155,6 @@ static int wait_or_whine(pid_t pid)\n \n int finish_command(struct child_process *cmd)\n {\n-\tif (cmd->close_in)\n-\t\tclose(cmd->in);\n-\tif (cmd->close_out)\n-\t\tclose(cmd->out);\n \treturn wait_or_whine(cmd->pid);\n }\n \ndiff --git a/run-command.h b/run-command.h\nindex 1fc781d..e9c84d0 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -19,8 +19,6 @@ struct child_process {\n \tint err;\n \tconst char *dir;\n \tconst char *const *env;\n-\tunsigned close_in:1;\n-\tunsigned close_out:1;\n \tunsigned no_stdin:1;\n \tunsigned no_stdout:1;\n \tunsigned no_stderr:1;\n-- \n1.5.4.1.126.ge5a7d\n"},{"id":"68907","messageId":"1203183399-4813-2-git-send-email-johannes.sixt@telecom.at","threadId":"12086","inReplyTo":"1203183399-4813-1-git-send-email-johannes.sixt@telecom.at","subject":"[PATCH] start_command(), if .in/.out > 0, closes file descriptors, not the callers","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-02-16T17:36:39Z","receivedAt":"2008-02-16T17:36:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Callers of start_command() can set the members .in and .out of struct\nchild_process to a value > 0 to specify that this descriptor is used as\nthe stdin or stdout of the child process.\n\nPreviously, if start_command() was successful, this descriptor was closed\nupon return. Here we now make sure that the descriptor is also closed in\ncase of failures. All callers are updated not to close the file descriptor\nthemselves after start_command() was called.\n\nSigned-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n builtin-send-pack.c  |   14 ++++++++------\n builtin-verify-tag.c |    1 -\n bundle.c             |    5 +++--\n run-command.c        |   20 +++++++++++++++++++-\n run-command.h        |   17 +++++++++++++++++\n 5 files changed, 47 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex ba9bc91..b0cfae8 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -404,12 +404,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \tif (!remote_tail)\n \t\tremote_tail = &remote_refs;\n \tif (match_refs(local_refs, remote_refs, &remote_tail,\n-\t\t\t\t\t       nr_refspec, refspec, flags))\n+\t\t       nr_refspec, refspec, flags)) {\n+\t\tclose(out);\n \t\treturn -1;\n+\t}\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr, \"No refs in common and none specified; doing nothing.\\n\"\n \t\t\t\"Perhaps you should specify a branch such as 'master'.\\n\");\n+\t\tclose(out);\n \t\treturn 0;\n \t}\n \n@@ -496,12 +499,11 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \n \tpacket_flush(out);\n \tif (new_refs && !args.dry_run) {\n-\t\tif (pack_objects(out, remote_refs) < 0) {\n-\t\t\tclose(out);\n+\t\tif (pack_objects(out, remote_refs) < 0)\n \t\t\treturn -1;\n-\t\t}\n \t}\n-\tclose(out);\n+\telse\n+\t\tclose(out);\n \n \tif (expect_status_report)\n \t\tret = receive_status(in, remote_refs);\n@@ -649,7 +651,7 @@ int send_pack(struct send_pack_args *my_args,\n \tconn = git_connect(fd, dest, args.receivepack, args.verbose ? CONNECT_VERBOSE : 0);\n \tret = do_send_pack(fd[0], fd[1], remote, dest, nr_heads, heads);\n \tclose(fd[0]);\n-\tclose(fd[1]);\n+\t/* do_send_pack always closes fd[1] */\n \tret |= finish_connect(conn);\n \treturn !!ret;\n }\ndiff --git a/builtin-verify-tag.c b/builtin-verify-tag.c\nindex b3010f9..f3ef11f 100644\n--- a/builtin-verify-tag.c\n+++ b/builtin-verify-tag.c\n@@ -45,7 +45,6 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)\n \tmemset(&gpg, 0, sizeof(gpg));\n \tgpg.argv = args_gpg;\n \tgpg.in = -1;\n-\tgpg.out = 1;\n \targs_gpg[2] = path;\n \tif (start_command(&gpg))\n \t\treturn error(\"could not run gpg.\");\ndiff --git a/bundle.c b/bundle.c\nindex f150c19..eac7350 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -335,8 +335,9 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tclose(rls.in);\n \tif (finish_command(&rls))\n \t\treturn error (\"pack-objects died\");\n-\n-\treturn bundle_to_stdout ? close(bundle_fd) : commit_lock_file(&lock);\n+\tif (!bundle_to_stdout)\n+\t\tcommit_lock_file(&lock);\n+\treturn 0;\n }\n \n int unbundle(struct bundle_header *header, int bundle_fd)\ndiff --git a/run-command.c b/run-command.c\nindex 2919330..6c35d3c 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -20,10 +20,18 @@ int start_command(struct child_process *cmd)\n \tint need_in, need_out, need_err;\n \tint fdin[2], fdout[2], fderr[2];\n \n+\t/*\n+\t * In case of errors we must keep the promise to close FDs\n+\t * that have been passed in in ->in, ->out, and ->err.\n+\t */\n+\n \tneed_in = !cmd->no_stdin && cmd->in < 0;\n \tif (need_in) {\n-\t\tif (pipe(fdin) < 0)\n+\t\tif (pipe(fdin) < 0) {\n+\t\t\tif (cmd->out > 1)\n+\t\t\t\tclose(cmd->out);\n \t\t\treturn -ERR_RUN_COMMAND_PIPE;\n+\t\t}\n \t\tcmd->in = fdin[1];\n \t}\n \n@@ -34,6 +42,8 @@ int start_command(struct child_process *cmd)\n \t\tif (pipe(fdout) < 0) {\n \t\t\tif (need_in)\n \t\t\t\tclose_pair(fdin);\n+\t\t\telse if (cmd->in)\n+\t\t\t\tclose(cmd->in);\n \t\t\treturn -ERR_RUN_COMMAND_PIPE;\n \t\t}\n \t\tcmd->out = fdout[0];\n@@ -44,8 +54,12 @@ int start_command(struct child_process *cmd)\n \t\tif (pipe(fderr) < 0) {\n \t\t\tif (need_in)\n \t\t\t\tclose_pair(fdin);\n+\t\t\telse if (cmd->in)\n+\t\t\t\tclose(cmd->in);\n \t\t\tif (need_out)\n \t\t\t\tclose_pair(fdout);\n+\t\t\telse if (cmd->out > 1)\n+\t\t\t\tclose(cmd->out);\n \t\t\treturn -ERR_RUN_COMMAND_PIPE;\n \t\t}\n \t\tcmd->err = fderr[0];\n@@ -55,8 +69,12 @@ int start_command(struct child_process *cmd)\n \tif (cmd->pid < 0) {\n \t\tif (need_in)\n \t\t\tclose_pair(fdin);\n+\t\telse if (cmd->in > 0)\n+\t\t\tclose(cmd->in);\n \t\tif (need_out)\n \t\t\tclose_pair(fdout);\n+\t\telse if (cmd->out > 1)\n+\t\t\tclose(cmd->out);\n \t\tif (need_err)\n \t\t\tclose_pair(fderr);\n \t\treturn -ERR_RUN_COMMAND_FORK;\ndiff --git a/run-command.h b/run-command.h\nindex e9c84d0..0e67472 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -14,6 +14,23 @@ enum {\n struct child_process {\n \tconst char **argv;\n \tpid_t pid;\n+\t/*\n+\t * Using .in, .out, .err:\n+\t * - Specify 0 to inherit stdin, stdout, stderr from parent.\n+\t * - Specify -1 to have a pipe allocated as follows:\n+\t *     .in: returns the writable pipe end; parent writes to it,\n+\t *          the readable pipe end becomes child's stdin\n+\t *     .out, .err: returns the readable pipe end; parent reads from\n+\t *          it, the writable pipe end becomes child's stdout/stderr\n+\t *   The caller of start_command() must close the returned FDs\n+\t *   after it has completed reading from/writing to it!\n+\t * - Specify > 0 to give away a FD as follows:\n+\t *     .in: a readable FD, becomes child's stdin\n+\t *     .out: a writable FD, becomes child's stdout/stderr\n+\t *     .err > 0 not supported\n+\t *   The specified FD is closed by start_command(), even in case\n+\t *   of errors!\n+\t */\n \tint in;\n \tint out;\n \tint err;\n-- \n1.5.4.1.126.ge5a7d\n"},{"id":"68941","messageId":"7vmyq07bqe.fsf@gitster.siamese.dyndns.org","threadId":"12086","inReplyTo":"1203183399-4813-2-git-send-email-johannes.sixt@telecom.at","subject":"Re: [PATCH] start_command(), if .in/.out > 0, closes file descriptors, not the callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-16T22:55:21Z","receivedAt":"2008-02-16T22:55:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <johannes.sixt@telecom.at> writes:\n\n> Callers of start_command() can set the members .in and .out of struct\n> child_process to a value > 0 to specify that this descriptor is used as\n> the stdin or stdout of the child process.\n>\n> Previously, if start_command() was successful, this descriptor was closed\n> upon return. Here we now make sure that the descriptor is also closed in\n> case of failures. All callers are updated not to close the file descriptor\n> themselves after start_command() was called.\n\nThese two patches make the calling convention more uniform and\nit feels like a good clean-up of the semantics.\n\nBut I have to wonder...\n\n> diff --git a/run-command.c b/run-command.c\n> index 2919330..6c35d3c 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -20,10 +20,18 @@ int start_command(struct child_process *cmd)\n>  \tint need_in, need_out, need_err;\n>  \tint fdin[2], fdout[2], fderr[2];\n>  \n> +\t/*\n> +\t * In case of errors we must keep the promise to close FDs\n> +\t * that have been passed in in ->in, ->out, and ->err.\n> +\t */\n> +\n>  \tneed_in = !cmd->no_stdin && cmd->in < 0;\n>  \tif (need_in) {\n> -\t\tif (pipe(fdin) < 0)\n> +\t\tif (pipe(fdin) < 0) {\n> +\t\t\tif (cmd->out > 1)\n> +\t\t\t\tclose(cmd->out);\n\nWhy check for \"2 or more\"?\n\nCould the caller have specified FD #1 (its own usual stdout) to\nbe used by the child?\n\n> @@ -34,6 +42,8 @@ int start_command(struct child_process *cmd)\n>  \t\tif (pipe(fdout) < 0) {\n>  \t\t\tif (need_in)\n>  \t\t\t\tclose_pair(fdin);\n> +\t\t\telse if (cmd->in)\n> +\t\t\t\tclose(cmd->in);\n\nLikewise, could the caller have told the child to use its own\nstdin?\n\n> @@ -55,8 +69,12 @@ int start_command(struct child_process *cmd)\n>  \tif (cmd->pid < 0) {\n>  \t\tif (need_in)\n>  \t\t\tclose_pair(fdin);\n> +\t\telse if (cmd->in > 0)\n> +\t\t\tclose(cmd->in);\n\nHere \"in\" is checked for \"1 or more\", unlike the above hunk...\n\n> diff --git a/run-command.h b/run-command.h\n> index e9c84d0..0e67472 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -14,6 +14,23 @@ enum {\n>  struct child_process {\n>  \tconst char **argv;\n>  \tpid_t pid;\n> +\t/*\n> +\t * Using .in, .out, .err:\n> +\t * - Specify 0 to inherit stdin, stdout, stderr from parent.\n\nWouldn't this be better if 0, 1, or 2 specify inheriting\nthese respectively?\n\nI am confused...\n\n> +\t * - Specify > 0 to give away a FD as follows:\n> +\t *     .in: a readable FD, becomes child's stdin\n> +\t *     .out: a writable FD, becomes child's stdout/stderr\n> +\t *     .err > 0 not supported\n> +\t *   The specified FD is closed by start_command(), even in case\n> +\t *   of errors!\n\nPerhaps you would need to spell out the semantic differences you\nare assigning to \"inherit\" vs \"give away\".  I presume the former\nis something run_command() would not touch vs the latter is\nclosed by run_command()?\n"},{"id":"68955","messageId":"20080217074252.GO24004@spearce.org","threadId":"12086","inReplyTo":"7vmyq07bqe.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] start_command(), if .in/.out > 0, closes file descriptors, not the callers","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-02-17T07:42:52Z","receivedAt":"2008-02-17T07:42:52Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Johannes Sixt <johannes.sixt@telecom.at> writes:\n> \n> These two patches make the calling convention more uniform and\n> it feels like a good clean-up of the semantics.\n...\n> > diff --git a/run-command.h b/run-command.h\n> > index e9c84d0..0e67472 100644\n> > --- a/run-command.h\n> > +++ b/run-command.h\n> > @@ -14,6 +14,23 @@ enum {\n> >  struct child_process {\n> >  \tconst char **argv;\n> >  \tpid_t pid;\n> > +\t/*\n> > +\t * Using .in, .out, .err:\n> > +\t * - Specify 0 to inherit stdin, stdout, stderr from parent.\n> \n> Wouldn't this be better if 0, 1, or 2 specify inheriting\n> these respectively?\n> \n> I am confused...\n\nUsing 0 to inherit stdin/stdout/stderr makes it easy for callers\nof run_command() to setup something like this:\n\n\tstruct child_process p;\n\tmemset(&p, 0, sizeof(p));\n\tp.git_cmd = 1;\n\tp.argv = args;\n\trun_command(&p);\n\nand have the child inherit stdin/stdout/stderr automatically,\nmuch as a fork() automatically inherits those into the child\nprocess.\n\nSo I agree with keeping 0 for inheriting, rather than needing to set\n\"p.in = 0; p.out = 1; p.err = 2\".  We don't always need (or want)\nthe redirection service.\n\nI think its OK (although maybe somewhat crazy) for the caller to\nset \"p.in = 1\" and thus ask start_command() to close(1) before\nit returns.\n\nWhat happens if the parent has already closed fds 1 and 2 but then\nopens a network socket somewhere and wants that socket to be stdin\nof the child?  Its at fd 1.  But the parent doesn't want to track\nthat fd either, she wants it to go into the child and only the child.\nSo it seems reasonable to make this the \"give away\" case, and that's\nwhat it always had been.\n\nOf course the same argument can be said for fd 0; if the parent does\nclose(0) before some other open/socket/pipe call then its obviously\npossible for the parent to get a fd that it wants the child to take\nover and close.\n\nWe're basically assuming that the parent will always keep its\nown stdin open if it will be spawning children.  We all know what\nhappens when we assume (we double close file descriptors) but I\nthink its a reasonable safe assumption to be making.\n \n-- \nShawn.\n"},{"id":"68960","messageId":"7vodag2egp.fsf@gitster.siamese.dyndns.org","threadId":"12086","inReplyTo":"20080217074252.GO24004@spearce.org","subject":"Re: [PATCH] start_command(), if .in/.out > 0, closes file descriptors, not the callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-17T08:07:34Z","receivedAt":"2008-02-17T08:07:34Z","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> Of course the same argument can be said for fd 0; if the parent does\n> close(0) before some other open/socket/pipe call then its obviously\n> possible for the parent to get a fd that it wants the child to take\n> over and close.\n>\n> We're basically assuming that the parent will always keep its\n> own stdin open if it will be spawning children.  We all know what\n> happens when we assume (we double close file descriptors) but I\n> think its a reasonable safe assumption to be making.\n\nOk, that unwritten assumption was what confused me.  We\ndefinitely need Documentation/technical/api-*.txt before any\nmore code, don't you agree?\n"},{"id":"68962","messageId":"20080217082049.GT24004@spearce.org","threadId":"12086","inReplyTo":"7vodag2egp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] start_command(), if .in/.out > 0, closes file descriptors, not the callers","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-02-17T08:20:49Z","receivedAt":"2008-02-17T08:20:49Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n> \n> > Of course the same argument can be said for fd 0; if the parent does\n> > close(0) before some other open/socket/pipe call then its obviously\n> > possible for the parent to get a fd that it wants the child to take\n> > over and close.\n> >\n> > We're basically assuming that the parent will always keep its\n> > own stdin open if it will be spawning children.  We all know what\n> > happens when we assume (we double close file descriptors) but I\n> > think its a reasonable safe assumption to be making.\n> \n> Ok, that unwritten assumption was what confused me.  We\n> definitely need Documentation/technical/api-*.txt before any\n> more code, don't you agree?\n\nYes.\n\nI'm far too tired to write it tonight.  Maybe I will find the\ntime and energy tomorrow.  Depends on how it goes at day-job\n(yes, must go back for yet another day this weekend).\n\nBut this API is messy enough that we need explicit rules of\nhow it should be used, and then make everyone match that.\n\nUnless Johannes Sixt beats me to it, my next patch to you will\nbe for a proposed documentation improvement.\n\n-- \nShawn.\n"},{"id":"68966","messageId":"200802171029.17850.johannes.sixt@telecom.at","threadId":"12086","inReplyTo":"7vmyq07bqe.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] start_command(), if .in/.out > 0, closes file descriptors, not the callers","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-02-17T09:29:17Z","receivedAt":"2008-02-17T09:29:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Saturday 16 February 2008 23:55, Junio C Hamano wrote:\n> Johannes Sixt <johannes.sixt@telecom.at> writes:\n> >  \tneed_in = !cmd->no_stdin && cmd->in < 0;\n> >  \tif (need_in) {\n> > -\t\tif (pipe(fdin) < 0)\n> > +\t\tif (pipe(fdin) < 0) {\n> > +\t\t\tif (cmd->out > 1)\n> > +\t\t\t\tclose(cmd->out);\n>\n> Why check for \"2 or more\"?\n\nLater in the code, where we set up the redirections in the child process, we \nhave this:\n\t\t...\n\t\t} else if (cmd->out > 1) {\n\t\t\tdup2(cmd->out, 1);\n\t\t\tclose(cmd->out);\n\t\t}\n\nand I thought it was good to also compare cmd->out > 1 in other situations. \nBut now that I think about it, this > 1 is to be understood like this:\n\n\t\tif (cmd->out > 0)\n\t\t\tdup2(cmd->out, 1) unless cmd->out == 1;\n\nso it's an optimization - an unnecessary one since dup2(1, 1) is supposed to \nsucceed and be a noop.\n\n> > +\t * - Specify > 0 to give away a FD as follows:\n> > +\t *     .in: a readable FD, becomes child's stdin\n> > +\t *     .out: a writable FD, becomes child's stdout/stderr\n> > +\t *     .err > 0 not supported\n> > +\t *   The specified FD is closed by start_command(), even in case\n> > +\t *   of errors!\n>\n> Perhaps you would need to spell out the semantic differences you\n> are assigning to \"inherit\" vs \"give away\".  I presume the former\n> is something run_command() would not touch vs the latter is\n> closed by run_command()?\n\nI'll clearify it.\n\n-- Hannes\n"},{"id":"69530","messageId":"200802212342.56629.johannes.sixt@telecom.at","threadId":"12086","inReplyTo":"200802171029.17850.johannes.sixt@telecom.at","subject":"[PATCH v2] start_command(), if .in/.out > 0, closes file descriptors, not the callers","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-02-21T22:42:56Z","receivedAt":"2008-02-21T22:42:56Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Callers of start_command() can set the members .in and .out of struct\nchild_process to a value > 0 to specify that this descriptor is used as\nthe stdin or stdout of the child process.\n\nPreviously, if start_command() was successful, this descriptor was closed\nupon return. Here we now make sure that the descriptor is also closed in\ncase of failures. All callers are updated not to close the file descriptor\nthemselves after start_command() was called.\n\nNote that earlier run_gpg_verify() of git-verify-tag set .out = 1, which\nworked because start_command() treated this as a special case, but now\nthis is incorrect because it closes the descriptor. The intent here is to\ninherit stdout to the child, which is achieved by .out = 0.\n\nSigned-off-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\nOn Sunday 17 February 2008 10:29, Johannes Sixt wrote:\n> On Saturday 16 February 2008 23:55, Junio C Hamano wrote:\n> > Johannes Sixt <johannes.sixt@telecom.at> writes:\n> > > +\t * - Specify > 0 to give away a FD as follows:\n> > > +\t *     .in: a readable FD, becomes child's stdin\n> > > +\t *     .out: a writable FD, becomes child's stdout/stderr\n> > > +\t *     .err > 0 not supported\n> > > +\t *   The specified FD is closed by start_command(), even in case\n> > > +\t *   of errors!\n> >\n> > Perhaps you would need to spell out the semantic differences you\n> > are assigning to \"inherit\" vs \"give away\".  I presume the former\n> > is something run_command() would not touch vs the latter is\n> > closed by run_command()?\n>\n> I'll clearify it.\n\nAnd here it is.\n\nThe other issues you raised -- ->out sometimes compared > 0, sometimes > 1,\nand treated different from ->in -- I addressed, too, by adjusting all uses\nthat were inconsitent. Note that there is still one ->out > 0, but that is\nbecause we don't have set up need_out at this time, and I also left the\ncmd->out > 1 in the child process alone.\n\nNote also the amended commit message that describes the change in\nbuiltin-verify-tag.c.\n\n-- Hannes\n\n builtin-send-pack.c  |   14 ++++++++------\n builtin-verify-tag.c |    1 -\n bundle.c             |    5 +++--\n run-command.c        |   22 ++++++++++++++++++++--\n run-command.h        |   18 ++++++++++++++++++\n 5 files changed, 49 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex ba9bc91..b0cfae8 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -404,12 +404,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \tif (!remote_tail)\n \t\tremote_tail = &remote_refs;\n \tif (match_refs(local_refs, remote_refs, &remote_tail,\n-\t\t\t\t\t       nr_refspec, refspec, flags))\n+\t\t       nr_refspec, refspec, flags)) {\n+\t\tclose(out);\n \t\treturn -1;\n+\t}\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr, \"No refs in common and none specified; doing nothing.\\n\"\n \t\t\t\"Perhaps you should specify a branch such as 'master'.\\n\");\n+\t\tclose(out);\n \t\treturn 0;\n \t}\n \n@@ -496,12 +499,11 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \n \tpacket_flush(out);\n \tif (new_refs && !args.dry_run) {\n-\t\tif (pack_objects(out, remote_refs) < 0) {\n-\t\t\tclose(out);\n+\t\tif (pack_objects(out, remote_refs) < 0)\n \t\t\treturn -1;\n-\t\t}\n \t}\n-\tclose(out);\n+\telse\n+\t\tclose(out);\n \n \tif (expect_status_report)\n \t\tret = receive_status(in, remote_refs);\n@@ -649,7 +651,7 @@ int send_pack(struct send_pack_args *my_args,\n \tconn = git_connect(fd, dest, args.receivepack, args.verbose ? CONNECT_VERBOSE : 0);\n \tret = do_send_pack(fd[0], fd[1], remote, dest, nr_heads, heads);\n \tclose(fd[0]);\n-\tclose(fd[1]);\n+\t/* do_send_pack always closes fd[1] */\n \tret |= finish_connect(conn);\n \treturn !!ret;\n }\ndiff --git a/builtin-verify-tag.c b/builtin-verify-tag.c\nindex b3010f9..f3ef11f 100644\n--- a/builtin-verify-tag.c\n+++ b/builtin-verify-tag.c\n@@ -45,7 +45,6 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)\n \tmemset(&gpg, 0, sizeof(gpg));\n \tgpg.argv = args_gpg;\n \tgpg.in = -1;\n-\tgpg.out = 1;\n \targs_gpg[2] = path;\n \tif (start_command(&gpg))\n \t\treturn error(\"could not run gpg.\");\ndiff --git a/bundle.c b/bundle.c\nindex 4352ce8..0ba5df1 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -336,8 +336,9 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tclose(rls.in);\n \tif (finish_command(&rls))\n \t\treturn error (\"pack-objects died\");\n-\n-\treturn bundle_to_stdout ? close(bundle_fd) : commit_lock_file(&lock);\n+\tif (!bundle_to_stdout)\n+\t\tcommit_lock_file(&lock);\n+\treturn 0;\n }\n \n int unbundle(struct bundle_header *header, int bundle_fd)\ndiff --git a/run-command.c b/run-command.c\nindex 2919330..743757c 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -20,10 +20,18 @@ int start_command(struct child_process *cmd)\n \tint need_in, need_out, need_err;\n \tint fdin[2], fdout[2], fderr[2];\n \n+\t/*\n+\t * In case of errors we must keep the promise to close FDs\n+\t * that have been passed in via ->in and ->out.\n+\t */\n+\n \tneed_in = !cmd->no_stdin && cmd->in < 0;\n \tif (need_in) {\n-\t\tif (pipe(fdin) < 0)\n+\t\tif (pipe(fdin) < 0) {\n+\t\t\tif (cmd->out > 0)\n+\t\t\t\tclose(cmd->out);\n \t\t\treturn -ERR_RUN_COMMAND_PIPE;\n+\t\t}\n \t\tcmd->in = fdin[1];\n \t}\n \n@@ -34,6 +42,8 @@ int start_command(struct child_process *cmd)\n \t\tif (pipe(fdout) < 0) {\n \t\t\tif (need_in)\n \t\t\t\tclose_pair(fdin);\n+\t\t\telse if (cmd->in)\n+\t\t\t\tclose(cmd->in);\n \t\t\treturn -ERR_RUN_COMMAND_PIPE;\n \t\t}\n \t\tcmd->out = fdout[0];\n@@ -44,8 +54,12 @@ int start_command(struct child_process *cmd)\n \t\tif (pipe(fderr) < 0) {\n \t\t\tif (need_in)\n \t\t\t\tclose_pair(fdin);\n+\t\t\telse if (cmd->in)\n+\t\t\t\tclose(cmd->in);\n \t\t\tif (need_out)\n \t\t\t\tclose_pair(fdout);\n+\t\t\telse if (cmd->out)\n+\t\t\t\tclose(cmd->out);\n \t\t\treturn -ERR_RUN_COMMAND_PIPE;\n \t\t}\n \t\tcmd->err = fderr[0];\n@@ -55,8 +69,12 @@ int start_command(struct child_process *cmd)\n \tif (cmd->pid < 0) {\n \t\tif (need_in)\n \t\t\tclose_pair(fdin);\n+\t\telse if (cmd->in)\n+\t\t\tclose(cmd->in);\n \t\tif (need_out)\n \t\t\tclose_pair(fdout);\n+\t\telse if (cmd->out)\n+\t\t\tclose(cmd->out);\n \t\tif (need_err)\n \t\t\tclose_pair(fderr);\n \t\treturn -ERR_RUN_COMMAND_FORK;\n@@ -118,7 +136,7 @@ int start_command(struct child_process *cmd)\n \n \tif (need_out)\n \t\tclose(fdout[1]);\n-\telse if (cmd->out > 1)\n+\telse if (cmd->out)\n \t\tclose(cmd->out);\n \n \tif (need_err)\ndiff --git a/run-command.h b/run-command.h\nindex e9c84d0..debe307 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -14,6 +14,24 @@ enum {\n struct child_process {\n \tconst char **argv;\n \tpid_t pid;\n+\t/*\n+\t * Using .in, .out, .err:\n+\t * - Specify 0 for no redirections (child inherits stdin, stdout,\n+\t *   stderr from parent).\n+\t * - Specify -1 to have a pipe allocated as follows:\n+\t *     .in: returns the writable pipe end; parent writes to it,\n+\t *          the readable pipe end becomes child's stdin\n+\t *     .out, .err: returns the readable pipe end; parent reads from\n+\t *          it, the writable pipe end becomes child's stdout/stderr\n+\t *   The caller of start_command() must close the returned FDs\n+\t *   after it has completed reading from/writing to it!\n+\t * - Specify > 0 to set a channel to a particular FD as follows:\n+\t *     .in: a readable FD, becomes child's stdin\n+\t *     .out: a writable FD, becomes child's stdout/stderr\n+\t *     .err > 0 not supported\n+\t *   The specified FD is closed by start_command(), even in case\n+\t *   of errors!\n+\t */\n \tint in;\n \tint out;\n \tint err;\n-- \n1.5.4.1.126.ge5a7d\n"},{"id":"69543","messageId":"7vpruprdqh.fsf@gitster.siamese.dyndns.org","threadId":"12086","inReplyTo":"200802212342.56629.johannes.sixt@telecom.at","subject":"Re: [PATCH v2] start_command(), if .in/.out > 0, closes file descriptors, not the callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-22T01:17:42Z","receivedAt":"2008-02-22T01:17:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, will queue.\n"}]}