{"thread":{"id":"8689","subject":"(resend) [PATCH] Don't ignore write failure from git-diff, git-log, etc.","startedAt":"2007-06-23T15:13:44Z","lastAt":"2007-06-26T13:33:08Z","messageCount":13,"participants":["Jim Meyering","Junio C Hamano","Linus Torvalds","Johannes Schindelin","Johannes Sixt","Matthias Lederhofer"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"45614","messageId":"87abuq1z6f.fsf@rho.meyering.net","threadId":"8689","inReplyTo":null,"subject":"(resend) [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-23T15:13:44Z","receivedAt":"2007-06-23T15:13:44Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Hi Jun,\n\nHere's a copy of my patch, from here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/48469/focus=48636\n\nJim\n\n-----------------------------\nFrom 42e3a6f676e9ae4e9640bc2ff36b7ab0b061a60e Mon Sep 17 00:00:00 2001\nFrom: Jim Meyering \nDate: Sat, 26 May 2007 13:43:07 +0200\nSubject: [PATCH] Don't ignore write failure from git-diff, git-log, etc.\n\nCurrently, when git-diff writes to a full device or gets an I/O error,\nit fails to detect the write error:\n\n $ git-diff |wc -c\n 3984\n $ git-diff > /dev/full && echo ignored write failure\n ignored write failure\n\ngit-log does the same thing:\n\n $ git-log -n1 > /dev/full && echo ignored write failure\n ignored write failure\n\nEach and every git command should report such a failure.\nSome already do, but with the patch below, they all do, and we\nwon't have to rely on code in each command's implementation to\nperform the right incantation.\n\n $ ./git-log -n1 > /dev/full\n fatal: write failure on standard output: No space left on device\n [Exit 128]\n $ ./git-diff > /dev/full\n fatal: write failure on standard output: No space left on device\n [Exit 128]\n\nYou can demonstrate this with git's own --version output, too:\n(but git --help detects the failure without this patch)\n\n $ ./git --version > /dev/full\n fatal: write failure on standard output: No space left on device\n [Exit 128]\n\nNote that the fcntl test (for whether the fileno may be closed) is\nrequired in order to avoid EBADF upon closing an already-closed stdout,\nas would happen for each git command that already closes stdout; I think\nupdate-index was the one I noticed in the failure of t5400, before I\nadded that test.\n\nAlso, to be consistent with e.g., write_or_die, do not\ndiagnose EPIPE write failures.\n\nSigned-off-by: Jim Meyering \n---\n git.c | 19 ++++++++++++++++++-\n 1 files changed, 18 insertions(+), 1 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 29b55a1..8258885 100644\n--- a/git.c\n+++ b/git.c\n@@ -308,6 +308,7 @@ static void handle_internal_command(int argc, const char **argv, char **envp)\n \tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n \t\tstruct cmd_struct *p = commands+i;\n \t\tconst char *prefix;\n+\t\tint status;\n \t\tif (strcmp(p->cmd, cmd))\n \t\t\tcontinue;\n\n@@ -321,7 +322,23 @@ static void handle_internal_command(int argc, const char **argv, char **envp)\n \t\t\tdie(\"%s must be run in a work tree\", cmd);\n \t\ttrace_argv_printf(argv, argc, \"trace: built-in: git\");\n\n-\t\texit(p->fn(argc, argv, prefix));\n+\t\tstatus = p->fn(argc, argv, prefix);\n+\n+\t\t/* Close stdout if necessary, and diagnose any failure\n+\t\t other than EPIPE. */\n+\t\tif (fcntl(fileno (stdout), F_GETFD) >= 0) {\n+\t\t\terrno = 0;\n+\t\t\tif ((ferror(stdout) || fclose(stdout))\n+\t\t\t && errno != EPIPE) {\n+\t\t\t\tif (errno == 0)\n+\t\t\t\t\tdie(\"write failure on standard output\");\n+\t\t\t\telse\n+\t\t\t\t\tdie(\"write failure on standard output\"\n+\t\t\t\t\t \": %s\", strerror(errno));\n+\t\t\t}\n+\t\t}\n+\n+\t\texit(status);\n \t}\n }\n\n--\n1.5.2.73.g18bece\n"},{"id":"45652","messageId":"7vzm2pwws8.fsf@assigned-by-dhcp.cox.net","threadId":"8689","inReplyTo":"87abuq1z6f.fsf@rho.meyering.net","subject":"Re: (resend) [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-24T09:01:59Z","receivedAt":"2007-06-24T09:01:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n> From: Jim Meyering \n> Date: Sat, 26 May 2007 13:43:07 +0200\n> Subject: [PATCH] Don't ignore write failure from git-diff, git-log, etc.\n>\n> Currently, when git-diff writes to a full device or gets an I/O error,\n> it fails to detect the write error:\n> ...\n> Also, to be consistent with e.g., write_or_die, do not\n> diagnose EPIPE write failures.\n\nI still do not like the fact that this patch makes an error from\nthe final stdout flushing override the return value from p->fn()\neven when the function already diagnosed an error, but otherwise\nI think it is a good change, as it allows us to catch one error\ncase that we currently don't, without introducing an annoying\nEPIPE diagnosis.\n\nNaks, or vetoes?\n"},{"id":"45674","messageId":"alpine.LFD.0.98.0706240951440.3593@woody.linux-foundation.org","threadId":"8689","inReplyTo":"7vzm2pwws8.fsf@assigned-by-dhcp.cox.net","subject":"Re: (resend) [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-24T17:08:01Z","receivedAt":"2007-06-24T17:08:01Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 24 Jun 2007, Junio C Hamano wrote:\n> \n> I still do not like the fact that this patch makes an error from\n> the final stdout flushing override the return value from p->fn()\n> even when the function already diagnosed an error\n\nYeah.\n\nI also don't think it's very _pretty_ code, and it violates my personal \ncoding standards by adding way too deep indentation for the new error \ncases. It was already three indents deep (reasonably fine, but that \nNOT_BARE test wass already pretty ugly), but now it becomes five \nindentation levels deep at its deepest, which is just a sign that things \nshould be split up.\n\nI'd also like to know why it does that fcntl() is done, and I also wonder \nabout that \"ferror()\" call: it is entirely possible that ferror() is set \ndue to EPIPE, and in that case, it will *not* set errno to EPIPE at all, \nso it will *still* complain about what I consider an invalid situation.\n\nI dunno. I think the ENOSPC worry is a very real and valid one, but I \nwould really tend prefer something different.\n\nHow about this following series of two patches instead, which I'll send as \nreplies to this email..\n\n\t\tLinus\n"},{"id":"45675","messageId":"alpine.LFD.0.98.0706241008060.3593@woody.linux-foundation.org","threadId":"8689","inReplyTo":"alpine.LFD.0.98.0706240951440.3593@woody.linux-foundation.org","subject":"[PATCH 1/2] Clean up internal command handling","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-24T17:10:40Z","receivedAt":"2007-06-24T17:10:40Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nThis should change no code at all, it just moves the definition of \"struct \ncmd_struct\" out, and then splits out the running of the right command into \nthe \"run_command()\" function.\n\nIt also removes the long-unused 'envp' pointer passing.\n\nThis is just preparation for adding some more error checking.\n\nSigned-off-by: Linus Torvalds \n---\n\n git.c | 52 ++++++++++++++++++++++++++++++----------------------\n 1 files changed, 30 insertions(+), 22 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 29b55a1..6c728e4 100644\n--- a/git.c\n+++ b/git.c\n@@ -216,14 +216,34 @@ const char git_version_string[] = GIT_VERSION;\n */\n #define NOT_BARE \t(1<<2)\n \n-static void handle_internal_command(int argc, const char **argv, char **envp)\n+struct cmd_struct {\n+\tconst char *cmd;\n+\tint (*fn)(int, const char **, const char *);\n+\tint option;\n+};\n+\n+static NORETURN void run_command(struct cmd_struct *p, int argc, const char **argv)\n+{\n+\tconst char *prefix;\n+\n+\tprefix = NULL;\n+\tif (p->option & RUN_SETUP)\n+\t\tprefix = setup_git_directory();\n+\tif (p->option & USE_PAGER)\n+\t\tsetup_pager();\n+\tif (p->option & NOT_BARE) {\n+\t\tif (is_bare_repository() || is_inside_git_dir())\n+\t\t\tdie(\"%s must be run in a work tree\", p->cmd);\n+\t}\n+\ttrace_argv_printf(argv, argc, \"trace: built-in: git\");\n+\n+\texit(p->fn(argc, argv, prefix));\n+}\n+\n+static void handle_internal_command(int argc, const char **argv)\n {\n \tconst char *cmd = argv[0];\n-\tstatic struct cmd_struct {\n-\t\tconst char *cmd;\n-\t\tint (*fn)(int, const char **, const char *);\n-\t\tint option;\n-\t} commands[] = {\n+\tstatic struct cmd_struct commands[] = {\n \t\t{ \"add\", cmd_add, RUN_SETUP | NOT_BARE },\n \t\t{ \"annotate\", cmd_annotate, RUN_SETUP | USE_PAGER },\n \t\t{ \"apply\", cmd_apply },\n@@ -307,25 +327,13 @@ static void handle_internal_command(int argc, const char **argv, char **envp)\n \n \tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n \t\tstruct cmd_struct *p = commands+i;\n-\t\tconst char *prefix;\n \t\tif (strcmp(p->cmd, cmd))\n \t\t\tcontinue;\n-\n-\t\tprefix = NULL;\n-\t\tif (p->option & RUN_SETUP)\n-\t\t\tprefix = setup_git_directory();\n-\t\tif (p->option & USE_PAGER)\n-\t\t\tsetup_pager();\n-\t\tif ((p->option & NOT_BARE) &&\n-\t\t\t\t(is_bare_repository() || is_inside_git_dir()))\n-\t\t\tdie(\"%s must be run in a work tree\", cmd);\n-\t\ttrace_argv_printf(argv, argc, \"trace: built-in: git\");\n-\n-\t\texit(p->fn(argc, argv, prefix));\n+\t\trun_command(p, argc, argv);\n \t}\n }\n \n-int main(int argc, const char **argv, char **envp)\n+int main(int argc, const char **argv)\n {\n \tconst char *cmd = argv[0] ? argv[0] : \"git-help\";\n \tchar *slash = strrchr(cmd, '/');\n@@ -358,7 +366,7 @@ int main(int argc, const char **argv, char **envp)\n \tif (!prefixcmp(cmd, \"git-\")) {\n \t\tcmd += 4;\n \t\targv[0] = cmd;\n-\t\thandle_internal_command(argc, argv, envp);\n+\t\thandle_internal_command(argc, argv);\n \t\tdie(\"cannot handle %s internally\", cmd);\n \t}\n \n@@ -390,7 +398,7 @@ int main(int argc, const char **argv, char **envp)\n \n \twhile (1) {\n \t\t/* See if it's an internal command */\n-\t\thandle_internal_command(argc, argv, envp);\n+\t\thandle_internal_command(argc, argv);\n \n \t\t/* .. then try the external ones */\n \t\texecv_git_cmd(argv);\n"},{"id":"45678","messageId":"alpine.LFD.0.98.0706241010480.3593@woody.linux-foundation.org","threadId":"8689","inReplyTo":"alpine.LFD.0.98.0706240951440.3593@woody.linux-foundation.org","subject":"[PATCH 2/2] Check for IO errors after running a command","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-24T17:29:33Z","receivedAt":"2007-06-24T17:29:33Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nThis is trying to implement the strict IO error checks that Jim Meyering \nsuggested, but explicitly limits it to just regular files. If a pipe gets \nclosed on us, we shouldn't complain about it.\n\n[ Side note: feel free to change the S_ISREG() to a !S_ISFIFO(). That \n would allow easier testing with /dev/full, which tends to be a character \n special device. That said, I think sockets and pipes are generally \n interchangeable, which is why I limited it to just regular files. ]\n\nIf the subcommand already returned an error, that takes precedence (and we \nassume that the subcommand already printed out any relevant messages \nrelating to it)\n\nSigned-off-by: Linus Torvalds \n---\n\nHmm? I'm not saying this is the only way to do this, but I think this is \nat least likely to be obviously just an improvement, and it leaves room \nfor further tweaking of the logic if Jim or others find other cases that \nshould be handled.\n\nSide note: I think I made a mistake in making the run_command() a NORETURN \nfunction and putting the exit() into it. It's probably better to instead \njust make it return \"int\", and make the caller do\n\n\texit(run_command(...));\n\nand that makes it much prettier to have \"run_command()\" just return early \nif an error happens (or doesn't happen).\n\nFor example, then we could just do\n\n\tstatus = p->fn(...);\n\tif (status)\n\t\treturn status;\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\nwhich makes it easy to explain what's going on, and avoids having any deep \nindentation at all.\n\nI dunno. This passes all the tests, but it's not like we currently test \nfor ENOSPC/EIO anyway, or even can do that. If we _just_ disable the thing \nfor pipes/sockets, we could add a test using /dev/full.\n\nI did check that changing it to !S_ISFIFO() gets the right behaviour, and \n\"git log | head\" doesn't complain, while \"git log > /dev/full\" does. So \nthis has gotten some very rudimentary testing, but not in the exact form \nI'm actually sending it out.\n\n git.c | 15 ++++++++++++++-\n 1 files changed, 14 insertions(+), 1 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 6c728e4..db0118a 100644\n--- a/git.c\n+++ b/git.c\n@@ -224,6 +224,8 @@ struct cmd_struct {\n \n static NORETURN void run_command(struct cmd_struct *p, int argc, const char **argv)\n {\n+\tint status;\n+\tstruct stat st;\n \tconst char *prefix;\n \n \tprefix = NULL;\n@@ -237,7 +239,18 @@ static NORETURN void run_command(struct cmd_struct *p, int argc, const char **ar\n \t}\n \ttrace_argv_printf(argv, argc, \"trace: built-in: git\");\n \n-\texit(p->fn(argc, argv, prefix));\n+\tstatus = p->fn(argc, argv, prefix);\n+\tif (status)\n+\t\texit(status);\n+\n+\t/* Check for ENOSPC and EIO errors.. */\n+\tif (!fstat(fileno(stdout), &st) && S_ISREG(st.st_mode)) {\n+\t\tif (ferror(stdout))\n+\t\t\tdie(\"write failure on standard output\");\n+\t\tif (fflush(stdout) || fclose(stdout))\n+\t\t\tdie(\"write failure on standard output: %s\", strerror(errno));\n+\t}\n+\texit(0);\n }\n \n static void handle_internal_command(int argc, const char **argv)\n"},{"id":"45684","messageId":"87abupyxls.fsf@rho.meyering.net","threadId":"8689","inReplyTo":"alpine.LFD.0.98.0706240951440.3593@woody.linux-foundation.org","subject":"Re: (resend) [PATCH] Don't ignore write failure from git-diff, git-log, etc.","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-24T19:13:35Z","receivedAt":"2007-06-24T19:13:35Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Linus Torvalds wrote:\n...\n> I also don't think it's very _pretty_ code, and it violates my personal\n> coding standards by adding way too deep indentation for the new error\n> cases. It was already three indents deep (reasonably fine, but that\n> NOT_BARE test wass already pretty ugly), but now it becomes five\n> indentation levels deep at its deepest, which is just a sign that things\n> should be split up.\n\nI too disliked the form of my patch, and said so.\n\n> I'd also like to know why it does that fcntl() is done,\n\nIn the quoted message, I explained that stdout was already\nclosed in some cases. The fcntl test avoids the EINVAL you'd\nget for closing an already-closed file descriptor.\n"},{"id":"45758","messageId":"7vy7i8xtap.fsf@assigned-by-dhcp.pobox.com","threadId":"8689","inReplyTo":"alpine.LFD.0.98.0706241010480.3593@woody.linux-foundation.org","subject":"Re: [PATCH 2/2] Check for IO errors after running a command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-06-25T09:44:14Z","receivedAt":"2007-06-25T09:44:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Side note: I think I made a mistake in making the run_command() a NORETURN \n> function and putting the exit() into it. It's probably better to instead \n> just make it return \"int\", and make the caller do\n>\n> \texit(run_command(...));\n>\n> and that makes it much prettier to have \"run_command()\" just return early \n> if an error happens (or doesn't happen).\n>\n> For example, then we could just do\n>\n> \tstatus = p->fn(...);\n> \tif (status)\n> \t\treturn status;\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> which makes it easy to explain what's going on, and avoids having any deep \n> indentation at all.\n\nI took the liberty of munging your two patches to follow your\ncomments above (it was a perfect guinea-pig opportunity for\nJohannes's \"rebase -i\").\n\nThe changes to git.c (run_command) conflicted with GIT_WORK_TREE\nchanges in a minor way. Matthias, could you sanity check the\nresult once I push it out to 'next', please?\n"},{"id":"45764","messageId":"Pine.LNX.4.64.0706251413530.4059@racer.site","threadId":"8689","inReplyTo":"7vy7i8xtap.fsf@assigned-by-dhcp.pobox.com","subject":"Re: [PATCH 2/2] Check for IO errors after running a command","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-06-25T13:20:59Z","receivedAt":"2007-06-25T13:20:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 25 Jun 2007, Junio C Hamano wrote:\n\n> I took the liberty of munging your two patches to follow your comments \n> above (it was a perfect guinea-pig opportunity for Johannes's \"rebase \n> -i\").\n\nAnd? How was that experience?\n\nI am actually quite pleased how it worked out in the end; I never \nunderstood the syntax of rebase, and stayed away from it for that reason. \nBy reworking patch-series into rebase -i, I learnt it on the way, and find \nit actually quite useful.\n\nOne thing we might consider, however: when rebasing, the current branch \ngets updated at each step. Some might consider this a bug, and prefer \nrebase to work on a detached HEAD, and only update the branch at the end, \nso that @{1} refers to the state _before_ rebase.\n\nThoughts?\n\nCiao,\nDscho\n"},{"id":"45765","messageId":"467FC62C.87F83541@eudaptics.com","threadId":"8689","inReplyTo":"Pine.LNX.4.64.0706251413530.4059@racer.site","subject":"Re: [PATCH 2/2] Check for IO errors after running a command","fromName":"Johannes Sixt","fromEmail":"j.sixt@eudaptics.com","sentAt":"2007-06-25T13:42:04Z","receivedAt":"2007-06-25T13:42:04Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Schindelin wrote:\n> One thing we might consider, however: when rebasing, the current branch\n> gets updated at each step. Some might consider this a bug, and prefer\n> rebase to work on a detached HEAD, and only update the branch at the end,\n> so that @{1} refers to the state _before_ rebase.\n\nYESSSS!\n\nFinding the commit before the rebase in the reflog is a nightmare.\n\n-- Hannes\n"},{"id":"45767","messageId":"877ipsw2sz.fsf@rho.meyering.net","threadId":"8689","inReplyTo":"7vy7i8xtap.fsf@assigned-by-dhcp.pobox.com","subject":"Re: [PATCH 2/2] Check for IO errors after running a command","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-25T14:01:48Z","receivedAt":"2007-06-25T14:01:48Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n> Linus Torvalds writes:\n...\n>> For example, then we could just do\n>>\n>> \tstatus = p->fn(...);\n>> \tif (status)\n>> \t\treturn status;\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>> which makes it easy to explain what's going on, and avoids having any deep\n>> indentation at all.\n>\n> I took the liberty of munging your two patches to follow your\n> comments above\n\nThat has the disadvantage of ignoring *all* pipe and socket write errors.\nIMHO, git would be better served if it didn't do that, since writing to\nthose can fail with EIO and even a new one: EACCES (though this latter\nis only for sockets). Also possible, according to POSIX: ENOBUFS.\n\nOf course, one can probably argue that those are all unlikely.\nThey may be even less likely than an actual EPIPE, but the point is\nthat people and tools using git plumbing should be able to rely on\nit to report such write failures, no matter how unusual they are.\n"},{"id":"45775","messageId":"alpine.LFD.0.98.0706250837550.3593@woody.linux-foundation.org","threadId":"8689","inReplyTo":"877ipsw2sz.fsf@rho.meyering.net","subject":"Re: [PATCH 2/2] Check for IO errors after running a command","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-25T15:57:43Z","receivedAt":"2007-06-25T15:57:43Z","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> That has the disadvantage of ignoring *all* pipe and socket write errors.\n\n.. which is the only wayt to do it.\n\nThere's *no*way* to tell what the error was for the case of\n\n\tif (ferror(stdout))\n\t\t..\n\nbecause the original errno has long long since been thrown away.\n\n> Of course, one can probably argue that those are all unlikely.\n> They may be even less likely than an actual EPIPE, but the point is\n> that people and tools using git plumbing should be able to rely on\n> it to report such write failures, no matter how unusual they are.\n\n..but since what you suggest is physically impossible in a half-way \nportable manner, and since all relevant such errors are likely to have \nhappened long before, I would suggest:\n\n - Use my patch\n\nOR\n\n - Stop using stdio. \n\n You *cannot* make stdio error handling sane. It's simply not possible. \n The whole point of stdio is to simplify things, but it does so to the \n point where portable and reliable error handling is not an option any \n more.\n\n Replace all command output that you care about with some stdio \n replacement. We do that for all the paths we *really* care about, where \n we use raw unistd IO and do our own buffering (ie things like the \n \"write_in_full()\" stuff and \"write_sha1_file()\" etc.\n\nAnybody who thinks he can handle errors with stdio is simply barking up \nthe wrogn tree. It can be done, but it can be done only by essentially \nmaking stdio be a really awkward way to do IO, and you're better off with \nthe raw unistd.h interfaces.\n\nIn particular, you need to make sure you check *each* return value of \nevery single stdio operation. Even then, you don't actually know if \n\"errno\" is set correctly if an error happens in the middle (ie most stdio \nroutines are just defined to return EOF or nonzero on error, and it's not \nat all clear that errno is reliable).\n\nIf you don't, a buffer flush error may have happened at any time, and \nwhatever error code it had has been thrown away, and turned into one \nsingle bit (the \"ferror()\" thing).\n\nAnd yes, some libc's might have extensions that actually guarantee more. \nBut even then I would be *very* surprised if they actually work and have \nbeen tested to any real degree (glibc does not. The stream error is a \nsingle bit. I checked)\n\nSo really: if you care about a particular read or write, use the \n\"write_in_full()\" and \"read_in_full()\" functions. NOTHING ELSE!\n\n\t\t\tLinus\n"},{"id":"45789","messageId":"871wfzvmgh.fsf@rho.meyering.net","threadId":"8689","inReplyTo":"alpine.LFD.0.98.0706241010480.3593@woody.linux-foundation.org","subject":"Re: [PATCH 2/2] Check for IO errors after running a command","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-06-25T19:54:54Z","receivedAt":"2007-06-25T19:54:54Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Linus Torvalds wrote:\n> This is trying to implement the strict IO error checks that Jim Meyering\n> suggested, but explicitly limits it to just regular files. If a pipe gets\n> closed on us, we shouldn't complain about it.\n...\n> Hmm? I'm not saying this is the only way to do this, but I think this is\n> at least likely to be obviously just an improvement, and it leaves room\n> for further tweaking of the logic if Jim or others find other cases that\n> should be handled.\n...\n> +\t/* Check for ENOSPC and EIO errors.. */\n> +\tif (!fstat(fileno(stdout), &st) && S_ISREG(st.st_mode)) {\n> +\t\tif (ferror(stdout))\n> +\t\t\tdie(\"write failure on standard output\");\n> +\t\tif (fflush(stdout) || fclose(stdout))\n> +\t\t\tdie(\"write failure on standard output: %s\", strerror(errno));\n> +\t}\n> +\texit(0);\n\nHere is a patch relative to \"next\", to restore some of the\nfunctionality that was provided by my initially-proposed change.\n\n>From 379aee16596d29b83c95068964c349399b9b9f47 Mon Sep 17 00:00:00 2001\nFrom: Jim Meyering \nDate: Mon, 25 Jun 2007 18:54:12 +0200\nSubject: [PATCH] When detecting write failure, print strerror when possible.\n\nDo not call \"die\" solely on the basis of ferror.\nInstead, call both ferror and fclose, and *then* decide whether/how to die.\n\nWithout this patch, some commands unnecessarily omit strerror(errno)\nwhen they fail:\n\n $ ./git-ls-tree 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\nFWIW, this version of close_stream is similar to the one\nI included in another recent patch, but, is slightly cleaner.\nAlso, rather than returning zero or EOF, this one simply returns\nzero or nonzero.\n\n* git.c (close_stream): New function.\n(run_command): Don't die solely because of ferror. Use close_stream.\n\nSigned-off-by: Jim Meyering \n---\n git.c | 26 ++++++++++++++++++++++----\n 1 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex b949cbb..b00bb1c 100644\n--- a/git.c\n+++ b/git.c\n@@ -246,6 +246,21 @@ struct cmd_struct {\n \tint option;\n };\n\n+static int\n+close_stream(FILE *stream)\n+{\n+\tint prev_fail = ferror(stream);\n+\tint fclose_fail = fclose(stream);\n+\n+\t/* If there was a previous failure, but fclose succeeded,\n+\t clear errno, since ferror does not set it, and its value\n+\t may be unrelated to the ferror-reported failure. */\n+\tif (prev_fail && !fclose_fail)\n+\t\terrno = 0;\n+\n+\treturn prev_fail || fclose_fail;\n+}\n+\n static int run_command(struct cmd_struct *p, int argc, const char **argv)\n {\n \tint status;\n@@ -274,10 +289,13 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)\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+\tif (close_stream(stdout)) {\n+\t\tif (errno == 0)\n+\t\t\tdie(\"write failure on standard output\");\n+\t\telse\n+\t\t\tdie(\"write failure on standard output: %s\",\n+\t\t\t strerror(errno));\n+\t}\n\n \treturn 0;\n }\n"},{"id":"45832","messageId":"20070626133308.GA11504@moooo.ath.cx","threadId":"8689","inReplyTo":"7vy7i8xtap.fsf@assigned-by-dhcp.pobox.com","subject":"Re: [PATCH 2/2] Check for IO errors after running a command","fromName":"Matthias Lederhofer","fromEmail":"matled@gmx.net","sentAt":"2007-06-26T13:33:08Z","receivedAt":"2007-06-26T13:33:08Z","isPatch":true,"sender":{"key":"matled@gmx.net","avatar":null},"body":"Junio C Hamano wrote:\n> The changes to git.c (run_command) conflicted with GIT_WORK_TREE\n> changes in a minor way. Matthias, could you sanity check the\n> result once I push it out to 'next', please?\n\nThe changes look fine, the tests pass and my own short manual test\npassed.\n"}]}