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

From: Jeff King <peff@peff.net>
Date: Dec 14, 2022, 20:28 UTC
Message-ID: <Y5ox4ib0KvslKlr/@coredump.intra.peff.net>
In-Reply-To: <Y5opRTGtrpoLsSF6@nand.local>

On Wed, Dec 14, 2022 at 02:51:33PM -0500, Taylor Blau wrote:

Show 18 quoted lines

> > +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.

Show 7 quoted lines

> 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