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

From: Taylor Blau <me@ttaylorr.com>
Date: Dec 14, 2022, 19:51 UTC
Message-ID: <Y5opRTGtrpoLsSF6@nand.local>
In-Reply-To: <Y5n3n7Gp2gKNMln3@coredump.intra.peff.net>

On Wed, Dec 14, 2022 at 11:19:43AM -0500, Jeff King wrote:

Show 31 quoted lines

> 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