{"thread":{"id":"8723","subject":"[PATCH] git-rev-list: give better diagnostic for failed write","startedAt":"2007-06-25T20:32:01Z","lastAt":"2007-06-30T18:42:09Z","messageCount":33,"participants":["Jim Meyering","Linus Torvalds","Jeff King","Theodore Tso","Frank Lichtenheld","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"45790","messageId":"87r6nzu666.fsf@rho.meyering.net","threadId":"8723","inReplyTo":null,"subject":"[PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-25T20:32:01Z","receivedAt":"2007-06-25T20:32:01Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"[this patch depends on the one I posted here:\n http://marc.info/?l=git&m=118280134031923&w=2 ]\n\nWithout this patch, git-rev-list unnecessarily omits strerror(errno)\nfrom its diagnostic, upon write failure:\n\n $ ./git-rev-list --max-count=1 HEAD > /dev/full\n fatal: write failure on standard output\n\nWith the patch, git reports the desired ENOSPC diagnostic:\n\n fatal: write failure on standard output: No space left on device\n\n* builtin-rev-list (show_commit): Don't fflush stdout here.\nInstead, let the fclose in main do it, so there's a better\nchance the underlying cause (errno) will be reported.\n\nSigned-off-by: Jim Meyering \n---\n builtin-rev-list.c | 1 -\n 1 files changed, 0 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex 813aadf..62f0ba9 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -100,7 +100,6 @@ static void show_commit(struct commit *commit)\n \t\tprintf(\"%s%c\", buf, hdr_termination);\n \t\tfree(buf);\n \t}\n-\tfflush(stdout);\n \tif (commit->parents) {\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n"},{"id":"45791","messageId":"alpine.LFD.0.98.0706251349540.8675@woody.linux-foundation.org","threadId":"8723","inReplyTo":"87r6nzu666.fsf@rho.meyering.net","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-25T20:59:21Z","receivedAt":"2007-06-25T20:59:21Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 25 Jun 2007, Jim Meyering wrote:\n>\n> [this patch depends on the one I posted here:\n> http://marc.info/?l=git&m=118280134031923&w=2 ]\n> \n> Without this patch, git-rev-list unnecessarily omits strerror(errno)\n> from its diagnostic, upon write failure:\n\nAnd this is a perfect example of what's wrong with the whole thing.\n\nDammit, how many times do I need to say this:\n\n - If you want reliable errors, don't use stdio!\n\nThat fflush is there FOR A REASON. You removed it FOR A MUCH LESS \nIMPORTANT REASON!\n\nThat fflush is there exactly because WE DO NOT WANT TO BUFFER the list of \ncommits, because that thing is meant very much to be used for pipelines, \nand it's quite common that the receiving end is going to do something \nasynchronous with the result, and can - and does - want the results as \nsoon as possible.\n\nIOW, things like \"gitk\" use git-rev-list exactly to get the list of \ncommits, and they want that list *incrementally*. They don't want to wait \nfor git-rev-list to have filled up some 8kB buffer of commits. Especially \nsince generating those commits can be slow if we're talking about a big \ntree and some path-limited stuff.\n\nSo for example, do something like\n\n\tgit rev-list HEAD -- drivers/char/drm/Makefile\n\nand if you don't see the result scroll a line at a time on a slower \nmachine, there's something *wrong*. \n\nJunio, I'm NAK'ing this very forcefully!\n\nJim: I don't know what I'm doing wrong, but I'm apparently not reaching \nyou. So let me try one more time:\n\n - stdio really isn't very good with error handling\n\n - if you use stdio, YOU HAD BETTER ACCEPT THAT\n\n - don't screw up basic functionality in your *insane* quest to get stdio \n to give you ENOSPC. It's not going to happen. Not that way. Just face \n the fact that stdio *will* throw error numbers away.\n\nThe whole notion of \"buffered IO\" and \"reliable errors\" is simply not \nsomething that goes well together.\n\n\t\t\tLinus\n"},{"id":"45800","messageId":"87bqf3u324.fsf@rho.meyering.net","threadId":"8723","inReplyTo":"87r6nzu666.fsf@rho.meyering.net","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-25T21:39:15Z","receivedAt":"2007-06-25T21:39:15Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Jim Meyering wrote:\n> [this patch depends on the one I posted here:\n> http://marc.info/?l=git&m=118280134031923&w=2 ]\n\nHere's a version of that patch that retains the fflush call\nand adds a comment explaining why it's needed.\n\nWithout this patch, git-rev-list unnecessarily omits strerror(errno)\nfrom its diagnostic, upon write failure:\n\n $ ./git-rev-list --max-count=1 HEAD > /dev/full\n fatal: write failure on standard output\n\nWith the patch, git reports the desired ENOSPC diagnostic:\n\n fatal: write failure on standard output: No space left on device\n\n* builtin-rev-list (show_commit): Diagnose a failed fflush call.\n\nSigned-off-by: Jim Meyering \n---\n builtin-rev-list.c | 7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex 813aadf..f13a594 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -100,7 +100,12 @@ static void show_commit(struct commit *commit)\n \t\tprintf(\"%s%c\", buf, hdr_termination);\n \t\tfree(buf);\n \t}\n-\tfflush(stdout);\n+\n+\t/* Flush regularly.\n+\t This is especially important for an asynchronous consumer. */\n+\tif (fflush(stdout))\n+\t\tdie(\"write failure on standard output: %s\", strerror(errno));\n+\n \tif (commit->parents) {\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n"},{"id":"45801","messageId":"878xa7u2gh.fsf@rho.meyering.net","threadId":"8723","inReplyTo":"alpine.LFD.0.98.0706251349540.8675@woody.linux-foundation.org","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-25T21:52:14Z","receivedAt":"2007-06-25T21:52:14Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Linus Torvalds wrote:\n\n> On Mon, 25 Jun 2007, Jim Meyering wrote:\n>>\n>> [this patch depends on the one I posted here:\n>> http://marc.info/?l=git&m=118280134031923&w=2 ]\n>>\n>> Without this patch, git-rev-list unnecessarily omits strerror(errno)\n>> from its diagnostic, upon write failure:\n>\n> And this is a perfect example of what's wrong with the whole thing.\n>\n> Dammit, how many times do I need to say this:\n>\n> - If you want reliable errors, don't use stdio!\n>\n> That fflush is there FOR A REASON. You removed it FOR A MUCH LESS\n> IMPORTANT REASON!\n\nWow. No need to curse and get into ALL_CAPS_MODE every time you\nreply to me. It does not advance your cause.\n\nRemember: I'm trying to improve existing code here.\nYou should save some of your ire for the person who wrote that code.\n\n> That fflush is there exactly because WE DO NOT WANT TO BUFFER the list of\n> commits, because that thing is meant very much to be used for pipelines,\n> and it's quite common that the receiving end is going to do something\n> asynchronous with the result, and can - and does - want the results as\n> soon as possible.\n\nThat's good to know. I'm glad you pointed it out. It would have been\nnice to have a comment. However, wouldn't it be better at least to check\nfor and report fflush failure? fflush usually does a write, after all.\nMost of the rest of the code is careful to diagnose write errors at\nthe source. Why not here?\n\nI've posted a revised patch.\n\n> IOW, things like \"gitk\" use git-rev-list exactly to get the list of\n> commits, and they want that list *incrementally*. They don't want to wait\n> for git-rev-list to have filled up some 8kB buffer of commits. Especially\n> since generating those commits can be slow if we're talking about a big\n> tree and some path-limited stuff.\n>\n> So for example, do something like\n>\n> \tgit rev-list HEAD -- drivers/char/drm/Makefile\n>\n> and if you don't see the result scroll a line at a time on a slower\n> machine, there's something *wrong*.\n>\n> Junio, I'm NAK'ing this very forcefully!\n>\n> Jim: I don't know what I'm doing wrong, but I'm apparently not reaching\n> you. So let me try one more time:\n>\n> - stdio really isn't very good with error handling\n>\n> - if you use stdio, YOU HAD BETTER ACCEPT THAT\n\nUsing stdio is fine, as long as you know and respect its limitations.\n\nIt's a real shame that you have to intersperse your often-valuable\nfeedback with such vitriol.\n"},{"id":"45802","messageId":"alpine.LFD.0.98.0706251450240.8675@woody.linux-foundation.org","threadId":"8723","inReplyTo":"87bqf3u324.fsf@rho.meyering.net","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-25T21:53:11Z","receivedAt":"2007-06-25T21:53:11Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 25 Jun 2007, Jim Meyering wrote:\n>\n> Here's a version of that patch that retains the fflush call\n> and adds a comment explaining why it's needed.\n\nOk. I will hereby just suggest to Junio that he just not take patches from \nyou.\n\nYou seem to be totally unable to ever really think or worry about your own \nlittle uninteresting test-case, and have shown yourself totally \nuninterested in anything anybody ever tells you.\n\nIn other words, you now screwed up EPIPE.\n\nAGAIN.\n\nAnd why? All apparently because you want \"disk full\" rather than just \n\"write error\".\n\nJim, you really need to see past your small test, and think about the \nbigger picture.\n\n\t\tLinus\n"},{"id":"45803","messageId":"87odj3sn5q.fsf@rho.meyering.net","threadId":"8723","inReplyTo":"alpine.LFD.0.98.0706251450240.8675@woody.linux-foundation.org","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-25T22:08:01Z","receivedAt":"2007-06-25T22:08:01Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Linus Torvalds wrote:\n> On Mon, 25 Jun 2007, Jim Meyering wrote:\n>>\n>> Here's a version of that patch that retains the fflush call\n>> and adds a comment explaining why it's needed.\n>\n> Ok. I will hereby just suggest to Junio that he just not take patches from\n> you.\n>\n> You seem to be totally unable to ever really think or worry about your own\n> little uninteresting test-case, and have shown yourself totally\n> uninterested in anything anybody ever tells you.\n>\n> In other words, you now screwed up EPIPE.\n>\n> AGAIN.\n>\n> And why? All apparently because you want \"disk full\" rather than just\n> \"write error\".\n>\n> Jim, you really need to see past your small test, and think about the\n> bigger picture.\n\nNo. I don't keep the \"small\", git-specific, picture in mind\nall the time. Git is the only project I contribute to with\nthis no-EPIPE restriction, and it doesn't come naturally yet.\n\nHere's the patch you seem to want.\n\n---------------------------------------------------------\nWithout this patch, git-rev-list unnecessarily omits strerror(errno)\nfrom its diagnostic, upon write failure:\n\n $ ./git-rev-list --max-count=1 HEAD > /dev/full\n fatal: write failure on standard output\n\nWith the patch, git reports the desired ENOSPC diagnostic:\n\n fatal: write failure on standard output: No space left on device\n\n* builtin-rev-list (show_commit): Diagnose a failed fflush call.\n\nSigned-off-by: Jim Meyering \n---\n builtin-rev-list.c | 7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex 813aadf..94f8fca 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -100,7 +100,12 @@ static void show_commit(struct commit *commit)\n \t\tprintf(\"%s%c\", buf, hdr_termination);\n \t\tfree(buf);\n \t}\n-\tfflush(stdout);\n+\n+\t/* Flush regularly.\n+\t This is especially important for an asynchronous consumer. */\n+\tif (fflush(stdout) && errno != EPIPE)\n+\t\tdie(\"write failure on standard output: %s\", strerror(errno));\n+\n \tif (commit->parents) {\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n"},{"id":"45804","messageId":"alpine.LFD.0.98.0706251505570.8675@woody.linux-foundation.org","threadId":"8723","inReplyTo":"878xa7u2gh.fsf@rho.meyering.net","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-25T22:20:16Z","receivedAt":"2007-06-25T22:20:16Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 25 Jun 2007, Jim Meyering wrote:\n> \n> Remember: I'm trying to improve existing code here.\n> You should save some of your ire for the person who wrote that code.\n\nEhh. Remind me who I should be pissed at when the old code was _better_ \nbefore your change?\n\nWith the current git.c, we report write errors quite well. We don't give \nthe exact output you want, but on a scale of 1-10, how important is that? \nPretty damn low on the list.\n\nAnd the reason I'm really really irritated at you is that you ignore me \nwhen I tell you what your bugs are.\n\n - I *told* you that EPIPE is special. What did you do? Ignore my advice, \n and made a broken patch that did exactly the opposite of what I told \n you.\n\n - And I *told* you that you shouldn't care about errno for stdio, because \n stdio was broken. What did you do? You again ignored my advice, and \n made _another_ broken patch, exactly the _opposite_ of what I told you.\n\nIf you really really *must* get that ENOSPC error string output, create a \nhelper function like\n\n\tvoid flush_or_die(FILE *f, const char *desc)\n\t{\n\t\tif (ferror(f))\n\t\t\tdie(\"write failure on %s\", desc)\n\t\tif (fflush(f)) {\n\t\t\tif (errno == EPIPE)\n\t\t\t\texit(0);\n\t\t\tdie(\"write failure on %s: %s\",\n\t\t\t\tdesc, strerror(errno));\n\t\t}\n\t}\n\nand then you can start adding calls to \"flush_or_die()\" to appropriate \nplaces. You could replace the \"fflush()\" in builtin-rev-list.c with a \n\"flush_or_die()\".\n\nAnd then you could add a call to \"log_tree_commit()\" (in log-tree.c), and \nthat would probably be an improvement too (especially if we start having \nthings like gitk parse \"git log\" output, and try to deprecate the old \nreally low-level plumbing a bit).\n\n\t\t\tLinus\n"},{"id":"45806","messageId":"alpine.LFD.0.98.0706251536240.8675@woody.linux-foundation.org","threadId":"8723","inReplyTo":"alpine.LFD.0.98.0706251505570.8675@woody.linux-foundation.org","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-25T22:56:11Z","receivedAt":"2007-06-25T22:56:11Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 25 Jun 2007, Linus Torvalds wrote:\n> \n> If you really really *must* get that ENOSPC error string output, create a \n> helper function like\n> \n> \tvoid flush_or_die(FILE *f, const char *desc)\n> \t{\n> \t\tif (ferror(f))\n> \t\t\tdie(\"write failure on %s\", desc)\n\nActually, even this is really nasty, and it's a case where the current \n\"git.c\" code can also fail.\n\nIt's an example of where EPIPE can actually cause a write failure that you \ncan never figure out what the reason for the error was, because the flush \nwas caused by something earlier (write too much for the buffer or \nwhatever), exactly because stdio throws the error away.\n\nSo after thinking about it some more, I would suggest just ignoring ferror \nentirely, and hoping that any errors are caught by the fflush().\n\nI do hate stdio error checking. In my opinion, there really is only *one* \ncorrect way to use stdio error checking: ignore it. It doesn't work. The \nthing is fundamentally mis-designed for error handling.\n\nSo I think the right solution would literally be to either not do this \nbroken error checking at all, or to rewrite the code that cares about \nerrors to not use stdio.\n\nThere's also another issue: regular files really *are* different from \npipes and sockets and other things. Not because of EPIPE, but because you \nwant different buffering behaviour. For a regular file, we really don't \neven care about the line buffering, and we'd actually be better off (from \na performance angle) without it.\n\nBut we don't have any sane way to save that kind of information (and we \ndefinitely do *not* want to do the \"fstat()\" thing on every flush). We \ncould use the stdio buffering mode, but\n\n - it's too weak (we want not line buffered or block buffered, we want \n basically \"record buffered\")\n\n - I don't think there are any portable ways to read it (only set it, \n using \"setbuf()\" and friends).\n\nAnyway, getting rid of stdio for writes we care about really *would* be a \nnice thing. But it's a lot of boring, nasty work.\n\nSo here's a patch that I think is acceptable. IT IS NOT PERFECT. Stdio \nsimply cannot do a good job on errors, but it has a comment about the case \nwhere it just decides to ignore ferror() instead of doing what I suggest \nabove.\n\nI'm not saying this is great. But doing this right really does require \navoiding stdio entirely.\n\nDoes this work for you?\n\n(I pass the \"desc\" string thing in case there is some future use where we \nalso want to use stdio, but we use it for something else than just regular \nstdout. Dunno).\n\n\t\tLinus\n\n---\n builtin-rev-list.c | 2 +-\n cache.h | 2 ++\n log-tree.c | 1 +\n write_or_die.c | 20 ++++++++++++++++++++\n 4 files changed, 24 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex 813aadf..3980bf4 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -100,7 +100,7 @@ static void show_commit(struct commit *commit)\n \t\tprintf(\"%s%c\", buf, hdr_termination);\n \t\tfree(buf);\n \t}\n-\tfflush(stdout);\n+\tflush_or_die(stdout, \"stdout\");\n \tif (commit->parents) {\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\ndiff --git a/cache.h b/cache.h\nindex ed83d92..aae9e2a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -532,6 +532,8 @@ extern char git_default_name[MAX_GITNAME];\n extern const char *git_commit_encoding;\n extern const char *git_log_output_encoding;\n \n+/* IO helper functions */\n+extern void flush_or_die(FILE *, const char *);\n extern int copy_fd(int ifd, int ofd);\n extern int read_in_full(int fd, void *buf, size_t count);\n extern int write_in_full(int fd, const void *buf, size_t count);\ndiff --git a/log-tree.c b/log-tree.c\nindex 0cf21bc..061ecf7 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -408,5 +408,6 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)\n \t\tshown = 1;\n \t}\n \topt->loginfo = NULL;\n+\tflush_or_die(stdout, \"stdout\");\n \treturn shown;\n }\ndiff --git a/write_or_die.c b/write_or_die.c\nindex 5c4bc85..84f411d 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -1,5 +1,25 @@\n #include \"cache.h\"\n \n+/*\n+ * Some cases use stdio, but want to flush after the write\n+ * to get error handling (and to get better interactive\n+ * behaviour - not buffering excessively).\n+ *\n+ * Of course, if the flush happened within the write itself,\n+ * we've already lost the error code, and cannot report it any\n+ * more. So we just ignore that case instead (and hope we get\n+ * the right error code on the flush).\n+ */\n+void flush_or_die(FILE *f, const char *desc)\n+{\n+\tif (fflush(f)) {\n+\t\tif (errno == EPIPE)\n+\t\t\texit(0);\n+\t\tdie(\"write failure on %s: %s\",\n+\t\t\tdesc, strerror(errno));\n+\t}\n+}\n+\n int read_in_full(int fd, void *buf, size_t count)\n {\n \tchar *p = buf;\n"},{"id":"45807","messageId":"alpine.LFD.0.98.0706251557090.8675@woody.linux-foundation.org","threadId":"8723","inReplyTo":"alpine.LFD.0.98.0706251536240.8675@woody.linux-foundation.org","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-25T23:01:58Z","receivedAt":"2007-06-25T23:01:58Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 25 Jun 2007, Linus Torvalds wrote:\n> \n> Actually, even this is really nasty, and it's a case where the current \n> \"git.c\" code can also fail.\n\nJust to clarify: this is why the git.c code obviously does the fstat(), in \ncase anybody wondered.\n\nSo I didn't mean to imply that the new git.c code in 'next' is wrong, I \njust meant to imply that the \"ferror()\"+\"fflush()\" sequence that it uses \nand that I copied for my example is a very unreliable sequence, and it \nbasically fails exactly because you can never know what caused the \nferror() to trigger - if it *ever* triggers, you're basically screwed.\n\nI wonder how many applications actually ever use ferror() and friends. \n\n\t\tLinus\n"},{"id":"45808","messageId":"alpine.LFD.0.98.0706251607000.8675@woody.linux-foundation.org","threadId":"8723","inReplyTo":"alpine.LFD.0.98.0706251536240.8675@woody.linux-foundation.org","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-25T23:16:56Z","receivedAt":"2007-06-25T23:16:56Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 25 Jun 2007, Linus Torvalds wrote:\n> \n> There's also another issue: regular files really *are* different from \n> pipes and sockets and other things. Not because of EPIPE, but because you \n> want different buffering behaviour. For a regular file, we really don't \n> even care about the line buffering, and we'd actually be better off (from \n> a performance angle) without it.\n\nJust for fun, I tried this out.\n\nDoing\n\n\ttime git log > logfile\n\non the kernel repo with and without the patch I just sent out, I get:\n\nWithout:\n\n\treal 0m1.361s\n\tuser 0m1.312s\n\tsys 0m0.040s\n\nWith:\n\n\treal 0m1.687s\n\tuser 0m1.392s\n\tsys 0m0.284s\n\nso doing the extra flushing does actually cost us (it's just fundamentally \nmore expensive to do disk IO on non-block-boundaries).\n\nIt would be much nicer if we only did it for sockets and pipes, which \ndon't have the same block-boundary issues anyway (there's still the system \ncall cost, but on a pipe/socket, the real costs tend to be elsewhere).\n\nAgain, this is something that a non-stdio-based buffering library would \neasily handle. You could just test the file descriptor _once_ at the \nbeginning, to see if it's a regular file or not. And then you could have \nthe error handling where it belongs (when the IO is actually done, and the \nerror actually happens) rather than in the callers using a bad interface \nthat sometimes loses 'errno'.\n\nBtw, to balance the above performance comment: doing the flush_or_die() \nobviously *does* mean that you get better performance in the odd cases. \nFor example, if you do\n\n\t[torvalds@woody linux]$ trap '' SIGPIPE\n\t[torvalds@woody linux]$ time git log | head\n\nI get get 0.002s, while it used to be:\n\n\treal 0m1.382s\n\tuser 0m1.340s\n\tsys 0m0.028s\n\njust because it did the whole thing regardless of any EPIPE errors.\n\nOf course, that case probably isn't very usual, but I could imagine that \nsome users of \"git blame -C --incremental\" could actually cause situations \nlike this (ie just stop listening when they got the part they're \ninterested in, and maybe they'd have some strange reason to ignore \nSIGPIPE).\n\nSo I'm not opposed to the patch I sent out, I just wanted to point out \nthat this is an area we *could* improve upon if we didn't do that stdio \nthing.\n\n\t\tLinus\n"},{"id":"45828","messageId":"20070626090605.GA4349@coredump.intra.peff.net","threadId":"8723","inReplyTo":"alpine.LFD.0.98.0706251536240.8675@woody.linux-foundation.org","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-06-26T09:06:05Z","receivedAt":"2007-06-26T09:06:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 25, 2007 at 03:56:11PM -0700, Linus Torvalds wrote:\n\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -408,5 +408,6 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)\n> \t\tshown = 1;\n> \t}\n> \topt->loginfo = NULL;\n> +\tflush_or_die(stdout, \"stdout\");\n> \treturn shown;\n> }\n\nI think flushing here is a good change regardless of the error checking.\nSometimes, when you are severely limiting commits, the whole output is\nsmaller than the buffer, and you end up waiting a long time for any\noutput even though your answer may have been found immediately.\n\nFor example, 'git-whatchanged -Sfoo' when 'foo' was introduced in the\nlast couple of commits (but wasn't referenced before) will have to\ncalculate diffs on all of history before producing output. Flushing\nafter every commit restores the illusion that git provides your answer\ninstaneously. :)\n\n-Peff\n"},{"id":"45838","messageId":"20070626171127.GA28810@thunk.org","threadId":"8723","inReplyTo":"alpine.LFD.0.98.0706251607000.8675@woody.linux-foundation.org","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Theodore Tso","fromEmail":"tytso@mit.edu","sentAt":"2007-06-26T17:11:28Z","receivedAt":"2007-06-26T17:11:28Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Mon, Jun 25, 2007 at 04:16:56PM -0700, Linus Torvalds wrote:\n> Again, this is something that a non-stdio-based buffering library would \n> easily handle. You could just test the file descriptor _once_ at the \n> beginning, to see if it's a regular file or not. And then you could have \n> the error handling where it belongs (when the IO is actually done, and the \n> error actually happens) rather than in the callers using a bad interface \n> that sometimes loses 'errno'.\n\nIs there something obviously wrong with doing something like this?\n\n\tif ((fstat(fileno(stdout), &st) < 0) &&\n\t !S_ISREG(st.st_mode))\n\t\tsetbuf(stdout, NULL);\n\nThis would change stdout to use completely unbuffered I/O we're not\nsending the output to a file.\n\n\t\t\t\t\t\t- Ted\n"},{"id":"45839","messageId":"alpine.LFD.0.98.0706261006570.8675@woody.linux-foundation.org","threadId":"8723","inReplyTo":"20070626090605.GA4349@coredump.intra.peff.net","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-26T17:12:58Z","receivedAt":"2007-06-26T17:12:58Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 26 Jun 2007, Jeff King wrote:\n> \n> I think flushing here is a good change regardless of the error checking.\n> Sometimes, when you are severely limiting commits, the whole output is\n> smaller than the buffer, and you end up waiting a long time for any\n> output even though your answer may have been found immediately.\n\nExactly. That's why it was done in git-rev-list, and why it's good to do \nin \"git log\" too.\n\nThe slowdown worries me a bit, but it's only noticeable with *lots* of \noutput - ie no path limiting (and no diffs - the diff generation becomes \nthe bottleneck if you do diffs). So the flushing slows down a case that we \ndo ridiculously well right now, so I doubt anybody will really care.\n\nIt's more a benchmarking issue: \"we can show the whole log of the kernel \nin under two seconds\", and it didn't slow down *that* much.\n\n> For example, 'git-whatchanged -Sfoo' when 'foo' was introduced in the\n> last couple of commits (but wasn't referenced before) will have to\n> calculate diffs on all of history before producing output. Flushing\n> after every commit restores the illusion that git provides your answer\n> instaneously. :)\n\nOn that note, it should probably also do it for the incremental output of \ngit blame.\n\n\t\tLinus\n---\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex f7e2c13..86ab6c7 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1459,6 +1459,7 @@ static void found_guilty_entry(struct blame_entry *ent)\n \t\t\t\tprintf(\"boundary\\n\");\n \t\t}\n \t\twrite_filename_info(suspect->path);\n+\t\tflush_or_die(stdout, \"stdout\");\n \t}\n }\n \n"},{"id":"45840","messageId":"alpine.LFD.0.98.0706261024210.8675@woody.linux-foundation.org","threadId":"8723","inReplyTo":"20070626171127.GA28810@thunk.org","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-26T17:32:23Z","receivedAt":"2007-06-26T17:32:23Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 26 Jun 2007, Theodore Tso wrote:\n> \n> Is there something obviously wrong with doing something like this?\n> \n> \tif ((fstat(fileno(stdout), &st) < 0) &&\n> \t !S_ISREG(st.st_mode))\n> \t\tsetbuf(stdout, NULL);\n> \n> This would change stdout to use completely unbuffered I/O we're not\n> sending the output to a file.\n\nWell, we might as well keep it line-buffered, so I'd use setvbuf(_IOLBF) \ninstead. Totally unbuffered is bad, since we often do printf's in smaller \nchunks.\n\nBut we actually _do_ want fully buffered from a performance angle. \nEspecially for the big stuff, which is usually the *diffs*, not the commit \nmessages. Not so much an issue with git-rev-list, but with \"git log -p\" \nyou would normally not want it line-buffered, and it's actually much nicer \nto let it be fully buffered and then do a flush at the end.\n\nEven pipes are often used for \"throughput\" stuff if you end up doing some \npost-processing (ie \"git log -p | gather-statistics\"), and yes, I actually \ndo things like that - it's nice for things like looking at how many lines \nhave been added during the last release cycle:\n\n\tgit log -p v2.6.21.. | grep '^+' | wc -l\n\nand I'd really like thigns like that to be close to optimal. \n\nHow much the system call overhead is, I don't know, though. So it might be \nworth testing out. Under Linux, you'll probably have a fairly hard time \nseeing any difference, but under other OS's you have both system call \nlatency issues *and* possible scheduling issues (ie the above kind of \npipeline can act very differently from a scheduling standpoint if you send \nlots of small things rather than buffer things a bit on the generating \nside)\n\n\t\tLinus\n"},{"id":"45852","messageId":"20070626220440.GB27828@thunk.org","threadId":"8723","inReplyTo":"alpine.LFD.0.98.0706261024210.8675@woody.linux-foundation.org","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Theodore Tso","fromEmail":"tytso@mit.edu","sentAt":"2007-06-26T22:04:40Z","receivedAt":"2007-06-26T22:04:40Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Tue, Jun 26, 2007 at 10:32:23AM -0700, Linus Torvalds wrote:\n> But we actually _do_ want fully buffered from a performance angle. \n> Especially for the big stuff, which is usually the *diffs*, not the commit \n> messages. Not so much an issue with git-rev-list, but with \"git log -p\" \n> you would normally not want it line-buffered, and it's actually much nicer \n> to let it be fully buffered and then do a flush at the end.\n\nWell, sometimes we do and sometimes we don't right? Some of it\ndepends on how large the *stuff* we are dumping out (diffs vs. commit\nobjects), and what the receiving process on the other bit of the\npipeline is doing with the data --- is it doing some kind of\nstatistical reduction (git-rev-log .. | wc -l) versus some kind of\nasynchronous processing (git-rev-log as used by gitk).\n\nSo maybe the answer is that when outputing to a file, we always use\nfull stdio buffering, and in other cases there should be some\nintelligent defaults plus command-line overrides. So when we emit\nlists of commit identifiers, it should probably be line buffered, and\nif it is diffs, it should probably be fully buffered, etc.?\n\n\t\t\t\t\t\t- Ted\n"},{"id":"45855","messageId":"alpine.LFD.0.98.0706261528130.8675@woody.linux-foundation.org","threadId":"8723","inReplyTo":"20070626220440.GB27828@thunk.org","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-26T22:32:39Z","receivedAt":"2007-06-26T22:32:39Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 26 Jun 2007, Theodore Tso wrote:\n> On Tue, Jun 26, 2007 at 10:32:23AM -0700, Linus Torvalds wrote:\n> > But we actually _do_ want fully buffered from a performance angle. \n> > Especially for the big stuff, which is usually the *diffs*, not the commit \n> > messages. Not so much an issue with git-rev-list, but with \"git log -p\" \n> > you would normally not want it line-buffered, and it's actually much nicer \n> > to let it be fully buffered and then do a flush at the end.\n> \n> Well, sometimes we do and sometimes we don't right?\n\nNo.\n\nWe basically _never_ want \"line buffered\" or \"unbuffered\", which is what \nstdio knows how to do. That sucks in _all_ cases.\n\nWhat we want is \"fully buffered\" for plain files, and \"record buffered\" \nfor anything else (where a \"record\" is basically the \"commit + optional \ndiff\").\n\nWe can get the record buffered by adding the fflush() calls, but the thing \nis, we'd want to _avoid_ that if it was a file. It's just that there is no \nway to set that kind of flag portably with stdio, we'd have to carry it \naround _separately_ from stdio, which is a big pain.\n\nBut if we decide that this only matters with stdout (which currently is \nwhat the patches have done), we could of course just make it a single \nglobal variable (like \"stdout\" itself already is). Then we could just make \ngit.c start out by testing stdout at startup and setting the global \nvariable.\n\n\t\tLinus\n"},{"id":"45897","messageId":"87wsxppygx.fsf@rho.meyering.net","threadId":"8723","inReplyTo":"alpine.LFD.0.98.0706251557090.8675@woody.linux-foundation.org","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-27T08:56:30Z","receivedAt":"2007-06-27T08:56:30Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Linus Torvalds wrote:\n...\n> So I didn't mean to imply that the new git.c code in 'next' is wrong, I\n> just meant to imply that the \"ferror()\"+\"fflush()\" sequence that it uses\n> and that I copied for my example is a very unreliable sequence, and it\n> basically fails exactly because you can never know what caused the\n> ferror() to trigger - if it *ever* triggers, you're basically screwed.\n>\n> I wonder how many applications actually ever use ferror() and friends.\n\nAt least among GNU programs, many do.\nHere are a few:\n\n gcc, make, diff, grep, tar, find, xargs, gawk\n\nAnd add to that 89 or 90 of the coreutils programs:\n\n $ git-grep -l close_stdout|grep 'src/.*\\.c'|wc -l\n 89\n\nBut none of those suppress EPIPE errors, so ferror works fine for them.\nThat's partly because the ferror-only situation is far less common\nthan the one in which we have a valid errno value.\n"},{"id":"45898","messageId":"87tzstpyck.fsf@rho.meyering.net","threadId":"8723","inReplyTo":"alpine.LFD.0.98.0706251505570.8675@woody.linux-foundation.org","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-27T08:59:07Z","receivedAt":"2007-06-27T08:59:07Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Linus Torvalds wrote:\n> On Mon, 25 Jun 2007, Jim Meyering wrote:\n>> Remember: I'm trying to improve existing code here.\n>> You should save some of your ire for the person who wrote that code.\n>\n> Ehh. Remind me who I should be pissed at when the old code was _better_\n> before your change?\n\nIf someone pointed out a bug in coreutils whereby a tool failed\nto give an accurate diagnostic, I would thank them (and honestly,\nI'd be grateful), even if their attempt at fixing it introduced\na horrible bug. Of course, if the original bug were mine, I might\nbe annoyed with myself, but certainly would not shoot the messenger.\n\n> With the current git.c, we report write errors quite well.\n\nYes, I'm glad (in spite of the tone of much of your feedback)\nthat my little improvements are making it into git.\n\n> We don't give\n> the exact output you want, but on a scale of 1-10, how important is that?\n> Pretty damn low on the list.\n\nIf you get a report of a git write error, you might well\nappreciate knowing right away whether it was due to EIO or to ENOSPC.\n\n> And the reason I'm really really irritated at you is that you ignore me\n> when I tell you what your bugs are.\n\nBe careful when throwing stones...\nI mentioned (without making a fuss) a regression you introduced,\n http://thread.gmane.org/gmane.comp.version-control.git/50742/focus=50917\nand you included similar code in a snippet in the very message to which\nI'm replying now. I do see that you omitted the troublesome ferror call\nfrom your eventual patch.\n\n> - I *told* you that EPIPE is special. What did you do? Ignore my advice,\n> and made a broken patch that did exactly the opposite of what I told\n> you.\n\nI did not ignore your advice. I considered it carefully and explained\nwhy I hold the opposing view. We've already beaten this to death, but\nfor the record, I'd summarize it like this:\n\nYou and I have a difference of opinion. I firmly believe that it is\nunnecessary and counterproductive (it is prohibited by POSIX for many\nof the tools I maintain) to suppress an EPIPE diagnostic and to exit\nsuccessfully in spite of a write error. Since EPIPE doesn't even arise\nin most normal situations, the only reason to do that is if you want\nto pander to the needs of some theoretical, broken-by-design (i.e.,\nSIGPIPE-ignoring) git Porcelain program. You want to add such code to\ngit, at every point where writing to stdout may fail.\n"},{"id":"45926","messageId":"alpine.LFD.0.98.0706270849230.8675@woody.linux-foundation.org","threadId":"8723","inReplyTo":"87tzstpyck.fsf@rho.meyering.net","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-27T16:06:55Z","receivedAt":"2007-06-27T16:06:55Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 27 Jun 2007, Jim Meyering wrote:\n> \n> If someone pointed out a bug in coreutils whereby a tool failed\n> to give an accurate diagnostic\n\nThe thing is, I don't think stdio output is \"important\" enough.\n\nIn fact, by definition, if the data was really important, it had better \nnot use stdio.\n\nSo look at where git uses \"real\" IO, and where it uses stdio, and you'll \nsee where the \"diagnostics\" matter.\n\nIn other words, the diagnostics matter for:\n - the actual git database (including refs).\n - doing things like \"git checkout\" that writes the working tree\n\nThose are the ones where we want to *guarantee* correctness. And that \nmeans that they don't use stdio (and if any of them do, then that really \n*is* a bug, and should be fixed asap)\n\nIn contrast, if you redirect stdout, that's a totally different level of \ncorrectness, and all bets are off. Exactly because we use stdio. If we \nreally cared, we shouldn't use stdio. \n\n> Be careful when throwing stones...\n> I mentioned (without making a fuss) a regression you introduced,\n> http://thread.gmane.org/gmane.comp.version-control.git/50742/focus=50917\n> and you included similar code in a snippet in the very message to which\n> I'm replying now. I do see that you omitted the troublesome ferror call\n> from your eventual patch.\n\nRight. There's a difference between an email saying \"ok, this is how you \nmight want to do it\" and then actually sitting down and thinking about it \nand coding it.\n\nIf you look closer, you'll notice that my initial suggestion wouldn't even \n*compile*. \n\nBy the time I sent out a patch, I had actually thought it through.\n\nAnd no, I don't guarantee that I do that all the time. I try to, but hey, \nmistakes happen. It's when the mistake is repeated over and over after \nbeing pointed out that I get irritated.\n\n> You and I have a difference of opinion. I firmly believe that it is\n> unnecessary and counterproductive (it is prohibited by POSIX for many\n> of the tools I maintain) to suppress an EPIPE diagnostic and to exit\n> successfully in spite of a write error.\n\nQuite frankly, the tools you maintain are mostly filters in the first \nplace! In other words, for you, stdout *matters*. That's the real data!\n\nBut look at something like \"cp\" one of these days, and ask yourself: \n\n - do you check error returns for the IO operations that actually do the \n copy (I hope yes). \n\n I also hope to high heaven that you would never use stdio for that IO. \n Because you *will* lose error information!\n\n - do you check the error returns of informational messages like the ones \n from the verbose output switch (cp -v).\n\nIf you answered \"no\" to the second question, you should really think about \nthat fact. Please realize that there's a *huge* difference in error \nchecking between \"core data\" and \"random information\".\n\nFor something like \"cat\", the core data is the actual stream. You care a \nlot more about that. For git, the core data is the working tree and the \ndatabase. I care about _that_. That's the stuff that should be protected \nand must never _ever_ fail silently.\n\nThe rest is really \"statistics\".\n\nLet's get it right, but unlike core data, that's mostly a politeness \nissue, much less important than \"oops, we just corrupted your archive\".\n\nAnd when we talk about politeness, avoiding the _idiotic_ error messages \nlike \"Broken pipe\" is a big part of it! The \"Broken pipe\" message means \neither:\n - the receiver already got everything he wanted (and you should damn well \n shut up about it)\n - the receiver had some problem (and it's damn well *that* which matters, \n since the sender was fine, and you shouldn't confuse the issue by \n writing bogus error messages)\n\nThe fact that you have some broken POSIX spec to worry about is not git's \nproblem. I hold git to higher standards.\n\n\t\tLinus\n"},{"id":"45997","messageId":"20070628190406.GC29279@thunk.org","threadId":"8723","inReplyTo":"alpine.LFD.0.98.0706261024210.8675@woody.linux-foundation.org","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Theodore Tso","fromEmail":"tytso@mit.edu","sentAt":"2007-06-28T19:04:06Z","receivedAt":"2007-06-28T19:04:06Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Tue, Jun 26, 2007 at 10:32:23AM -0700, Linus Torvalds wrote:\n> But we actually _do_ want fully buffered from a performance angle. \n> Especially for the big stuff, which is usually the *diffs*, not the commit \n> messages. Not so much an issue with git-rev-list, but with \"git log -p\" \n> you would normally not want it line-buffered, and it's actually much nicer \n> to let it be fully buffered and then do a flush at the end.\n> \n> Even pipes are often used for \"throughput\" stuff if you end up doing some \n> post-processing (ie \"git log -p | gather-statistics\"), and yes, I actually \n> do things like that - it's nice for things like looking at how many lines \n> have been added during the last release cycle\n> \n> \tgit log -p v2.6.21.. | grep '^+' | wc -l\n> \n> and I'd really like thigns like that to be close to optimal. \n> \n> How much the system call overhead is, I don't know, though. So it might be \n> worth testing out....\n\nSo just for yuks, I devised the following patch, and did some measurements....\n\nFor the above pipeline, the result was hardly worth mentioning:\n\nWith flushing:\n\n% time git log -p v2.6.21.. | grep '^+' | wc -l\n\nreal 0m22.330s\nuser 0m21.512s\nsys 0m0.807s\n\n# of write() system calls according to strace -c: 15167\n\nWithout flushing:\n\n% time git log -p v2.6.21.. | grep '^+' | wc -l\n\nreal 0m22.367s\nuser 0m21.355s\nsys 0m0.720s\n\n# of write() system calls according to strace -c: 11373\n\nSo here's the worst case:\n\n% time git-rev-list HEAD > /dev/null\nreal 0m1.575s\nuser 0m1.477s\nsys 0m0.053s\n# of write() system calls according to strace -c: 582\n\n% (export GIT_NEVER_FLUSH_STDOUT=t; time git-rev-list HEAD > /dev/null) \nreal 0m1.535s\nuser 0m1.463s\nsys 0m0.027s\n# of write() system calls according to strace -c: 58055\n\n> Under Linux, you'll probably have a fairly hard time \n> seeing any difference, but under other OS's you have both system call \n> latency issues *and* possible scheduling issues (ie the above kind of \n> pipeline can act very differently from a scheduling standpoint if you send \n> lots of small things rather than buffer things a bit on the generating \n> side)\n\nIndeed. So it's not at all clear this patch is worth applying, but\nmaybe it would make a difference on some other OS; of course, this\npatch also obviates the original intent of Jim Meyering's patch, since\nit means that we won't print a useful error message if stdout has been\nredirected to a file and the write returns ENOSPC, since we won't be\nfflush() when stdout is redirected to a file. \n\nThe added fflush() calls to the incremental git-blame and the\ngit-log-*/git-whatchanged might make it worthwhile for tools that\ndepend on those outputs and want faster user response time. So maybe\nadding the fflush() call might be worthwhile for those programs.\n\n\t\t\t\t\t\t- Ted\n\n\ncommit 7f483ec6366f62d52199e3edefa292a110fcb5c8\nAuthor: Theodore Ts'o \nDate: Thu Jun 28 14:10:58 2007 -0400\n\n Don't fflush(stdout) when it's not helpful\n \n This patch arose from a discussion started by Jim Meyering's patch\n whose intention was to provide better diagnostics for failed writes.\n Linus proposed a better way to do things, which also had the added\n benefit that adding a fflush() to git-log-* operations and incremental\n git-blame operations could improve interactive respose time feel, at\n the cost of making things a bit slower when we aren't piping the\n output to a downstream program.\n \n This patch skips the fflush() calls when stdout is a regular file, or\n if the environment variable GIT_NEVER_FLUSH_STDOUT is set. This\n latter can speed up a command such as:\n \n \t(export GIT_NEVER_FLUSH_STDOUT=t; git-rev-list HEAD | wc -l)\n \n a tiny amount.\n \n Cc: Linus Torvalds \n Signed-off-by: \"Theodore Ts'o\" \n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex f7e2c13..da23a6f 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1459,6 +1459,7 @@ static void found_guilty_entry(struct blame_entry *ent)\n \t\t\t\tprintf(\"boundary\\n\");\n \t\t}\n \t\twrite_filename_info(suspect->path);\n+\t\tmaybe_flush_or_die(stdout, \"stdout\");\n \t}\n }\n \ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex 813aadf..86db8b0 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -100,7 +100,7 @@ static void show_commit(struct commit *commit)\n \t\tprintf(\"%s%c\", buf, hdr_termination);\n \t\tfree(buf);\n \t}\n-\tfflush(stdout);\n+\tmaybe_flush_or_die(stdout, \"stdout\");\n \tif (commit->parents) {\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\ndiff --git a/cache.h b/cache.h\nindex ed83d92..0525c4e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -532,6 +532,8 @@ extern char git_default_name[MAX_GITNAME];\n extern const char *git_commit_encoding;\n extern const char *git_log_output_encoding;\n \n+/* IO helper functions */\n+extern void maybe_flush_or_die(FILE *, const char *);\n extern int copy_fd(int ifd, int ofd);\n extern int read_in_full(int fd, void *buf, size_t count);\n extern int write_in_full(int fd, const void *buf, size_t count);\ndiff --git a/log-tree.c b/log-tree.c\nindex 0cf21bc..ced3f33 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -408,5 +408,6 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)\n \t\tshown = 1;\n \t}\n \topt->loginfo = NULL;\n+\tmaybe_flush_or_die(stdout, \"stdout\");\n \treturn shown;\n }\ndiff --git a/write_or_die.c b/write_or_die.c\nindex 5c4bc85..2cebeb5 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -1,5 +1,43 @@\n #include \"cache.h\"\n \n+/*\n+ * Some cases use stdio, but want to flush after the write\n+ * to get error handling (and to get better interactive\n+ * behaviour - not buffering excessively).\n+ *\n+ * Of course, if the flush happened within the write itself,\n+ * we've already lost the error code, and cannot report it any\n+ * more. So we just ignore that case instead (and hope we get\n+ * the right error code on the flush).\n+ *\n+ * If the file handle is stdout, and stdout is a file, then skip the\n+ * flush entirely since it's not needed.\n+ */\n+void maybe_flush_or_die(FILE *f, const char *desc)\n+{\n+\tstatic int stdout_is_file = -1;\n+\tstruct stat st;\n+\n+\tif (f == stdout) {\n+\t\tif (stdout_is_file < 0) {\n+\t\t\tif (getenv(\"GIT_NEVER_FLUSH_STDOUT\") ||\n+\t\t\t ((fstat(fileno(stdout), &st) == 0) &&\n+\t\t\t S_ISREG(st.st_mode)))\n+\t\t\t\tstdout_is_file = 1;\n+\t\t\telse\n+\t\t\t\tstdout_is_file = 0;\n+\t\t}\n+\t\tif (stdout_is_file)\n+\t\t\treturn;\n+\t}\n+\tif (fflush(f)) {\n+\t\tif (errno == EPIPE)\n+\t\t\texit(0);\n+\t\tdie(\"write failure on %s: %s\",\n+\t\t\tdesc, strerror(errno));\n+\t}\n+}\n+\n int read_in_full(int fd, void *buf, size_t count)\n {\n \tchar *p = buf;\n"},{"id":"46001","messageId":"20070628213451.GB22455@coredump.intra.peff.net","threadId":"8723","inReplyTo":"20070628190406.GC29279@thunk.org","subject":"Re: [PATCH] git-rev-list: give better diagnostic for failed write","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-06-28T21:34:51Z","receivedAt":"2007-06-28T21:34:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 28, 2007 at 03:04:06PM -0400, Theodore Tso wrote:\n\n> This patch skips the fflush() calls when stdout is a regular file, or\n> if the environment variable GIT_NEVER_FLUSH_STDOUT is set. This\n> latter can speed up a command such as:\n> \n> \t(export GIT_NEVER_FLUSH_STDOUT=t; git-rev-list HEAD | wc -l)\n\nI wonder if this would be more natural in the opposite form:\n\n GIT_FLUSH_STDOUT=1 git-rev-list HEAD\n\nIn general, you don't want to do the flushing unless:\n - it's going to the pager\n - some program is reading incrementally\n\nIn the first case, we can just turn on GIT_FLUSH_STDOUT when we kick off\nthe pager. In the second case, that program can just add the variable to\nits invocation.\n\nOn top of which, in your patch the type of output trumps the environment\nvariable, which seems backwards. In other words, I can't do this:\n\nGIT_FLUSH_EVEN_THOUGH_ITS_A_FILE=1 git-rev-list HEAD >file\n[in another window] tail -f file\n\nI would think an explicit preference from a variable should override any\nguesses.\n\n-Peff\n"},{"id":"46009","messageId":"20070628235319.GD29279@thunk.org","threadId":"8723","inReplyTo":"20070628213451.GB22455@coredump.intra.peff.net","subject":"[PATCH] Don't fflush(stdout) when it's not helpful","fromName":"Theodore Tso","fromEmail":"tytso@mit.edu","sentAt":"2007-06-28T23:53:19Z","receivedAt":"2007-06-28T23:53:19Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Thu, Jun 28, 2007 at 05:34:51PM -0400, Jeff King wrote:\n> On top of which, in your patch the type of output trumps the environment\n> variable, which seems backwards. In other words, I can't do this:\n> \n> GIT_FLUSH_EVEN_THOUGH_ITS_A_FILE=1 git-rev-list HEAD >file\n> [in another window] tail -f file\n> \n> I would think an explicit preference from a variable should override any\n> guesses.\n\nGood point. Here's a revised patch where GIT_FLUSH=0 and GIT_FLASH=1\ntrumps any hueristics.\n\nMy comments about this making only trivial differences on Linux still\napply (although it does make things slightly more optimal). I suspect\nit might make more of a difference on MacOS, but I haven't had time to\nbenchmark it.\n\n\t\t\t\t\t\t\t- Ted\n\n---\ncommit 422becc0f8430d2386ceed92f224a94c9047751e\nAuthor: Theodore Ts'o \nDate: Thu Jun 28 14:10:58 2007 -0400\n\n Don't fflush(stdout) when it's not helpful\n \n This patch arose from a discussion started by Jim Meyering's patch\n whose intention was to provide better diagnostics for failed writes.\n Linus proposed a better way to do things, which also had the added\n benefit that adding a fflush() to git-log-* operations and incremental\n git-blame operations could improve interactive respose time feel, at\n the cost of making things a bit slower when we aren't piping the\n output to a downstream program.\n \n This patch skips the fflush() calls when stdout is a regular file, or\n if the environment variable GIT_FLUSH is set to \"0\". This latter can\n speed up a command such as:\n \n GIT_FLUSH=0 strace -c -f -e write time git-rev-list HEAD | wc -l\n \n a tiny amount.\n \n Cc: Linus Torvalds \n Signed-off-by: \"Theodore Ts'o\" \n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 20b5b7b..8269148 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -396,6 +396,16 @@ other\n 'GIT_PAGER'::\n \tThis environment variable overrides `$PAGER`.\n \n+'GIT_FLUSH'::\n+\tIf this environment variable is set to \"1\", then commands such\n+\tas git-blame (in incremental mode), git-rev-list, git-log,\n+\tgit-whatchanged, etc., will force a flush of the output stream\n+\tafter each commit-oriented record have been flushed. If this\n+\tvariable is set to \"0\", the output of these commands will be done\n+\tusing completely buffered I/O. If this environment variable is\n+\tnot set, git will choose buffered or record-oriented flushing\n+\tbased on whether stdout appears to be redirected to a file or not.\n+\n 'GIT_TRACE'::\n \tIf this variable is set to \"1\", \"2\" or \"true\" (comparison\n \tis case insensitive), git will print `trace:` messages on\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex f7e2c13..da23a6f 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1459,6 +1459,7 @@ static void found_guilty_entry(struct blame_entry *ent)\n \t\t\t\tprintf(\"boundary\\n\");\n \t\t}\n \t\twrite_filename_info(suspect->path);\n+\t\tmaybe_flush_or_die(stdout, \"stdout\");\n \t}\n }\n \ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex 813aadf..86db8b0 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -100,7 +100,7 @@ static void show_commit(struct commit *commit)\n \t\tprintf(\"%s%c\", buf, hdr_termination);\n \t\tfree(buf);\n \t}\n-\tfflush(stdout);\n+\tmaybe_flush_or_die(stdout, \"stdout\");\n \tif (commit->parents) {\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\ndiff --git a/cache.h b/cache.h\nindex ed83d92..0525c4e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -532,6 +532,8 @@ extern char git_default_name[MAX_GITNAME];\n extern const char *git_commit_encoding;\n extern const char *git_log_output_encoding;\n \n+/* IO helper functions */\n+extern void maybe_flush_or_die(FILE *, const char *);\n extern int copy_fd(int ifd, int ofd);\n extern int read_in_full(int fd, void *buf, size_t count);\n extern int write_in_full(int fd, const void *buf, size_t count);\ndiff --git a/log-tree.c b/log-tree.c\nindex 0cf21bc..ced3f33 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -408,5 +408,6 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)\n \t\tshown = 1;\n \t}\n \topt->loginfo = NULL;\n+\tmaybe_flush_or_die(stdout, \"stdout\");\n \treturn shown;\n }\ndiff --git a/write_or_die.c b/write_or_die.c\nindex 5c4bc85..9929a15 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -1,5 +1,47 @@\n #include \"cache.h\"\n \n+/*\n+ * Some cases use stdio, but want to flush after the write\n+ * to get error handling (and to get better interactive\n+ * behaviour - not buffering excessively).\n+ *\n+ * Of course, if the flush happened within the write itself,\n+ * we've already lost the error code, and cannot report it any\n+ * more. So we just ignore that case instead (and hope we get\n+ * the right error code on the flush).\n+ *\n+ * If the file handle is stdout, and stdout is a file, then skip the\n+ * flush entirely since it's not needed.\n+ */\n+void maybe_flush_or_die(FILE *f, const char *desc)\n+{\n+\tstatic int stdout_is_file = -1;\n+\tstruct stat st;\n+\tchar *cp;\n+\n+\tif (f == stdout) {\n+\t\tif (stdout_is_file < 0) {\n+\t\t\tcp = getenv(\"GIT_FLUSH\");\n+\t\t\tif (cp)\n+\t\t\t\tstdout_is_file = (atoi(cp) == 0);\n+\t\t\telse if (getenv(\"GIT_NEVER_FLUSH_STDOUT\") ||\n+\t\t\t\t ((fstat(fileno(stdout), &st) == 0) &&\n+\t\t\t\t S_ISREG(st.st_mode)))\n+\t\t\t\tstdout_is_file = 1;\n+\t\t\telse\n+\t\t\t\tstdout_is_file = 0;\n+\t\t}\n+\t\tif (stdout_is_file)\n+\t\t\treturn;\n+\t}\n+\tif (fflush(f)) {\n+\t\tif (errno == EPIPE)\n+\t\t\texit(0);\n+\t\tdie(\"write failure on %s: %s\",\n+\t\t\tdesc, strerror(errno));\n+\t}\n+}\n+\n int read_in_full(int fd, void *buf, size_t count)\n {\n \tchar *p = buf;\n"},{"id":"46014","messageId":"20070629010507.GL12721@planck.djpig.de","threadId":"8723","inReplyTo":"20070628235319.GD29279@thunk.org","subject":"Re: [PATCH] Don't fflush(stdout) when it's not helpful","fromName":"Frank Lichtenheld","fromEmail":"frank@lichtenheld.de","sentAt":"2007-06-29T01:05:08Z","receivedAt":"2007-06-29T01:05:08Z","isPatch":true,"sender":{"key":"frank@lichtenheld.de","avatar":"https://gravatar.com/avatar/b9f1d4b120e138f157c9e480d0818197c474628923786adb98f30017cdb99c3c?d=mp&s=160"},"body":"On Thu, Jun 28, 2007 at 07:53:19PM -0400, Theodore Tso wrote:\n> Good point. Here's a revised patch where GIT_FLUSH=0 and GIT_FLASH=1\n> trumps any hueristics.\n\n[...]\n\n> +\t\t\telse if (getenv(\"GIT_NEVER_FLUSH_STDOUT\") ||\n> +\t\t\t\t ((fstat(fileno(stdout), &st) == 0) &&\n> +\t\t\t\t S_ISREG(st.st_mode)))\n> +\t\t\t\tstdout_is_file = 1;\n> +\t\t\telse\n> +\t\t\t\tstdout_is_file = 0;\n> +\t\t}\n\nAny particular reason why you still check for GIT_NEVER_FLUSH_STDOUT in\naddition to GIT_FLUSH?\n\nGruesse,\n-- \nFrank Lichtenheld \nwww: http://www.djpig.de/\n"},{"id":"46017","messageId":"20070629034838.GF29279@thunk.org","threadId":"8723","inReplyTo":"20070629010507.GL12721@planck.djpig.de","subject":"Re: [PATCH] Don't fflush(stdout) when it's not helpful","fromName":"Theodore Tso","fromEmail":"tytso@mit.edu","sentAt":"2007-06-29T03:48:38Z","receivedAt":"2007-06-29T03:48:38Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Fri, Jun 29, 2007 at 03:05:08AM +0200, Frank Lichtenheld wrote:\n> Any particular reason why you still check for GIT_NEVER_FLUSH_STDOUT in\n> addition to GIT_FLUSH?\n\nYup, I forgot to remove it. :-)\n\n\t\t\t\t\t- Ted\n\n---\ncommit 473065f89f27de476d12d774141009cd4f2600c4\nAuthor: Theodore Ts'o \nDate: Thu Jun 28 14:10:58 2007 -0400\n\n Don't fflush(stdout) when it's not helpful\n \n This patch arose from a discussion started by Jim Meyering's patch\n whose intention was to provide better diagnostics for failed writes.\n Linus proposed a better way to do things, which also had the added\n benefit that adding a fflush() to git-log-* operations and incremental\n git-blame operations could improve interactive respose time feel, at\n the cost of making things a bit slower when we aren't piping the\n output to a downstream program.\n \n This patch skips the fflush() calls when stdout is a regular file, or\n if the environment variable GIT_FLUSH is set to \"0\". This latter can\n speed up a command such as:\n \n GIT_FLUSH=0 strace -c -f -e write time git-rev-list HEAD | wc -l\n \n a tiny amount.\n \n Cc: Linus Torvalds \n Signed-off-by: \"Theodore Ts'o\" \n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 20b5b7b..8269148 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -396,6 +396,16 @@ other\n 'GIT_PAGER'::\n \tThis environment variable overrides `$PAGER`.\n \n+'GIT_FLUSH'::\n+\tIf this environment variable is set to \"1\", then commands such\n+\tas git-blame (in incremental mode), git-rev-list, git-log,\n+\tgit-whatchanged, etc., will force a flush of the output stream\n+\tafter each commit-oriented record have been flushed. If this\n+\tvariable is set to \"0\", the output of these commands will be done\n+\tusing completely buffered I/O. If this environment variable is\n+\tnot set, git will choose buffered or record-oriented flushing\n+\tbased on whether stdout appears to be redirected to a file or not.\n+\n 'GIT_TRACE'::\n \tIf this variable is set to \"1\", \"2\" or \"true\" (comparison\n \tis case insensitive), git will print `trace:` messages on\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex f7e2c13..da23a6f 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1459,6 +1459,7 @@ static void found_guilty_entry(struct blame_entry *ent)\n \t\t\t\tprintf(\"boundary\\n\");\n \t\t}\n \t\twrite_filename_info(suspect->path);\n+\t\tmaybe_flush_or_die(stdout, \"stdout\");\n \t}\n }\n \ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex 813aadf..86db8b0 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -100,7 +100,7 @@ static void show_commit(struct commit *commit)\n \t\tprintf(\"%s%c\", buf, hdr_termination);\n \t\tfree(buf);\n \t}\n-\tfflush(stdout);\n+\tmaybe_flush_or_die(stdout, \"stdout\");\n \tif (commit->parents) {\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\ndiff --git a/cache.h b/cache.h\nindex ed83d92..0525c4e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -532,6 +532,8 @@ extern char git_default_name[MAX_GITNAME];\n extern const char *git_commit_encoding;\n extern const char *git_log_output_encoding;\n \n+/* IO helper functions */\n+extern void maybe_flush_or_die(FILE *, const char *);\n extern int copy_fd(int ifd, int ofd);\n extern int read_in_full(int fd, void *buf, size_t count);\n extern int write_in_full(int fd, const void *buf, size_t count);\ndiff --git a/log-tree.c b/log-tree.c\nindex 0cf21bc..ced3f33 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -408,5 +408,6 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)\n \t\tshown = 1;\n \t}\n \topt->loginfo = NULL;\n+\tmaybe_flush_or_die(stdout, \"stdout\");\n \treturn shown;\n }\ndiff --git a/write_or_die.c b/write_or_die.c\nindex 5c4bc85..3fd7c5c 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -1,5 +1,46 @@\n #include \"cache.h\"\n \n+/*\n+ * Some cases use stdio, but want to flush after the write\n+ * to get error handling (and to get better interactive\n+ * behaviour - not buffering excessively).\n+ *\n+ * Of course, if the flush happened within the write itself,\n+ * we've already lost the error code, and cannot report it any\n+ * more. So we just ignore that case instead (and hope we get\n+ * the right error code on the flush).\n+ *\n+ * If the file handle is stdout, and stdout is a file, then skip the\n+ * flush entirely since it's not needed.\n+ */\n+void maybe_flush_or_die(FILE *f, const char *desc)\n+{\n+\tstatic int stdout_is_file = -1;\n+\tstruct stat st;\n+\tchar *cp;\n+\n+\tif (f == stdout) {\n+\t\tif (stdout_is_file < 0) {\n+\t\t\tcp = getenv(\"GIT_FLUSH\");\n+\t\t\tif (cp)\n+\t\t\t\tstdout_is_file = (atoi(cp) == 0);\n+\t\t\telse if ((fstat(fileno(stdout), &st) == 0) &&\n+\t\t\t\t S_ISREG(st.st_mode))\n+\t\t\t\tstdout_is_file = 1;\n+\t\t\telse\n+\t\t\t\tstdout_is_file = 0;\n+\t\t}\n+\t\tif (stdout_is_file)\n+\t\t\treturn;\n+\t}\n+\tif (fflush(f)) {\n+\t\tif (errno == EPIPE)\n+\t\t\texit(0);\n+\t\tdie(\"write failure on %s: %s\",\n+\t\t\tdesc, strerror(errno));\n+\t}\n+}\n+\n int read_in_full(int fd, void *buf, size_t count)\n {\n \tchar *p = buf;\n"},{"id":"46028","messageId":"20070629063819.GA23138@coredump.intra.peff.net","threadId":"8723","inReplyTo":"20070629034838.GF29279@thunk.org","subject":"Re: [PATCH] Don't fflush(stdout) when it's not helpful","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-06-29T06:38:19Z","receivedAt":"2007-06-29T06:38:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 28, 2007 at 11:48:38PM -0400, Theodore Tso wrote:\n\n> +void maybe_flush_or_die(FILE *f, const char *desc)\n> +{\n> +\tstatic int stdout_is_file = -1;\n> +\tstruct stat st;\n> +\tchar *cp;\n> +\n> +\tif (f == stdout) {\n> +\t\tif (stdout_is_file < 0) {\n> +\t\t\tcp = getenv(\"GIT_FLUSH\");\n> +\t\t\tif (cp)\n> +\t\t\t\tstdout_is_file = (atoi(cp) == 0);\n> +\t\t\telse if ((fstat(fileno(stdout), &st) == 0) &&\n> +\t\t\t\t S_ISREG(st.st_mode))\n> +\t\t\t\tstdout_is_file = 1;\n> +\t\t\telse\n> +\t\t\t\tstdout_is_file = 0;\n> +\t\t}\n> +\t\tif (stdout_is_file)\n> +\t\t\treturn;\n> +\t}\n\nLooks much better to me, but I have one minor nit: stdout_is_file is a\npoor name, since it can mean either that stdout is a file, or that\nflushing was explicitly turned off. Naming it something like\nstdout_wants_flush would make much more sense. Though it's not a huge\ndeal since the function is fairly short, I think it makes things a\nlittle easier to read (I had to double-check the negation on atoi(cp) ==\n0 before I convinced myself the code was correct).\n\n-Peff\n"},{"id":"46030","messageId":"7vmyyjgrxk.fsf@assigned-by-dhcp.pobox.com","threadId":"8723","inReplyTo":"20070629063819.GA23138@coredump.intra.peff.net","subject":"Re: [PATCH] Don't fflush(stdout) when it's not helpful","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-29T07:07:03Z","receivedAt":"2007-06-29T07:07:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jun 28, 2007 at 11:48:38PM -0400, Theodore Tso wrote:\n>\n>> +void maybe_flush_or_die(FILE *f, const char *desc)\n>> +{\n>> +\tstatic int stdout_is_file = -1;\n>> +\tstruct stat st;\n>> +\tchar *cp;\n>> +\n>> +\tif (f == stdout) {\n>> +\t\tif (stdout_is_file < 0) {\n>> +\t\t\tcp = getenv(\"GIT_FLUSH\");\n>> +\t\t\tif (cp)\n>> +\t\t\t\tstdout_is_file = (atoi(cp) == 0);\n>> +\t\t\telse if ((fstat(fileno(stdout), &st) == 0) &&\n>> +\t\t\t\t S_ISREG(st.st_mode))\n>> +\t\t\t\tstdout_is_file = 1;\n>> ...\n>\n> Looks much better to me, but I have one minor nit: stdout_is_file is a\n> poor name,...\n\nThanks for bringing it up, as I had the same \"Huh?\" moment.\nI would probably call that simply \"do_not_flush\". Or name the\nvariable \"flush_stdout\" and swap all the logic.\n\n\tif (f == stdout) {\n \tif (flush_stdout < 0) {\n \tcp = getenv(\"GIT_FLUSH_STDOUT\");\n if (cp)\n \tflush_stdout = !!atoi(cp);\n\t\t\telse if ((fstat(fileno(stdout), &st) == 0) &&\n\t\t\t\t!S_ISREG(st.st_mode))\n\t\t\t\tflush_stdout = 0;\n\t\t\telse\n \tflush_stdout = 1;\n\t\t}\n if (!flush_stdout)\n \treturn;\n\t}\n"},{"id":"46051","messageId":"alpine.LFD.0.98.0706290851480.8675@woody.linux-foundation.org","threadId":"8723","inReplyTo":"7vmyyjgrxk.fsf@assigned-by-dhcp.pobox.com","subject":"Re: [PATCH] Don't fflush(stdout) when it's not helpful","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-29T16:06:22Z","receivedAt":"2007-06-29T16:06:22Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 29 Jun 2007, Junio C Hamano wrote:\n> \n> Thanks for bringing it up, as I had the same \"Huh?\" moment.\n> I would probably call that simply \"do_not_flush\". Or name the\n> variable \"flush_stdout\" and swap all the logic.\n\nI think that patch looks fine, but I also think that there is a more \nfundamental problem with this approach:\n\n - all these patches basically break the whole _point_ of Jim's original \n reason for wanting this!\n\nSo remember, we had two different reasons for flushing:\n\n - interactivity over a pipe. \n\n Tools like \"gitk\" and \"git gui blame\" all run waiting for input, and we \n want to give them the commit data as soon as possible.\n\n - error checking on a filesystem.\n\n Yes, \"ferror()\" _after_ the write shows an error happened, but doesn't \n show what error it was. Doing a fflush() is a lot more likely to \n actually give the right errors.\n\nSo non-files want flushing due to latency issues, and files want flushing \ndue to error handling. And the patch under discussion basically broke the \nerror handling.\n\n(It turns out that it doesn't break it for the trivial \"/dev/full\" \ntestcase, since that doesn't show up as a file, but it would break it for \nthe *real* case of a filesystem that becomes full).\n\nOne option is to change the git.c thing to do\n\n\tif (fflush(stdout))\n\t\tdie(\"write failure on standard output: %s\", strerror(errno));\n\tif (ferror(stdout))\n\t\tdie(\"unknown write failure on standard output\");\n\tif (fclose(stdout))\n\t\tdie(\"close failure on standard output: %s\", strerror(errno));\n\nwhere the \"fflush()\" is done first (regardless of ferror), just in the \n(probably futile) hope of getting the right errno.\n\n\t\tLinus\n"},{"id":"46055","messageId":"20070629174046.GC16268@thunk.org","threadId":"8723","inReplyTo":"alpine.LFD.0.98.0706290851480.8675@woody.linux-foundation.org","subject":"Re: [PATCH] Don't fflush(stdout) when it's not helpful","fromName":"Theodore Tso","fromEmail":"tytso@mit.edu","sentAt":"2007-06-29T17:40:46Z","receivedAt":"2007-06-29T17:40:46Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Fri, Jun 29, 2007 at 09:06:22AM -0700, Linus Torvalds wrote:\n> I think that patch looks fine, but I also think that there is a more \n> fundamental problem with this approach:\n> \n> - all these patches basically break the whole _point_ of Jim's original \n> reason for wanting this!\n\nYeah, I pointed that out in my first patch. It had seemed that\ninteractivity over a pipe was considered more important, though when\nwe started talking about things. :-)\n\nIt looks like from my reading of the standard that ferror(f) should\nnot change the state of the file handle f. So the following patch I\nthink should work; it checks ferror(f), and if it indicates that there\nis an error, we try a flush to get the error message. I've tested\nunder Linux and it gives the correct error message in the \"git log >\n/mnt/full-filesystem\" case, and I believe it should DTRT on other\nsystems.\n\nComments?\n\n\t\t\t\t\t\t- Ted\n\ncommit 93a96f94028106687412acbb771bb18ee7ec5560\nAuthor: Theodore Ts'o \nDate: Thu Jun 28 14:10:58 2007 -0400\n\n Don't fflush(stdout) when it's not helpful\n \n This patch arose from a discussion started by Jim Meyering's patch\n whose intention was to provide better diagnostics for failed writes.\n Linus proposed a better way to do things, which also had the added\n benefit that adding a fflush() to git-log-* operations and incremental\n git-blame operations could improve interactive respose time feel, at\n the cost of making things a bit slower when we aren't piping the\n output to a downstream program.\n \n This patch skips the fflush() calls when stdout is a regular file, or\n if the environment variable GIT_FLUSH is set to \"0\". This latter can\n speed up a command such as:\n \n GIT_FLUSH=0 strace -c -f -e write time git-rev-list HEAD | wc -l\n \n a tiny amount.\n \n Cc: Linus Torvalds \n Signed-off-by: \"Theodore Ts'o\" \n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 20b5b7b..8269148 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -396,6 +396,16 @@ other\n 'GIT_PAGER'::\n \tThis environment variable overrides `$PAGER`.\n \n+'GIT_FLUSH'::\n+\tIf this environment variable is set to \"1\", then commands such\n+\tas git-blame (in incremental mode), git-rev-list, git-log,\n+\tgit-whatchanged, etc., will force a flush of the output stream\n+\tafter each commit-oriented record have been flushed. If this\n+\tvariable is set to \"0\", the output of these commands will be done\n+\tusing completely buffered I/O. If this environment variable is\n+\tnot set, git will choose buffered or record-oriented flushing\n+\tbased on whether stdout appears to be redirected to a file or not.\n+\n 'GIT_TRACE'::\n \tIf this variable is set to \"1\", \"2\" or \"true\" (comparison\n \tis case insensitive), git will print `trace:` messages on\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex f7e2c13..da23a6f 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1459,6 +1459,7 @@ static void found_guilty_entry(struct blame_entry *ent)\n \t\t\t\tprintf(\"boundary\\n\");\n \t\t}\n \t\twrite_filename_info(suspect->path);\n+\t\tmaybe_flush_or_die(stdout, \"stdout\");\n \t}\n }\n \ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex 813aadf..86db8b0 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -100,7 +100,7 @@ static void show_commit(struct commit *commit)\n \t\tprintf(\"%s%c\", buf, hdr_termination);\n \t\tfree(buf);\n \t}\n-\tfflush(stdout);\n+\tmaybe_flush_or_die(stdout, \"stdout\");\n \tif (commit->parents) {\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\ndiff --git a/cache.h b/cache.h\nindex ed83d92..0525c4e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -532,6 +532,8 @@ extern char git_default_name[MAX_GITNAME];\n extern const char *git_commit_encoding;\n extern const char *git_log_output_encoding;\n \n+/* IO helper functions */\n+extern void maybe_flush_or_die(FILE *, const char *);\n extern int copy_fd(int ifd, int ofd);\n extern int read_in_full(int fd, void *buf, size_t count);\n extern int write_in_full(int fd, const void *buf, size_t count);\ndiff --git a/log-tree.c b/log-tree.c\nindex 0cf21bc..ced3f33 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -408,5 +408,6 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)\n \t\tshown = 1;\n \t}\n \topt->loginfo = NULL;\n+\tmaybe_flush_or_die(stdout, \"stdout\");\n \treturn shown;\n }\ndiff --git a/write_or_die.c b/write_or_die.c\nindex 5c4bc85..e125e11 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -1,5 +1,45 @@\n #include \"cache.h\"\n \n+/*\n+ * Some cases use stdio, but want to flush after the write\n+ * to get error handling (and to get better interactive\n+ * behaviour - not buffering excessively).\n+ *\n+ * Of course, if the flush happened within the write itself,\n+ * we've already lost the error code, and cannot report it any\n+ * more. So we just ignore that case instead (and hope we get\n+ * the right error code on the flush).\n+ *\n+ * If the file handle is stdout, and stdout is a file, then skip the\n+ * flush entirely since it's not needed.\n+ */\n+void maybe_flush_or_die(FILE *f, const char *desc)\n+{\n+\tstatic int skip_stdout_flush = -1;\n+\tstruct stat st;\n+\tchar *cp;\n+\n+\tif (f == stdout) {\n+\t\tif (skip_stdout_flush < 0) {\n+\t\t\tcp = getenv(\"GIT_FLUSH\");\n+\t\t\tif (cp)\n+\t\t\t\tskip_stdout_flush = (atoi(cp) == 0);\n+\t\t\telse if ((fstat(fileno(stdout), &st) == 0) &&\n+\t\t\t\t S_ISREG(st.st_mode))\n+\t\t\t\tskip_stdout_flush = 1;\n+\t\t\telse\n+\t\t\t\tskip_stdout_flush = 0;\n+\t\t}\n+\t\tif (skip_stdout_flush && !ferror(f))\n+\t\t\treturn;\n+\t}\n+\tif (fflush(f)) {\n+\t\tif (errno == EPIPE)\n+\t\t\texit(0);\n+\t\tdie(\"write failure on %s: %s\", desc, strerror(errno));\n+\t}\n+}\n+\n int read_in_full(int fd, void *buf, size_t count)\n {\n \tchar *p = buf;\n"},{"id":"46056","messageId":"alpine.LFD.0.98.0706291641590.8675@woody.linux-foundation.org","threadId":"8723","inReplyTo":"20070629174046.GC16268@thunk.org","subject":"Re: [PATCH] Don't fflush(stdout) when it's not helpful","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-29T23:43:14Z","receivedAt":"2007-06-29T23:43:14Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 29 Jun 2007, Theodore Tso wrote:\n> \n> Comments?\n\nLooks ok to me. \n\nThis should probably be paired up with the change to git.c (in \"next\") to \ndo the \"fflush()\" before the \"ferror()\" too, in case the error is pending.\n\n\t\tLinus\n"},{"id":"46059","messageId":"7vlke2dw6w.fsf@assigned-by-dhcp.pobox.com","threadId":"8723","inReplyTo":"alpine.LFD.0.98.0706291641590.8675@woody.linux-foundation.org","subject":"Re: [PATCH] Don't fflush(stdout) when it's not helpful","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-30T02:15:35Z","receivedAt":"2007-06-30T02:15:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Fri, 29 Jun 2007, Theodore Tso wrote:\n>> \n>> Comments?\n>\n> Looks ok to me. \n>\n> This should probably be paired up with the change to git.c (in \"next\") to \n> do the \"fflush()\" before the \"ferror()\" too, in case the error is pending.\n\nDo you mean this part?\n\n+\t/* Somebody closed stdout? */\n+\tif (fstat(fileno(stdout), &st))\n+\t\treturn 0;\n+\t/* Ignore write errors for pipes and sockets.. */\n+\tif (S_ISFIFO(st.st_mode) || S_ISSOCK(st.st_mode))\n+\t\treturn 0;\n+\n+\t/* Check for ENOSPC and EIO errors.. */\n+\tif (ferror(stdout))\n+\t\tdie(\"write failure on standard output\");\n+\tif (fflush(stdout) || fclose(stdout))\n+\t\tdie(\"write failure on standard output: %s\", strerror(errno));\n+\n+\treturn 0;\n+}\n\nI was planning to push this out to 'master' this weekend.\n"},{"id":"46063","messageId":"alpine.LFD.0.98.0706292114350.8675@woody.linux-foundation.org","threadId":"8723","inReplyTo":"7vlke2dw6w.fsf@assigned-by-dhcp.pobox.com","subject":"Re: [PATCH] Don't fflush(stdout) when it's not helpful","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-30T04:24:41Z","receivedAt":"2007-06-30T04:24:41Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 29 Jun 2007, Junio C Hamano wrote:\n> \n> Do you mean this part?\n> \n> +\t/* Somebody closed stdout? */\n> +\tif (fstat(fileno(stdout), &st))\n> +\t\treturn 0;\n> +\t/* Ignore write errors for pipes and sockets.. */\n> +\tif (S_ISFIFO(st.st_mode) || S_ISSOCK(st.st_mode))\n> +\t\treturn 0;\n> +\n> +\t/* Check for ENOSPC and EIO errors.. */\n> +\tif (ferror(stdout))\n> +\t\tdie(\"write failure on standard output\");\n> +\tif (fflush(stdout) || fclose(stdout))\n> +\t\tdie(\"write failure on standard output: %s\", strerror(errno));\n> +\n> +\treturn 0;\n> +}\n> \n> I was planning to push this out to 'master' this weekend.\n\nI think that code is fine, but switching the order around could probably \nmake it less likely that stdio loses the errno for us. \n\nSo doing the last part in a different order, and making it say\n\n\t/* Check for ENOSPC and EIO errors.. */\n\tif (fflush(stdout))\n\t\tdie(\"write failure on standard output: %s\", strerror(errno));\n\tif (ferror(stdout))\n\t\tdie(\"unknown write failure on standard output\");\n\tif (fclose(stdout))\n\t\tdie(\"close failed on standard output: %s\", strerror(errno));\n\treturn 0;\n\nmay recover at least non-transient errors.\n\nIt's still not perfect. As I've been harping on, stdio simply isn't very \ngood for error reporting. For example, if an IO error happened, you'd want \nto see EIO, wouldn't you? And yes, that's what the kernel would return. \nHowever, with buffered stdio (and flushing outside of our control), what \nwould likely happen is that some intermediate error return _does_ return \nEIO, but then the kernel might decide to re-mount the filesystem read-only \ndue to the error, and the actual *report* for us might be\n\n\t\"write failure on standard output: read-only filesystem\"\n\nwhich lost the EIO. \n\nBut even worse, if the output happened to be buffer-aligned, stdio will \nhave thrown the error out entirely, and the \"fflush()\" will return 0, and \nthen we end up with that \"unknown write failure\" after all.\n\nOr we might have had a ENOSPC at some point, but removed a temp-file, and \nthe final fflush() doesn't error out at all, so now the incomplete write \ngot done (with one or more buffer chunks missing), and we get \"unknown \nwrite failure\" again, because we again lost the ENOSPC.\n\nSo you basically cannot get \"perfect\" with stdio. It's impossible. But the \nabove re-ordering will at least get you _closer_, and *most* of the time \nyou'll get exactly the error you'd expect.\n\n(I'm not a huge fan of \"most of the time it works\", but that's stdio for \nyou).\n\n\t\tLinus\n"},{"id":"46090","messageId":"20070630142741.GA28917@thunk.org","threadId":"8723","inReplyTo":"alpine.LFD.0.98.0706292114350.8675@woody.linux-foundation.org","subject":"Re: [PATCH] Don't fflush(stdout) when it's not helpful","fromName":"Theodore Tso","fromEmail":"tytso@mit.edu","sentAt":"2007-06-30T14:27:41Z","receivedAt":"2007-06-30T14:27:41Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Fri, Jun 29, 2007 at 09:24:41PM -0700, Linus Torvalds wrote:\n> So you basically cannot get \"perfect\" with stdio. It's impossible. But the \n> above re-ordering will at least get you _closer_, and *most* of the time \n> you'll get exactly the error you'd expect.\n\nWell, we *can* get perfect with stdio, but I can pretty much guarantee\nno one will like it. We could wrap printf, fprintf, purchar, et. al,\nand check each of them for an error return. The spec doesn't\ncurrently specify that errno is supposed to be set, but a defect[1]\nwas filed last year saying that fprintf et.al, are indeed supposed to\nset errno and not throw it away.\n\nIt would be a real pain in the *ss, though....\n\n\t\t\t\t\t\t\t- Ted\n\n[1] http://www.opengroup.org/austin/mailarchives/ag-review/msg02161.html\n"},{"id":"46113","messageId":"7vbqexcmim.fsf@assigned-by-dhcp.cox.net","threadId":"8723","inReplyTo":"alpine.LFD.0.98.0706292114350.8675@woody.linux-foundation.org","subject":"Re: [PATCH] Don't fflush(stdout) when it's not helpful","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-30T18:42:09Z","receivedAt":"2007-06-30T18:42:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n>> I was planning to push this out to 'master' this weekend.\n>\n> I think that code is fine, but switching the order around could probably \n> make it less likely that stdio loses the errno for us. \n>\n> So doing the last part in a different order, and making it say\n>\n> \t/* Check for ENOSPC and EIO errors.. */\n> \tif (fflush(stdout))\n> \t\tdie(\"write failure on standard output: %s\", strerror(errno));\n> \tif (ferror(stdout))\n> \t\tdie(\"unknown write failure on standard output\");\n> \tif (fclose(stdout))\n> \t\tdie(\"close failed on standard output: %s\", strerror(errno));\n> \treturn 0;\n>\n> may recover at least non-transient errors.\n\nThat makes sense, to a certain degree, given that we do not\ncheck every printf(). I'll forge your signature as usual ;-)\n\nThanks.\n"}]}