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
