On 06/11/2018 02:33, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy <pclo...@gmail.com> writes:
>
>> There are a few issues with opterror()
>>
>> - it tries to assemble an English sentence from pieces. This is not
>> great for translators because we give them pieces instead of a full
>> sentence.
>>
>> - It's a wrapper around error() and needs some hack to let the
>> compiler know it always returns -1.
>>
>> - Since it takes a string instead of printf format, one call site has
>> to assemble the string manually before passing to it.
>>
>> Kill it and produce the option name with optname(). The user will use
>> error() directly. This solves the second and third problems.
>
> The proposed log message is not very friendly to reviewers, as there
> is no hint what optname() does nor where it came from; it turns out
> that this patch introduces it.
>
> Introduce optname() that does the early half of original
> opterror() to come up with the name of the option reported back
> to the user, and use it to kill opterror(). The callers of
> opterror() now directly call error() using the string returned
> by opterror() instead.
>
> or something like that perhaps.
>
> Theoretically not very friendly to topics in flight, but I do not
> expect there would be any right now that wants to add new callers of
> opterror().
>
> I do agree with the reasoning behind this change. Thanks for
> working on it.
>
Also, this patch does not replace opterror() calls outside of
the 'parse-options.c' file with optname(). This tickles my
static-check.pl script, since optname() is an external function
which is only called from 'parse-options.c'.
So, at present, optname() could be marked as a local 'static'
symbol. However, I could also imagine it being used by new callers
outside of 'parse-options.c' in the future. (maybe) Your call. ;-)
ATB,
Ramsay Jones
