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

[Jeff King <peff@peff.net>]

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