Web lists-archives.com

Re: [PATCH v2 12/16] parse-options: replace opterror() with optname()





On 06/11/2018 02:33, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@xxxxxxxxx> 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