From 3b331e92671469614662830402103848a8004b97 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 11 Aug 2015 14:06:15 -0400 Subject: [PATCH 1/2] vreportf: report to arbitrary filehandles The vreportf function always goes to stderr, but run-command wants child errors to go to the parent's original stderr. To solve this, commit a5487dd duplicates the stderr fd and installs die and error handlers to direct the output appropriately (which later turned into the vwritef function). This has two downsides, though: - we make multiple calls to write(), which contradicts the "write at once" logic from d048a96 (print warning/error/fatal messages in one shot, 2007-11-09). - the custom handlers basically duplicate the normal handlers. They're only a few lines of code, but we should not have to repeat the magic "exit(128)", for example. We can solve the first by using fdopen() on the duplicated descriptor. We can't pass this to vreportf, but we could introduce a new vreportf_to to handle it. However, to fix the second problem, we instead introduce a new "set_error_handle" function, which lets the normal vreportf calls output to a handle besides stderr. Thus we can get rid of our custom handlers entirely, and just ask the regular handlers to output to our new descriptor. And as vwritef has no more callers, it can just go away. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- git-compat-util.h | 2 +- run-command.c | 17 ++--------------- usage.c | 22 +++++++++------------- 3 files changed, 12 insertions(+), 29 deletions(-) diff --git a/git-compat-util.h b/git-compat-util.h index c6d391f864..076461e8c8 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -389,7 +389,6 @@ struct strbuf; /* General helper functions */ extern void vreportf(const char *prefix, const char *err, va_list params); -extern void vwritef(int fd, const char *prefix, const char *err, va_list params); extern NORETURN void usage(const char *err); extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2))); extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2))); @@ -425,6 +424,7 @@ static inline int const_error(void) extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params)); extern void set_error_routine(void (*routine)(const char *err, va_list params)); extern void set_die_is_recursing_routine(int (*routine)(void)); +extern void set_error_handle(FILE *); extern int starts_with(const char *str, const char *prefix); diff --git a/run-command.c b/run-command.c index 4d73e90fad..0d01671c1f 100644 --- a/run-command.c +++ b/run-command.c @@ -200,7 +200,6 @@ static int execv_shell_cmd(const char **argv) #endif #ifndef GIT_WINDOWS_NATIVE -static int child_err = 2; static int child_notifier = -1; static void notify_parent(void) @@ -212,17 +211,6 @@ static void notify_parent(void) */ xwrite(child_notifier, "", 1); } - -static NORETURN void die_child(const char *err, va_list params) -{ - vwritef(child_err, "fatal: ", err, params); - exit(128); -} - -static void error_child(const char *err, va_list params) -{ - vwritef(child_err, "error: ", err, params); -} #endif static inline void set_cloexec(int fd) @@ -362,11 +350,10 @@ fail_pipe: * in subsequent call paths use the parent's stderr. */ if (cmd->no_stderr || need_err) { - child_err = dup(2); + int child_err = dup(2); set_cloexec(child_err); + set_error_handle(fdopen(child_err, "w")); } - set_die_routine(die_child); - set_error_routine(error_child); close(notify_pipe[0]); set_cloexec(notify_pipe[1]); diff --git a/usage.c b/usage.c index ed146453ca..e4fa6d2f03 100644 --- a/usage.c +++ b/usage.c @@ -6,23 +6,14 @@ #include "git-compat-util.h" #include "cache.h" +static FILE *error_handle; + void vreportf(const char *prefix, const char *err, va_list params) { char msg[4096]; + FILE *fh = error_handle ? error_handle : stderr; vsnprintf(msg, sizeof(msg), err, params); - fprintf(stderr, "%s%s\n", prefix, msg); -} - -void vwritef(int fd, const char *prefix, const char *err, va_list params) -{ - char msg[4096]; - int len = vsnprintf(msg, sizeof(msg), err, params); - if (len > sizeof(msg)) - len = sizeof(msg); - - write_in_full(fd, prefix, strlen(prefix)); - write_in_full(fd, msg, len); - write_in_full(fd, "\n", 1); + fprintf(fh, "%s%s\n", prefix, msg); } static NORETURN void usage_builtin(const char *err, va_list params) @@ -76,6 +67,11 @@ void set_die_is_recursing_routine(int (*routine)(void)) die_is_recursing = routine; } +void set_error_handle(FILE *fh) +{ + error_handle = fh; +} + void NORETURN usagef(const char *err, ...) { va_list params; From f4c3edc0b156362a92bf9de4f0ec794e90a757fc Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 11 Aug 2015 14:13:59 -0400 Subject: [PATCH 2/2] vreportf: avoid intermediate buffer When we call "die(fmt, args...)", we end up in vreportf with two pieces of information: 1. The prefix "fatal: " 2. The original fmt and va_list of args. We format item (2) into a temporary buffer, and then fprintf the prefix and the temporary buffer, along with a newline. This has the unfortunate side effect of truncating any error messages that are longer than 4096 bytes. Instead, let's use separate calls for the prefix and newline, letting us hand the item (2) directly to vfprintf. This is essentially undoing d048a96 (print warning/error/fatal messages in one shot, 2007-11-09), which tried to have the whole output end up in a single `write` call. But we can address this instead by explicitly requesting line-buffering for the output handle, and by making sure that the buffer is empty before we start (so that outputting the prefix does not cause a flush due to hitting the buffer limit). We may still break the output into two writes if the content is larger than our buffer, but there's not much we can do there; depending on the stdio implementation, that might have happened even with a single fprintf call. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- usage.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/usage.c b/usage.c index e4fa6d2f03..82ff13163b 100644 --- a/usage.c +++ b/usage.c @@ -7,13 +7,21 @@ #include "cache.h" static FILE *error_handle; +static int tweaked_error_buffering; void vreportf(const char *prefix, const char *err, va_list params) { - char msg[4096]; FILE *fh = error_handle ? error_handle : stderr; - vsnprintf(msg, sizeof(msg), err, params); - fprintf(fh, "%s%s\n", prefix, msg); + + fflush(fh); + if (!tweaked_error_buffering) { + setvbuf(fh, NULL, _IOLBF, 0); + tweaked_error_buffering = 1; + } + + fputs(prefix, fh); + vfprintf(fh, err, params); + fputc('\n', fh); } static NORETURN void usage_builtin(const char *err, va_list params) @@ -70,6 +78,7 @@ void set_die_is_recursing_routine(int (*routine)(void)) void set_error_handle(FILE *fh) { error_handle = fh; + tweaked_error_buffering = 0; } void NORETURN usagef(const char *err, ...)