{"thread":{"id":"27930","subject":"[PATCH/RFC] notice error exit from pager","startedAt":"2011-07-26T21:04:02Z","lastAt":"2011-08-01T20:35:44Z","messageCount":7,"participants":["Clemens Buchacher","Linus Torvalds","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"172094","messageId":"20110726210401.GA25207@toss.lan","threadId":"27930","inReplyTo":null,"subject":"[PATCH/RFC] notice error exit from pager","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-07-26T21:04:02Z","receivedAt":"2011-07-26T21:04:02Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"If the pager fails to run, git produces no output, e.g.:\n\n $ GIT_PAGER=not-a-command git log\n\nThe error reporting fails for two reasons:\n\n (1) start_command: There is a mechanism that detects errors during\n     execvp introduced in 2b541bf8 (start_command: detect execvp\n     failures early). The child writes one byte to a pipe only if\n     execvp fails.  The parent waits for either EOF, when the\n     successful execvp automatically closes the pipe (see\n     FD_CLOEXEC in fcntl(1)), or it reads a single byte, in which\n     case it knows that the execvp failed. This mechanism is\n     incompatible with the workaround introduced in 35ce8622\n     (pager: Work around window resizing bug in 'less'), which\n     waits for input from the parent before the exec. Since both\n     the parent and the child are waiting for input from each\n     other, that would result in a deadlock. In order to avoid\n     that, the mechanism is disabled by closing the child_notifier\n     file descriptor. That is my best guess, at least, from reading\n     the code. Hannes, can you confirm if this analysis is correct?\n\n (2) finish_command: The parent correctly detects the 127 exit\n     status from the child, but the error output goes nowhere,\n     since by that time it is already being redirected to the\n     child.\n\nNo simple solution for (1) comes to mind. I am not sure if this bug\nis still relevant. I could not reproduce it here with less version\n436. If this bug was fixed a long time ago, maybe we can remove the\nworkaround by now?\n\nNumber (2) can be solved by not sending error output to the pager.\nBut I am not completely satisfied with that solution.  Not\nredirecting error output to the pager can result in the pager\noverwriting error output with standard output. Right now, I can't\nthink of any common situations where this would be the case. But,\nfor example, a git log on a broken repo with some missing commits\nwill abort at some point with \"error: could not read <sha1> [...]\".\nWith this patch, if there is at least one page of standard output\nbefore that error, the error message is not visible at all and the\noutput simply stops as if git log had reached the root commit.\n\nThe easiest solution is to produce the error output in the child.\nBelow is a patch for that. The previous code seems to go through\nsome pain to let the parent handle the error output. But I don't\nsee the point. Is there more to this than meets my eye?\n\nClemens\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n run-command.c |   11 ++++++-----\n 1 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 70e8a24..944a882 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -127,9 +127,6 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n \t\tif (code == 127) {\n \t\t\tcode = -1;\n \t\t\tfailed_errno = ENOENT;\n-\t\t\tif (!silent_exec_failure)\n-\t\t\t\terror(\"cannot run %s: %s\", argv0,\n-\t\t\t\t\tstrerror(ENOENT));\n \t\t}\n \t} else {\n \t\terror(\"waitpid is confused (%s)\", argv0);\n@@ -287,10 +284,14 @@ fail_pipe:\n \t\t * Do not check for cmd->silent_exec_failure; the parent\n \t\t * process will check it when it sees this exit code.\n \t\t */\n-\t\tif (errno == ENOENT)\n+\t\tif (errno == ENOENT) {\n+\t\t\tif (!cmd->silent_exec_failure)\n+\t\t\t\terror(\"cannot run %s: %s\", cmd->argv[0],\n+\t\t\t\t\tstrerror(ENOENT));\n \t\t\texit(127);\n-\t\telse\n+\t\t} else {\n \t\t\tdie_errno(\"cannot exec '%s'\", cmd->argv[0]);\n+\t\t}\n \t}\n \tif (cmd->pid < 0)\n \t\terror(\"cannot fork() for %s: %s\", cmd->argv[0],\n-- \n1.7.3.1.105.g84915\n"},{"id":"172096","messageId":"CA+55aFxdAWvPDV3iYtuoQhUe6=yKUaWSX8kw2UTujq5Yckj9ZQ@mail.gmail.com","threadId":"27930","inReplyTo":"20110726210401.GA25207@toss.lan","subject":"Re: [PATCH/RFC] notice error exit from pager","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2011-07-26T21:35:54Z","receivedAt":"2011-07-26T21:35:54Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Tue, Jul 26, 2011 at 2:04 PM, Clemens Buchacher <drizzd@aon.at> wrote:\n>\n> No simple solution for (1) comes to mind. I am not sure if this bug\n> is still relevant. I could not reproduce it here with less version\n> 436. If this bug was fixed a long time ago, maybe we can remove the\n> workaround by now?\n\nHmm. I can confirm that it doesn't happen for me either any more,\nalthough I don't know if that is an actual less bugfix, or it was\ntiming-dependent.\n\nThat said, the workaround has other nice properties too, like delaying\nthe screen cleaning that \"less\" often does. So I don't know if we\nreally want to get rid of it.\n\n> Number (2) can be solved by not sending error output to the pager.\n\nI think your patch looks reasonable.\n\n                  Linus\n"},{"id":"172188","messageId":"4E3068D4.4050006@kdbg.org","threadId":"27930","inReplyTo":"20110726210401.GA25207@toss.lan","subject":"Re: [PATCH/RFC] notice error exit from pager","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-07-27T19:36:52Z","receivedAt":"2011-07-27T19:36:52Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 26.07.2011 23:04, schrieb Clemens Buchacher:\n> If the pager fails to run, git produces no output, e.g.:\n> \n>  $ GIT_PAGER=not-a-command git log\n> \n> The error reporting fails for two reasons:\n> \n>  (1) start_command: There is a mechanism that detects errors during\n>      execvp introduced in 2b541bf8 (start_command: detect execvp\n>      failures early). ... This mechanism is\n>      incompatible with the workaround introduced in 35ce8622\n>      (pager: Work around window resizing bug in 'less')\n\nYou analysis is correct. I think the bug in less was fixed shortly after\nthis workaround was introduced. I would like to remove this workaround\nfor a different reason, but it seems there are people who are quite fond\nof it ;)\n\n> diff --git a/run-command.c b/run-command.c\n> index 70e8a24..944a882 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -127,9 +127,6 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n>  \t\tif (code == 127) {\n>  \t\t\tcode = -1;\n>  \t\t\tfailed_errno = ENOENT;\n> -\t\t\tif (!silent_exec_failure)\n> -\t\t\t\terror(\"cannot run %s: %s\", argv0,\n> -\t\t\t\t\tstrerror(ENOENT));\n>  \t\t}\n>  \t} else {\n>  \t\terror(\"waitpid is confused (%s)\", argv0);\n> @@ -287,10 +284,14 @@ fail_pipe:\n>  \t\t * Do not check for cmd->silent_exec_failure; the parent\n>  \t\t * process will check it when it sees this exit code.\n>  \t\t */\n> -\t\tif (errno == ENOENT)\n> +\t\tif (errno == ENOENT) {\n> +\t\t\tif (!cmd->silent_exec_failure)\n> +\t\t\t\terror(\"cannot run %s: %s\", cmd->argv[0],\n> +\t\t\t\t\tstrerror(ENOENT));\n\nThis change is not good enough: There is no guarantee that this message\ngoes to the right channel (stderr in the child can be redirected). Look\ncarefully: We set a special die routine in the forked child that ensures\nthat the die() messages are written to the right channel; we do not have\na similar facility for error() messages (or do we?).\n\n>  \t\t\texit(127);\n> -\t\telse\n> +\t\t} else {\n>  \t\t\tdie_errno(\"cannot exec '%s'\", cmd->argv[0]);\n> +\t\t}\n>  \t}\n>  \tif (cmd->pid < 0)\n>  \t\terror(\"cannot fork() for %s: %s\", cmd->argv[0],\n\n-- Hannes\n"},{"id":"172192","messageId":"20110727213234.GA22159@toss.lan","threadId":"27930","inReplyTo":"4E3068D4.4050006@kdbg.org","subject":"[PATCH] error_routine: use parent's stderr if exec fails","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-07-27T21:32:34Z","receivedAt":"2011-07-27T21:32:34Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"The new process's error output may be redirected elsewhere, but if\nthe exec fails, output should still go to the parent's stderr. This\nhas already been done for the die_routine. Do the same for\nerror_routine.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nOn Wed, Jul 27, 2011 at 09:36:52PM +0200, Johannes Sixt wrote:\n> \n> This change is not good enough: There is no guarantee that this message\n> goes to the right channel (stderr in the child can be redirected).\n\nAh, good point. I skipped over that part thinking it was not\nrelevant.  This should take care of it.\n\nClemens\n\n git-compat-util.h |    2 ++\n run-command.c     |   15 +++++++--------\n usage.c           |   18 ++++++++++++++++++\n 3 files changed, 27 insertions(+), 8 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex a75530d..07cbfe9 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -240,6 +240,7 @@ extern char *gitbasename(char *);\n \n /* General helper functions */\n extern void vreportf(const char *prefix, const char *err, va_list params);\n+extern void vwritef(int fd, const char *prefix, const char *err, va_list params);\n extern NORETURN void usage(const char *err);\n extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n@@ -248,6 +249,7 @@ extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n \n extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));\n+extern void set_error_routine(void (*routine)(const char *err, va_list params));\n \n extern int prefixcmp(const char *str, const char *prefix);\n extern int suffixcmp(const char *str, const char *suffix);\ndiff --git a/run-command.c b/run-command.c\nindex c752174..a2796c4 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -77,16 +77,14 @@ static void notify_parent(void)\n \n static NORETURN void die_child(const char *err, va_list params)\n {\n-\tchar msg[4096];\n-\tint len = vsnprintf(msg, sizeof(msg), err, params);\n-\tif (len > sizeof(msg))\n-\t\tlen = sizeof(msg);\n-\n-\twrite_in_full(child_err, \"fatal: \", 7);\n-\twrite_in_full(child_err, msg, len);\n-\twrite_in_full(child_err, \"\\n\", 1);\n+\tvwritef(child_err, \"fatal: \", err, params);\n \texit(128);\n }\n+\n+static void error_child(const char *err, va_list params)\n+{\n+\tvwritef(child_err, \"error: \", err, params);\n+}\n #endif\n \n static inline void set_cloexec(int fd)\n@@ -214,6 +212,7 @@ fail_pipe:\n \t\t\tset_cloexec(child_err);\n \t\t}\n \t\tset_die_routine(die_child);\n+\t\tset_error_routine(error_child);\n \n \t\tclose(notify_pipe[0]);\n \t\tset_cloexec(notify_pipe[1]);\ndiff --git a/usage.c b/usage.c\nindex b5e67e3..a2a6678 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -4,6 +4,7 @@\n  * Copyright (C) Linus Torvalds, 2005\n  */\n #include \"git-compat-util.h\"\n+#include \"cache.h\"\n \n void vreportf(const char *prefix, const char *err, va_list params)\n {\n@@ -12,6 +13,18 @@ void vreportf(const char *prefix, const char *err, va_list params)\n \tfprintf(stderr, \"%s%s\\n\", prefix, msg);\n }\n \n+void vwritef(int fd, const char *prefix, const char *err, va_list params)\n+{\n+\tchar msg[4096];\n+\tint len = vsnprintf(msg, sizeof(msg), err, params);\n+\tif (len > sizeof(msg))\n+\t\tlen = sizeof(msg);\n+\n+\twrite_in_full(fd, prefix, strlen(prefix));\n+\twrite_in_full(fd, msg, len);\n+\twrite_in_full(fd, \"\\n\", 1);\n+}\n+\n static NORETURN void usage_builtin(const char *err, va_list params)\n {\n \tvreportf(\"usage: \", err, params);\n@@ -46,6 +59,11 @@ void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list param\n \tdie_routine = routine;\n }\n \n+void set_error_routine(void (*routine)(const char *err, va_list params))\n+{\n+\terror_routine = routine;\n+}\n+\n void NORETURN usagef(const char *err, ...)\n {\n \tva_list params;\n-- \n1.7.3.1.105.g84915\n"},{"id":"172555","messageId":"20110801175921.GA17092@toss","threadId":"27930","inReplyTo":"20110726210401.GA25207@toss.lan","subject":"[PATCH] notice error exit from pager","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-08-01T17:59:21Z","receivedAt":"2011-08-01T17:59:21Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"If the pager fails to run, git produces no output, e.g.:\n\n $ GIT_PAGER=not-a-command git log\n\nThe error reporting fails for two reasons:\n\n (1) start_command: There is a mechanism that detects errors during\n     execvp introduced in 2b541bf8 (start_command: detect execvp\n     failures early). The child writes one byte to a pipe only if\n     execvp fails.  The parent waits for either EOF, when the\n     successful execvp automatically closes the pipe (see\n     FD_CLOEXEC in fcntl(1)), or it reads a single byte, in which\n     case it knows that the execvp failed. This mechanism is\n     incompatible with the workaround introduced in 35ce8622\n     (pager: Work around window resizing bug in 'less'), which\n     waits for input from the parent before the exec. Since both\n     the parent and the child are waiting for input from each\n     other, that would result in a deadlock. In order to avoid\n     that, the mechanism is disabled by closing the child_notifier\n     file descriptor.\n\n (2) finish_command: The parent correctly detects the 127 exit\n     status from the child, but the error output goes nowhere,\n     since by that time it is already being redirected to the\n     child.\n\nNo simple solution for (1) comes to mind.\n\nNumber (2) can be solved by not sending error output to the pager.\nNot redirecting error output to the pager can result in the pager\noverwriting error output with standard output, however.\n\nSince there is no reliable way to handle error reporting in the\nparent, produce the output in the child instead.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nNothing new compared to the RFC, just a slightly trimmed commit\nmessage.\n\n run-command.c |   15 ++++++---------\n 1 files changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 5c91f37..a2796c4 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -125,9 +125,6 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n \t\tif (code == 127) {\n \t\t\tcode = -1;\n \t\t\tfailed_errno = ENOENT;\n-\t\t\tif (!silent_exec_failure)\n-\t\t\t\terror(\"cannot run %s: %s\", argv0,\n-\t\t\t\t\tstrerror(ENOENT));\n \t\t}\n \t} else {\n \t\terror(\"waitpid is confused (%s)\", argv0);\n@@ -282,14 +279,14 @@ fail_pipe:\n \t\t} else {\n \t\t\texecvp(cmd->argv[0], (char *const*) cmd->argv);\n \t\t}\n-\t\t/*\n-\t\t * Do not check for cmd->silent_exec_failure; the parent\n-\t\t * process will check it when it sees this exit code.\n-\t\t */\n-\t\tif (errno == ENOENT)\n+\t\tif (errno == ENOENT) {\n+\t\t\tif (!cmd->silent_exec_failure)\n+\t\t\t\terror(\"cannot run %s: %s\", cmd->argv[0],\n+\t\t\t\t\tstrerror(ENOENT));\n \t\t\texit(127);\n-\t\telse\n+\t\t} else {\n \t\t\tdie_errno(\"cannot exec '%s'\", cmd->argv[0]);\n+\t\t}\n \t}\n \tif (cmd->pid < 0)\n \t\terror(\"cannot fork() for %s: %s\", cmd->argv[0],\n-- \n1.7.3.1.105.g84915\n"},{"id":"172595","messageId":"7vd3goj38n.fsf@alter.siamese.dyndns.org","threadId":"27930","inReplyTo":"20110801175921.GA17092@toss","subject":"Re: [PATCH] notice error exit from pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-01T20:17:28Z","receivedAt":"2011-08-01T20:17:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> Since there is no reliable way to handle error reporting in the\n> parent, produce the output in the child instead.\n\nHmm, how does this interact with your earlier \"error_routine: use parent's\nstderr if exec fails\" patch?\n\n> diff --git a/run-command.c b/run-command.c\n> index 5c91f37..a2796c4 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -125,9 +125,6 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n>  \t\tif (code == 127) {\n>  \t\t\tcode = -1;\n>  \t\t\tfailed_errno = ENOENT;\n> -\t\t\tif (!silent_exec_failure)\n> -\t\t\t\terror(\"cannot run %s: %s\", argv0,\n> -\t\t\t\t\tstrerror(ENOENT));\n>  \t\t}\n>  \t} else {\n>  \t\terror(\"waitpid is confused (%s)\", argv0);\n> @@ -282,14 +279,14 @@ fail_pipe:\n>  \t\t} else {\n>  \t\t\texecvp(cmd->argv[0], (char *const*) cmd->argv);\n>  \t\t}\n> -\t\t/*\n> -\t\t * Do not check for cmd->silent_exec_failure; the parent\n> -\t\t * process will check it when it sees this exit code.\n> -\t\t */\n> -\t\tif (errno == ENOENT)\n> +\t\tif (errno == ENOENT) {\n> +\t\t\tif (!cmd->silent_exec_failure)\n> +\t\t\t\terror(\"cannot run %s: %s\", cmd->argv[0],\n> +\t\t\t\t\tstrerror(ENOENT));\n>  \t\t\texit(127);\n> -\t\telse\n> +\t\t} else {\n>  \t\t\tdie_errno(\"cannot exec '%s'\", cmd->argv[0]);\n> +\t\t}\n>  \t}\n>  \tif (cmd->pid < 0)\n>  \t\terror(\"cannot fork() for %s: %s\", cmd->argv[0],\n"},{"id":"172596","messageId":"20110801203544.GA22431@toss","threadId":"27930","inReplyTo":"7vd3goj38n.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] notice error exit from pager","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-08-01T20:35:44Z","receivedAt":"2011-08-01T20:35:44Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Mon, Aug 01, 2011 at 01:17:28PM -0700, Junio C Hamano wrote:\n> Clemens Buchacher <drizzd@aon.at> writes:\n> \n> > Since there is no reliable way to handle error reporting in the\n> > parent, produce the output in the child instead.\n> \n> Hmm, how does this interact with your earlier \"error_routine: use parent's\n> stderr if exec fails\" patch?\n\nThe use case addressed in this patch (i.e., the pager) does not\nstrictly dependent on the previous patch. But Johannes noted that\nin general, error output of the child could be redirected, hiding\nthe execve failure again. The error_routine patch avoids that by\nwriting to the parent's stderr stream.\n\nClemens\n"}]}