Re: [PATCH v3 4/6] usage: add show_usage_and_exit_if_asked()

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 16, 2025, 17:22 UTC
Message-ID: <xmqqbjw6u101.fsf@gitster.g>
In-Reply-To: <20250116103620.GB773990@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

>> 	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.

Show 6 quoted lines

> 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.

Show 22 quoted lines

>> -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 *".