{"thread":{"id":"14825","subject":"[PATCH] Use line buffering for standard output","startedAt":"2008-08-03T21:26:08Z","lastAt":"2008-08-04T10:18:40Z","messageCount":8,"participants":["Anders Melchiorsen","Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"86109","messageId":"1217798768-18021-1-git-send-email-mail@cup.kalibalik.dk","threadId":"14825","inReplyTo":null,"subject":"[PATCH] Use line buffering for standard output","fromName":"Anders Melchiorsen","fromEmail":"mail@cup.kalibalik.dk","sentAt":"2008-08-03T21:26:08Z","receivedAt":"2008-08-03T21:26:08Z","isPatch":true,"sender":{"key":"mail@cup.kalibalik.dk","avatar":null},"body":"Normally, stdout is fully buffered, unless it refers to a terminal\ndevice. This gives problems when fork() is in play: the buffer is\ncloned and output appears twice.\n\nBy always setting stdout to line buffering, we make the output work\nidentically for all output devices.\n\nSigned-off-by: Anders Melchiorsen \n---\n\nOn #git, blix mentioned that running git clone through a pipe made it\noutput the \"Initialized empty\" line twice. This seems to be due to\nbad interactions between fork() and buffered stdio.\n\nRather than putting in flushing at all the right places, this\nsledgehammer fix simply reverts to line buffering for all output devices.\n\n\nAnders.\n\n\n git.c | 6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 37b1d76..040b900 100644\n--- a/git.c\n+++ b/git.c\n@@ -421,6 +421,12 @@ int main(int argc, const char **argv)\n \tint done_alias = 0;\n \n \t/*\n+\t * Use line buffering, even if we do not have interactive\n+\t * output. Full buffering mixes badly with fork().\n+\t */\n+\tsetvbuf(stdout, NULL, _IOLBF, 0);\n+\n+\t/*\n \t * Take the basename of argv[0] as the command\n \t * name, and the dirname as the default exec_path\n \t * if we don't have anything better.\n-- \n1.5.6.4\n"},{"id":"86110","messageId":"7vhca193th.fsf@gitster.siamese.dyndns.org","threadId":"14825","inReplyTo":"1217798768-18021-1-git-send-email-mail@cup.kalibalik.dk","subject":"Re: [PATCH] Use line buffering for standard output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-03T21:38:50Z","receivedAt":"2008-08-03T21:38:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Melchiorsen writes:\n\n> Normally, stdout is fully buffered, unless it refers to a terminal\n> device. This gives problems when fork() is in play: the buffer is\n> cloned and output appears twice.\n\nI thought we have been careful to flush() them before we fork but perhaps\nthere are recent changes that were not careful. Wouldn't it be better to\nfix them first, independent of this change?\n"},{"id":"86111","messageId":"alpine.LFD.1.10.0808031444270.3668@nehalem.linux-foundation.org","threadId":"14825","inReplyTo":"1217798768-18021-1-git-send-email-mail@cup.kalibalik.dk","subject":"Re: [PATCH] Use line buffering for standard output","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-08-03T21:46:18Z","receivedAt":"2008-08-03T21:46:18Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 3 Aug 2008, Anders Melchiorsen wrote:\n>\n> Normally, stdout is fully buffered, unless it refers to a terminal\n> device. This gives problems when fork() is in play: the buffer is\n> cloned and output appears twice.\n> \n> By always setting stdout to line buffering, we make the output work\n> identically for all output devices.\n\nPlease don't.\n\nThis is a huge peformance issue for things like\n\n\tgit log -p > file\n\nwhere we really want it to be fully buffered.\n\nSo please just find the place where we do a fork() without flushing \npending output...\n\n(We really shouldn't have all that many \"fork()\" calls left, I thought - \nthe Windows stuff means that most of it should be abstracted away. So it's \nnot like we're talking about hundreds of sites, there should be just a \ncouple).\n\n\t\tLinus\n"},{"id":"86115","messageId":"87hca1ogto.fsf@cup.kalibalik.dk","threadId":"14825","inReplyTo":"alpine.LFD.1.10.0808031444270.3668@nehalem.linux-foundation.org","subject":"Re: [PATCH] Use line buffering for standard output","fromName":"Anders Melchiorsen","fromEmail":"mail@cup.kalibalik.dk","sentAt":"2008-08-03T22:48:51Z","receivedAt":"2008-08-03T22:48:51Z","isPatch":true,"sender":{"key":"mail@cup.kalibalik.dk","avatar":null},"body":"Linus Torvalds writes:\n\n> On Sun, 3 Aug 2008, Anders Melchiorsen wrote:\n>>\n>> By always setting stdout to line buffering, we make the output work\n>> identically for all output devices.\n>\n> Please don't.\n>\n> This is a huge peformance issue for things like\n>\n> \tgit log -p > file\n>\n> where we really want it to be fully buffered.\n>\n> So please just find the place where we do a fork() without flushing\n> pending output...\n\nSure. The sledgehammer approach was partly to get some advice on the\nproper solution. I now realize that you have generally been careful\nabout this, and so a single flush should be enough.\n\nBelow are two alternative proposals, one local and one global. Both of\nthem fix the problem for me, but maybe you were even thinking about a\nthird place?\n\nFor the run-command.c one, I was not sure whether to put it inside or\noutside the ifdef, and I also was not sure whether to add it for\nstart_command(). Not having other testcases, and not knowing Windows,\nthis is the way it ended up.\n\n\nCheers,\nAnders.\n\n\n\nFrom: Anders Melchiorsen \nDate: Mon, 4 Aug 2008 00:21:49 +0200\nSubject: [PATCH] Flush stdout in init-db\n\nBefore this change, clone outputs \"Initialized empty ...\" twice\nif output is piped.\n\nSigned-off-by: Anders Melchiorsen \n---\n builtin-init-db.c | 4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-init-db.c b/builtin-init-db.c\nindex baf0d09..954c7e9 100644\n--- a/builtin-init-db.c\n+++ b/builtin-init-db.c\n@@ -315,11 +315,13 @@ int init_db(const char *template_dir, unsigned int flags)\n \t\tgit_config_set(\"receive.denyNonFastforwards\", \"true\");\n \t}\n \n-\tif (!(flags & INIT_DB_QUIET))\n+\tif (!(flags & INIT_DB_QUIET)) {\n \t\tprintf(\"%s%s Git repository in %s/\\n\",\n \t\t reinit ? \"Reinitialized existing\" : \"Initialized empty\",\n \t\t shared_repository ? \" shared\" : \"\",\n \t\t get_git_dir());\n+\t\tfflush(stdout);\n+\t}\n \n \treturn 0;\n }\n\n\n\nFrom: Anders Melchiorsen \nDate: Mon, 4 Aug 2008 00:35:40 +0200\nSubject: [PATCH] Flush standard output in start_async\n\nThis prevents double output in case stdout is redirected.\n\nSigned-off-by: Anders Melchiorsen \n---\n run-command.c | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex a3b28a6..67be079 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -304,6 +304,9 @@ int start_async(struct async *async)\n \tasync->out = pipe_out[0];\n \n #ifndef __MINGW32__\n+\t/* Flush output before fork() to avoid cloning the buffer */\n+\tfflush(stdout);\n+\n \tasync->pid = fork();\n \tif (async->pid < 0) {\n \t\terror(\"fork (async) failed: %s\", strerror(errno));\n"},{"id":"86131","messageId":"7vej557hlq.fsf@gitster.siamese.dyndns.org","threadId":"14825","inReplyTo":"87hca1ogto.fsf@cup.kalibalik.dk","subject":"Re: [PATCH] Use line buffering for standard output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-04T00:24:01Z","receivedAt":"2008-08-04T00:24:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Melchiorsen writes:\n\n> From: Anders Melchiorsen \n> Date: Mon, 4 Aug 2008 00:35:40 +0200\n> Subject: [PATCH] Flush standard output in start_async\n>\n> This prevents double output in case stdout is redirected.\n>\n> Signed-off-by: Anders Melchiorsen \n> ---\n> run-command.c | 3 +++\n> 1 files changed, 3 insertions(+), 0 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index a3b28a6..67be079 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -304,6 +304,9 @@ int start_async(struct async *async)\n> \tasync->out = pipe_out[0];\n> \n> #ifndef __MINGW32__\n> +\t/* Flush output before fork() to avoid cloning the buffer */\n> +\tfflush(stdout);\n> +\n> \tasync->pid = fork();\n> \tif (async->pid < 0) {\n> \t\terror(\"fork (async) failed: %s\", strerror(errno));\n\nI think this with s/stdout/NULL/ would be a reasonable thing to do.\n"},{"id":"86132","messageId":"1217809803-23391-1-git-send-email-mail@cup.kalibalik.dk","threadId":"14825","inReplyTo":"7vej557hlq.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] Flush output in start_async","fromName":"Anders Melchiorsen","fromEmail":"mail@cup.kalibalik.dk","sentAt":"2008-08-04T00:30:03Z","receivedAt":"2008-08-04T00:30:03Z","isPatch":true,"sender":{"key":"mail@cup.kalibalik.dk","avatar":null},"body":"This prevents double output in case stdout is redirected.\n\nSigned-off-by: Anders Melchiorsen \n---\n run-command.c | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex a3b28a6..6af83c5 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -304,6 +304,9 @@ int start_async(struct async *async)\n \tasync->out = pipe_out[0];\n \n #ifndef __MINGW32__\n+\t/* Flush stdio before fork() to avoid cloning buffers */\n+\tfflush(NULL);\n+\n \tasync->pid = fork();\n \tif (async->pid < 0) {\n \t\terror(\"fork (async) failed: %s\", strerror(errno));\n-- \n1.5.6.4\n"},{"id":"86151","messageId":"alpine.LFD.1.10.0808032241260.3668@nehalem.linux-foundation.org","threadId":"14825","inReplyTo":"7vej557hlq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Use line buffering for standard output","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-08-04T05:50:44Z","receivedAt":"2008-08-04T05:50:44Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 3 Aug 2008, Junio C Hamano wrote:\n\n> Anders Melchiorsen writes:\n> > \n> > #ifndef __MINGW32__\n> > +\t/* Flush output before fork() to avoid cloning the buffer */\n> > +\tfflush(stdout);\n> > +\n> > \tasync->pid = fork();\n> > \tif (async->pid < 0) {\n> > \t\terror(\"fork (async) failed: %s\", strerror(errno));\n> \n> I think this with s/stdout/NULL/ would be a reasonable thing to do.\n\nAgreed, I think that's the right thing to do. \n\nThere's another fork there in start_command(), I suspect we should do it \nthere too: it's a \"generic\" path, so it should try to be safe.\n\nThe other ones look ok from a quick scan. I don't know the imap-send.c \ncode, but it's from outside people who hopefully know what they were \ndoing. The other ones don't seem to be using stdio before the fork (except \nfor things like \"die()\" ;)\n\nThere is a \"fork()\" in a _comment_ in builtin-ls-tree.c, and that one \ndefinitely should have a fflush(NULL) in front of it. But it _is_ just a \ncomment, and rather than addign a fflush() there, it would probably be \nbetter to turn it into a \"start_command()\" or something like that.\n\n\t\t\tLinus.\n"},{"id":"86172","messageId":"1217845120-28134-1-git-send-email-mail@cup.kalibalik.dk","threadId":"14825","inReplyTo":"alpine.LFD.1.10.0808032241260.3668@nehalem.linux-foundation.org","subject":"[PATCH] Add output flushing before fork()","fromName":"Anders Melchiorsen","fromEmail":"mail@cup.kalibalik.dk","sentAt":"2008-08-04T10:18:40Z","receivedAt":"2008-08-04T10:18:40Z","isPatch":true,"sender":{"key":"mail@cup.kalibalik.dk","avatar":null},"body":"This adds fflush(NULL) before fork() in start_command(), to keep\nthe generic interface safe.\n\nA remaining use of fork() with no flushing is in a comment in\nshow_tree(). Rewrite that comment to use start_command().\n\nSigned-off-by: Anders Melchiorsen \n---\n\nThis fixes up the remaining two spots that Linus suggested.\n\n\n builtin-ls-tree.c | 13 ++++++-------\n run-command.c | 1 +\n 2 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin-ls-tree.c b/builtin-ls-tree.c\nindex d25767a..cb61717 100644\n--- a/builtin-ls-tree.c\n+++ b/builtin-ls-tree.c\n@@ -66,17 +66,16 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,\n \t\t/*\n \t\t * Maybe we want to have some recursive version here?\n \t\t *\n-\t\t * Something like:\n+\t\t * Something similar to this incomplete example:\n \t\t *\n \t\tif (show_subprojects(base, baselen, pathname)) {\n-\t\t\tif (fork()) {\n-\t\t\t\tchdir(base);\n-\t\t\t\texec ls-tree;\n-\t\t\t}\n-\t\t\twaitpid();\n+\t\t\tstruct child_process ls_tree;\n+\n+\t\t\tls_tree.dir = base;\n+\t\t\tls_tree.argv = ls-tree;\n+\t\t\tstart_command(&ls_tree);\n \t\t}\n \t\t *\n-\t\t * ..or similar..\n \t\t */\n \t\ttype = commit_type;\n \t} else if (S_ISDIR(mode)) {\ndiff --git a/run-command.c b/run-command.c\nindex 6af83c5..bbb9c77 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -68,6 +68,7 @@ int start_command(struct child_process *cmd)\n \ttrace_argv_printf(cmd->argv, \"trace: run_command:\");\n \n #ifndef __MINGW32__\n+\tfflush(NULL);\n \tcmd->pid = fork();\n \tif (!cmd->pid) {\n \t\tif (cmd->no_stdin)\n-- \n1.5.6.4\n"}]}