Re: [PATCH v2 12/16] parse-options: replace opterror() with optname()
- Date: Tue, 6 Nov 2018 14:02:42 +0000
- From: Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxxx>
- Subject: 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
>> - 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
> 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. ;-)