{"thread":{"id":"40057","subject":"[PATCH] usage: try harder to format very long error messages","startedAt":"2015-08-11T16:17:53Z","lastAt":"2015-08-11T18:13:59Z","messageCount":7,"participants":["Jeff King","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"267834","messageId":"20150811161752.GA14829@sigill.intra.peff.net","threadId":"40057","inReplyTo":null,"subject":"[PATCH] usage: try harder to format very long error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-11T16:17:53Z","receivedAt":"2015-08-11T16:17:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use a fixed-size buffer of 4096 bytes to format die() and\nerror() messages. We explicitly avoided using a strbuf or\nother fanciness here, because we want to make sure that we\nreport the message even in the face of malloc() failure\n(after all, we might even be dying due to a malloc call).\n\nIn most cases, this buffer is long enough to show any\nreasonable message. However, if you are experimenting with\n_unreasonable_ things (e.g., filenames approaching 4096\nbytes), the messages can be truncated, making them\nconfusing. E.g., seeing:\n\n  error: cannot stat 'aaaaaaaaaaaaaaaaaaaaaa\n\nis much less useful than:\n\n  error: cannot stat 'aaaaaaaaaaaaaaaaaaaaaaa/foo': File too long\n\n(these are truncated for the sake of readability; obviously\nreal examples are much longer. Just imagine many lines full\nof \"a\"'s).\n\nThis patch teaches vreportf and vwritef to at least try\nusing a dynamically sized buffer before falling back to the\nfixed-size buffer. For most long errors (which are typically\nreporting problems with syscalls on long filenames), this\nmeans we'll generally see the full message. And in case that\nfails, we still print the truncated message, but with a note\nthat it was truncated.\n\nNote that die_errno does not need the same treatment for its\nfixed-size buffers, as they are a combination of:\n\n  - our fixed-size string constants, without placeholders\n    expanded (so a literal \"cannot stat '%s'\", without %s\n    expanded to arbitrary data)\n\n  - the results of strerror(errno)\n\nboth of which should be predictably small.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nSo this solution tries to change vreportf and vwritef as little as\npossible, and ends up slightly complex as a result. But reading\nvreportf's:\n\n  vsnprintf(msg, sizeof(msg), err, params);\n  fprintf(stderr, \"%s%s\\n\", prefix, msg);\n\nI had to wonder why this wasn't just:\n\n  fputs(prefix, stderr);\n  vfprintf(stderr, err, params);\n\nIn fact, we used to do this, but changed it in d048a96 (print\nwarning/error/fatal messages in one shot, 2007-11-09). I'm not sure of\nthe reasoning there, though. I would expect stdio to generally write\nthat as a single shot already, assuming:\n\n  - there isn't anything in the stderr buffer already (i.e., we do not\n    need to flush somewhere in the middle)\n\n  - our prefix does not have a newline (which it doesn't; it is\n    \"error:\", \"fatal:\", etc).\n\nIOW, I wonder if it would be enough to simply fflush(stderr) before and\nafter to make sure we keep the buffer clear. I also wonder if this is\neven enough as-is; if the resulting message is larger than stdio's\nbuffer size, I'd expect it to come through across several writes anyway.\n\nAs for vwritef, it exists solely to work over write(), _and_ it doesn't\nget the \"one-shot\" thing right (which is probably OK, as we use it only\nwhen exec fails). But we could probably teach run-command to fdopen(),\nand get rid of it entirely (in favor of teaching vreportf to take a\nFILE* handle instead of assuming stderr).\n\nSo I dunno. I think the solution here works fine, but maybe we should be\ntaking the opportunity to simplify.\n\n usage.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------\n 1 file changed, 64 insertions(+), 8 deletions(-)\n\ndiff --git a/usage.c b/usage.c\nindex ed14645..78b0d75 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -6,23 +6,79 @@\n #include \"git-compat-util.h\"\n #include \"cache.h\"\n \n+/*\n+ * This buffer allows you to try formatting a full message, but if malloc\n+ * fails, will fall back to a fixed-size buffer and truncate the message.\n+ * If we truncate the message, it adds a note indicating so.\n+ *\n+ * No initialization is necessary before robust_buf_fmt, and after it returns,\n+ * \"buf\" points to the contents (whether truncated or not). You should always\n+ * robust_buf_free the result, which handles both cases.\n+ */\n+struct robust_buf {\n+\tchar *buf;\n+\tchar fixed[4096];\n+};\n+\n+static int robust_buf_fmt(struct robust_buf *rb,\n+\t\t\t  const char *fmt, va_list ap)\n+{\n+\tstatic const char trunc[] = \" [message truncated]\";\n+\tstatic const char broken[] = \"BUG: your vsnprintf is broken\";\n+\tva_list cp;\n+\tint len;\n+\n+\tva_copy(cp, ap);\n+\tlen = vsnprintf(rb->fixed, sizeof(rb->fixed), fmt, cp);\n+\tva_end(cp);\n+\n+\tif (len < 0) {\n+\t\tmemcpy(rb->fixed, broken, sizeof(broken));\n+\t\trb->buf = rb->fixed;\n+\t\treturn sizeof(broken) - 1;\n+\t}\n+\tif (len < sizeof(rb->fixed)) {\n+\t\trb->buf = rb->fixed;\n+\t\treturn len;\n+\t}\n+\n+\trb->buf = malloc(len + 1);\n+\tif (!rb->buf) {\n+\t\tmemcpy(rb->fixed + sizeof(rb->fixed) - sizeof(trunc),\n+\t\t       trunc, sizeof(trunc));\n+\t\trb->buf = rb->fixed;\n+\t\treturn sizeof(rb->fixed) - 1;\n+\t}\n+\n+\tif (vsnprintf(rb->buf, len + 1, fmt, ap) >= len)\n+\t\t; /* insatiable vsnprintf, nothing we can do */\n+\treturn len;\n+}\n+\n+static void robust_buf_free(struct robust_buf *rb)\n+{\n+\tif (rb->buf != rb->fixed)\n+\t\tfree(rb->buf);\n+}\n+\n void vreportf(const char *prefix, const char *err, va_list params)\n {\n-\tchar msg[4096];\n-\tvsnprintf(msg, sizeof(msg), err, params);\n-\tfprintf(stderr, \"%s%s\\n\", prefix, msg);\n+\tstruct robust_buf msg;\n+\trobust_buf_fmt(&msg, err, params);\n+\tfprintf(stderr, \"%s%s\\n\", prefix, msg.buf);\n+\trobust_buf_free(&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+\tstruct robust_buf msg;\n+\tint len = robust_buf_fmt(&msg, err, params);\n \n \twrite_in_full(fd, prefix, strlen(prefix));\n-\twrite_in_full(fd, msg, len);\n+\twrite_in_full(fd, msg.buf, len);\n \twrite_in_full(fd, \"\\n\", 1);\n+\n+\trobust_buf_free(&msg);\n }\n \n static NORETURN void usage_builtin(const char *err, va_list params)\n-- \n2.5.0.417.g2db689c\n"},{"id":"267835","messageId":"CAGZ79kbL6DuXoQi72z_5=PZy2+HPh_L4+FJ_diokJi6s3uP57g@mail.gmail.com","threadId":"40057","inReplyTo":"20150811161752.GA14829@sigill.intra.peff.net","subject":"Re: [PATCH] usage: try harder to format very long error messages","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-11T16:28:34Z","receivedAt":"2015-08-11T16:28:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 11, 2015 at 9:17 AM, Jeff King <peff@peff.net> wrote:\n> We use a fixed-size buffer of 4096 bytes to format die() and\n> error() messages. We explicitly avoided using a strbuf or\n> other fanciness here, because we want to make sure that we\n> report the message even in the face of malloc() failure\n> (after all, we might even be dying due to a malloc call).\n\nWould it make sense to allocate memory in the early startup phase\nfor a possible error message?\nSo instead of putting 4kb on the stack we'd just have an unused 16kb\non the heap.\n\n>\n> In most cases, this buffer is long enough to show any\n> reasonable message. However, if you are experimenting with\n> _unreasonable_ things (e.g., filenames approaching 4096\n> bytes), the messages can be truncated, making them\n> confusing. E.g., seeing:\n>\n>   error: cannot stat 'aaaaaaaaaaaaaaaaaaaaaa\n>\n> is much less useful than:\n>\n>   error: cannot stat 'aaaaaaaaaaaaaaaaaaaaaaa/foo': File too long\n>\n> (these are truncated for the sake of readability; obviously\n> real examples are much longer. Just imagine many lines full\n> of \"a\"'s).\n>\n> This patch teaches vreportf and vwritef to at least try\n> using a dynamically sized buffer before falling back to the\n> fixed-size buffer. For most long errors (which are typically\n> reporting problems with syscalls on long filenames), this\n> means we'll generally see the full message. And in case that\n> fails, we still print the truncated message, but with a note\n> that it was truncated.\n>\n> Note that die_errno does not need the same treatment for its\n> fixed-size buffers, as they are a combination of:\n>\n>   - our fixed-size string constants, without placeholders\n>     expanded (so a literal \"cannot stat '%s'\", without %s\n>     expanded to arbitrary data)\n>\n>   - the results of strerror(errno)\n>\n> both of which should be predictably small.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> So this solution tries to change vreportf and vwritef as little as\n> possible, and ends up slightly complex as a result. But reading\n> vreportf's:\n>\n>   vsnprintf(msg, sizeof(msg), err, params);\n>   fprintf(stderr, \"%s%s\\n\", prefix, msg);\n>\n> I had to wonder why this wasn't just:\n>\n>   fputs(prefix, stderr);\n>   vfprintf(stderr, err, params);\n>\n> In fact, we used to do this, but changed it in d048a96 (print\n> warning/error/fatal messages in one shot, 2007-11-09). I'm not sure of\n> the reasoning there, though. I would expect stdio to generally write\n> that as a single shot already, assuming:\n>\n>   - there isn't anything in the stderr buffer already (i.e., we do not\n>     need to flush somewhere in the middle)\n>\n>   - our prefix does not have a newline (which it doesn't; it is\n>     \"error:\", \"fatal:\", etc).\n>\n> IOW, I wonder if it would be enough to simply fflush(stderr) before and\n> after to make sure we keep the buffer clear. I also wonder if this is\n> even enough as-is; if the resulting message is larger than stdio's\n> buffer size, I'd expect it to come through across several writes anyway.\n>\n> As for vwritef, it exists solely to work over write(), _and_ it doesn't\n> get the \"one-shot\" thing right (which is probably OK, as we use it only\n> when exec fails). But we could probably teach run-command to fdopen(),\n> and get rid of it entirely (in favor of teaching vreportf to take a\n> FILE* handle instead of assuming stderr).\n>\n> So I dunno. I think the solution here works fine, but maybe we should be\n> taking the opportunity to simplify.\n>\n>  usage.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------\n>  1 file changed, 64 insertions(+), 8 deletions(-)\n>\n> diff --git a/usage.c b/usage.c\n> index ed14645..78b0d75 100644\n> --- a/usage.c\n> +++ b/usage.c\n> @@ -6,23 +6,79 @@\n>  #include \"git-compat-util.h\"\n>  #include \"cache.h\"\n>\n> +/*\n> + * This buffer allows you to try formatting a full message, but if malloc\n> + * fails, will fall back to a fixed-size buffer and truncate the message.\n> + * If we truncate the message, it adds a note indicating so.\n> + *\n> + * No initialization is necessary before robust_buf_fmt, and after it returns,\n> + * \"buf\" points to the contents (whether truncated or not). You should always\n> + * robust_buf_free the result, which handles both cases.\n> + */\n> +struct robust_buf {\n> +       char *buf;\n> +       char fixed[4096];\n> +};\n> +\n> +static int robust_buf_fmt(struct robust_buf *rb,\n> +                         const char *fmt, va_list ap)\n> +{\n> +       static const char trunc[] = \" [message truncated]\";\n> +       static const char broken[] = \"BUG: your vsnprintf is broken\";\n> +       va_list cp;\n> +       int len;\n> +\n> +       va_copy(cp, ap);\n> +       len = vsnprintf(rb->fixed, sizeof(rb->fixed), fmt, cp);\n> +       va_end(cp);\n> +\n> +       if (len < 0) {\n> +               memcpy(rb->fixed, broken, sizeof(broken));\n> +               rb->buf = rb->fixed;\n> +               return sizeof(broken) - 1;\n> +       }\n> +       if (len < sizeof(rb->fixed)) {\n> +               rb->buf = rb->fixed;\n> +               return len;\n> +       }\n> +\n> +       rb->buf = malloc(len + 1);\n> +       if (!rb->buf) {\n> +               memcpy(rb->fixed + sizeof(rb->fixed) - sizeof(trunc),\n> +                      trunc, sizeof(trunc));\n> +               rb->buf = rb->fixed;\n> +               return sizeof(rb->fixed) - 1;\n> +       }\n> +\n> +       if (vsnprintf(rb->buf, len + 1, fmt, ap) >= len)\n> +               ; /* insatiable vsnprintf, nothing we can do */\n> +       return len;\n> +}\n> +\n> +static void robust_buf_free(struct robust_buf *rb)\n> +{\n> +       if (rb->buf != rb->fixed)\n> +               free(rb->buf);\n> +}\n> +\n>  void vreportf(const char *prefix, const char *err, va_list params)\n>  {\n> -       char msg[4096];\n> -       vsnprintf(msg, sizeof(msg), err, params);\n> -       fprintf(stderr, \"%s%s\\n\", prefix, msg);\n> +       struct robust_buf msg;\n> +       robust_buf_fmt(&msg, err, params);\n> +       fprintf(stderr, \"%s%s\\n\", prefix, msg.buf);\n> +       robust_buf_free(&msg);\n>  }\n>\n>  void vwritef(int fd, const char *prefix, const char *err, va_list params)\n>  {\n> -       char msg[4096];\n> -       int len = vsnprintf(msg, sizeof(msg), err, params);\n> -       if (len > sizeof(msg))\n> -               len = sizeof(msg);\n> +       struct robust_buf msg;\n> +       int len = robust_buf_fmt(&msg, err, params);\n>\n>         write_in_full(fd, prefix, strlen(prefix));\n> -       write_in_full(fd, msg, len);\n> +       write_in_full(fd, msg.buf, len);\n>         write_in_full(fd, \"\\n\", 1);\n> +\n> +       robust_buf_free(&msg);\n>  }\n>\n>  static NORETURN void usage_builtin(const char *err, va_list params)\n> --\n> 2.5.0.417.g2db689c\n>\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"267836","messageId":"20150811163355.GA15521@sigill.intra.peff.net","threadId":"40057","inReplyTo":"CAGZ79kbL6DuXoQi72z_5=PZy2+HPh_L4+FJ_diokJi6s3uP57g@mail.gmail.com","subject":"Re: [PATCH] usage: try harder to format very long error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-11T16:33:55Z","receivedAt":"2015-08-11T16:33:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 11, 2015 at 09:28:34AM -0700, Stefan Beller wrote:\n\n> On Tue, Aug 11, 2015 at 9:17 AM, Jeff King <peff@peff.net> wrote:\n> > We use a fixed-size buffer of 4096 bytes to format die() and\n> > error() messages. We explicitly avoided using a strbuf or\n> > other fanciness here, because we want to make sure that we\n> > report the message even in the face of malloc() failure\n> > (after all, we might even be dying due to a malloc call).\n> \n> Would it make sense to allocate memory in the early startup phase\n> for a possible error message?\n> So instead of putting 4kb on the stack we'd just have an unused 16kb\n> on the heap.\n\nIsn't that just punting on the problem? Now your 16kb filename will get\ntruncated messages (in general we cannot even work with such files, but\nit is nice if the error message telling us so is readable).\n\nIf stack space is the problem, we can just put 16kb in BSS. But I think\nwe really do want something that grows to the appropriate size. Or we\nneed to start being more clever about our truncation. E.g., printing:\n\n  error: unable to stat 'aaaaaaaaa[...]aaaaaa/foo': File too long\n\nwhere the \"[...]\" is literally what we would print. The trouble with\nthat approach is that it is hard to intercept large strings without\nre-implementing all of stdio's formatting.\n\n-Peff\n"},{"id":"267837","messageId":"xmqqtws5ss14.fsf@gitster.dls.corp.google.com","threadId":"40057","inReplyTo":"20150811161752.GA14829@sigill.intra.peff.net","subject":"Re: [PATCH] usage: try harder to format very long error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-11T16:34:31Z","receivedAt":"2015-08-11T16:34:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> We use a fixed-size buffer of 4096 bytes to format die() and\n> error() messages. We explicitly avoided using a strbuf or\n> other fanciness here, because we want to make sure that we\n> report the message even in the face of malloc() failure\n> (after all, we might even be dying due to a malloc call).\n\nYes, that was the rationale behind the \"we use a fixed-buffer and\ntry not to allocate in this codepath\", I think.\n\n> As for vwritef, it exists solely to work over write(), _and_ it doesn't\n> get the \"one-shot\" thing right (which is probably OK, as we use it only\n> when exec fails). But we could probably teach run-command to fdopen(),\n> and get rid of it entirely (in favor of teaching vreportf to take a\n> FILE* handle instead of assuming stderr).\n\nSounds like a plan ;-)\n\nThe patch looks very sensible.\n"},{"id":"267841","messageId":"20150811180524.GB15521@sigill.intra.peff.net","threadId":"40057","inReplyTo":"xmqqtws5ss14.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] usage: try harder to format very long error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-11T18:05:25Z","receivedAt":"2015-08-11T18:05:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 11, 2015 at 09:34:31AM -0700, Junio C Hamano wrote:\n\n> > As for vwritef, it exists solely to work over write(), _and_ it doesn't\n> > get the \"one-shot\" thing right (which is probably OK, as we use it only\n> > when exec fails). But we could probably teach run-command to fdopen(),\n> > and get rid of it entirely (in favor of teaching vreportf to take a\n> > FILE* handle instead of assuming stderr).\n> \n> Sounds like a plan ;-)\n\nSo here's an alternative series that does this, along with the\nsingle-fprintf thing I mentioned. The first patch is actually orthogonal\nto the second; we would probably want it whichever path we choose to fix\nvreportf's truncation (I'd just have to rebase the earlier patch on top\nof it if we go that route).\n\n  [1/2]: vreportf: report to arbitrary filehandles\n  [2/2]: vreportf: avoid intermediate buffer\n\n-Peff\n"},{"id":"267842","messageId":"20150811180614.GA18002@sigill.intra.peff.net","threadId":"40057","inReplyTo":"20150811180524.GB15521@sigill.intra.peff.net","subject":"[PATCH 1/2] vreportf: report to arbitrary filehandles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-11T18:06:15Z","receivedAt":"2015-08-11T18:06:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The vreportf function always goes to stderr, but run-command\nwants child errors to go to the parent's original stderr. To\nsolve this, commit a5487dd duplicates the stderr fd and\ninstalls die and error handlers to direct the output\nappropriately (which later turned into the vwritef\nfunction). This has two downsides, though:\n\n  - we make multiple calls to write(), which contradicts the\n    \"write at once\" logic from d048a96 (print\n    warning/error/fatal messages in one shot, 2007-11-09).\n\n  - the custom handlers basically duplicate the normal\n    handlers.  They're only a few lines of code, but we\n    should not have to repeat the magic \"exit(128)\", for\n    example.\n\nWe can solve the first by using fdopen() on the duplicated\ndescriptor. We can't pass this to vreportf, but we could\nintroduce a new vreportf_to to handle it.\n\nHowever, to fix the second problem, we instead introduce a\nnew \"set_error_handle\" function, which lets the normal\nvreportf calls output to a handle besides stderr. Thus we\ncan get rid of our custom handlers entirely, and just ask\nthe regular handlers to output to our new descriptor.\n\nAnd as vwritef has no more callers, it can just go away.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n git-compat-util.h |  2 +-\n run-command.c     | 17 ++---------------\n usage.c           | 22 +++++++++-------------\n 3 files changed, 12 insertions(+), 29 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex c6d391f..076461e 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -389,7 +389,6 @@ struct strbuf;\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@@ -425,6 +424,7 @@ static inline int const_error(void)\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 extern void set_die_is_recursing_routine(int (*routine)(void));\n+extern void set_error_handle(FILE *);\n \n extern int starts_with(const char *str, const char *prefix);\n \ndiff --git a/run-command.c b/run-command.c\nindex 4d73e90..0d01671 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -200,7 +200,6 @@ static int execv_shell_cmd(const char **argv)\n #endif\n \n #ifndef GIT_WINDOWS_NATIVE\n-static int child_err = 2;\n static int child_notifier = -1;\n \n static void notify_parent(void)\n@@ -212,17 +211,6 @@ static void notify_parent(void)\n \t */\n \txwrite(child_notifier, \"\", 1);\n }\n-\n-static NORETURN void die_child(const char *err, va_list params)\n-{\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@@ -362,11 +350,10 @@ fail_pipe:\n \t\t * in subsequent call paths use the parent's stderr.\n \t\t */\n \t\tif (cmd->no_stderr || need_err) {\n-\t\t\tchild_err = dup(2);\n+\t\t\tint child_err = dup(2);\n \t\t\tset_cloexec(child_err);\n+\t\t\tset_error_handle(fdopen(child_err, \"w\"));\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 ed14645..e4fa6d2 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -6,23 +6,14 @@\n #include \"git-compat-util.h\"\n #include \"cache.h\"\n \n+static FILE *error_handle;\n+\n void vreportf(const char *prefix, const char *err, va_list params)\n {\n \tchar msg[4096];\n+\tFILE *fh = error_handle ? error_handle : stderr;\n \tvsnprintf(msg, sizeof(msg), err, 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+\tfprintf(fh, \"%s%s\\n\", prefix, msg);\n }\n \n static NORETURN void usage_builtin(const char *err, va_list params)\n@@ -76,6 +67,11 @@ void set_die_is_recursing_routine(int (*routine)(void))\n \tdie_is_recursing = routine;\n }\n \n+void set_error_handle(FILE *fh)\n+{\n+\terror_handle = fh;\n+}\n+\n void NORETURN usagef(const char *err, ...)\n {\n \tva_list params;\n-- \n2.5.0.417.g2db689c\n"},{"id":"267844","messageId":"20150811181359.GB18002@sigill.intra.peff.net","threadId":"40057","inReplyTo":"20150811180524.GB15521@sigill.intra.peff.net","subject":"[PATCH 2/2] vreportf: avoid intermediate buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-11T18:13:59Z","receivedAt":"2015-08-11T18:13:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we call \"die(fmt, args...)\", we end up in vreportf with\ntwo pieces of information:\n\n  1. The prefix \"fatal: \"\n\n  2. The original fmt and va_list of args.\n\nWe format item (2) into a temporary buffer, and then fprintf\nthe prefix and the temporary buffer, along with a newline.\nThis has the unfortunate side effect of truncating any error\nmessages that are longer than 4096 bytes.\n\nInstead, let's use separate calls for the prefix and\nnewline, letting us hand the item (2) directly to vfprintf.\nThis is essentially undoing d048a96 (print\nwarning/error/fatal messages in one shot, 2007-11-09), which\ntried to have the whole output end up in a single `write`\ncall.\n\nBut we can address this instead by explicitly requesting\nline-buffering for the output handle, and by making sure\nthat the buffer is empty before we start (so that outputting\nthe prefix does not cause a flush due to hitting the buffer\nlimit).\n\nWe may still break the output into two writes if the content\nis larger than our buffer, but there's not much we can do\nthere; depending on the stdio implementation, that might\nhave happened even with a single fprintf call.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI am not 100% sure on the last statement. On my system, it seems that:\n\n  fprintf(stderr, \"%s%s\\n\", prefix, msg);\n\nwill generally result in a single write when stderr is unbuffered (i.e.,\nit's default state). Which feels very magical, since it clearly must be\npreparing the output in a single buffer to feed to write, and the\ncontents of prefix and msg may easily exceed BUFSIZ. It looks like\nglibc internally uses an 8K buffer to generate \"unbuffered\" content.\nE.g., if I do:\n\n  strace -o /dev/fd/3 -e write \\\n  git --no-pager log --$(perl -e 'print \"a\" x 4096') \\\n  3>&2 2>/dev/null\n\nI get:\n\n  write(2, \"fatal: unrecognized argument: --\"..., 4129) = 4129\n\nand if I bump the 4096 to 16384, I get:\n\n  write(2, \"fatal: unrecognized argument: --\"..., 8192) = 8192\n  write(2, \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\"..., 8192) = 8192\n  write(2, \"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\"..., 33) = 33\n\nSo that's kind of weird. I suspect other stdio implementations may\nbehave differently, and AFAIK the standard is quiet on the subject (so\nit would be OK for an implementation to output the prefix, the msg, and\nthe newline in separate writes, no matter their length).\n\n usage.c | 15 ++++++++++++---\n 1 file changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/usage.c b/usage.c\nindex e4fa6d2..82ff131 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -7,13 +7,21 @@\n #include \"cache.h\"\n \n static FILE *error_handle;\n+static int tweaked_error_buffering;\n \n void vreportf(const char *prefix, const char *err, va_list params)\n {\n-\tchar msg[4096];\n \tFILE *fh = error_handle ? error_handle : stderr;\n-\tvsnprintf(msg, sizeof(msg), err, params);\n-\tfprintf(fh, \"%s%s\\n\", prefix, msg);\n+\n+\tfflush(fh);\n+\tif (!tweaked_error_buffering) {\n+\t\tsetvbuf(fh, NULL, _IOLBF, 0);\n+\t\ttweaked_error_buffering = 1;\n+\t}\n+\n+\tfputs(prefix, fh);\n+\tvfprintf(fh, err, params);\n+\tfputc('\\n', fh);\n }\n \n static NORETURN void usage_builtin(const char *err, va_list params)\n@@ -70,6 +78,7 @@ void set_die_is_recursing_routine(int (*routine)(void))\n void set_error_handle(FILE *fh)\n {\n \terror_handle = fh;\n+\ttweaked_error_buffering = 0;\n }\n \n void NORETURN usagef(const char *err, ...)\n-- \n2.5.0.417.g2db689c\n"}]}