From afc1a946b256a54ba4abf530f0a720393aad461e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 14 Dec 2022 11:18:49 -0500 Subject: [PATCH 1/5] ref-filter: reject arguments to %(HEAD) 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 Acked-by: Taylor Blau Signed-off-by: Junio C Hamano --- 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; } From a33d0fae76ab95e88d383793cac41934920296ba Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 14 Dec 2022 11:19:43 -0500 Subject: [PATCH 2/5] ref-filter: factor out "%(foo) does not take arguments" errors 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 Acked-by: Taylor Blau Signed-off-by: Junio C Hamano --- 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 From dda4fc1a849e2407c30b4619d64221c2b38bd570 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 14 Dec 2022 11:20:19 -0500 Subject: [PATCH 3/5] ref-filter: factor out "unrecognized %(foo) arg" errors 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 Acked-by: Taylor Blau Signed-off-by: Junio C Hamano --- 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 From 1955ef10edf3c888dcb237728c81dd6e81df4960 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 14 Dec 2022 11:23:53 -0500 Subject: [PATCH 4/5] ref-filter: truncate atom names in error messages 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 Acked-by: Taylor Blau Signed-off-by: Junio C Hamano --- 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 From 285da4321a9351d4c99dd1685a4aa2dfb47ff62e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 14 Dec 2022 11:24:03 -0500 Subject: [PATCH 5/5] ref-filter: convert email atom parser to use err_bad_arg() 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 Acked-by: Taylor Blau Signed-off-by: Junio C Hamano --- 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; }