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@xxxxxxxx> > > --- > > 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