[PATCH 0/5] minor ref-filter error-reporting bug-fixes

[PATCH 0/5] minor ref-filter error-reporting bug-fixes

[Jeff King]

This series fixes a few small inconsistencies in the error-reporting
from ref-filter's atom parsers. Mostly I noticed this while dealing with
unused parameters in one of the parsers, which led to noticing a few
other small bugs. And I hope the result is a little cleaner, as it
should reduce the number of strings needing translation.

  [1/5]: ref-filter: reject arguments to %(HEAD)
  [2/5]: ref-filter: factor out "%(foo) does not take arguments" errors
  [3/5]: ref-filter: factor out "unrecognized %(foo) arg" errors
  [4/5]: ref-filter: truncate atom names in error messages
  [5/5]: ref-filter: convert email atom parser to use err_bad_arg()

 ref-filter.c            | 44 +++++++++++++++++++++++++++++------------
 t/t6300-for-each-ref.sh | 18 +++++++++++++++++
 2 files changed, 49 insertions(+), 13 deletions(-)

-Peff

[PATCH 1/5] ref-filter: reject arguments to %(HEAD)

[Jeff King]

The %(HEAD) atom doesn't take any arguments, but unlike other atoms in
the same boat (objecttype, deltabase, etc), it does not detect this
situation and complain. Let's make it consistent with the others.

Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index caf10ab23e..08ac5f886e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -571,8 +571,10 @@ static int rest_atom_parser(struct ref_format *format, struct used_atom *atom,
 }
 
 static int head_atom_parser(struct ref_format *format, struct used_atom *atom,
-			    const char *arg, struct strbuf *unused_err)
+			    const char *arg, struct strbuf *err)
 {
+	if (arg)
+		return strbuf_addf_ret(err, -1, _("%%(HEAD) does not take arguments"));
 	atom->u.head = resolve_refdup("HEAD", RESOLVE_REF_READING, NULL, NULL);
 	return 0;
 }
-- 
2.39.0.550.g5015380a67

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

[Jeff King]

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;
+}
+
 static int color_atom_parser(struct ref_format *format, struct used_atom *atom,
 			     const char *color_value, struct strbuf *err)
 {
@@ -317,7 +323,7 @@ static int objecttype_atom_parser(struct ref_format *format, struct used_atom *a
 				  const char *arg, struct strbuf *err)
 {
 	if (arg)
-		return strbuf_addf_ret(err, -1, _("%%(objecttype) does not take arguments"));
+		return err_no_arg(err, "objecttype");
 	if (*atom->name == '*')
 		oi_deref.info.typep = &oi_deref.type;
 	else
@@ -349,7 +355,7 @@ static int deltabase_atom_parser(struct ref_format *format, struct used_atom *at
 				 const char *arg, struct strbuf *err)
 {
 	if (arg)
-		return strbuf_addf_ret(err, -1, _("%%(deltabase) does not take arguments"));
+		return err_no_arg(err, "deltabase");
 	if (*atom->name == '*')
 		oi_deref.info.delta_base_oid = &oi_deref.delta_base_oid;
 	else
@@ -361,7 +367,7 @@ static int body_atom_parser(struct ref_format *format, struct used_atom *atom,
 			    const char *arg, struct strbuf *err)
 {
 	if (arg)
-		return strbuf_addf_ret(err, -1, _("%%(body) does not take arguments"));
+		return err_no_arg(err, "body");
 	atom->u.contents.option = C_BODY_DEP;
 	return 0;
 }
@@ -565,7 +571,7 @@ static int rest_atom_parser(struct ref_format *format, struct used_atom *atom,
 			    const char *arg, struct strbuf *err)
 {
 	if (arg)
-		return strbuf_addf_ret(err, -1, _("%%(rest) does not take arguments"));
+		return err_no_arg(err, "rest");
 	format->use_rest = 1;
 	return 0;
 }
@@ -574,7 +580,7 @@ static int head_atom_parser(struct ref_format *format, struct used_atom *atom,
 			    const char *arg, struct strbuf *err)
 {
 	if (arg)
-		return strbuf_addf_ret(err, -1, _("%%(HEAD) does not take arguments"));
+		return err_no_arg(err, "HEAD");
 	atom->u.head = resolve_refdup("HEAD", RESOLVE_REF_READING, NULL, NULL);
 	return 0;
 }
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index fa38b87441..8d99658ef8 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -1242,6 +1242,12 @@ test_expect_success 'basic atom: rest must fail' '
 	test_must_fail git for-each-ref --format="%(rest)" refs/heads/main
 '
 
+test_expect_success 'HEAD atom does not take arguments' '
+	test_must_fail git for-each-ref --format="%(HEAD:foo)" 2>err &&
+	echo "fatal: %(HEAD) does not take arguments" >expect &&
+	test_cmp expect err
+'
+
 test_expect_success 'trailer parsing not fooled by --- line' '
 	git commit --allow-empty -F - <<-\EOF &&
 	this is the subject
-- 
2.39.0.550.g5015380a67

[PATCH 3/5] ref-filter: factor out "unrecognized %(foo) arg" errors

[Jeff King]

Atom parsers that take arguments generally have a catch-all for "this
arg is not recognized". Most of them use the same printf template, which
is good, because it makes life easier for translators. Let's pull this
template into a helper function, which makes the code in the parsers
shorter and avoids any possibility of differences.

As with the previous commit, we'll pick an arbitrary atom to make sure
the test suite covers this code.

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 639b18ab36..271d619da9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -234,6 +234,12 @@ static int err_no_arg(struct strbuf *sb, const char *name)
 	return -1;
 }
 
+static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg)
+{
+	strbuf_addf(sb, _("unrecognized %%(%s) argument: %s"), name, arg);
+	return -1;
+}
+
 static int color_atom_parser(struct ref_format *format, struct used_atom *atom,
 			     const char *color_value, struct strbuf *err)
 {
@@ -347,7 +353,7 @@ static int objectsize_atom_parser(struct ref_format *format, struct used_atom *a
 		else
 			oi.info.disk_sizep = &oi.disk_size;
 	} else
-		return strbuf_addf_ret(err, -1, _("unrecognized %%(%s) argument: %s"), "objectsize", arg);
+		return err_bad_arg(err, "objectsize", arg);
 	return 0;
 }
 
@@ -380,7 +386,7 @@ static int subject_atom_parser(struct ref_format *format, struct used_atom *atom
 	else if (!strcmp(arg, "sanitize"))
 		atom->u.contents.option = C_SUB_SANITIZE;
 	else
-		return strbuf_addf_ret(err, -1, _("unrecognized %%(%s) argument: %s"), "subject", arg);
+		return err_bad_arg(err, "subject", arg);
 	return 0;
 }
 
@@ -434,7 +440,7 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato
 		if (strtoul_ui(arg, 10, &atom->u.contents.nlines))
 			return strbuf_addf_ret(err, -1, _("positive value expected contents:lines=%s"), arg);
 	} else
-		return strbuf_addf_ret(err, -1, _("unrecognized %%(%s) argument: %s"), "contents", arg);
+		return err_bad_arg(err, "contents", arg);
 	return 0;
 }
 
@@ -446,7 +452,7 @@ static int raw_atom_parser(struct ref_format *format, struct used_atom *atom,
 	else if (!strcmp(arg, "size"))
 		atom->u.raw_data.option = RAW_LENGTH;
 	else
-		return strbuf_addf_ret(err, -1, _("unrecognized %%(%s) argument: %s"), "raw", arg);
+		return err_bad_arg(err, "raw", arg);
 	return 0;
 }
 
@@ -563,7 +569,7 @@ static int if_atom_parser(struct ref_format *format, struct used_atom *atom,
 	} else if (skip_prefix(arg, "notequals=", &atom->u.if_then_else.str)) {
 		atom->u.if_then_else.cmp_status = COMPARE_UNEQUAL;
 	} else
-		return strbuf_addf_ret(err, -1, _("unrecognized %%(%s) argument: %s"), "if", arg);
+		return err_bad_arg(err, "if", arg);
 	return 0;
 }
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 8d99658ef8..010ba5a2cb 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -1248,6 +1248,12 @@ test_expect_success 'HEAD atom does not take arguments' '
 	test_cmp expect err
 '
 
+test_expect_success 'subject atom rejects unknown arguments' '
+	test_must_fail git for-each-ref --format="%(subject:foo)" 2>err &&
+	echo "fatal: unrecognized %(subject) argument: foo" >expect &&
+	test_cmp expect err
+'
+
 test_expect_success 'trailer parsing not fooled by --- line' '
 	git commit --allow-empty -F - <<-\EOF &&
 	this is the subject
-- 
2.39.0.550.g5015380a67

[PATCH 4/5] ref-filter: truncate atom names in error messages

[Jeff King]

If you pass a bogus argument to %(refname), you may end up with a
message like this:

  $ git for-each-ref --format='%(refname:foo)'
  fatal: unrecognized %(refname:foo) argument: foo

which is confusing. It should just say:

  fatal: unrecognized %(refname) argument: foo

which is clearer, and is consistent with most other atom parsers. Those
other parsers do not have the same problem because they pass the atom
name from a string literal in the parser function. But because the
parser for %(refname) also handles %(upstream) and %(push), it instead
uses atom->name, which includes the arguments. The oid atom parser which
handles %(tree), %(parent), etc suffers from the same problem.

It seems like the cleanest fix would be for atom->name to be _just_ the
name, since there's already a separate "args" field. But since that
field is also used for other things, we can't change it easily (e.g.,
it's how we find things in the used_atoms array, and clearly %(refname)
and %(refname:short) are not the same thing).

Instead, we'll teach our error_bad_arg() function to stop at the first
":". This is a little hacky, as we're effectively re-parsing the name,
but the format is simple enough to do this as a one-liner, and this
localizes the change to the error-reporting code.

We'll give the same treatment to err_no_arg(). None of its callers use
this atom->name trick, but it's worth future-proofing it while we're
here.

Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c            | 12 ++++++++----
 t/t6300-for-each-ref.sh |  6 ++++++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 271d619da9..f40bc4d9c9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -230,13 +230,17 @@ static int strbuf_addf_ret(struct strbuf *sb, int ret, const char *fmt, ...)
 
 static int err_no_arg(struct strbuf *sb, const char *name)
 {
-	strbuf_addf(sb, _("%%(%s) does not take arguments"), name);
+	size_t namelen = strchrnul(name, ':') - name;
+	strbuf_addf(sb, _("%%(%.*s) does not take arguments"),
+		    (int)namelen, name);
 	return -1;
 }
 
 static int err_bad_arg(struct strbuf *sb, const char *name, const char *arg)
 {
-	strbuf_addf(sb, _("unrecognized %%(%s) argument: %s"), name, arg);
+	size_t namelen = strchrnul(name, ':') - name;
+	strbuf_addf(sb, _("unrecognized %%(%.*s) argument: %s"),
+		    (int)namelen, name, arg);
 	return -1;
 }
 
@@ -274,7 +278,7 @@ static int refname_atom_parser_internal(struct refname_atom *atom, const char *a
 		if (strtol_i(arg, 10, &atom->rstrip))
 			return strbuf_addf_ret(err, -1, _("Integer value expected refname:rstrip=%s"), arg);
 	} else
-		return strbuf_addf_ret(err, -1, _("unrecognized %%(%s) argument: %s"), name, arg);
+		return err_bad_arg(err, name, arg);
 	return 0;
 }
 
@@ -471,7 +475,7 @@ static int oid_atom_parser(struct ref_format *format, struct used_atom *atom,
 		if (atom->u.oid.length < MINIMUM_ABBREV)
 			atom->u.oid.length = MINIMUM_ABBREV;
 	} else
-		return strbuf_addf_ret(err, -1, _("unrecognized %%(%s) argument: %s"), atom->name, arg);
+		return err_bad_arg(err, atom->name, arg);
 	return 0;
 }
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 010ba5a2cb..2ae1fc721b 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -1254,6 +1254,12 @@ test_expect_success 'subject atom rejects unknown arguments' '
 	test_cmp expect err
 '
 
+test_expect_success 'refname atom rejects unknown arguments' '
+	test_must_fail git for-each-ref --format="%(refname:foo)" 2>err &&
+	echo "fatal: unrecognized %(refname) argument: foo" >expect &&
+	test_cmp expect err
+'
+
 test_expect_success 'trailer parsing not fooled by --- line' '
 	git commit --allow-empty -F - <<-\EOF &&
 	this is the subject
-- 
2.39.0.550.g5015380a67

[PATCH 5/5] ref-filter: convert email atom parser to use err_bad_arg()

[Jeff King]

The error message for a bogus argument to %(authoremail), etc, is:

   $ git for-each-ref --format='%(authoremail:foo)'
   fatal: unrecognized email option: foo

Saying just "email" is a little vague; most of the other atom parsers
would use the full name "%(authoremail)", but we can't do that here
because the same function also handles %(taggeremail), etc. Until
recently, passing atom->name was a bad idea, because it erroneously
included the arguments in the atom name. But since the previous commit
taught err_bad_arg() to handle this, we can now do so and get:

  fatal: unrecognized %(authoremail) argument: foo

which is consistent with other atoms.

Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index f40bc4d9c9..733b0149e8 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -489,7 +489,7 @@ static int person_email_atom_parser(struct ref_format *format, struct used_atom
 	else if (!strcmp(arg, "localpart"))
 		atom->u.email_option.option = EO_LOCALPART;
 	else
-		return strbuf_addf_ret(err, -1, _("unrecognized email option: %s"), arg);
+		return err_bad_arg(err, atom->name, arg);
 	return 0;
 }
 
-- 
2.39.0.550.g5015380a67

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

[Taylor Blau]

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);
  }

Thanks,
Taylor

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

[Taylor Blau]

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

Re: [PATCH 4/5] ref-filter: truncate atom names in error messages

[Taylor Blau]

On Wed, Dec 14, 2022 at 11:23:53AM -0500, Jeff King wrote:
> It seems like the cleanest fix would be for atom->name to be _just_ the
> name, since there's already a separate "args" field. But since that
> field is also used for other things, we can't change it easily (e.g.,
> it's how we find things in the used_atoms array, and clearly %(refname)
> and %(refname:short) are not the same thing).
>
> Instead, we'll teach our error_bad_arg() function to stop at the first
> ":". This is a little hacky, as we're effectively re-parsing the name,
> but the format is simple enough to do this as a one-liner, and this
> localizes the change to the error-reporting code.
>
> We'll give the same treatment to err_no_arg(). None of its callers use
> this atom->name trick, but it's worth future-proofing it while we're
> here.

For what it's worth, I think that this balance of a somewhat-hacky
implementation against a more significant and trickier refactoring is
well thought-out and the right decision, IMHO.

Thanks,
Taylor

Re: [PATCH 0/5] minor ref-filter error-reporting bug-fixes

[Taylor Blau]

On Wed, Dec 14, 2022 at 11:18:19AM -0500, Jeff King wrote:
> This series fixes a few small inconsistencies in the error-reporting
> from ref-filter's atom parsers. Mostly I noticed this while dealing with
> unused parameters in one of the parsers, which led to noticing a few
> other small bugs. And I hope the result is a little cleaner, as it
> should reduce the number of strings needing translation.
>
>   [1/5]: ref-filter: reject arguments to %(HEAD)
>   [2/5]: ref-filter: factor out "%(foo) does not take arguments" errors
>   [3/5]: ref-filter: factor out "unrecognized %(foo) arg" errors
>   [4/5]: ref-filter: truncate atom names in error messages
>   [5/5]: ref-filter: convert email atom parser to use err_bad_arg()

I gave this series a careful read and found it all very satisfying. The
implementation is straightforward and leaves us with several much
friendlier error messages in the ref-filter's atom parsing code.

I left one comment throughout, but it was clarified when I read through
the rest of the series. So this one looks ready to pick up to my eyes.

Thanks,
Taylor

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

[Jeff King]

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

Re: [PATCH 4/5] ref-filter: truncate atom names in error messages

[Jeff King]

On Wed, Dec 14, 2022 at 03:05:05PM -0500, Taylor Blau wrote:

> On Wed, Dec 14, 2022 at 11:23:53AM -0500, Jeff King wrote:
> > It seems like the cleanest fix would be for atom->name to be _just_ the
> > name, since there's already a separate "args" field. But since that
> > field is also used for other things, we can't change it easily (e.g.,
> > it's how we find things in the used_atoms array, and clearly %(refname)
> > and %(refname:short) are not the same thing).
> >
> > Instead, we'll teach our error_bad_arg() function to stop at the first
> > ":". This is a little hacky, as we're effectively re-parsing the name,
> > but the format is simple enough to do this as a one-liner, and this
> > localizes the change to the error-reporting code.
> >
> > We'll give the same treatment to err_no_arg(). None of its callers use
> > this atom->name trick, but it's worth future-proofing it while we're
> > here.
> 
> For what it's worth, I think that this balance of a somewhat-hacky
> implementation against a more significant and trickier refactoring is
> well thought-out and the right decision, IMHO.

By the way, I did try the other change, to make atom->name just contain
the name with no args. There are a bunch of pitfalls in
parse_ref_filter_atom(), including:

  - don't use atom_len; it's off-by-one when looking at "atom" and not
    "sp" when there's a "*" dereference

  - the "args" pointer in the struct actually points into the name
    string. I don't think anybody relies on that, but I'm not 100% sure
    because...

  - once you deal with that, then it segfaults mysteriously, because
    the numeric index between used_atom and the computed values gets out
    of sync. That's where I gave up.

Which isn't to say it isn't do-able, or it wouldn't even make the
ref-filter code cleaner overall if somebody did that refactoring. But it
seemed like too much for solving this one little problem.

Here's the patch where I stopped, for posterity:

diff --git a/ref-filter.c b/ref-filter.c
index caf10ab23e..3a2a7d0271 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -716,7 +716,7 @@ static int parse_ref_filter_atom(struct ref_format *format,
 	used_atom_cnt++;
 	REALLOC_ARRAY(used_atom, used_atom_cnt);
 	used_atom[at].atom_type = i;
-	used_atom[at].name = xmemdupz(atom, ep - atom);
+	used_atom[at].name = xmemdupz(atom, (arg ? arg : ep) - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
 	used_atom[at].source = valid_atom[i].source;
 	if (used_atom[at].source == SOURCE_OBJ) {
@@ -726,8 +726,10 @@ static int parse_ref_filter_atom(struct ref_format *format,
 			oi.info.contentp = &oi.content;
 	}
 	if (arg) {
-		arg = used_atom[at].name + (arg - atom) + 1;
-		if (!*arg) {
+		arg++; /* skip ':' */
+		if (arg < ep) {
+			arg = xmemdupz(arg, ep - arg);
+		} else {
 			/*
 			 * Treat empty sub-arguments list as NULL (i.e.,
 			 * "%(atom:)" is equivalent to "%(atom)").

Its relative shortness does not represent the great confusion I had in
producing it.

-Peff