Re: [PATCH 2/5] ref-filter: factor out "%(foo) does not take arguments" errors

[Taylor Blau <me@ttaylorr.com>]

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