| Message ID | patch-07.10-b0b313795c7-20210928T130905Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix bug, use existing enums | expand |
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. > 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
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.
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")); } return 0; }
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. 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(-)