| Message ID | Y5n3n7Gp2gKNMln3@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a33d0fae76ab95e88d383793cac41934920296ba |
| Headers | show |
| Series | minor ref-filter error-reporting bug-fixes | expand |
On Wed, Dec 14, 2022 at 11:19:43AM -0500, Jeff King wrote: > Many atom parsers give the same error message, differing only in the > name of the atom. If we use "%s does not take arguments", that should > make life easier for translators, as they only need to translate one > string. And in doing so, we can easily pull it into a helper function to > make sure they are all using the exact same string. > > I've added a basic test here for %(HEAD), just to make sure this code is > exercised at all in the test suite. We could cover each such atom, but > the effort-to-reward ratio of trying to maintain an exhaustive list > doesn't seem worth it. > > Signed-off-by: Jeff King <peff@peff.net> > --- > ref-filter.c | 16 +++++++++++----- > t/t6300-for-each-ref.sh | 6 ++++++ > 2 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/ref-filter.c b/ref-filter.c > index 08ac5f886e..639b18ab36 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -228,6 +228,12 @@ static int strbuf_addf_ret(struct strbuf *sb, int ret, const char *fmt, ...) > return ret; > } > > +static int err_no_arg(struct strbuf *sb, const char *name) > +{ > + strbuf_addf(sb, _("%%(%s) does not take arguments"), name); > + return -1; > +} > + Why introduce such a function? strbuf_addf_ret() already takes a format string with additional vargs, so it should suffice to replace existing calls with: return strbuf_addf_ret(err, -1, _("%%(%s) does not take arguments"), "objecttype"); Playing devil's advocate for a moment, I suppose arguments in favor of err_no_arg() might be: - It does not require callers to repeat the translation key each time. - It requires fewer characters to call. So I think either is fine, though it might be cleaner to implement err_no_arg() in terms of strbuf_addf_ret() like: static int err_no_arg(struct strbuf *sb, const char *name) { return strbuf_addf_ret(sb, -1, _("%%(%s) does not take arguments"), name); } Thanks, Taylor
On Wed, Dec 14, 2022 at 02:51:33PM -0500, Taylor Blau wrote: > On Wed, Dec 14, 2022 at 11:19:43AM -0500, Jeff King wrote: > > Many atom parsers give the same error message, differing only in the > > name of the atom. If we use "%s does not take arguments", that should > > make life easier for translators, as they only need to translate one > > string. And in doing so, we can easily pull it into a helper function to > > make sure they are all using the exact same string. > > > > I've added a basic test here for %(HEAD), just to make sure this code is > > exercised at all in the test suite. We could cover each such atom, but > > the effort-to-reward ratio of trying to maintain an exhaustive list > > doesn't seem worth it. > > > > Signed-off-by: Jeff King <peff@peff.net> > > --- > > ref-filter.c | 16 +++++++++++----- > > t/t6300-for-each-ref.sh | 6 ++++++ > > 2 files changed, 17 insertions(+), 5 deletions(-) > > > > diff --git a/ref-filter.c b/ref-filter.c > > index 08ac5f886e..639b18ab36 100644 > > --- a/ref-filter.c > > +++ b/ref-filter.c > > @@ -228,6 +228,12 @@ static int strbuf_addf_ret(struct strbuf *sb, int ret, const char *fmt, ...) > > return ret; > > } > > > > +static int err_no_arg(struct strbuf *sb, const char *name) > > +{ > > + strbuf_addf(sb, _("%%(%s) does not take arguments"), name); > > + return -1; > > +} > > + > > Why introduce such a function? strbuf_addf_ret() already takes a format > string with additional vargs, so it should suffice to replace existing > calls with: > > return strbuf_addf_ret(err, -1, _("%%(%s) does not take arguments"), "objecttype"); > > Playing devil's advocate for a moment, I suppose arguments in favor of > err_no_arg() might be: > > - It does not require callers to repeat the translation key each time. > - It requires fewer characters to call. > > So I think either is fine, though it might be cleaner to implement > err_no_arg() in terms of strbuf_addf_ret() like: > > static int err_no_arg(struct strbuf *sb, const char *name) > { > return strbuf_addf_ret(sb, -1, _("%%(%s) does not take arguments"), name); > } Ah, the later patches make it clear why you pulled this into its own function. Perhaps a blurb in the patch message along the lines of: "this doesn't need to live in its own function, but doing so will make a subsequent change much easier" would be helpful, but I don't think it's a big deal. Thanks, Taylor
On Wed, Dec 14, 2022 at 02:51:33PM -0500, Taylor Blau wrote: > > +static int err_no_arg(struct strbuf *sb, const char *name) > > +{ > > + strbuf_addf(sb, _("%%(%s) does not take arguments"), name); > > + return -1; > > +} > > + > > Why introduce such a function? strbuf_addf_ret() already takes a format > string with additional vargs, so it should suffice to replace existing > calls with: > > return strbuf_addf_ret(err, -1, _("%%(%s) does not take arguments"), "objecttype"); > > Playing devil's advocate for a moment, I suppose arguments in favor of > err_no_arg() might be: > > - It does not require callers to repeat the translation key each time. > - It requires fewer characters to call. Yes. My primary motivation was avoiding repeated strings that are supposed to be the same (but nothing is checking). You could also accomplish that by pulling the format string into a variable, but I think that readability suffers (since you don't see the format string in the addf call that is passing in the varargs). As you saw, I ended up also making the function more complicated in a later patch, though that really did come later and wasn't part of my motivation (for once my commit messages were actually written in order!). I considered going back and mentioning it in this commit message, but I though the "don't repeat yourself" motivation was sufficient. Maybe that's not so, though. > So I think either is fine, though it might be cleaner to implement > err_no_arg() in terms of strbuf_addf_ret() like: > > static int err_no_arg(struct strbuf *sb, const char *name) > { > return strbuf_addf_ret(sb, -1, _("%%(%s) does not take arguments"), name); > } That was actually what I wrote initially, but it seemed less readable to me. In the middle of a parsing function which is conditionally reporting an error, smooshing two lines to one has value. Here in a helper, it seemed like a net negative. Maybe it's just me, though. -Peff
diff --git a/ref-filter.c b/ref-filter.c index 08ac5f886e..639b18ab36 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -228,6 +228,12 @@ static int strbuf_addf_ret(struct strbuf *sb, int ret, const char *fmt, ...) return ret; } +static int err_no_arg(struct strbuf *sb, const char *name) +{ + strbuf_addf(sb, _("%%(%s) does not take arguments"), name); + return -1; +} + static int color_atom_parser(struct ref_format *format, struct used_atom *atom, const char *color_value, struct strbuf *err) { @@ -317,7 +323,7 @@ static int objecttype_atom_parser(struct ref_format *format, struct used_atom *a const char *arg, struct strbuf *err) { if (arg) - return strbuf_addf_ret(err, -1, _("%%(objecttype) does not take arguments")); + return err_no_arg(err, "objecttype"); if (*atom->name == '*') oi_deref.info.typep = &oi_deref.type; else @@ -349,7 +355,7 @@ static int deltabase_atom_parser(struct ref_format *format, struct used_atom *at const char *arg, struct strbuf *err) { if (arg) - return strbuf_addf_ret(err, -1, _("%%(deltabase) does not take arguments")); + return err_no_arg(err, "deltabase"); if (*atom->name == '*') oi_deref.info.delta_base_oid = &oi_deref.delta_base_oid; else @@ -361,7 +367,7 @@ static int body_atom_parser(struct ref_format *format, struct used_atom *atom, const char *arg, struct strbuf *err) { if (arg) - return strbuf_addf_ret(err, -1, _("%%(body) does not take arguments")); + return err_no_arg(err, "body"); atom->u.contents.option = C_BODY_DEP; return 0; } @@ -565,7 +571,7 @@ static int rest_atom_parser(struct ref_format *format, struct used_atom *atom, const char *arg, struct strbuf *err) { if (arg) - return strbuf_addf_ret(err, -1, _("%%(rest) does not take arguments")); + return err_no_arg(err, "rest"); format->use_rest = 1; return 0; } @@ -574,7 +580,7 @@ static int head_atom_parser(struct ref_format *format, struct used_atom *atom, const char *arg, struct strbuf *err) { if (arg) - return strbuf_addf_ret(err, -1, _("%%(HEAD) does not take arguments")); + return err_no_arg(err, "HEAD"); atom->u.head = resolve_refdup("HEAD", RESOLVE_REF_READING, NULL, NULL); return 0; } diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index fa38b87441..8d99658ef8 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -1242,6 +1242,12 @@ test_expect_success 'basic atom: rest must fail' ' test_must_fail git for-each-ref --format="%(rest)" refs/heads/main ' +test_expect_success 'HEAD atom does not take arguments' ' + test_must_fail git for-each-ref --format="%(HEAD:foo)" 2>err && + echo "fatal: %(HEAD) does not take arguments" >expect && + test_cmp expect err +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject
Many atom parsers give the same error message, differing only in the name of the atom. If we use "%s does not take arguments", that should make life easier for translators, as they only need to translate one string. And in doing so, we can easily pull it into a helper function to make sure they are all using the exact same string. I've added a basic test here for %(HEAD), just to make sure this code is exercised at all in the test suite. We could cover each such atom, but the effort-to-reward ratio of trying to maintain an exhaustive list doesn't seem worth it. Signed-off-by: Jeff King <peff@peff.net> --- ref-filter.c | 16 +++++++++++----- t/t6300-for-each-ref.sh | 6 ++++++ 2 files changed, 17 insertions(+), 5 deletions(-)