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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Sep 29 2021, Taylor Blau wrote:

> On Tue, Sep 28, 2021 at 03:14:28PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> 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.

As an aside: Did you intend for this to work:

    git commit-graph write --max-new-filters=123 --no-max-new-filters

It's in the docstring, but then you're using OPT_CALLBACK_F(), but just
to set a flag of "0", so a OPT_CALLBACK() would do, along with a
PARSE_OPT_NONEG.

I'm about to re-roll this, but won't change that, but I think it
probably makes sense as a follow-on cleanuup.

I think you'd probably want a BUG_ON_OPT_NEG() instead for the "unset"
handling here. This seems like another case of mixing the state of
parse_options() with that of flags for the underlying API that we
discussed elsewhere either for this command or multi-pack-index. But
more on that below...

Also the usage if --no-* is wanted should not be:

    [--no-max-new-filters] [--max-new-filters <n>]

But is currently:

    [--[no-]max-new-filters <n>]

Which says the --no-* will take the <n>, but it won't.

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

Yeah I just reproduced the existing output here.

> (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*).

...on the "more on that below", looks like you intended that -1 handling
in some way, but I don't really see why yet, other than the "mixing the
state" I noted above.