| Message ID | 20250116012524.1557441-5-gitster@pobox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Send help text from "git cmd -h" to stdout | expand |
On Wed, Jan 15, 2025 at 05:25:21PM -0800, Junio C Hamano wrote: > Introduce a helper function that captures the common pattern > > if (argc == 2 && !strcmp(argv[1], "-h")) > usage(usage); > > and replaces it with > > show_usage_and_exit_if_asked(argc, argv, usage); > > to help correct these code paths. I found the name hard to distinguish from the earlier helper, show_usage_help_and_exit_if_asked(). The difference is that one takes only a usage string, and the other takes the usage string along with options. Maybe the other should be: show_usage_and_exit_if_asked_with_options(); ? It just keeps getting longer and longer... I think the "and_exit" could probably be implied, since showing the usage is always a final thing (just like in usage() and usage_with_options()). So: show_usage_if_asked(); show_usage_with_options_if_asked(); ? I dunno. We are in deep bikeshed territory. I otherwise like what the patch is doing. ;) > -static void vreportf(const char *prefix, const char *err, va_list params) > +static void vfdreportf(int fd, const char *prefix, const char *err, va_list params) > { > char msg[4096]; > char *p, *pend = msg + sizeof(msg); > @@ -32,8 +32,14 @@ static void vreportf(const char *prefix, const char *err, va_list params) > } > > *(p++) = '\n'; /* we no longer need a NUL */ > - 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. My main motivation is being less ugly, but I think it would also potentially prevent a bug. If there was unflushed data in stdout as we go into vdreportf(), then we'd get an out of order write. That's why the fflush() is there in the first place (it's of course weird that we are using write() in the first place, but IIRC it's for avoiding sheared writes of error messages). -Peff
On Thu, Jan 16, 2025 at 05:36:20AM -0500, Jeff King wrote: > > -static void vreportf(const char *prefix, const char *err, va_list params) > > +static void vfdreportf(int fd, const char *prefix, const char *err, va_list params) > > { > > char msg[4096]; > > char *p, *pend = msg + sizeof(msg); > > @@ -32,8 +32,14 @@ static void vreportf(const char *prefix, const char *err, va_list params) > > } > > > > *(p++) = '\n'; /* we no longer need a NUL */ > > - fflush(stderr); > > - write_in_full(2, msg, p - msg); > > + if (fd == 2) > > + fflush(stderr); > > + write_in_full(fd, msg, p - msg); > > +} Oh, I meant to mention also: I wondered about the other references to stderr in this function. But we hit those only when we fail to format the error message that was asked for, so continuing to send them to stderr is the right thing (i.e., your patch is right but I wanted to help out other reviewers). -Peff
Jeff King <peff@peff.net> writes: >> show_usage_and_exit_if_asked(argc, argv, usage); >> >> to help correct these code paths. > > I found the name hard to distinguish from the earlier helper, As we all know that usage_with_options() and usage() exits, show_usage_with_options_if_asked() show_usage_if_asked() may be good enough, I wonder? "show" -> "help" may give us better names. > I think the "and_exit" could probably be implied, since showing the > usage is always a final thing (just like in usage() and > usage_with_options()). So: > > show_usage_if_asked(); > show_usage_with_options_if_asked(); Ah, We came to the same conclusion. The only thing we haven't made it obvious is that "show" implies the output goes to the standard output stream, while without "show", the output goes to the standard error stream. I wonder if these help better: show_help_if_asked(); show_help_with_options_if_asked() "show help" explains the output goes to where the "help" message would go ;-) But probably "if-asked" is clear enough indication that the output is made in response to an explicit end-user request, so let's take the show_(usage|usage_with_options)_if_asked as the final names. >> -static void vreportf(const char *prefix, const char *err, va_list params) >> +static void vfdreportf(int fd, const char *prefix, const char *err, va_list params) >> { >> char msg[4096]; >> char *p, *pend = msg + sizeof(msg); >> @@ -32,8 +32,14 @@ static void vreportf(const char *prefix, const char *err, va_list params) >> } >> >> *(p++) = '\n'; /* we no longer need a NUL */ >> - 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 *".
On Thu, Jan 16, 2025 at 09:22:54AM -0800, Junio C Hamano wrote: > >> - 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); }
Jeff King <peff@peff.net> writes: > You said "may become a bit tricky" above, but unless I'm missing > something, it's just: I didn't mean "tricky to write". I meant it would become tricky to reason about while reading the resulting code why we flush only when fh is stderr. > 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. But as vreportf() has always been about stderr, when that old commit talks about "stdio", it is talking about stderr, isn't it? > 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. We are going to exit immediately at the end of this function, so there is no point in avoiding an fflush() call that may not be needed. So I am not fundamentally opposed to unconditionally flushing before doing the write(). Thanks.
diff --git a/git-compat-util.h b/git-compat-util.h index e283c46c6f..6edf50bc95 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -701,6 +701,8 @@ int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2))); void warning(const char *err, ...) __attribute__((format (printf, 1, 2))); void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2))); +void show_usage_and_exit_if_asked(int ac, const char **av, const char *err); + #ifndef NO_OPENSSL #ifdef APPLE_COMMON_CRYPTO #include "compat/apple-common-crypto.h" diff --git a/usage.c b/usage.c index 47709006c1..26cd54a511 100644 --- a/usage.c +++ b/usage.c @@ -8,7 +8,7 @@ #include "gettext.h" #include "trace2.h" -static void vreportf(const char *prefix, const char *err, va_list params) +static void vfdreportf(int fd, const char *prefix, const char *err, va_list params) { char msg[4096]; char *p, *pend = msg + sizeof(msg); @@ -32,8 +32,14 @@ static void vreportf(const char *prefix, const char *err, va_list params) } *(p++) = '\n'; /* we no longer need a NUL */ - fflush(stderr); - write_in_full(2, msg, p - msg); + if (fd == 2) + fflush(stderr); + write_in_full(fd, msg, p - msg); +} + +static void vreportf(const char *prefix, const char *err, va_list params) +{ + vfdreportf(2, prefix, err, params); } static NORETURN void usage_builtin(const char *err, va_list params) @@ -173,6 +179,22 @@ void NORETURN usage(const char *err) usagef("%s", err); } +static void show_usage_and_exit_if_asked_helper(const char *err, ...) +{ + va_list params; + + va_start(params, err); + vfdreportf(1, _("usage: "), err, params); + va_end(params); + exit(129); +} + +void show_usage_and_exit_if_asked(int ac, const char **av, const char *err) +{ + if (ac == 2 && !strcmp(av[1], "-h")) + show_usage_and_exit_if_asked_helper(err); +} + void NORETURN die(const char *err, ...) { va_list params;
Some commands call usage() when they are asked to give the help message with "git cmd -h", but this has the same problem as we fixed with callers of usage_with_options() for the same purpose. Introduce a helper function that captures the common pattern if (argc == 2 && !strcmp(argv[1], "-h")) usage(usage); and replaces it with show_usage_and_exit_if_asked(argc, argv, usage); to help correct these code paths. Note that this helper function still exits with status 129, and t0012 insists on it. After converting all the mistaken callers of usage_with_options() to call this new helper, we may want to address it---the end user is asking us to give the help text, and we are doing exactly as asked, so there is no reason to exit with non-zero status. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- git-compat-util.h | 2 ++ usage.c | 28 +++++++++++++++++++++++++--- 2 files changed, 27 insertions(+), 3 deletions(-)