{"thread":{"id":"54068","subject":"[PATCH 1/2] t6300: unify %(trailers) and %(contents:trailers) tests","startedAt":"2020-08-19T12:52:46Z","lastAt":"2020-08-26T20:48:55Z","messageCount":31,"participants":["Hariom Verma via GitGitGadget","Junio C Hamano","Eric Sunshine","Hariom verma","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"404003","messageId":"bd0bb8d0ef0936866c2a957e5391424a7481a33c.1597841551.git.gitgitgadget@gmail.com","threadId":"54068","inReplyTo":"pull.707.git.1597841551.gitgitgadget@gmail.com","subject":"[PATCH 1/2] t6300: unify %(trailers) and %(contents:trailers) tests","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-19T12:52:30Z","receivedAt":"2020-08-19T12:52:46Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nCurrently, there are different tests for testing %(trailers) and\n%(contents:trailers) causing redundant copy.\n\nIts time to get rid of duplicate code.\n\nMentored-by: Christian Couder \nMentored-by: Heba Waly \nSigned-off-by: Hariom Verma \n---\n t/t6300-for-each-ref.sh | 50 +++++++++--------------------------------\n 1 file changed, 11 insertions(+), 39 deletions(-)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex a83579fbdf..495848c881 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -776,60 +776,39 @@ test_expect_success 'set up trailers for next test' '\n '\n \n test_expect_success '%(trailers:unfold) unfolds trailers' '\n-\tgit for-each-ref --format=\"%(trailers:unfold)\" refs/heads/master >actual &&\n \t{\n \t\tunfold expect &&\n+\tgit for-each-ref --format=\"%(trailers:unfold)\" refs/heads/master >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:unfold)\" refs/heads/master >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success '%(trailers:only) shows only \"key: value\" trailers' '\n-\tgit for-each-ref --format=\"%(trailers:only)\" refs/heads/master >actual &&\n \t{\n \t\tgrep -v patch.description expect &&\n+\tgit for-each-ref --format=\"%(trailers:only)\" refs/heads/master >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:only)\" refs/heads/master >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '\n-\tgit for-each-ref --format=\"%(trailers:only,unfold)\" refs/heads/master >actual &&\n-\tgit for-each-ref --format=\"%(trailers:unfold,only)\" refs/heads/master >reverse &&\n-\ttest_cmp actual reverse &&\n \t{\n \t\tgrep -v patch.description expect &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success '%(contents:trailers:unfold) unfolds trailers' '\n-\tgit for-each-ref --format=\"%(contents:trailers:unfold)\" refs/heads/master >actual &&\n-\t{\n-\t\tunfold expect &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success '%(contents:trailers:only) shows only \"key: value\" trailers' '\n-\tgit for-each-ref --format=\"%(contents:trailers:only)\" refs/heads/master >actual &&\n-\t{\n-\t\tgrep -v patch.description expect &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' '\n+\tgit for-each-ref --format=\"%(trailers:only,unfold)\" refs/heads/master >actual &&\n+\tgit for-each-ref --format=\"%(trailers:unfold,only)\" refs/heads/master >reverse &&\n+\ttest_cmp actual reverse &&\n+\ttest_cmp expect actual &&\n \tgit for-each-ref --format=\"%(contents:trailers:only,unfold)\" refs/heads/master >actual &&\n \tgit for-each-ref --format=\"%(contents:trailers:unfold,only)\" refs/heads/master >reverse &&\n \ttest_cmp actual reverse &&\n-\t{\n-\t\tgrep -v patch.description expect &&\n \ttest_cmp expect actual\n '\n \n@@ -839,14 +818,7 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '\n \tfatal: unknown %(trailers) argument: unsupported\n \tEOF\n \ttest_must_fail git for-each-ref --format=\"%(trailers:unsupported)\" 2>actual &&\n-\ttest_i18ncmp expect actual\n-'\n-\n-test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '\n-\t# error message cannot be checked under i18n\n-\tcat >expect <<-EOF &&\n-\tfatal: unknown %(trailers) argument: unsupported\n-\tEOF\n+\ttest_i18ncmp expect actual &&\n \ttest_must_fail git for-each-ref --format=\"%(contents:trailers:unsupported)\" 2>actual &&\n \ttest_i18ncmp expect actual\n '\n-- \ngitgitgadget\n\n"},{"id":"404004","messageId":"pull.707.git.1597841551.gitgitgadget@gmail.com","threadId":"54068","inReplyTo":null,"subject":"[PATCH 0/2] Fix trailers atom bug and improved tests","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-19T12:52:29Z","receivedAt":"2020-08-19T12:52:48Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"Currently, there exists a bug in 'contents' atom. It does not show any error\nif used with modifier 'trailers' and semicolon is missing before trailers\narguments. This small patch series is focused on fixing that bug and also\nunified 'trailers' and 'contents:trailers' tests. Thus, removed duplicate\ncode from t6300 and made tests more compact.\n\nHariom Verma (2):\n t6300: unify %(trailers) and %(contents:trailers) tests\n ref-filter: 'contents:trailers' show error if `:` is missing\n\n ref-filter.c | 21 +++++++++++++++---\n t/t6300-for-each-ref.sh | 49 +++++++++++++----------------------------\n 2 files changed, 33 insertions(+), 37 deletions(-)\n\n\nbase-commit: 2befe97201e1f3175cce557866c5822793624b5a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-707%2Fharry-hov%2Ffix-trailers-atom-bug-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-707/harry-hov/fix-trailers-atom-bug-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/707\n-- \ngitgitgadget\n"},{"id":"404005","messageId":"7daf9335a501b99c29e299f72823fcb7e549e748.1597841551.git.gitgitgadget@gmail.com","threadId":"54068","inReplyTo":"pull.707.git.1597841551.gitgitgadget@gmail.com","subject":"[PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-19T12:52:31Z","receivedAt":"2020-08-19T12:52:54Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nThe 'contents' atom does not show any error if used with 'trailers'\natom and semicolon is missing before trailers arguments.\n\ne.g %(contents:trailersonly) works, while it shouldn't.\n\nIt is definitely not an expected behavior.\n\nLet's fix this bug.\n\nMentored-by: Christian Couder \nMentored-by: Heba Waly \nSigned-off-by: Hariom Verma \n---\n ref-filter.c | 21 ++++++++++++++++++---\n t/t6300-for-each-ref.sh | 9 +++++++++\n 2 files changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ba85869755..dc31fbbe51 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -332,6 +332,22 @@ static int trailers_atom_parser(const struct ref_format *format, struct used_ato\n \treturn 0;\n }\n \n+static int check_format_field(const char *arg, const char *field, const char **option)\n+{\n+\tconst char *opt;\n+\tif (skip_prefix(arg, field, &opt)) {\n+\t\tif (*opt == '\\0') {\n+\t\t\t*option = NULL;\n+\t\t\treturn 1;\n+\t\t}\n+\t\telse if (*opt == ':') {\n+\t\t\t*option = ++opt;\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,\n \t\t\t\tconst char *arg, struct strbuf *err)\n {\n@@ -345,9 +361,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato\n \t\tatom->u.contents.option = C_SIG;\n \telse if (!strcmp(arg, \"subject\"))\n \t\tatom->u.contents.option = C_SUB;\n-\telse if (skip_prefix(arg, \"trailers\", &arg)) {\n-\t\tskip_prefix(arg, \":\", &arg);\n-\t\tif (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))\n+\telse if (check_format_field(arg, \"trailers\", &arg)) {\n+\t\tif (trailers_atom_parser(format, atom, arg, err))\n \t\t\treturn -1;\n \t} else if (skip_prefix(arg, \"lines=\", &arg)) {\n \t\tatom->u.contents.option = C_LINES;\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 495848c881..cb1508cef5 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -823,6 +823,15 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '\n+\t# error message cannot be checked under i18n\n+\tcat >expect <<-EOF &&\n+\tfatal: unrecognized %(contents) argument: trailersonly\n+\tEOF\n+\ttest_must_fail git for-each-ref --format=\"%(contents:trailersonly)\" 2>actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n test_expect_success 'basic atom: head contents:trailers' '\n \tgit for-each-ref --format=\"%(contents:trailers)\" refs/heads/master >actual &&\n \tsanitize_pgp actual.clean &&\n-- \ngitgitgadget\n"},{"id":"404044","messageId":"xmqq1rk2v8y5.fsf@gitster.c.googlers.com","threadId":"54068","inReplyTo":"bd0bb8d0ef0936866c2a957e5391424a7481a33c.1597841551.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] t6300: unify %(trailers) and %(contents:trailers) tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-19T17:31:30Z","receivedAt":"2020-08-19T17:31:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Hariom Verma via GitGitGadget\" writes:\n\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index a83579fbdf..495848c881 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -776,60 +776,39 @@ test_expect_success 'set up trailers for next test' '\n> '\n> \n> test_expect_success '%(trailers:unfold) unfolds trailers' '\n> -\tgit for-each-ref --format=\"%(trailers:unfold)\" refs/heads/master >actual &&\n> \t{\n> \t\tunfold \t\techo\n> \t} >expect &&\n> +\tgit for-each-ref --format=\"%(trailers:unfold)\" refs/heads/master >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit for-each-ref --format=\"%(contents:trailers:unfold)\" refs/heads/master >actual &&\n> \ttest_cmp expect actual\n> '\n\nHmph, what is this one doing? Ah, OK, trailers:unfold is tested as\nbefore (just the steps to prepare 'expect' and 'actual' got swapped),\nand because the same expectation holds for contents:trailers:unfold,\nwe can test it at the same. Makes sense.\n\n> test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '\n> -\tgit for-each-ref --format=\"%(trailers:only,unfold)\" refs/heads/master >actual &&\n> -\tgit for-each-ref --format=\"%(trailers:unfold,only)\" refs/heads/master >reverse &&\n> -\ttest_cmp actual reverse &&\n> \t{\n> \t\tgrep -v patch.description \t\techo\n> \t} >expect &&\n> +\tgit for-each-ref --format=\"%(trailers:only,unfold)\" refs/heads/master >actual &&\n> +\tgit for-each-ref --format=\"%(trailers:unfold,only)\" refs/heads/master >reverse &&\n> +\ttest_cmp actual reverse &&\n> +\ttest_cmp expect actual &&\n\nThis uses different pattern. It may be cleaner to test one side at\na time, as we have prepared the 'expect' that should be the same for\nboth, and compare with the expected pattern one at a time; that would\neliminate the need for 'reverse', too. I.e.\n\n\t{\n\t\tgrep -v patch.description trailers | unfold && echo\n\t} >expect &&\n\tgit for-each-ref ... only,unfold ... >actual &&\n\ttest_cmp expect actual &&\n\tgit for-each-ref ... unfold,only ... >actual &&\n\ttest_cmp expect actual &&\n\n> @@ -839,14 +818,7 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '\n> \tfatal: unknown %(trailers) argument: unsupported\n> \tEOF\n> \ttest_must_fail git for-each-ref --format=\"%(trailers:unsupported)\" 2>actual &&\n> -\ttest_i18ncmp expect actual\n> -'\n> -\n> -test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '\n> -\t# error message cannot be checked under i18n\n> -\tcat >expect <<-EOF &&\n> -\tfatal: unknown %(trailers) argument: unsupported\n> -\tEOF\n> +\ttest_i18ncmp expect actual &&\n> \ttest_must_fail git for-each-ref --format=\"%(contents:trailers:unsupported)\" 2>actual &&\n> \ttest_i18ncmp expect actual\n> '\n\nDoesn't this highlight a small bug, where an end-user request for an\nunknown %(contents:trailers:unsupported) is flagged as an error\nabout %(trailers)? Is it OK because we expect that users who use\nthe longer %(contents:trailers) to know that it is a synonym for\n%(trailers) and the latter is the official way to write it?\n\nThanks.\n"},{"id":"404051","messageId":"xmqqv9hettag.fsf@gitster.c.googlers.com","threadId":"54068","inReplyTo":"7daf9335a501b99c29e299f72823fcb7e549e748.1597841551.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-19T17:55:03Z","receivedAt":"2020-08-19T17:55:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Hariom Verma via GitGitGadget\" writes:\n\n> From: Hariom Verma \n>\n> The 'contents' atom does not show any error if used with 'trailers'\n> atom and semicolon is missing before trailers arguments.\n>\n> e.g %(contents:trailersonly) works, while it shouldn't.\n>\n> It is definitely not an expected behavior.\n>\n> Let's fix this bug.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Heba Waly \n> Signed-off-by: Hariom Verma \n> ---\n\nNice spotting. 7a5edbdb (ref-filter.c: parse trailers arguments\nwith %(contents) atom, 2017-10-01) talks about being deliberate\nabout the case where skip_prefix(\":\") does not find a colon after\nthe \"trailers\" token, but from the message it is clear that it\nexpected that the case happens only when \"trailers\" is at the end of\nthe string.\n\nThe new helper that is overly verbose and may be overkill.\n\nShouldn't this be clear enough, equivalent and sufficient?\n\n\telse if (skip_prefix(arg, \"trailers\", &arg) &&\n\t\t (!*arg || *arg == ':'))) {\n\t\tif (trailers_atom_parser(...);\n\nThat is, we not just make sure the string begins with \"trailers\",\nbut also make sure it either (1) ends the string (i.e. the token is\njust \"trailers\"), or (2) is followed by a colon ':', before entering\nthe block to handle \"trailers[:anything]\". If we later add a new\natom \"trailersonly\", that will not be handled here, but elsewhere in\nthe \"else if\" cascade.\n\n> ref-filter.c | 21 ++++++++++++++++++---\n> t/t6300-for-each-ref.sh | 9 +++++++++\n> 2 files changed, 27 insertions(+), 3 deletions(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index ba85869755..dc31fbbe51 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -332,6 +332,22 @@ static int trailers_atom_parser(const struct ref_format *format, struct used_ato\n> \treturn 0;\n> }\n> \n> +static int check_format_field(const char *arg, const char *field, const char **option)\n> +{\n> +\tconst char *opt;\n> +\tif (skip_prefix(arg, field, &opt)) {\n> +\t\tif (*opt == '\\0') {\n> +\t\t\t*option = NULL;\n> +\t\t\treturn 1;\n> +\t\t}\n> +\t\telse if (*opt == ':') {\n> +\t\t\t*option = ++opt;\n> +\t\t\treturn 1;\n> +\t\t}\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,\n> \t\t\t\tconst char *arg, struct strbuf *err)\n> {\n> @@ -345,9 +361,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato\n> \t\tatom->u.contents.option = C_SIG;\n> \telse if (!strcmp(arg, \"subject\"))\n> \t\tatom->u.contents.option = C_SUB;\n> -\telse if (skip_prefix(arg, \"trailers\", &arg)) {\n> -\t\tskip_prefix(arg, \":\", &arg);\n> -\t\tif (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))\n> +\telse if (check_format_field(arg, \"trailers\", &arg)) {\n> +\t\tif (trailers_atom_parser(format, atom, arg, err))\n> \t\t\treturn -1;\n> \t} else if (skip_prefix(arg, \"lines=\", &arg)) {\n> \t\tatom->u.contents.option = C_LINES;\n"},{"id":"404055","messageId":"xmqqmu2qtpxp.fsf@gitster.c.googlers.com","threadId":"54068","inReplyTo":"xmqqv9hettag.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-19T19:07:30Z","receivedAt":"2020-08-19T19:07:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Hariom Verma via GitGitGadget\" writes:\n>\n>> From: Hariom Verma \n>>\n>> The 'contents' atom does not show any error if used with 'trailers'\n>> atom and semicolon is missing before trailers arguments.\n>>\n>> e.g %(contents:trailersonly) works, while it shouldn't.\n>>\n>> It is definitely not an expected behavior.\n>>\n>> Let's fix this bug.\n>>\n>> Mentored-by: Christian Couder \n>> Mentored-by: Heba Waly \n>> Signed-off-by: Hariom Verma \n>> ---\n>\n> Nice spotting. 7a5edbdb (ref-filter.c: parse trailers arguments\n> with %(contents) atom, 2017-10-01) talks about being deliberate\n> about the case where skip_prefix(\":\") does not find a colon after\n> the \"trailers\" token, but from the message it is clear that it\n> expected that the case happens only when \"trailers\" is at the end of\n> the string.\n>\n> The new helper that is overly verbose and may be overkill.\n>\n> Shouldn't this be clear enough, equivalent and sufficient?\n>\n> \telse if (skip_prefix(arg, \"trailers\", &arg) &&\n> \t\t (!*arg || *arg == ':'))) {\n> \t\tif (trailers_atom_parser(...);\n\nAh, no, even with \"*arg++ == ':'. This moves arg past \"trailers\" if\ngiven \"trailersandsomegarbage\" and the next one in \"else if\" cascade\nwould look at \"andsomegarbage\"---which is not what we want.\n\n>> +static int check_format_field(const char *arg, const char *field, const char **option)\n>> +{\n>> +\tconst char *opt;\n>> +\tif (skip_prefix(arg, field, &opt)) {\n>> +\t\tif (*opt == '\\0') {\n>> +\t\t\t*option = NULL;\n>> +\t\t\treturn 1;\n>> +\t\t}\n>> +\t\telse if (*opt == ':') {\n>> +\t\t\t*option = ++opt;\n>> +\t\t\treturn 1;\n>> +\t\t}\n>> +\t}\n>> +\treturn 0;\n>> +}\n\nAnd the helper does not have such a breakage. It looks good.\n\nThanks.\n"},{"id":"404060","messageId":"CAPig+cS398dm4W5Q2DnK+bGvw0mOG3916dHPbZ=y1JNrqz1G-w@mail.gmail.com","threadId":"54068","inReplyTo":"xmqqmu2qtpxp.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-19T19:39:07Z","receivedAt":"2020-08-19T19:39:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 19, 2020 at 3:07 PM Junio C Hamano wrote:\n> Junio C Hamano writes:\n> > \"Hariom Verma via GitGitGadget\" writes:\n> >> +static int check_format_field(const char *arg, const char *field, const char **option)\n> >> +{\n> >> + else if (*opt == ':') {\n> >> + *option = ++opt;\n> >> + return 1;\n> >> + }\n>\n> And the helper does not have such a breakage. It looks good.\n\nOne minor comment (not worth a re-roll): I personally found:\n\n *option = ++opt;\n\nmore confusing than:\n\n *option = opt + 1;\n\nThe `++opt` places a higher cognitive load on the reader. As a\nreviewer, I had to go back and carefully reread the function to see if\nthe side-effect of `++opt` had some impact which I didn't notice on\nthe first readthrough. The simpler `opt + 1` does not have a\nside-effect, thus is easier to reason about (and doesn't require me to\nre-study the function when I encounter it).\n"},{"id":"404071","messageId":"xmqqsgcis2zc.fsf@gitster.c.googlers.com","threadId":"54068","inReplyTo":"CAPig+cS398dm4W5Q2DnK+bGvw0mOG3916dHPbZ=y1JNrqz1G-w@mail.gmail.com","subject":"Re: [PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-19T22:08:39Z","receivedAt":"2020-08-19T22:08:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Wed, Aug 19, 2020 at 3:07 PM Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>> > \"Hariom Verma via GitGitGadget\" writes:\n>> >> +static int check_format_field(const char *arg, const char *field, const char **option)\n>> >> +{\n>> >> + else if (*opt == ':') {\n>> >> + *option = ++opt;\n>> >> + return 1;\n>> >> + }\n>>\n>> And the helper does not have such a breakage. It looks good.\n>\n> One minor comment (not worth a re-roll): I personally found:\n>\n> *option = ++opt;\n>\n> more confusing than:\n>\n> *option = opt + 1;\n>\n> The `++opt` places a higher cognitive load on the reader. As a\n> reviewer, I had to go back and carefully reread the function to see if\n> the side-effect of `++opt` had some impact which I didn't notice on\n> the first readthrough. The simpler `opt + 1` does not have a\n> side-effect, thus is easier to reason about (and doesn't require me to\n> re-study the function when I encounter it).\n\nThat makes the two of us ... thanks.\n"},{"id":"404102","messageId":"CA+CkUQ9qz1=xvpdtTy49W5Uru3ONJ5R1zUCSdi7OXJCnZxTdzA@mail.gmail.com","threadId":"54068","inReplyTo":"xmqqsgcis2zc.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2020-08-20T17:19:13Z","receivedAt":"2020-08-20T17:19:32Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi,\n\nOn Thu, Aug 20, 2020 at 3:38 AM Junio C Hamano wrote:\n>\n> Eric Sunshine writes:\n>\n> > On Wed, Aug 19, 2020 at 3:07 PM Junio C Hamano wrote:\n> >> Junio C Hamano writes:\n> >> > \"Hariom Verma via GitGitGadget\" writes:\n> >> >> +static int check_format_field(const char *arg, const char *field, const char **option)\n> >> >> +{\n> >> >> + else if (*opt == ':') {\n> >> >> + *option = ++opt;\n> >> >> + return 1;\n> >> >> + }\n> >>\n> >> And the helper does not have such a breakage. It looks good.\n> >\n> > One minor comment (not worth a re-roll): I personally found:\n> >\n> > *option = ++opt;\n> >\n> > more confusing than:\n> >\n> > *option = opt + 1;\n> >\n> > The `++opt` places a higher cognitive load on the reader. As a\n> > reviewer, I had to go back and carefully reread the function to see if\n> > the side-effect of `++opt` had some impact which I didn't notice on\n> > the first readthrough. The simpler `opt + 1` does not have a\n> > side-effect, thus is easier to reason about (and doesn't require me to\n> > re-study the function when I encounter it).\n>\n> That makes the two of us ... thanks.\n\nIt seems like the score is 2-0.\nI guess I'm going with winning side.\n\nWill be improved in next version.\n\nThanks,\nHariom\n"},{"id":"404125","messageId":"CA+CkUQ_z8RL=g32aWm5bx6+-W8SHBUxaOd8tWfxa7wfEWiNQJA@mail.gmail.com","threadId":"54068","inReplyTo":"xmqq1rk2v8y5.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] t6300: unify %(trailers) and %(contents:trailers) tests","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2020-08-21T10:03:13Z","receivedAt":"2020-08-21T10:03:30Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi,\n\nOn Wed, Aug 19, 2020 at 11:01 PM Junio C Hamano wrote:\n>\n> \"Hariom Verma via GitGitGadget\" writes:\n>\n> > @@ -839,14 +818,7 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '\n> > fatal: unknown %(trailers) argument: unsupported\n> > EOF\n> > test_must_fail git for-each-ref --format=\"%(trailers:unsupported)\" 2>actual &&\n> > - test_i18ncmp expect actual\n> > -'\n> > -\n> > -test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '\n> > - # error message cannot be checked under i18n\n> > - cat >expect <<-EOF &&\n> > - fatal: unknown %(trailers) argument: unsupported\n> > - EOF\n> > + test_i18ncmp expect actual &&\n> > test_must_fail git for-each-ref --format=\"%(contents:trailers:unsupported)\" 2>actual &&\n> > test_i18ncmp expect actual\n> > '\n>\n> Doesn't this highlight a small bug, where an end-user request for an\n> unknown %(contents:trailers:unsupported) is flagged as an error\n> about %(trailers)? Is it OK because we expect that users who use\n> the longer %(contents:trailers) to know that it is a synonym for\n> %(trailers) and the latter is the official way to write it?\n\nMaybe.\n\nAnother way of thinking is...\n'trailers' is an argument to 'contents', likewise here 'unsupported'\nis an argument to trailers.\nTechnically, the error message is correct.\n\nAgain, I think views on this are highly subjective.\n\nThanks,\nHariom\n"},{"id":"404126","messageId":"pull.707.v2.git.1598004663.gitgitgadget@gmail.com","threadId":"54068","inReplyTo":"pull.707.git.1597841551.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] Fix trailers atom bug and improved tests","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-21T10:11:00Z","receivedAt":"2020-08-21T10:11:13Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"Currently, there exists a bug in 'contents' atom. It does not show any error\nif used with modifier 'trailers' and semicolon is missing before trailers\narguments. This small patch series is focused on fixing that bug and also\nunified 'trailers' and 'contents:trailers' tests. Thus, removed duplicate\ncode from t6300 and made tests more compact.\n\nHariom Verma (2):\n t6300: unify %(trailers) and %(contents:trailers) tests\n ref-filter: 'contents:trailers' show error if `:` is missing\n\n ref-filter.c | 21 +++++++++++++---\n t/t6300-for-each-ref.sh | 55 ++++++++++++++---------------------------\n 2 files changed, 36 insertions(+), 40 deletions(-)\n\n\nbase-commit: 675a4aaf3b226c0089108221b96559e0baae5de9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-707%2Fharry-hov%2Ffix-trailers-atom-bug-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-707/harry-hov/fix-trailers-atom-bug-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/707\n\nRange-diff vs v1:\n\n 1: bd0bb8d0ef ! 1: 4816aa3cfa t6300: unify %(trailers) and %(contents:trailers) tests\n @@ t/t6300-for-each-ref.sh: test_expect_success 'set up trailers for next test' '\n -\n -test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' '\n +\tgit for-each-ref --format=\"%(trailers:only,unfold)\" refs/heads/master >actual &&\n -+\tgit for-each-ref --format=\"%(trailers:unfold,only)\" refs/heads/master >reverse &&\n -+\ttest_cmp actual reverse &&\n +\ttest_cmp expect actual &&\n ++\tgit for-each-ref --format=\"%(trailers:unfold,only)\" refs/heads/master >actual &&\n ++\ttest_cmp actual actual &&\n \tgit for-each-ref --format=\"%(contents:trailers:only,unfold)\" refs/heads/master >actual &&\n - \tgit for-each-ref --format=\"%(contents:trailers:unfold,only)\" refs/heads/master >reverse &&\n - \ttest_cmp actual reverse &&\n +-\tgit for-each-ref --format=\"%(contents:trailers:unfold,only)\" refs/heads/master >reverse &&\n +-\ttest_cmp actual reverse &&\n -\t{\n -\t\tgrep -v patch.description expect &&\n - \ttest_cmp expect actual\n +-\ttest_cmp expect actual\n ++\ttest_cmp expect actual &&\n ++\tgit for-each-ref --format=\"%(contents:trailers:unfold,only)\" refs/heads/master >actual &&\n ++\ttest_cmp actual actual\n '\n \n + test_expect_success '%(trailers) rejects unknown trailers arguments' '\n @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers) rejects unknown trailers arguments' '\n \tfatal: unknown %(trailers) argument: unsupported\n \tEOF\n 2: 7daf9335a5 ! 2: 39aa46bce7 ref-filter: 'contents:trailers' show error if `:` is missing\n @@ ref-filter.c: static int trailers_atom_parser(const struct ref_format *format, s\n +\t\t\treturn 1;\n +\t\t}\n +\t\telse if (*opt == ':') {\n -+\t\t\t*option = ++opt;\n ++\t\t\t*option = opt + 1;\n +\t\t\treturn 1;\n +\t\t}\n +\t}\n\n-- \ngitgitgadget\n"},{"id":"404127","messageId":"4816aa3cfa04093c3b6c845eb914817e97b126b0.1598004663.git.gitgitgadget@gmail.com","threadId":"54068","inReplyTo":"pull.707.v2.git.1598004663.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] t6300: unify %(trailers) and %(contents:trailers) tests","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-21T10:11:01Z","receivedAt":"2020-08-21T10:11:14Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nCurrently, there are different tests for testing %(trailers) and\n%(contents:trailers) causing redundant copy.\n\nIts time to get rid of duplicate code.\n\nMentored-by: Christian Couder \nMentored-by: Heba Waly \nSigned-off-by: Hariom Verma \n---\n t/t6300-for-each-ref.sh | 56 +++++++++++------------------------------\n 1 file changed, 14 insertions(+), 42 deletions(-)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex a83579fbdf..0570380344 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -776,61 +776,40 @@ test_expect_success 'set up trailers for next test' '\n '\n \n test_expect_success '%(trailers:unfold) unfolds trailers' '\n-\tgit for-each-ref --format=\"%(trailers:unfold)\" refs/heads/master >actual &&\n \t{\n \t\tunfold expect &&\n+\tgit for-each-ref --format=\"%(trailers:unfold)\" refs/heads/master >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:unfold)\" refs/heads/master >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success '%(trailers:only) shows only \"key: value\" trailers' '\n-\tgit for-each-ref --format=\"%(trailers:only)\" refs/heads/master >actual &&\n \t{\n \t\tgrep -v patch.description expect &&\n+\tgit for-each-ref --format=\"%(trailers:only)\" refs/heads/master >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:only)\" refs/heads/master >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '\n-\tgit for-each-ref --format=\"%(trailers:only,unfold)\" refs/heads/master >actual &&\n-\tgit for-each-ref --format=\"%(trailers:unfold,only)\" refs/heads/master >reverse &&\n-\ttest_cmp actual reverse &&\n \t{\n \t\tgrep -v patch.description expect &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success '%(contents:trailers:unfold) unfolds trailers' '\n-\tgit for-each-ref --format=\"%(contents:trailers:unfold)\" refs/heads/master >actual &&\n-\t{\n-\t\tunfold expect &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success '%(contents:trailers:only) shows only \"key: value\" trailers' '\n-\tgit for-each-ref --format=\"%(contents:trailers:only)\" refs/heads/master >actual &&\n-\t{\n-\t\tgrep -v patch.description expect &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' '\n+\tgit for-each-ref --format=\"%(trailers:only,unfold)\" refs/heads/master >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(trailers:unfold,only)\" refs/heads/master >actual &&\n+\ttest_cmp actual actual &&\n \tgit for-each-ref --format=\"%(contents:trailers:only,unfold)\" refs/heads/master >actual &&\n-\tgit for-each-ref --format=\"%(contents:trailers:unfold,only)\" refs/heads/master >reverse &&\n-\ttest_cmp actual reverse &&\n-\t{\n-\t\tgrep -v patch.description expect &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:unfold,only)\" refs/heads/master >actual &&\n+\ttest_cmp actual actual\n '\n \n test_expect_success '%(trailers) rejects unknown trailers arguments' '\n@@ -839,14 +818,7 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '\n \tfatal: unknown %(trailers) argument: unsupported\n \tEOF\n \ttest_must_fail git for-each-ref --format=\"%(trailers:unsupported)\" 2>actual &&\n-\ttest_i18ncmp expect actual\n-'\n-\n-test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '\n-\t# error message cannot be checked under i18n\n-\tcat >expect <<-EOF &&\n-\tfatal: unknown %(trailers) argument: unsupported\n-\tEOF\n+\ttest_i18ncmp expect actual &&\n \ttest_must_fail git for-each-ref --format=\"%(contents:trailers:unsupported)\" 2>actual &&\n \ttest_i18ncmp expect actual\n '\n-- \ngitgitgadget\n\n"},{"id":"404128","messageId":"39aa46bce700cc9a4ca49f38922e3a7ebf14a52c.1598004663.git.gitgitgadget@gmail.com","threadId":"54068","inReplyTo":"pull.707.v2.git.1598004663.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-21T10:11:02Z","receivedAt":"2020-08-21T10:11:14Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nThe 'contents' atom does not show any error if used with 'trailers'\natom and semicolon is missing before trailers arguments.\n\ne.g %(contents:trailersonly) works, while it shouldn't.\n\nIt is definitely not an expected behavior.\n\nLet's fix this bug.\n\nMentored-by: Christian Couder \nMentored-by: Heba Waly \nSigned-off-by: Hariom Verma \n---\n ref-filter.c | 21 ++++++++++++++++++---\n t/t6300-for-each-ref.sh | 9 +++++++++\n 2 files changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ba85869755..fa131c4854 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -332,6 +332,22 @@ static int trailers_atom_parser(const struct ref_format *format, struct used_ato\n \treturn 0;\n }\n \n+static int check_format_field(const char *arg, const char *field, const char **option)\n+{\n+\tconst char *opt;\n+\tif (skip_prefix(arg, field, &opt)) {\n+\t\tif (*opt == '\\0') {\n+\t\t\t*option = NULL;\n+\t\t\treturn 1;\n+\t\t}\n+\t\telse if (*opt == ':') {\n+\t\t\t*option = opt + 1;\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,\n \t\t\t\tconst char *arg, struct strbuf *err)\n {\n@@ -345,9 +361,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato\n \t\tatom->u.contents.option = C_SIG;\n \telse if (!strcmp(arg, \"subject\"))\n \t\tatom->u.contents.option = C_SUB;\n-\telse if (skip_prefix(arg, \"trailers\", &arg)) {\n-\t\tskip_prefix(arg, \":\", &arg);\n-\t\tif (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))\n+\telse if (check_format_field(arg, \"trailers\", &arg)) {\n+\t\tif (trailers_atom_parser(format, atom, arg, err))\n \t\t\treturn -1;\n \t} else if (skip_prefix(arg, \"lines=\", &arg)) {\n \t\tatom->u.contents.option = C_LINES;\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 0570380344..6d535653d9 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -823,6 +823,15 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '\n+\t# error message cannot be checked under i18n\n+\tcat >expect <<-EOF &&\n+\tfatal: unrecognized %(contents) argument: trailersonly\n+\tEOF\n+\ttest_must_fail git for-each-ref --format=\"%(contents:trailersonly)\" 2>actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n test_expect_success 'basic atom: head contents:trailers' '\n \tgit for-each-ref --format=\"%(contents:trailers)\" refs/heads/master >actual &&\n \tsanitize_pgp actual.clean &&\n-- \ngitgitgadget\n"},{"id":"404151","messageId":"CAPig+cRxCvHG70Nd00zBxYFuecu6+Z6uDP8ooN3rx9vPagoYBA@mail.gmail.com","threadId":"54068","inReplyTo":"39aa46bce700cc9a4ca49f38922e3a7ebf14a52c.1598004663.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-21T16:56:56Z","receivedAt":"2020-08-21T16:59:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 21, 2020 at 6:11 AM Hariom Verma via GitGitGadget\n wrote:\n> The 'contents' atom does not show any error if used with 'trailers'\n> atom and semicolon is missing before trailers arguments.\n\nDo you mean s/semicolon/colon/ ?\n\n> e.g %(contents:trailersonly) works, while it shouldn't.\n>\n> It is definitely not an expected behavior.\n>\n> Let's fix this bug.\n>\n> Signed-off-by: Hariom Verma \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -332,6 +332,22 @@ static int trailers_atom_parser(const struct ref_format *format, struct used_ato\n> +static int check_format_field(const char *arg, const char *field, const char **option)\n> +{\n> + const char *opt;\n> + if (skip_prefix(arg, field, &opt)) {\n> + if (*opt == '\\0') {\n> + *option = NULL;\n> + return 1;\n> + }\n> + else if (*opt == ':') {\n> + *option = opt + 1;\n> + return 1;\n> + }\n> + }\n> + return 0;\n> +}\n\nNot necessarily worth a re-roll, but rather than introducing all the\nabove new code...\n\n> @@ -345,9 +361,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato\n> - else if (skip_prefix(arg, \"trailers\", &arg)) {\n> - skip_prefix(arg, \":\", &arg);\n> - if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))\n> + else if (check_format_field(arg, \"trailers\", &arg)) {\n> + if (trailers_atom_parser(format, atom, arg, err))\n> return -1;\n\n...an alternative would have been something like:\n\n else if (!strcmp(arg, \"trailers\")) {\n if (trailers_atom_parser(format, atom, NULL, err))\n return -1;\n } else if (skip_prefix(arg, \"trailers:\", &arg)) {\n if (trailers_atom_parser(format, atom, arg, err))\n return -1;\n }\n\nwhich is quite simple to reason about (though has the cost of a tiny\nbit of duplication).\n\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> @@ -823,6 +823,15 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '\n> +test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '\n\ns/semicolon/colon/\n\n> + # error message cannot be checked under i18n\n\nWhat is this comment about? I realize that you copied it from other\nnearby tests, but I find that it muddies rather than clarifies.\n\n> + cat >expect <<-EOF &&\n> + fatal: unrecognized %(contents) argument: trailersonly\n> + EOF\n> + test_must_fail git for-each-ref --format=\"%(contents:trailersonly)\" 2>actual &&\n> + test_i18ncmp expect actual\n> +'\n"},{"id":"404194","messageId":"xmqqeenz95bj.fsf@gitster.c.googlers.com","threadId":"54068","inReplyTo":"CAPig+cRxCvHG70Nd00zBxYFuecu6+Z6uDP8ooN3rx9vPagoYBA@mail.gmail.com","subject":"Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-21T19:17:36Z","receivedAt":"2020-08-21T19:17:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> ...an alternative would have been something like:\n>\n> else if (!strcmp(arg, \"trailers\")) {\n> if (trailers_atom_parser(format, atom, NULL, err))\n> return -1;\n> } else if (skip_prefix(arg, \"trailers:\", &arg)) {\n> if (trailers_atom_parser(format, atom, arg, err))\n> return -1;\n> }\n>\n> which is quite simple to reason about (though has the cost of a tiny\n> bit of duplication).\n\nYeah, that looks quite simple and straight-forward.\n\n>> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n>> @@ -823,6 +823,15 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '\n>> +test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '\n>\n> s/semicolon/colon/\n\nDefinitely.\n\n>\n>> + # error message cannot be checked under i18n\n>\n> What is this comment about? I realize that you copied it from other\n> nearby tests, but I find that it muddies rather than clarifies.\n\nYup. If a patch changes test_cmp with test_i18ncmp, the above\nmessage belongs to its commit log message, but it is overkill to\nhave it as an in-line comment in every place where test_i18ncmp gets\nused.\n\nThanks for a review.\n\n>> + cat >expect <<-EOF &&\n>> + fatal: unrecognized %(contents) argument: trailersonly\n>> + EOF\n>> + test_must_fail git for-each-ref --format=\"%(contents:trailersonly)\" 2>actual &&\n>> + test_i18ncmp expect actual\n>> +'\n"},{"id":"404209","messageId":"383476b1778eb0d62c6cf013008388f72065beb0.1598043976.git.gitgitgadget@gmail.com","threadId":"54068","inReplyTo":"pull.707.v3.git.1598043976.gitgitgadget@gmail.com","subject":"[PATCH v3 1/4] t6300: unify %(trailers) and %(contents:trailers) tests","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-21T21:06:13Z","receivedAt":"2020-08-21T21:06:23Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nCurrently, there are different tests for testing %(trailers) and\n%(contents:trailers) causing redundant copy.\n\nIts time to get rid of duplicate code.\n\nMentored-by: Christian Couder \nMentored-by: Heba Waly \nSigned-off-by: Hariom Verma \n---\n t/t6300-for-each-ref.sh | 56 +++++++++++------------------------------\n 1 file changed, 14 insertions(+), 42 deletions(-)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex a83579fbdf..0570380344 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -776,61 +776,40 @@ test_expect_success 'set up trailers for next test' '\n '\n \n test_expect_success '%(trailers:unfold) unfolds trailers' '\n-\tgit for-each-ref --format=\"%(trailers:unfold)\" refs/heads/master >actual &&\n \t{\n \t\tunfold expect &&\n+\tgit for-each-ref --format=\"%(trailers:unfold)\" refs/heads/master >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:unfold)\" refs/heads/master >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success '%(trailers:only) shows only \"key: value\" trailers' '\n-\tgit for-each-ref --format=\"%(trailers:only)\" refs/heads/master >actual &&\n \t{\n \t\tgrep -v patch.description expect &&\n+\tgit for-each-ref --format=\"%(trailers:only)\" refs/heads/master >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:only)\" refs/heads/master >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '\n-\tgit for-each-ref --format=\"%(trailers:only,unfold)\" refs/heads/master >actual &&\n-\tgit for-each-ref --format=\"%(trailers:unfold,only)\" refs/heads/master >reverse &&\n-\ttest_cmp actual reverse &&\n \t{\n \t\tgrep -v patch.description expect &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success '%(contents:trailers:unfold) unfolds trailers' '\n-\tgit for-each-ref --format=\"%(contents:trailers:unfold)\" refs/heads/master >actual &&\n-\t{\n-\t\tunfold expect &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success '%(contents:trailers:only) shows only \"key: value\" trailers' '\n-\tgit for-each-ref --format=\"%(contents:trailers:only)\" refs/heads/master >actual &&\n-\t{\n-\t\tgrep -v patch.description expect &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' '\n+\tgit for-each-ref --format=\"%(trailers:only,unfold)\" refs/heads/master >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(trailers:unfold,only)\" refs/heads/master >actual &&\n+\ttest_cmp actual actual &&\n \tgit for-each-ref --format=\"%(contents:trailers:only,unfold)\" refs/heads/master >actual &&\n-\tgit for-each-ref --format=\"%(contents:trailers:unfold,only)\" refs/heads/master >reverse &&\n-\ttest_cmp actual reverse &&\n-\t{\n-\t\tgrep -v patch.description expect &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:unfold,only)\" refs/heads/master >actual &&\n+\ttest_cmp actual actual\n '\n \n test_expect_success '%(trailers) rejects unknown trailers arguments' '\n@@ -839,14 +818,7 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '\n \tfatal: unknown %(trailers) argument: unsupported\n \tEOF\n \ttest_must_fail git for-each-ref --format=\"%(trailers:unsupported)\" 2>actual &&\n-\ttest_i18ncmp expect actual\n-'\n-\n-test_expect_success '%(contents:trailers) rejects unknown trailers arguments' '\n-\t# error message cannot be checked under i18n\n-\tcat >expect <<-EOF &&\n-\tfatal: unknown %(trailers) argument: unsupported\n-\tEOF\n+\ttest_i18ncmp expect actual &&\n \ttest_must_fail git for-each-ref --format=\"%(contents:trailers:unsupported)\" 2>actual &&\n \ttest_i18ncmp expect actual\n '\n-- \ngitgitgadget\n\n"},{"id":"404210","messageId":"659b9835dcd0b38ac3972eb19c08c3bf26dccc80.1598043976.git.gitgitgadget@gmail.com","threadId":"54068","inReplyTo":"pull.707.v3.git.1598043976.gitgitgadget@gmail.com","subject":"[PATCH v3 2/4] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-21T21:06:14Z","receivedAt":"2020-08-21T21:06:26Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nThe 'contents' atom does not show any error if used with 'trailers'\natom and colon is missing before trailers arguments.\n\ne.g %(contents:trailersonly) works, while it shouldn't.\n\nIt is definitely not an expected behavior.\n\nLet's fix this bug.\n\nAcked-by: Eric Sunshine \nMentored-by: Christian Couder \nMentored-by: Heba Waly \nSigned-off-by: Hariom Verma \n---\n ref-filter.c | 8 +++++---\n t/t6300-for-each-ref.sh | 8 ++++++++\n 2 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ba85869755..8ba0e31915 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -345,9 +345,11 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato\n \t\tatom->u.contents.option = C_SIG;\n \telse if (!strcmp(arg, \"subject\"))\n \t\tatom->u.contents.option = C_SUB;\n-\telse if (skip_prefix(arg, \"trailers\", &arg)) {\n-\t\tskip_prefix(arg, \":\", &arg);\n-\t\tif (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))\n+\telse if (!strcmp(arg, \"trailers\")) {\n+\t\tif (trailers_atom_parser(format, atom, NULL, err))\n+\t\t\treturn -1;\n+\t} else if (skip_prefix(arg, \"trailers:\", &arg)) {\n+\t\tif (trailers_atom_parser(format, atom, arg, err))\n \t\t\treturn -1;\n \t} else if (skip_prefix(arg, \"lines=\", &arg)) {\n \t\tatom->u.contents.option = C_LINES;\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 0570380344..fdf2c442c5 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -823,6 +823,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '\n+\tcat >expect <<-EOF &&\n+\tfatal: unrecognized %(contents) argument: trailersonly\n+\tEOF\n+\ttest_must_fail git for-each-ref --format=\"%(contents:trailersonly)\" 2>actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n test_expect_success 'basic atom: head contents:trailers' '\n \tgit for-each-ref --format=\"%(contents:trailers)\" refs/heads/master >actual &&\n \tsanitize_pgp actual.clean &&\n-- \ngitgitgadget\n\n"},{"id":"404211","messageId":"pull.707.v3.git.1598043976.gitgitgadget@gmail.com","threadId":"54068","inReplyTo":"pull.707.v2.git.1598004663.gitgitgadget@gmail.com","subject":"[PATCH v3 0/4] [GSoC] Fix trailers atom bug and improved tests","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-21T21:06:12Z","receivedAt":"2020-08-21T21:06:26Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"Currently, there exists a bug in 'contents' atom. It does not show any error\nif used with modifier 'trailers' and semicolon is missing before trailers\narguments. This small patch series is focused on fixing that bug and also\nunified 'trailers' and 'contents:trailers' tests. Thus, removed duplicate\ncode from t6300 and made tests more compact.\n\nChange log since v2:\n\n * Used simplified logic as per suggested by Eric (here \n https://public-inbox.org/git/CAPig+cRxCvHG70Nd00zBxYFuecu6+Z6uDP8ooN3rx9vPagoYBA@mail.gmail.com/\n )\n * Unified trailer formatting logic for pretty.c and ref-filter.c\n\nHariom Verma (4):\n t6300: unify %(trailers) and %(contents:trailers) tests\n ref-filter: 'contents:trailers' show error if `:` is missing\n pretty.c: refactor trailer logic to `format_set_trailers_options()`\n ref-filter: using pretty.c logic for trailers\n\n Documentation/git-for-each-ref.txt | 36 ++++++--\n Hariom Verma via GitGitGadget | 0\n pretty.c | 83 +++++++++++-------\n pretty.h | 11 +++\n ref-filter.c | 43 +++++-----\n t/t6300-for-each-ref.sh | 133 ++++++++++++++++++++++-------\n 6 files changed, 219 insertions(+), 87 deletions(-)\n create mode 100644 Hariom Verma via GitGitGadget\n\n\nbase-commit: 675a4aaf3b226c0089108221b96559e0baae5de9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-707%2Fharry-hov%2Ffix-trailers-atom-bug-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-707/harry-hov/fix-trailers-atom-bug-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/707\n\nRange-diff vs v2:\n\n 1: 4816aa3cfa = 1: 383476b177 t6300: unify %(trailers) and %(contents:trailers) tests\n 2: 39aa46bce7 ! 2: 659b9835dc ref-filter: 'contents:trailers' show error if `:` is missing\n @@ Commit message\n ref-filter: 'contents:trailers' show error if `:` is missing\n \n The 'contents' atom does not show any error if used with 'trailers'\n - atom and semicolon is missing before trailers arguments.\n + atom and colon is missing before trailers arguments.\n \n e.g %(contents:trailersonly) works, while it shouldn't.\n \n @@ Commit message\n \n Let's fix this bug.\n \n + Acked-by: Eric Sunshine \n Mentored-by: Christian Couder \n Mentored-by: Heba Waly \n Signed-off-by: Hariom Verma \n \n ## ref-filter.c ##\n -@@ ref-filter.c: static int trailers_atom_parser(const struct ref_format *format, struct used_ato\n - \treturn 0;\n - }\n - \n -+static int check_format_field(const char *arg, const char *field, const char **option)\n -+{\n -+\tconst char *opt;\n -+\tif (skip_prefix(arg, field, &opt)) {\n -+\t\tif (*opt == '\\0') {\n -+\t\t\t*option = NULL;\n -+\t\t\treturn 1;\n -+\t\t}\n -+\t\telse if (*opt == ':') {\n -+\t\t\t*option = opt + 1;\n -+\t\t\treturn 1;\n -+\t\t}\n -+\t}\n -+\treturn 0;\n -+}\n -+\n - static int contents_atom_parser(const struct ref_format *format, struct used_atom *atom,\n - \t\t\t\tconst char *arg, struct strbuf *err)\n - {\n @@ ref-filter.c: static int contents_atom_parser(const struct ref_format *format, struct used_ato\n \t\tatom->u.contents.option = C_SIG;\n \telse if (!strcmp(arg, \"subject\"))\n @@ ref-filter.c: static int contents_atom_parser(const struct ref_format *format, s\n -\telse if (skip_prefix(arg, \"trailers\", &arg)) {\n -\t\tskip_prefix(arg, \":\", &arg);\n -\t\tif (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))\n -+\telse if (check_format_field(arg, \"trailers\", &arg)) {\n ++\telse if (!strcmp(arg, \"trailers\")) {\n ++\t\tif (trailers_atom_parser(format, atom, NULL, err))\n ++\t\t\treturn -1;\n ++\t} else if (skip_prefix(arg, \"trailers:\", &arg)) {\n +\t\tif (trailers_atom_parser(format, atom, arg, err))\n \t\t\treturn -1;\n \t} else if (skip_prefix(arg, \"lines=\", &arg)) {\n @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers) rejects unknown traile\n '\n \n +test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '\n -+\t# error message cannot be checked under i18n\n +\tcat >expect <<-EOF &&\n +\tfatal: unrecognized %(contents) argument: trailersonly\n +\tEOF\n -: ---------- > 3: 712ab9aacf pretty.c: refactor trailer logic to `format_set_trailers_options()`\n -: ---------- > 4: d491be5d10 ref-filter: using pretty.c logic for trailers\n\n-- \ngitgitgadget\n"},{"id":"404212","messageId":"d491be5d10991189f7ec6ead739c1d1500e437a1.1598043976.git.gitgitgadget@gmail.com","threadId":"54068","inReplyTo":"pull.707.v3.git.1598043976.gitgitgadget@gmail.com","subject":"[PATCH v3 4/4] ref-filter: using pretty.c logic for trailers","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-21T21:06:16Z","receivedAt":"2020-08-21T21:06:27Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nNow, ref-filter is using pretty.c logic for setting trailer options.\n\nNew to ref-filter:\n :key= - only show trailers with specified key.\n :valueonly[=val] - only show the value part.\n :separator= - inserted between trailer lines\n\nEnhancement to existing options(now can take value and its optional):\n :only[=val]\n :unfold[=val]\n\n'val' can be: true, on, yes or false, off, no.\n\nMentored-by: Christian Couder \nMentored-by: Heba Waly \nSigned-off-by: Hariom Verma \n---\n Documentation/git-for-each-ref.txt | 36 +++++++--\n ref-filter.c | 35 ++++-----\n t/t6300-for-each-ref.sh | 115 +++++++++++++++++++++++++----\n 3 files changed, 151 insertions(+), 35 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 2ea71c5f6c..f8e15916bc 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -254,11 +254,37 @@ contents:lines=N::\n \tThe first `N` lines of the message.\n \n Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]\n-are obtained as `trailers` (or by using the historical alias\n-`contents:trailers`). Non-trailer lines from the trailer block can be omitted\n-with `trailers:only`. Whitespace-continuations can be removed from trailers so\n-that each trailer appears on a line by itself with its full content with\n-`trailers:unfold`. Both can be used together as `trailers:unfold,only`.\n+are obtained as `trailers[:options]` (or by using the historical alias\n+`contents:trailers[:options]`). Valid [:option] are:\n+** 'key=': only show trailers with specified key. Matching is done\n+ case-insensitively and trailing colon is optional. If option is\n+ given multiple times trailer lines matching any of the keys are\n+ shown. This option automatically enables the `only` option so that\n+ non-trailer lines in the trailer block are hidden. If that is not\n+ desired it can be disabled with `only=false`. E.g.,\n+ `%(trailers:key=Reviewed-by)` shows trailer lines with key\n+ `Reviewed-by`.\n+** 'only[=val]': select whether non-trailer lines from the trailer\n+ block should be included. The `only` keyword may optionally be\n+ followed by an equal sign and one of `true`, `on`, `yes` to omit or\n+ `false`, `off`, `no` to show the non-trailer lines. If option is\n+ given without value it is enabled. If given multiple times the last\n+ value is used.\n+** 'separator=': specify a separator inserted between trailer\n+ lines. When this option is not given each trailer line is\n+ terminated with a line feed character. The string SEP may contain\n+ the literal formatting codes described above. To use comma as\n+ separator one must use `%x2C` as it would otherwise be parsed as\n+ next option. If separator option is given multiple times only the\n+ last one is used. E.g., `%(trailers:key=Ticket,separator=%x2C )`\n+ shows all trailer lines whose key is \"Ticket\" separated by a comma\n+ and a space.\n+** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`\n+ option was given. In same way as to for `only` it can be followed\n+ by an equal sign and explicit value. E.g.,\n+ `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n+** 'valueonly[=val]': skip over the key part of the trailer line and only\n+ show the value part. Also this optionally allows explicit value.\n \n For sorting purposes, fields with numeric values sort in numeric order\n (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8ba0e31915..20f5b829ee 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -67,6 +67,11 @@ struct refname_atom {\n \tint lstrip, rstrip;\n };\n \n+struct ref_trailer_buf {\n+\tstruct string_list filter_list;\n+\tstruct strbuf sepbuf;\n+} ref_trailer_buf;\n+\n static struct expand_data {\n \tstruct object_id oid;\n \tenum object_type type;\n@@ -307,28 +312,24 @@ static int subject_atom_parser(const struct ref_format *format, struct used_atom\n static int trailers_atom_parser(const struct ref_format *format, struct used_atom *atom,\n \t\t\t\tconst char *arg, struct strbuf *err)\n {\n-\tstruct string_list params = STRING_LIST_INIT_DUP;\n-\tint i;\n-\n \tatom->u.contents.trailer_opts.no_divider = 1;\n-\n \tif (arg) {\n-\t\tstring_list_split(¶ms, arg, ',', -1);\n-\t\tfor (i = 0; i < params.nr; i++) {\n-\t\t\tconst char *s = params.items[i].string;\n-\t\t\tif (!strcmp(s, \"unfold\"))\n-\t\t\t\tatom->u.contents.trailer_opts.unfold = 1;\n-\t\t\telse if (!strcmp(s, \"only\"))\n-\t\t\t\tatom->u.contents.trailer_opts.only_trailers = 1;\n-\t\t\telse {\n-\t\t\t\tstrbuf_addf(err, _(\"unknown %%(trailers) argument: %s\"), s);\n-\t\t\t\tstring_list_clear(¶ms, 0);\n-\t\t\t\treturn -1;\n-\t\t\t}\n+\t\tconst char *argbuf = xstrfmt(\"%s)\", arg);\n+\t\tconst char *err_arg = NULL;\n+\n+\t\tif (format_set_trailers_options(&atom->u.contents.trailer_opts,\n+\t\t\t&ref_trailer_buf.filter_list,\n+\t\t\t&ref_trailer_buf.sepbuf,\n+\t\t\t&argbuf, &err_arg)) {\n+\t\t\tif (!err_arg)\n+\t\t\t\tstrbuf_addf(err, _(\"expected %%(trailers:key=)\"));\n+\t\t\telse\n+\t\t\t\tstrbuf_addf(err, _(\"unknown %%(trailers) argument: %s\"), err_arg);\n+\t\t\tfree((char *)err_arg);\n+\t\t\treturn -1;\n \t\t}\n \t}\n \tatom->u.contents.option = C_TRAILERS;\n-\tstring_list_clear(¶ms, 0);\n \treturn 0;\n }\n \ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex fdf2c442c5..664af8588a 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -786,14 +786,32 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success '%(trailers:only) shows only \"key: value\" trailers' '\n+test_show_key_value_trailers () {\n+\toption=\"$1\"\n+\ttest_expect_success \"%($option) shows only 'key: value' trailers\" '\n+\t\t{\n+\t\t\tgrep -v patch.description expect &&\n+\t\tgit for-each-ref --format=\"%($option)\" refs/heads/master >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/master >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+}\n+\n+test_show_key_value_trailers 'trailers:only'\n+test_show_key_value_trailers 'trailers:only=no,only=true'\n+test_show_key_value_trailers 'trailers:only=yes'\n+\n+test_expect_success '%(trailers:only=no) shows all trailers' '\n \t{\n-\t\tgrep -v patch.description expect &&\n-\tgit for-each-ref --format=\"%(trailers:only)\" refs/heads/master >actual &&\n+\tgit for-each-ref --format=\"%(trailers:only=no)\" refs/heads/master >actual &&\n \ttest_cmp expect actual &&\n-\tgit for-each-ref --format=\"%(contents:trailers:only)\" refs/heads/master >actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:only=no)\" refs/heads/master >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -812,17 +830,88 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' '\n \ttest_cmp actual actual\n '\n \n-test_expect_success '%(trailers) rejects unknown trailers arguments' '\n-\t# error message cannot be checked under i18n\n-\tcat >expect <<-EOF &&\n-\tfatal: unknown %(trailers) argument: unsupported\n-\tEOF\n-\ttest_must_fail git for-each-ref --format=\"%(trailers:unsupported)\" 2>actual &&\n-\ttest_i18ncmp expect actual &&\n-\ttest_must_fail git for-each-ref --format=\"%(contents:trailers:unsupported)\" 2>actual &&\n-\ttest_i18ncmp expect actual\n+test_trailer_option() {\n+\ttitle=\"$1\"\n+\toption=\"$2\"\n+\texpect=\"$3\"\n+\ttest_expect_success \"$title\" '\n+\t\techo $expect >expect &&\n+\t\tgit for-each-ref --format=\"%($option)\" refs/heads/master >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/master >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+}\n+\n+test_trailer_option '%(trailers:key=foo) shows that trailer' \\\n+\t'trailers:key=Signed-off-by' 'Signed-off-by: A U Thor \\n'\n+test_trailer_option '%(trailers:key=foo) is case insensitive' \\\n+\t'trailers:key=SiGned-oFf-bY' 'Signed-off-by: A U Thor \\n'\n+test_trailer_option '%(trailers:key=foo:) trailing colon also works' \\\n+\t'trailers:key=Signed-off-by:' 'Signed-off-by: A U Thor \\n'\n+test_trailer_option '%(trailers:key=foo) multiple keys' \\\n+\t'trailers:key=Reviewed-by:,key=Signed-off-by' 'Reviewed-by: A U Thor \\nSigned-off-by: A U Thor \\n'\n+test_trailer_option '%(trailers:key=nonexistent) becomes empty' \\\n+\t'trailers:key=Shined-off-by:' ''\n+\n+test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' '\n+\t{\n+\t\tgrep -v patch.description expect &&\n+\tgit for-each-ref --format=\"%(trailers:key=Acked-by)\" refs/heads/master >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:key=Acked-by)\" refs/heads/master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '\n+\t{\n+\t\tunfold expect &&\n+\tgit for-each-ref --format=\"%(trailers:key=Signed-Off-by,unfold)\" refs/heads/master >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:key=Signed-Off-by,unfold)\" refs/heads/master >actual &&\n+\ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes nontrailer lines' '\n+\t{\n+\t\techo \"Signed-off-by: A U Thor \" &&\n+\t\tgrep patch.description expect &&\n+\tgit for-each-ref --format=\"%(trailers:key=Signed-off-by,only=no)\" refs/heads/master >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:key=Signed-off-by,only=no)\" refs/heads/master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_trailer_option '%(trailers:key=foo,valueonly) shows only value' \\\n+\t'trailers:key=Signed-off-by,valueonly' 'A U Thor \\n'\n+test_trailer_option '%(trailers:separator) changes separator' \\\n+\t'trailers:separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by: A U Thor ,Signed-off-by: A U Thor '\n+\n+test_failing_trailer_option () {\n+\ttitle=\"$1\"\n+\toption=\"$2\"\n+\terror=\"$3\"\n+\ttest_expect_success \"$title\" '\n+\t\t# error message cannot be checked under i18n\n+\t\techo $error >expect &&\n+\t\ttest_must_fail git for-each-ref --format=\"%($option)\" refs/heads/master 2>actual &&\n+\t\ttest_i18ncmp expect actual &&\n+\t\ttest_must_fail git for-each-ref --format=\"%(contents:$option)\" refs/heads/master 2>actual &&\n+\t\ttest_i18ncmp expect actual\n+\t'\n+}\n+\n+test_failing_trailer_option '%(trailers:key) without value is error' \\\n+\t'trailers:key' 'fatal: expected %(trailers:key=)'\n+test_failing_trailer_option '%(trailers) rejects unknown trailers arguments' \\\n+\t'trailers:unsupported' 'fatal: unknown %(trailers) argument: unsupported'\n+\n test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '\n \tcat >expect <<-EOF &&\n \tfatal: unrecognized %(contents) argument: trailersonly\n-- \ngitgitgadget\n"},{"id":"404213","messageId":"712ab9aacf240a02d808af6b6837e682b929493c.1598043976.git.gitgitgadget@gmail.com","threadId":"54068","inReplyTo":"pull.707.v3.git.1598043976.gitgitgadget@gmail.com","subject":"[PATCH v3 3/4] pretty.c: refactor trailer logic to `format_set_trailers_options()`","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-21T21:06:15Z","receivedAt":"2020-08-21T21:06:33Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nRefactored trailers formatting logic inside pretty.c to a new function\n`format_set_trailers_options()`.\n\nAlso, introduced a code to get invalid trailer arguments. As we would\nlike to use same logic in ref-filter, it's nice to get invalid trailer\nargument. This will allow us to print accurate error message, while\nusing `format_set_trailers_options()` in ref-filter.\n\nMentored-by: Christian Couder \nMentored-by: Heba Waly \nSigned-off-by: Hariom Verma \n---\n Hariom Verma via GitGitGadget | 0\n pretty.c | 83 ++++++++++++++++++++++-------------\n pretty.h | 11 +++++\n 3 files changed, 64 insertions(+), 30 deletions(-)\n create mode 100644 Hariom Verma via GitGitGadget\n\ndiff --git a/Hariom Verma via GitGitGadget b/Hariom Verma via GitGitGadget\nnew file mode 100644\nindex 0000000000..e69de29bb2\ndiff --git a/pretty.c b/pretty.c\nindex 2a3d46bf42..bd8d38e27b 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1147,6 +1147,55 @@ static int format_trailer_match_cb(const struct strbuf *key, void *ud)\n \treturn 0;\n }\n \n+int format_set_trailers_options(struct process_trailer_options *opts,\n+\t\t\t\tstruct string_list *filter_list,\n+\t\t\t\tstruct strbuf *sepbuf,\n+\t\t\t\tconst char **arg,\n+\t\t\t\tconst char **err)\n+{\n+\tfor (;;) {\n+\t\tconst char *argval;\n+\t\tsize_t arglen;\n+\n+\t\tif (**arg != ')') {\n+\t\t\tsize_t vallen = strcspn(*arg, \"=,)\");\n+\t\t\tconst char *valstart = xstrndup(*arg, vallen);\n+\t\t\tif (strcmp(valstart, \"key\") &&\n+\t\t\t\tstrcmp(valstart, \"separator\") &&\n+\t\t\t\tstrcmp(valstart, \"only\") &&\n+\t\t\t\tstrcmp(valstart, \"valueonly\") &&\n+\t\t\t\tstrcmp(valstart, \"unfold\")) {\n+\t\t\t\t\t*err = xstrdup(valstart);\n+\t\t\t\t\treturn 1;\n+\t\t\t\t}\n+\t\t\tfree((char *)valstart);\n+\t\t}\n+\t\tif (match_placeholder_arg_value(*arg, \"key\", arg, &argval, &arglen)) {\n+\t\t\tuintptr_t len = arglen;\n+\n+\t\t\tif (!argval)\n+\t\t\t\treturn 1;\n+\n+\t\t\tif (len && argval[len - 1] == ':')\n+\t\t\t\tlen--;\n+\t\t\tstring_list_append(filter_list, argval)->util = (char *)len;\n+\n+\t\t\topts->filter = format_trailer_match_cb;\n+\t\t\topts->filter_data = filter_list;\n+\t\t\topts->only_trailers = 1;\n+\t\t} else if (match_placeholder_arg_value(*arg, \"separator\", arg, &argval, &arglen)) {\n+\t\t\tchar *fmt = xstrndup(argval, arglen);\n+\t\t\tstrbuf_expand(sepbuf, fmt, strbuf_expand_literal_cb, NULL);\n+\t\t\tfree(fmt);\n+\t\t\topts->separator = sepbuf;\n+\t\t} else if (!match_placeholder_bool_arg(*arg, \"only\", arg, &opts->only_trailers) &&\n+\t\t\t\t!match_placeholder_bool_arg(*arg, \"unfold\", arg, &opts->unfold) &&\n+\t\t\t\t!match_placeholder_bool_arg(*arg, \"valueonly\", arg, &opts->value_only))\n+\t\t\tbreak;\n+\t}\n+\treturn 0;\n+}\n+\n static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\tconst char *placeholder,\n \t\t\t\tvoid *context)\n@@ -1417,41 +1466,14 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\tstruct string_list filter_list = STRING_LIST_INIT_NODUP;\n \t\tstruct strbuf sepbuf = STRBUF_INIT;\n \t\tsize_t ret = 0;\n+\t\tconst char *unused = NULL;\n \n \t\topts.no_divider = 1;\n \n \t\tif (*arg == ':') {\n \t\t\targ++;\n-\t\t\tfor (;;) {\n-\t\t\t\tconst char *argval;\n-\t\t\t\tsize_t arglen;\n-\n-\t\t\t\tif (match_placeholder_arg_value(arg, \"key\", &arg, &argval, &arglen)) {\n-\t\t\t\t\tuintptr_t len = arglen;\n-\n-\t\t\t\t\tif (!argval)\n-\t\t\t\t\t\tgoto trailer_out;\n-\n-\t\t\t\t\tif (len && argval[len - 1] == ':')\n-\t\t\t\t\t\tlen--;\n-\t\t\t\t\tstring_list_append(&filter_list, argval)->util = (char *)len;\n-\n-\t\t\t\t\topts.filter = format_trailer_match_cb;\n-\t\t\t\t\topts.filter_data = &filter_list;\n-\t\t\t\t\topts.only_trailers = 1;\n-\t\t\t\t} else if (match_placeholder_arg_value(arg, \"separator\", &arg, &argval, &arglen)) {\n-\t\t\t\t\tchar *fmt;\n-\n-\t\t\t\t\tstrbuf_reset(&sepbuf);\n-\t\t\t\t\tfmt = xstrndup(argval, arglen);\n-\t\t\t\t\tstrbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL);\n-\t\t\t\t\tfree(fmt);\n-\t\t\t\t\topts.separator = &sepbuf;\n-\t\t\t\t} else if (!match_placeholder_bool_arg(arg, \"only\", &arg, &opts.only_trailers) &&\n-\t\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold) &&\n-\t\t\t\t\t !match_placeholder_bool_arg(arg, \"valueonly\", &arg, &opts.value_only))\n-\t\t\t\t\tbreak;\n-\t\t\t}\n+\t\t\tif (format_set_trailers_options(&opts, &filter_list, &sepbuf, &arg, &unused))\n+\t\t\t\tgoto trailer_out;\n \t\t}\n \t\tif (*arg == ')') {\n \t\t\tformat_trailers_from_commit(sb, msg + c->subject_off, &opts);\n@@ -1460,6 +1482,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \ttrailer_out:\n \t\tstring_list_clear(&filter_list, 0);\n \t\tstrbuf_release(&sepbuf);\n+\t\tfree((char *)unused);\n \t\treturn ret;\n \t}\n \ndiff --git a/pretty.h b/pretty.h\nindex 071f2fb8e4..cfe2e8b39b 100644\n--- a/pretty.h\n+++ b/pretty.h\n@@ -6,6 +6,7 @@\n \n struct commit;\n struct strbuf;\n+struct process_trailer_options;\n \n /* Commit formats */\n enum cmit_fmt {\n@@ -139,4 +140,14 @@ const char *format_subject(struct strbuf *sb, const char *msg,\n /* Check if \"cmit_fmt\" will produce an empty output. */\n int commit_format_is_empty(enum cmit_fmt);\n \n+/*\n+ * Set values of fields in \"struct process_trailer_options\"\n+ * according to trailers arguments.\n+ */\n+int format_set_trailers_options(struct process_trailer_options *opts,\n+\t\t\t\tstruct string_list *filter_list,\n+\t\t\t\tstruct strbuf *sepbuf,\n+\t\t\t\tconst char **arg,\n+\t\t\t\tconst char **err);\n+\n #endif /* PRETTY_H */\n-- \ngitgitgadget\n\n"},{"id":"404215","messageId":"CAPig+cSxjRoBE9FNqBW_xSkct6F23HmVSPhta_b4YD+MJERcTA@mail.gmail.com","threadId":"54068","inReplyTo":"659b9835dcd0b38ac3972eb19c08c3bf26dccc80.1598043976.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/4] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-21T21:13:09Z","receivedAt":"2020-08-21T21:13:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 21, 2020 at 5:06 PM Hariom Verma via GitGitGadget\n wrote:\n> The 'contents' atom does not show any error if used with 'trailers'\n> atom and colon is missing before trailers arguments.\n>\n> e.g %(contents:trailersonly) works, while it shouldn't.\n>\n> It is definitely not an expected behavior.\n>\n> Let's fix this bug.\n>\n> Acked-by: Eric Sunshine \n\nI didn't \"ack\" this patch. If you think some sort of attribution with\nmy name is warranted, then a \"Helped-by:\" would be more appropriate.\n\n> Signed-off-by: Hariom Verma \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -345,9 +345,11 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato\n> - else if (skip_prefix(arg, \"trailers\", &arg)) {\n> - skip_prefix(arg, \":\", &arg);\n> - if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))\n> + else if (!strcmp(arg, \"trailers\")) {\n> + if (trailers_atom_parser(format, atom, NULL, err))\n> + return -1;\n> + } else if (skip_prefix(arg, \"trailers:\", &arg)) {\n> + if (trailers_atom_parser(format, atom, arg, err))\n> return -1;\n\nThis looks better and easier to reason about (but I may be biased in\nthinking so).\n\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> @@ -823,6 +823,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '\n> +test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '\n\nThis still needs a s/semicolon/colon/ (mentioned in my previous review).\n"},{"id":"404236","messageId":"CA+CkUQ-2sCguVx2SVJhEZj0WJefxFDt28HD=R5WD_wk25sZV0A@mail.gmail.com","threadId":"54068","inReplyTo":"CAPig+cSxjRoBE9FNqBW_xSkct6F23HmVSPhta_b4YD+MJERcTA@mail.gmail.com","subject":"Re: [PATCH v3 2/4] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2020-08-21T16:19:14Z","receivedAt":"2020-08-21T21:49:29Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi Eric,\n\nOn Sat, Aug 22, 2020 at 2:43 AM Eric Sunshine wrote:\n>\n> On Fri, Aug 21, 2020 at 5:06 PM Hariom Verma via GitGitGadget\n> wrote:\n> > The 'contents' atom does not show any error if used with 'trailers'\n> > atom and colon is missing before trailers arguments.\n> >\n> > e.g %(contents:trailersonly) works, while it shouldn't.\n> >\n> > It is definitely not an expected behavior.\n> >\n> > Let's fix this bug.\n> >\n> > Acked-by: Eric Sunshine \n>\n> I didn't \"ack\" this patch. If you think some sort of attribution with\n> my name is warranted, then a \"Helped-by:\" would be more appropriate.\n\nSorry about that. Fixing in the next version.\n\n> > Signed-off-by: Hariom Verma \n> > ---\n> > diff --git a/ref-filter.c b/ref-filter.c\n> > @@ -345,9 +345,11 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato\n> > - else if (skip_prefix(arg, \"trailers\", &arg)) {\n> > - skip_prefix(arg, \":\", &arg);\n> > - if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))\n> > + else if (!strcmp(arg, \"trailers\")) {\n> > + if (trailers_atom_parser(format, atom, NULL, err))\n> > + return -1;\n> > + } else if (skip_prefix(arg, \"trailers:\", &arg)) {\n> > + if (trailers_atom_parser(format, atom, arg, err))\n> > return -1;\n>\n> This looks better and easier to reason about (but I may be biased in\n> thinking so).\n\nThanks for the review.\n\n> > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> > @@ -823,6 +823,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '\n> > +test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '\n>\n> This still needs a s/semicolon/colon/ (mentioned in my previous review).\n\nSorry, I missed that too.\n\nThanks,\nHariom\n"},{"id":"404239","messageId":"xmqqk0xr7jht.fsf@gitster.c.googlers.com","threadId":"54068","inReplyTo":"CAPig+cSxjRoBE9FNqBW_xSkct6F23HmVSPhta_b4YD+MJERcTA@mail.gmail.com","subject":"Re: [PATCH v3 2/4] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-21T21:54:22Z","receivedAt":"2020-08-21T21:54:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Fri, Aug 21, 2020 at 5:06 PM Hariom Verma via GitGitGadget\n> wrote:\n>> The 'contents' atom does not show any error if used with 'trailers'\n>> atom and colon is missing before trailers arguments.\n>>\n>> e.g %(contents:trailersonly) works, while it shouldn't.\n>>\n>> It is definitely not an expected behavior.\n>>\n>> Let's fix this bug.\n>>\n>> Acked-by: Eric Sunshine \n>\n> I didn't \"ack\" this patch. If you think some sort of attribution with\n> my name is warranted, then a \"Helped-by:\" would be more appropriate.\n\nYes, I did exactly that after moving it just above Hariom's sign-off.\n\n>> Signed-off-by: Hariom Verma \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> @@ -345,9 +345,11 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato\n>> - else if (skip_prefix(arg, \"trailers\", &arg)) {\n>> - skip_prefix(arg, \":\", &arg);\n>> - if (trailers_atom_parser(format, atom, *arg ? arg : NULL, err))\n>> + else if (!strcmp(arg, \"trailers\")) {\n>> + if (trailers_atom_parser(format, atom, NULL, err))\n>> + return -1;\n>> + } else if (skip_prefix(arg, \"trailers:\", &arg)) {\n>> + if (trailers_atom_parser(format, atom, arg, err))\n>> return -1;\n>\n> This looks better and easier to reason about (but I may be biased in\n> thinking so).\n>\n>> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n>> @@ -823,6 +823,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' '\n>> +test_expect_success 'if arguments, %(contents:trailers) shows error if semicolon is missing' '\n>\n> This still needs a s/semicolon/colon/ (mentioned in my previous review).\n\nYup. Tweaked while queueing.\n\nThanks always for sharp eyes.\n"},{"id":"404240","messageId":"xmqqft8f7jeu.fsf@gitster.c.googlers.com","threadId":"54068","inReplyTo":"pull.707.v3.git.1598043976.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/4] [GSoC] Fix trailers atom bug and improved tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-21T21:56:09Z","receivedAt":"2020-08-21T21:56:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Hariom Verma via GitGitGadget\" writes:\n\n> Currently, there exists a bug in 'contents' atom. It does not show any error\n> if used with modifier 'trailers' and semicolon is missing before trailers\n> arguments. This small patch series is focused on fixing that bug and also\n> unified 'trailers' and 'contents:trailers' tests. Thus, removed duplicate\n> code from t6300 and made tests more compact.\n\nI think we should focus on completing the first two patches and send\nthem to 'next' down to 'master', before extending the scope of the\ntopic by piling more patches that do not have to be part of the\ntopic. Let's take the other two separately from the first two.\n\nThanks.\n"},{"id":"404254","messageId":"CA+CkUQ-+z2e+ni8UQOEtCOS2zEXhSU9HK3D_Dr935AiLj4GGzw@mail.gmail.com","threadId":"54068","inReplyTo":"xmqqft8f7jeu.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 0/4] [GSoC] Fix trailers atom bug and improved tests","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2020-08-22T14:03:37Z","receivedAt":"2020-08-22T14:03:52Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi,\n\nOn Sat, Aug 22, 2020 at 3:26 AM Junio C Hamano wrote:\n>\n> \"Hariom Verma via GitGitGadget\" writes:\n>\n> > Currently, there exists a bug in 'contents' atom. It does not show any error\n> > if used with modifier 'trailers' and semicolon is missing before trailers\n> > arguments. This small patch series is focused on fixing that bug and also\n> > unified 'trailers' and 'contents:trailers' tests. Thus, removed duplicate\n> > code from t6300 and made tests more compact.\n>\n> I think we should focus on completing the first two patches and send\n> them to 'next' down to 'master', before extending the scope of the\n> topic by piling more patches that do not have to be part of the\n> topic. Let's take the other two separately from the first two.\n\nSure.\n\nThanks,\nHariom\n"},{"id":"404303","messageId":"CA+CkUQ8Gst2RTaXY6t+ytWu_9Pu7eqnRYRrnawRwYd_NN=u0Lg@mail.gmail.com","threadId":"54068","inReplyTo":"xmqqeenz95bj.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2020-08-23T19:25:42Z","receivedAt":"2020-08-24T00:55:57Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi,\n\nOn Sat, Aug 22, 2020 at 12:47 AM Junio C Hamano wrote:\n>\n> Eric Sunshine writes:\n>\n> > ...an alternative would have been something like:\n> >\n> > else if (!strcmp(arg, \"trailers\")) {\n> > if (trailers_atom_parser(format, atom, NULL, err))\n> > return -1;\n> > } else if (skip_prefix(arg, \"trailers:\", &arg)) {\n> > if (trailers_atom_parser(format, atom, arg, err))\n> > return -1;\n> > }\n> >\n> > which is quite simple to reason about (though has the cost of a tiny\n> > bit of duplication).\n>\n> Yeah, that looks quite simple and straight-forward.\n\nNo doubt, it looks good for \"contents:trailers\".\n\nWhat if In future we would like to expand functionalities of other\n'contents' options?\n\nRecently, I sent a patch series \"Improvements to ref-filter\"[1]. A\npatch in this patch series introduced \"sanitize\" modifier to \"subject\"\natom. i.e \"%(subject:sanitize)\".\n\nWhat if in the future we also want \"%(contents:subject:sanitize)\" to work?\nWe can duplicate code again. Something like:\n```\n} else if (!strcmp(arg, \"trailers\")) {\n if (trailers_atom_parser(format, atom, NULL, err))\n return -1;\n} else if (skip_prefix(arg, \"trailers:\", &arg)) {\n if (trailers_atom_parser(format, atom, arg, err))\n return -1;\n} else if (!strcmp(arg, \"subject\")) {\n if (subject_atom_parser(format, atom, NULL, err))\n return -1;\n} else if (skip_prefix(arg, \"subject:\", &arg)) {\n if (subject_atom_parser(format, atom, arg, err))\n return -1;\n}\n```\n\nOR\n\nWe can just simply use helper. Something like:\n```\nelse if (check_format_field(arg, \"subject\", &arg)) {\n if (subject_atom_parser(format, atom, arg, err))\n return -1;\n} else if (check_format_field(arg, \"trailers\", &arg)) {\n if (trailers_atom_parser(format, atom, arg, err))\n return -1;\n```\nWe can use this helper any number of times, whenever there is a need.\n\nSorry, I missed saying this earlier. But I don't prefer duplicating\nthe code here.\n\nThanks,\nHariom\n\n[1]: https://public-inbox.org/git/pull.684.v4.git.1598046110.gitgitgadget@gmail.com/#t\n"},{"id":"404305","messageId":"CAPig+cScdV1ORSbqDuUiOEvCd6TYgkR=3GK8OCUu4yuoKVy5Pg@mail.gmail.com","threadId":"54068","inReplyTo":"CA+CkUQ8Gst2RTaXY6t+ytWu_9Pu7eqnRYRrnawRwYd_NN=u0Lg@mail.gmail.com","subject":"Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-24T03:49:30Z","receivedAt":"2020-08-24T03:49:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 23, 2020 at 8:56 PM Hariom verma wrote:\n> On Sat, Aug 22, 2020 at 12:47 AM Junio C Hamano wrote:\n> > Eric Sunshine writes:\n> > > ...an alternative would have been something like:\n> > >\n> > > else if (!strcmp(arg, \"trailers\")) {\n> > > if (trailers_atom_parser(format, atom, NULL, err))\n> > > return -1;\n> > > } else if (skip_prefix(arg, \"trailers:\", &arg)) {\n> > > if (trailers_atom_parser(format, atom, arg, err))\n> > > return -1;\n> > > }\n> > >\n> > > which is quite simple to reason about (though has the cost of a tiny\n> > > bit of duplication).\n> >\n> > Yeah, that looks quite simple and straight-forward.\n>\n> Recently, I sent a patch series \"Improvements to ref-filter\"[1]. A\n> patch in this patch series introduced \"sanitize\" modifier to \"subject\"\n> atom. i.e \"%(subject:sanitize)\".\n>\n> What if in the future we also want \"%(contents:subject:sanitize)\" to work?\n> We can use this helper any number of times, whenever there is a need.\n>\n> Sorry, I missed saying this earlier. But I don't prefer duplicating\n> the code here.\n\nPushing back on a reviewer suggestion is fine. Explaining the reason\nfor your position -- as you do here -- helps reviewers understand why\nyou feel the way you do. My review suggestion about making it easier\nto reason about the code while avoiding a brand new function, at the\ncost of a minor amount of duplication, was made in the context of this\none-off case in which the function increased cognitive load and was\nused just once (not knowing that you envisioned future callers). If\nyou expect the new function to be re-used by upcoming changes, then\nthat may be a good reason to keep it. Stating so in the commit message\nwill help reviewers see beyond the immediate patch or patch series.\n\nAside from a couple minor style violations[1,2], I don't particularly\noppose the helper function, though I have a quibble with the name\ncheck_format_field(), which I don't find helpful, and which (at least\nfor me) increases the cognitive load. The increased cognitive load, I\nthink, comes not only from the function name not spelling out what the\nfunction actually does, but also because the function is dual-purpose:\nit's both checking that the argument matches a particular token\n(\"trailers\", in this case) and extracting the sub-argument. Perhaps\nnaming it match_and_extract_subarg() or something similar would help,\nthough that's a mouthful.\n\nBut the observation about the function being dual-purpose (thus\npotentially confusing) brings up other questions. For instance, is it\ntoo special-purpose? If you foresee more callers in the future with\nmultiple-token arguments such as `%(content:subject:sanitize)`, should\nthe function provide more assistance by splitting out each of the\nsub-arguments rather than stopping at the first? Taking that even\nfurther, a generalized helper for \"splitting\" arguments like that\nmight be useful at the top-level of contents_atom_parser() too, rather\nthan only for specific arguments, such as \"trailers\". Of course, this\nmay all be way too ambitious for this little bug fix series or even\nfor whatever upcoming changes you're planning, thus not worth\npursuing.\n\nAs for the helper's implementation, I might have written it like this:\n\n static int check_format_field(...)\n {\n const char *opt\n if (!strcmp(arg, field))\n *option = NULL;\n else if (skip_prefix(arg, field, opt) && *opt == ':')\n *option = opt + 1;\n else\n return 0;\n return 1;\n }\n\nwhich is more compact and closer to what I suggested earlier for\navoiding the helper function in the first place. But, of course,\nprogramming is quite subjective, and you may find your implementation\neasier to reason about. Plus, your version has the benefit of being\nslightly more optimal since it avoids an extra string scan, although\nthat probably is mostly immaterial considering that\ncontents_atom_parser() itself contains a long chain of potentially\nsub-optimal strcmp() and skip_prefix() calls.\n\n\nFootnotes\n\n[1]: use `if (!*opt)` rather than `if (*opt == '\\0')`\n[2]: cuddle the closing brace and `else` on the same line like this:\n `} else if (...) {`\n"},{"id":"404385","messageId":"CA+CkUQ_eRqOB8Ushg-BcEmjRxEZSs7tmPnZcb8GUTwz3R55Xhg@mail.gmail.com","threadId":"54068","inReplyTo":"CAPig+cScdV1ORSbqDuUiOEvCd6TYgkR=3GK8OCUu4yuoKVy5Pg@mail.gmail.com","subject":"Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2020-08-24T23:32:00Z","receivedAt":"2020-08-24T23:32:19Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi,\n\nOn Mon, Aug 24, 2020 at 9:19 AM Eric Sunshine wrote:\n>\n> On Sun, Aug 23, 2020 at 8:56 PM Hariom verma wrote:\n> > On Sat, Aug 22, 2020 at 12:47 AM Junio C Hamano wrote:\n> > > Eric Sunshine writes:\n> > > > ...an alternative would have been something like:\n> > > >\n> > > > else if (!strcmp(arg, \"trailers\")) {\n> > > > if (trailers_atom_parser(format, atom, NULL, err))\n> > > > return -1;\n> > > > } else if (skip_prefix(arg, \"trailers:\", &arg)) {\n> > > > if (trailers_atom_parser(format, atom, arg, err))\n> > > > return -1;\n> > > > }\n> > > >\n> > > > which is quite simple to reason about (though has the cost of a tiny\n> > > > bit of duplication).\n> > >\n> > > Yeah, that looks quite simple and straight-forward.\n> >\n> > Recently, I sent a patch series \"Improvements to ref-filter\"[1]. A\n> > patch in this patch series introduced \"sanitize\" modifier to \"subject\"\n> > atom. i.e \"%(subject:sanitize)\".\n> >\n> > What if in the future we also want \"%(contents:subject:sanitize)\" to work?\n> > We can use this helper any number of times, whenever there is a need.\n> >\n> > Sorry, I missed saying this earlier. But I don't prefer duplicating\n> > the code here.\n>\n> Pushing back on a reviewer suggestion is fine. Explaining the reason\n> for your position -- as you do here -- helps reviewers understand why\n> you feel the way you do. My review suggestion about making it easier\n> to reason about the code while avoiding a brand new function, at the\n> cost of a minor amount of duplication, was made in the context of this\n> one-off case in which the function increased cognitive load and was\n> used just once (not knowing that you envisioned future callers). If\n> you expect the new function to be re-used by upcoming changes, then\n> that may be a good reason to keep it. Stating so in the commit message\n> will help reviewers see beyond the immediate patch or patch series.\n\nYeah. I should have mentioned this in the commit message.\n\n> Aside from a couple minor style violations[1,2], I don't particularly\n> oppose the helper function, though I have a quibble with the name\n> check_format_field(), which I don't find helpful, and which (at least\n> for me) increases the cognitive load. The increased cognitive load, I\n> think, comes not only from the function name not spelling out what the\n> function actually does, but also because the function is dual-purpose:\n> it's both checking that the argument matches a particular token\n> (\"trailers\", in this case) and extracting the sub-argument. Perhaps\n> naming it match_and_extract_subarg() or something similar would help,\n> though that's a mouthful.\n\nI will fix those violations.\nAlso, \"match_and_extract_subarg()\" looks good to me.\n\n> But the observation about the function being dual-purpose (thus\n> potentially confusing) brings up other questions. For instance, is it\n> too special-purpose? If you foresee more callers in the future with\n> multiple-token arguments such as `%(content:subject:sanitize)`, should\n> the function provide more assistance by splitting out each of the\n> sub-arguments rather than stopping at the first? Taking that even\n> further, a generalized helper for \"splitting\" arguments like that\n> might be useful at the top-level of contents_atom_parser() too, rather\n> than only for specific arguments, such as \"trailers\". Of course, this\n> may all be way too ambitious for this little bug fix series or even\n> for whatever upcoming changes you're planning, thus not worth\n> pursuing.\n\nSplitting sub-arguments is done at \"_atom_parser()\".\nIf you mean pre-splitting every argument...\nsomething like: ['contents', 'subject', 'sanitize'] for\n`%(content:subject:sanitize)` in `contents_atom_parser()` ? I'm not\nable to see how it can be useful.\n\nSorry, If I got your concerned wrong.\n\n> As for the helper's implementation, I might have written it like this:\n>\n> static int check_format_field(...)\n> {\n> const char *opt\n> if (!strcmp(arg, field))\n> *option = NULL;\n> else if (skip_prefix(arg, field, opt) && *opt == ':')\n> *option = opt + 1;\n> else\n> return 0;\n> return 1;\n> }\n>\n> which is more compact and closer to what I suggested earlier for\n> avoiding the helper function in the first place. But, of course,\n> programming is quite subjective, and you may find your implementation\n> easier to reason about. Plus, your version has the benefit of being\n> slightly more optimal since it avoids an extra string scan, although\n> that probably is mostly immaterial considering that\n> contents_atom_parser() itself contains a long chain of potentially\n> sub-optimal strcmp() and skip_prefix() calls.\n\n\"programming is quite subjective\"\nYeah, I couldn't agree more.\n\nThe change you suggested looks good too. But I'm little inclined to my\nkeeping my changes. I'm curious, what others have to say on this.\n\nThanks,\nHariom\n"},{"id":"404499","messageId":"CAP8UFD03Am94_84FvRPxEdt_AG74864eQ4TimggKtUYWjJYqCg@mail.gmail.com","threadId":"54068","inReplyTo":"CA+CkUQ_eRqOB8Ushg-BcEmjRxEZSs7tmPnZcb8GUTwz3R55Xhg@mail.gmail.com","subject":"Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-08-26T06:18:29Z","receivedAt":"2020-08-26T06:18:47Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Tue, Aug 25, 2020 at 1:32 AM Hariom verma wrote:\n\n> On Mon, Aug 24, 2020 at 9:19 AM Eric Sunshine wrote:\n> >\n> > On Sun, Aug 23, 2020 at 8:56 PM Hariom verma wrote:\n\n> > > Recently, I sent a patch series \"Improvements to ref-filter\"[1]. A\n> > > patch in this patch series introduced \"sanitize\" modifier to \"subject\"\n> > > atom. i.e \"%(subject:sanitize)\".\n> > >\n> > > What if in the future we also want \"%(contents:subject:sanitize)\" to work?\n> > > We can use this helper any number of times, whenever there is a need.\n> > >\n> > > Sorry, I missed saying this earlier. But I don't prefer duplicating\n> > > the code here.\n> >\n> > Pushing back on a reviewer suggestion is fine. Explaining the reason\n> > for your position -- as you do here -- helps reviewers understand why\n> > you feel the way you do. My review suggestion about making it easier\n> > to reason about the code while avoiding a brand new function, at the\n> > cost of a minor amount of duplication, was made in the context of this\n> > one-off case in which the function increased cognitive load and was\n> > used just once (not knowing that you envisioned future callers). If\n> > you expect the new function to be re-used by upcoming changes, then\n> > that may be a good reason to keep it. Stating so in the commit message\n> > will help reviewers see beyond the immediate patch or patch series.\n>\n> Yeah. I should have mentioned this in the commit message.\n\nI agree.\n\n> > Aside from a couple minor style violations[1,2], I don't particularly\n> > oppose the helper function, though I have a quibble with the name\n> > check_format_field(), which I don't find helpful, and which (at least\n> > for me) increases the cognitive load. The increased cognitive load, I\n> > think, comes not only from the function name not spelling out what the\n> > function actually does, but also because the function is dual-purpose:\n> > it's both checking that the argument matches a particular token\n> > (\"trailers\", in this case) and extracting the sub-argument. Perhaps\n> > naming it match_and_extract_subarg() or something similar would help,\n> > though that's a mouthful.\n>\n> I will fix those violations.\n> Also, \"match_and_extract_subarg()\" looks good to me.\n\nI am not sure about the \"subarg\" part of the name. In the for-each-ref\ndoc, names inside %(...) are called \"field names\", and parts after \":\"\nare called \"options\". So it might be better to have \"field_option\"\ninstead of \"subarg\" in the name.\n\nI think we could also get rid of the \"match_and_\" part of the\nsuggestion, in the same way as skip_prefix() is not called\nmatch_and_skip_prefix(). Readers can just expect that if there is no\nmatch the function will return 0.\n\nSo maybe \"extract_field_option()\".\n\n> > But the observation about the function being dual-purpose (thus\n> > potentially confusing) brings up other questions. For instance, is it\n> > too special-purpose? If you foresee more callers in the future with\n> > multiple-token arguments such as `%(content:subject:sanitize)`, should\n> > the function provide more assistance by splitting out each of the\n> > sub-arguments rather than stopping at the first? Taking that even\n> > further, a generalized helper for \"splitting\" arguments like that\n> > might be useful at the top-level of contents_atom_parser() too, rather\n> > than only for specific arguments, such as \"trailers\". Of course, this\n> > may all be way too ambitious for this little bug fix series or even\n> > for whatever upcoming changes you're planning, thus not worth\n> > pursuing.\n>\n> Splitting sub-arguments is done at \"_atom_parser()\".\n> If you mean pre-splitting every argument...\n> something like: ['contents', 'subject', 'sanitize'] for\n> `%(content:subject:sanitize)` in `contents_atom_parser()` ? I'm not\n> able to see how it can be useful.\n\nYeah, it seems to me that such a splitting would require a complete\nrewrite of the current code, so I am not sure it's an interesting way\nforward for now. And anyway adding extract_field_option() goes in the\nright direction of abstracting the parsing and making the code\nsimpler, more efficient and likely more correct.\n\n> Sorry, If I got your concerned wrong.\n>\n> > As for the helper's implementation, I might have written it like this:\n> >\n> > static int check_format_field(...)\n> > {\n> > const char *opt\n> > if (!strcmp(arg, field))\n> > *option = NULL;\n> > else if (skip_prefix(arg, field, opt) && *opt == ':')\n> > *option = opt + 1;\n> > else\n> > return 0;\n> > return 1;\n> > }\n> >\n> > which is more compact and closer to what I suggested earlier for\n> > avoiding the helper function in the first place. But, of course,\n> > programming is quite subjective, and you may find your implementation\n> > easier to reason about. Plus, your version has the benefit of being\n> > slightly more optimal since it avoids an extra string scan, although\n> > that probably is mostly immaterial considering that\n> > contents_atom_parser() itself contains a long chain of potentially\n> > sub-optimal strcmp() and skip_prefix() calls.\n>\n> \"programming is quite subjective\"\n> Yeah, I couldn't agree more.\n>\n> The change you suggested looks good too. But I'm little inclined to my\n> keeping my changes. I'm curious, what others have to say on this.\n\nI also prefer a slightly more optimal one even if it's a bit less compact.\n\nThanks,\nChristian.\n"},{"id":"404500","messageId":"CAP8UFD0Ds816PfQFwX+1wQhpjaCHZFOF3dK76SRUzt23uS9jPg@mail.gmail.com","threadId":"54068","inReplyTo":"CAP8UFD03Am94_84FvRPxEdt_AG74864eQ4TimggKtUYWjJYqCg@mail.gmail.com","subject":"Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-08-26T06:22:19Z","receivedAt":"2020-08-26T06:22:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Aug 26, 2020 at 8:18 AM Christian Couder\n wrote:\n\n> I think we could also get rid of the \"match_and_\" part of the\n> suggestion, in the same way as skip_prefix() is not called\n> match_and_skip_prefix(). Readers can just expect that if there is no\n> match the function will return 0.\n>\n> So maybe \"extract_field_option()\".\n\nIf we want to hint more that it works in the way as skip_prefix(), we\ncould call it \"skip_field()\".\n"},{"id":"404550","messageId":"CA+CkUQ_M=q9bkxjM9b+5DRkRBoFRnzhnsCUB-gX9GEeW6H5SVw@mail.gmail.com","threadId":"54068","inReplyTo":"CAP8UFD03Am94_84FvRPxEdt_AG74864eQ4TimggKtUYWjJYqCg@mail.gmail.com","subject":"Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2020-08-26T15:18:41Z","receivedAt":"2020-08-26T20:48:55Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi,\n\nOn Wed, Aug 26, 2020 at 11:48 AM Christian Couder\n wrote:\n>\n> Hi,\n>\n> On Tue, Aug 25, 2020 at 1:32 AM Hariom verma wrote:\n>\n> > On Mon, Aug 24, 2020 at 9:19 AM Eric Sunshine wrote:\n>\n> > > Aside from a couple minor style violations[1,2], I don't particularly\n> > > oppose the helper function, though I have a quibble with the name\n> > > check_format_field(), which I don't find helpful, and which (at least\n> > > for me) increases the cognitive load. The increased cognitive load, I\n> > > think, comes not only from the function name not spelling out what the\n> > > function actually does, but also because the function is dual-purpose:\n> > > it's both checking that the argument matches a particular token\n> > > (\"trailers\", in this case) and extracting the sub-argument. Perhaps\n> > > naming it match_and_extract_subarg() or something similar would help,\n> > > though that's a mouthful.\n> >\n> > I will fix those violations.\n> > Also, \"match_and_extract_subarg()\" looks good to me.\n>\n> I am not sure about the \"subarg\" part of the name. In the for-each-ref\n> doc, names inside %(...) are called \"field names\", and parts after \":\"\n> are called \"options\". So it might be better to have \"field_option\"\n> instead of \"subarg\" in the name.\n>\n> I think we could also get rid of the \"match_and_\" part of the\n> suggestion, in the same way as skip_prefix() is not called\n> match_and_skip_prefix(). Readers can just expect that if there is no\n> match the function will return 0.\n>\n> So maybe \"extract_field_option()\".\n\nMakes sense to me.\n\n> > > But the observation about the function being dual-purpose (thus\n> > > potentially confusing) brings up other questions. For instance, is it\n> > > too special-purpose? If you foresee more callers in the future with\n> > > multiple-token arguments such as `%(content:subject:sanitize)`, should\n> > > the function provide more assistance by splitting out each of the\n> > > sub-arguments rather than stopping at the first? Taking that even\n> > > further, a generalized helper for \"splitting\" arguments like that\n> > > might be useful at the top-level of contents_atom_parser() too, rather\n> > > than only for specific arguments, such as \"trailers\". Of course, this\n> > > may all be way too ambitious for this little bug fix series or even\n> > > for whatever upcoming changes you're planning, thus not worth\n> > > pursuing.\n> >\n> > Splitting sub-arguments is done at \"_atom_parser()\".\n> > If you mean pre-splitting every argument...\n> > something like: ['contents', 'subject', 'sanitize'] for\n> > `%(content:subject:sanitize)` in `contents_atom_parser()` ? I'm not\n> > able to see how it can be useful.\n>\n> Yeah, it seems to me that such a splitting would require a complete\n> rewrite of the current code, so I am not sure it's an interesting way\n> forward for now. And anyway adding extract_field_option() goes in the\n> right direction of abstracting the parsing and making the code\n> simpler, more efficient and likely more correct.\n>\n> > Sorry, If I got your concerned wrong.\n> >\n> > > As for the helper's implementation, I might have written it like this:\n> > >\n> > > static int check_format_field(...)\n> > > {\n> > > const char *opt\n> > > if (!strcmp(arg, field))\n> > > *option = NULL;\n> > > else if (skip_prefix(arg, field, opt) && *opt == ':')\n> > > *option = opt + 1;\n> > > else\n> > > return 0;\n> > > return 1;\n> > > }\n> > >\n> > > which is more compact and closer to what I suggested earlier for\n> > > avoiding the helper function in the first place. But, of course,\n> > > programming is quite subjective, and you may find your implementation\n> > > easier to reason about. Plus, your version has the benefit of being\n> > > slightly more optimal since it avoids an extra string scan, although\n> > > that probably is mostly immaterial considering that\n> > > contents_atom_parser() itself contains a long chain of potentially\n> > > sub-optimal strcmp() and skip_prefix() calls.\n> >\n> > \"programming is quite subjective\"\n> > Yeah, I couldn't agree more.\n> >\n> > The change you suggested looks good too. But I'm little inclined to my\n> > keeping my changes. I'm curious, what others have to say on this.\n>\n> I also prefer a slightly more optimal one even if it's a bit less compact.\n\n+1\n\nThanks,\nHariom\n"}]}