Re: [PATCH 07/10] commit-graph: stop using optname()

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 29, 2021, 17:28 UTC
Message-ID: <YVSiOMaKNoDZ3SlO@nand.local>
In-Reply-To: <patch-07.10-b0b313795c7-20210928T130905Z-avarab@gmail.com>

On Tue, Sep 28, 2021 at 03:14:28PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 9 quoted lines

> Stop using optname() in builtin/commit-graph.c to emit an error with
> the --max-new-filters option. This changes code added in 809e0327f57
> (builtin/commit-graph.c: introduce '--max-new-filters=<n>',
> 2020-09-18).
>
> See 9440b831ad5 (parse-options: replace opterror() with optname(),
> 2018-11-10) for why using optname() like this is considered bad,
> i.e. it's assembling human-readable output piecemeal, and the "option
> `X'" at the start can't be translated.

In fact, using optname there (which blames to me) was a mistake for an even simpler reason: there is no abbreviated form of `--max-new-filters`, and we know that by the time this error is emitted that we got the positive form instead of `--no-max-new-filters`.

So we could have just written the option's name verbatim, and given translators something easier to work with.

Show 20 quoted lines

> It didn't matter in this case, but this code was also buggy in its use
> of "opt->flags" to optname(), that function expects flags, but not
> *those* flags.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/commit-graph.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 0386f5c7755..36552db89fe 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -172,8 +172,7 @@ static int write_option_max_new_filters(const struct option *opt,
>  		const char *s;
>  		*to = strtol(arg, (char **)&s, 10);
>  		if (*s)
> -			return error(_("%s expects a numerical value"),
> -				     optname(opt, opt->flags));
> +			return error(_("option `max-new-filters' expects a numerical value"));

Makes sense. The `'-style quoting is still weird to me. It is consistent with some of the conversions in 9440b831ad5, but most importantly with how parse-options.c:get_value() behaves (because it calls optname underneath).

(This has nothing to do with your patch, but I thought the custom write_option_max_new_filters callback was weird when I wrote it. It's working around trying to make the negated form set a value to `-1` instead of `0`. But it's an annoying hack, because we have to call strtol() ourselves when we're not negated. *sigh*).

Looks good to me.

Thanks, Taylor