Re: [PATCH v3 4/6] usage: add show_usage_and_exit_if_asked()
- From
Jeff King <peff@peff.net>
- Date
- Jan 16, 2025, 21:54 UTC
- Message-ID
- <20250116215431.GA2338133@coredump.intra.peff.net>
- In-Reply-To
- <xmqqbjw6u101.fsf@gitster.g>
On Thu, Jan 16, 2025 at 09:22:54AM -0800, Junio C Hamano wrote:
Show 19 quoted lines
> >> - fflush(stderr); > >> - write_in_full(2, msg, p - msg); > >> + if (fd == 2) > >> + fflush(stderr); > >> + write_in_full(fd, msg, p - msg); > >> +} > > > > Gross. :) I think the existing code is conceptually: > > > > write_in_full(fileno(stderr), msg, p - msg); > > > > In which case vfreportf() could just take a FILE*, flush it and then > > write. > > Sure, but these "stderr" are real error reporting that need to stay > to be stderr, and flush needs to be done only when our true payload > goes to fd#2 and I do not think these fflush() are about stdio calls > made by the caller _before_ it called this function. It may become > a bit tricky to read the resulting code if we pass "FILE *".
I think the flush _is_ about earlier stdio calls in the process; that's what 116d1fa6c6 (vreportf(): avoid relying on stdio buffering, 2019-10-30) says. I do think it's unlikely for the process to write to stdout() before processing "-h", but it seems like we should do the safer thing as a general principle, unless it ends up hard to do so.
You said "may become a bit tricky" above, but unless I'm missing something, it's just:
--- usage.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/usage.c b/usage.c index d9f323a0bd..533d296e41 100644 --- a/usage.c +++ b/usage.c @@ -8,7 +8,7 @@ #include "gettext.h" #include "trace2.h" -static void vfdreportf(int fd, const char *prefix, const char *err, va_list params) +static void vfreportf(FILE *fh, const char *prefix, const char *err, va_list params) { char msg[4096]; char *p, *pend = msg + sizeof(msg); @@ -32,14 +32,13 @@ static void vfdreportf(int fd, const char *prefix, const char *err, va_list para } *(p++) = '\n'; /* we no longer need a NUL */ - if (fd == 2) - fflush(stderr); - write_in_full(fd, msg, p - msg); + fflush(fh); + write_in_full(fileno(fh), msg, p - msg); } static void vreportf(const char *prefix, const char *err, va_list params) { - vfdreportf(2, prefix, err, params); + vfreportf(stderr, prefix, err, params); } static NORETURN void usage_builtin(const char *err, va_list params) @@ -184,7 +183,7 @@ static void show_usage_if_asked_helper(const char *err, ...) va_list params; va_start(params, err); - vfdreportf(1, _("usage: "), err, params); + vfreportf(stdout, _("usage: "), err, params); va_end(params); exit(129); }