{"thread":{"id":"54195","subject":"[PATCH 1/2] pretty.c: refactor trailer logic to `format_set_trailers_options()`","startedAt":"2020-09-05T19:48:18Z","lastAt":"2021-02-13T01:53:45Z","messageCount":43,"participants":["Hariom Verma via GitGitGadget","René Scharfe","Christian Couder","Junio C Hamano","Hariom verma","Ævar Arnfjörð Bjarmason","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"405080","messageId":"4180f54c1de51a3b9e29ef42b035bb31215875e3.1599335291.git.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":"pull.726.git.1599335291.gitgitgadget@gmail.com","subject":"[PATCH 1/2] pretty.c: refactor trailer logic to `format_set_trailers_options()`","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-05T19:48:10Z","receivedAt":"2020-09-05T19:48:18Z","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":"405081","messageId":"pull.726.git.1599335291.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":null,"subject":"[PATCH 0/2] Unify trailers formatting logic for pretty.c and ref-filter.c","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-05T19:48:09Z","receivedAt":"2020-09-05T19:48:23Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"Currently, there exists a separate logic for %(trailers) in \"pretty.{c,h}\"\nand \"ref-filter.{c,h}\". Both are actually doing the same thing, why not use\nthe same code for both of them?\n\nThis patch series is focused on unifying the \"%(trailers)\" logic for both\n'pretty.{c,h}' and 'ref-filter.{c,h}'. So, we can have one logic for\ntrailers.\n\nNote: We used pretty's logic in ref-filter because it supports more\n%(trailers) options.\n\nHariom Verma (2):\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 | 35 ++++-----\n t/t6300-for-each-ref.sh | 115 +++++++++++++++++++++++++----\n 6 files changed, 215 insertions(+), 65 deletions(-)\n create mode 100644 Hariom Verma via GitGitGadget\n\n\nbase-commit: 3a238e539bcdfe3f9eb5010fd218640c1b499f7a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-726%2Fharry-hov%2Funify-trailers-logic-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-726/harry-hov/unify-trailers-logic-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/726\n-- \ngitgitgadget\n"},{"id":"405082","messageId":"a3e1629826236c2bf7e2512af4acd13e72683687.1599335291.git.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":"pull.726.git.1599335291.gitgitgadget@gmail.com","subject":"[PATCH 2/2] ref-filter: using pretty.c logic for trailers","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-05T19:48:11Z","receivedAt":"2020-09-05T19:48:23Z","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 58adee7d18..aa3dc9f43b 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 colon is missing' '\n \tcat >expect <<-EOF &&\n \tfatal: unrecognized %(contents) argument: trailersonly\n-- \ngitgitgadget\n"},{"id":"405085","messageId":"bf4423d5-c0ee-6bef-59ff-fcde003ec463@web.de","threadId":"54195","inReplyTo":"4180f54c1de51a3b9e29ef42b035bb31215875e3.1599335291.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] pretty.c: refactor trailer logic to `format_set_trailers_options()`","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-09-05T21:59:46Z","receivedAt":"2020-09-05T21:59:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.09.20 um 21:48 schrieb Hariom Verma via GitGitGadget:\n> From: Hariom Verma \n>\n> Refactored trailers formatting logic inside pretty.c to a new function\n> `format_set_trailers_options()`.\n>\n> Also, introduced a code to get invalid trailer arguments. As we would\n> like to use same logic in ref-filter, it's nice to get invalid trailer\n> argument. This will allow us to print accurate error message, while\n> using `format_set_trailers_options()` in ref-filter.\n\nThis error reporting feature is probably worth its own separate commit.\n\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Heba Waly \n> Signed-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>\n> diff --git a/Hariom Verma via GitGitGadget b/Hariom Verma via GitGitGadget\n> new file mode 100644\n> index 0000000000..e69de29bb2\n> diff --git a/pretty.c b/pretty.c\n> index 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\nThis function is supposed to allocate *err, so it shouldn't be const.\n\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\nThis is owned by this block and released a few lines down, so it\nshouldn't be const.\n\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\nWeird indentation of the second and later strcmp() calls. And they\nduplicate the checks done by the parsing code below, right?\n\n> +\t\t\t\t\t*err = xstrdup(valstart);\n\nHere's the *err allocation mentioned above.\n\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\nYou didn't add this, but I wonder what's up with the arglen-in-util\ntrickery here -- strcasecmp() might even be faster.\n\nAnd also why all this expensive-looking %(trailers) parsing is done\nfor each commit and not just once.\n\n(Both outside the scope of this series, unless you want to address\nthem as well.)\n\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\nWeird indentation of the second and third match_placeholder_bool_arg()\ncalls.\n\n> +\t\t\tbreak;\n\nIf you need to capture an invalid argument value, you can do that here\nwithout adding duplicate checks.\n\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\nThis function is going to free() unused later, so it shouldn't be const.\n\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\nThe match_placeholder_bool_arg() calls were still properly indented\nhere.\n\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\nHere is the free() call I mentioned above.\n\n> \t\treturn ret;\n> \t}\n>\n> diff --git a/pretty.h b/pretty.h\n> index 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>\n\n"},{"id":"415596","messageId":"pull.726.v2.git.1611954543.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":"pull.726.git.1599335291.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Unify trailers formatting logic for pretty.c and ref-filter.c","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-29T21:09:00Z","receivedAt":"2021-01-29T21:09:54Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"Currently, there exists a separate logic for %(trailers) in \"pretty.{c,h}\"\nand \"ref-filter.{c,h}\". Both are actually doing the same thing, why not use\nthe same code for both of them?\n\nThis is the 2nd version of the patch series that I sent few months back. It\nis focused on unifying the \"%(trailers)\" logic for both 'pretty.{c,h}' and\n'ref-filter.{c,h}'. So, we can have one logic for trailers.\n\nv2 changes:\n\n * Contains Improvements as suggested by \"René Scharfe\" l.s.r@web.de 1\n [https://public-inbox.org/git/bf4423d5-c0ee-6bef-59ff-fcde003ec463@web.de/]\n * A new trailer option was introduced to pretty.c when I was absent i.e\n \"key_value_separator\". Updated the patch series with latest changes.\n\nSorry for taking a long break.\n\nLink to previous version:\nhttps://public-inbox.org/git/pull.726.git.1599335291.gitgitgadget@gmail.com/\n\nHariom Verma (3):\n pretty.c: refactor trailer logic to `format_set_trailers_options()`\n pretty.c: capture invalid trailer argument\n ref-filter: use pretty.c logic for trailers\n\n Documentation/git-for-each-ref.txt | 39 ++++++++--\n pretty.c | 95 +++++++++++++----------\n pretty.h | 12 +++\n ref-filter.c | 36 +++++----\n t/t6300-for-each-ref.sh | 119 +++++++++++++++++++++++++----\n 5 files changed, 228 insertions(+), 73 deletions(-)\n\n\nbase-commit: e6362826a0409539642a5738db61827e5978e2e4\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-726%2Fharry-hov%2Funify-trailers-logic-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-726/harry-hov/unify-trailers-logic-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/726\n\nRange-diff vs v1:\n\n 1: 4180f54c1de ! 1: fc5fd5217df pretty.c: refactor trailer logic to `format_set_trailers_options()`\n @@ Commit message\n pretty.c: refactor trailer logic to `format_set_trailers_options()`\n \n Refactored trailers formatting logic inside pretty.c to a new function\n - `format_set_trailers_options()`.\n -\n - Also, introduced a code to get invalid trailer arguments. As we would\n - like to use same logic in ref-filter, it's nice to get invalid trailer\n - argument. This will allow us to print accurate error message, while\n - using `format_set_trailers_options()` in ref-filter.\n + `format_set_trailers_options()`. This change will allow us to reuse\n + the same logic in other places.\n \n Mentored-by: Christian Couder \n Mentored-by: Heba Waly \n Signed-off-by: Hariom Verma \n \n - ## Hariom Verma via GitGitGadget (new) ##\n -\n ## pretty.c ##\n @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)\n \treturn 0;\n @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)\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 ++\t\t\t\tstruct strbuf *kvsepbuf,\n ++\t\t\t\tconst char **arg)\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 @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)\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\tchar *fmt;\n ++\t\t\tfmt = 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_arg_value(*arg, \"key_value_separator\", arg, &argval, &arglen)) {\n ++\t\t\tchar *fmt;\n ++\t\t\tfmt = xstrndup(argval, arglen);\n ++\t\t\tstrbuf_expand(kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);\n ++\t\t\tfree(fmt);\n ++\t\t\topts->key_value_separator = kvsepbuf;\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\t !match_placeholder_bool_arg(*arg, \"unfold\", arg, &opts->unfold) &&\n ++\t\t\t !match_placeholder_bool_arg(*arg, \"keyonly\", arg, &opts->key_only) &&\n ++\t\t\t !match_placeholder_bool_arg(*arg, \"valueonly\", arg, &opts->value_only))\n +\t\t\tbreak;\n +\t}\n +\treturn 0;\n @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)\n \t\t\t\tconst char *placeholder,\n \t\t\t\tvoid *context)\n @@ pretty.c: 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 @@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\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_arg_value(arg, \"key_value_separator\", &arg, &argval, &arglen)) {\n +-\t\t\t\t\tchar *fmt;\n +-\n +-\t\t\t\t\tstrbuf_reset(&kvsepbuf);\n +-\t\t\t\t\tfmt = xstrndup(argval, arglen);\n +-\t\t\t\t\tstrbuf_expand(&kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);\n +-\t\t\t\t\tfree(fmt);\n +-\t\t\t\t\topts.key_value_separator = &kvsepbuf;\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, \"keyonly\", &arg, &opts.key_only) &&\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\tif (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg))\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 -@@ pretty.c: 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 - \n \n ## pretty.h ##\n @@\n @@ pretty.h\n \n /* Commit formats */\n enum cmit_fmt {\n -@@ pretty.h: 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 +@@ pretty.h: int commit_format_is_empty(enum cmit_fmt);\n + /* Make subject of commit message suitable for filename */\n + void format_sanitized_subject(struct strbuf *sb, const char *msg, size_t len);\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 ++\t\t\tstruct string_list *filter_list,\n ++\t\t\tstruct strbuf *sepbuf,\n ++\t\t\tstruct strbuf *kvsepbuf,\n ++\t\t\tconst char **arg);\n +\n #endif /* PRETTY_H */\n -: ----------- > 2: 245e48eb683 pretty.c: capture invalid trailer argument\n 2: a3e16298262 ! 3: 7b8cfb2721c ref-filter: using pretty.c logic for trailers\n @@ Metadata\n Author: Hariom Verma \n \n ## Commit message ##\n - ref-filter: using pretty.c logic for trailers\n + ref-filter: use pretty.c logic for trailers\n \n Now, ref-filter is using pretty.c logic for setting trailer options.\n \n New 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 + :separator= - inserted between trailer lines.\n + :key_value_separator= - inserted between trailer lines\n \n Enhancement to existing options(now can take value and its optional):\n :only[=val]\n @@ Documentation/git-for-each-ref.txt: contents:lines=N::\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 ++ the literal formatting codes. To use comma as separator one must use\n ++ `%x2C` as it would otherwise be parsed as next option. If separator\n ++ option is given multiple times only the last one is used.\n ++ E.g., `%(trailers:key=Ticket,separator=%x2C)` shows all trailer lines\n ++ whose key is \"Ticket\" separated by a comma.\n ++** 'key_value_separator=': specify a separator inserted between\n ++ key and value. The string SEP may contain the literal formatting codes.\n ++ E.g., `%(trailers:key=Ticket,key_value_separator=%x2C)` shows all trailer\n ++ lines whose key is \"Ticket\" with key and value separated by a comma.\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 @@ ref-filter.c: struct refname_atom {\n +struct ref_trailer_buf {\n +\tstruct string_list filter_list;\n +\tstruct strbuf sepbuf;\n ++\tstruct strbuf kvsepbuf;\n +} ref_trailer_buf;\n +\n static struct expand_data {\n @@ ref-filter.c: static int subject_atom_parser(const struct ref_format *format, st\n -\tint i;\n -\n \tatom->u.contents.trailer_opts.no_divider = 1;\n --\n + \n \tif (arg) {\n -\t\tstring_list_split(¶ms, arg, ',', -1);\n -\t\tfor (i = 0; i < params.nr; i++) {\n @@ ref-filter.c: static int subject_atom_parser(const struct ref_format *format, st\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 ++\t\tchar *invalid_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&ref_trailer_buf.kvsepbuf,\n ++\t\t\t&argbuf, &invalid_arg)) {\n ++\t\t\tif (!invalid_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\t\tstrbuf_addf(err, _(\"unknown %%(trailers) argument: %s\"), invalid_arg);\n ++\t\t\tfree((char *)invalid_arg);\n +\t\t\treturn -1;\n \t\t}\n \t}\n @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:unfold) unfolds trailer\n +\t\t\tgrep -v patch.description expect &&\n -+\t\tgit for-each-ref --format=\"%($option)\" refs/heads/master >actual &&\n ++\t\tgit for-each-ref --format=\"%($option)\" refs/heads/main >actual &&\n +\t\ttest_cmp expect actual &&\n -+\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/master >actual &&\n ++\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/main >actual &&\n +\t\ttest_cmp expect actual\n +\t'\n +}\n @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:unfold) unfolds trailer\n +\t\tcat trailers &&\n \t\techo\n \t} >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 +-\tgit for-each-ref --format=\"%(trailers:only)\" refs/heads/main >actual &&\n ++\tgit for-each-ref --format=\"%(trailers:only=no)\" refs/heads/main >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 +-\tgit for-each-ref --format=\"%(contents:trailers:only)\" refs/heads/main >actual &&\n ++\tgit for-each-ref --format=\"%(contents:trailers:only=no)\" refs/heads/main >actual &&\n \ttest_cmp expect actual\n '\n \n @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un\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\tgit for-each-ref --format=\"%($option)\" refs/heads/main >actual &&\n +\t\ttest_cmp expect actual &&\n -+\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/master >actual &&\n ++\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/main >actual &&\n +\t\ttest_cmp expect actual\n +\t'\n +}\n @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un\n +\t\tgrep -v patch.description expect &&\n -+\tgit for-each-ref --format=\"%(trailers:key=Acked-by)\" refs/heads/master >actual &&\n ++\tgit for-each-ref --format=\"%(trailers:key=Acked-by)\" refs/heads/main >actual &&\n +\ttest_cmp expect actual &&\n -+\tgit for-each-ref --format=\"%(contents:trailers:key=Acked-by)\" refs/heads/master >actual &&\n ++\tgit for-each-ref --format=\"%(contents:trailers:key=Acked-by)\" refs/heads/main >actual &&\n +\ttest_cmp expect actual\n +'\n +\n @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un\n +\t\tunfold expect &&\n -+\tgit for-each-ref --format=\"%(trailers:key=Signed-Off-by,unfold)\" refs/heads/master >actual &&\n ++\tgit for-each-ref --format=\"%(trailers:key=Signed-Off-by,unfold)\" refs/heads/main >actual &&\n +\ttest_cmp expect actual &&\n -+\tgit for-each-ref --format=\"%(contents:trailers:key=Signed-Off-by,unfold)\" refs/heads/master >actual &&\n ++\tgit for-each-ref --format=\"%(contents:trailers:key=Signed-Off-by,unfold)\" refs/heads/main >actual &&\n +\ttest_cmp expect actual\n '\n \n @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un\n +\t\tgrep patch.description expect &&\n -+\tgit for-each-ref --format=\"%(trailers:key=Signed-off-by,only=no)\" refs/heads/master >actual &&\n ++\tgit for-each-ref --format=\"%(trailers:key=Signed-off-by,only=no)\" refs/heads/main >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 ++\tgit for-each-ref --format=\"%(contents:trailers:key=Signed-off-by,only=no)\" refs/heads/main >actual &&\n +\ttest_cmp expect actual\n +'\n +\n @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un\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 ++test_trailer_option '%(trailers:key_value_separator) changes key-value separator' \\\n ++\t'trailers:key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor \\nSigned-off-by,A U Thor \\n'\n ++test_trailer_option '%(trailers:separator,key_value_separator) changes both separators' \\\n ++\t'trailers:separator=%x2C,key_value_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 @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un\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_must_fail git for-each-ref --format=\"%($option)\" refs/heads/main 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_must_fail git for-each-ref --format=\"%(contents:$option)\" refs/heads/main 2>actual &&\n +\t\ttest_i18ncmp expect actual\n +\t'\n +}\n\n-- \ngitgitgadget\n"},{"id":"415597","messageId":"fc5fd5217dfc105f3e03a9800a35209a985775a4.1611954543.git.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":"pull.726.v2.git.1611954543.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] pretty.c: refactor trailer logic to `format_set_trailers_options()`","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-29T21:09:01Z","receivedAt":"2021-01-29T21:09:56Z","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()`. This change will allow us to reuse\nthe same logic in other places.\n\nMentored-by: Christian Couder \nMentored-by: Heba Waly \nSigned-off-by: Hariom Verma \n---\n pretty.c | 85 ++++++++++++++++++++++++++++++--------------------------\n pretty.h | 11 ++++++++\n 2 files changed, 57 insertions(+), 39 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 3922f6f9f24..bb6a3c634ac 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1148,6 +1148,50 @@ 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\tstruct strbuf *kvsepbuf,\n+\t\t\t\tconst char **arg)\n+{\n+\tfor (;;) {\n+\t\tconst char *argval;\n+\t\tsize_t arglen;\n+\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;\n+\t\t\tfmt = 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_arg_value(*arg, \"key_value_separator\", arg, &argval, &arglen)) {\n+\t\t\tchar *fmt;\n+\t\t\tfmt = xstrndup(argval, arglen);\n+\t\t\tstrbuf_expand(kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);\n+\t\t\tfree(fmt);\n+\t\t\topts->key_value_separator = kvsepbuf;\n+\t\t} else if (!match_placeholder_bool_arg(*arg, \"only\", arg, &opts->only_trailers) &&\n+\t\t\t !match_placeholder_bool_arg(*arg, \"unfold\", arg, &opts->unfold) &&\n+\t\t\t !match_placeholder_bool_arg(*arg, \"keyonly\", arg, &opts->key_only) &&\n+\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@@ -1425,45 +1469,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\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_arg_value(arg, \"key_value_separator\", &arg, &argval, &arglen)) {\n-\t\t\t\t\tchar *fmt;\n-\n-\t\t\t\t\tstrbuf_reset(&kvsepbuf);\n-\t\t\t\t\tfmt = xstrndup(argval, arglen);\n-\t\t\t\t\tstrbuf_expand(&kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);\n-\t\t\t\t\tfree(fmt);\n-\t\t\t\t\topts.key_value_separator = &kvsepbuf;\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, \"keyonly\", &arg, &opts.key_only) &&\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, &kvsepbuf, &arg))\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);\ndiff --git a/pretty.h b/pretty.h\nindex 7ce6c0b437b..7369cf7e148 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@@ -142,4 +143,14 @@ int commit_format_is_empty(enum cmit_fmt);\n /* Make subject of commit message suitable for filename */\n void format_sanitized_subject(struct strbuf *sb, const char *msg, size_t len);\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\tstruct string_list *filter_list,\n+\t\t\tstruct strbuf *sepbuf,\n+\t\t\tstruct strbuf *kvsepbuf,\n+\t\t\tconst char **arg);\n+\n #endif /* PRETTY_H */\n-- \ngitgitgadget\n\n"},{"id":"415598","messageId":"245e48eb6835cae4e61f65af780b766d990d4b5f.1611954543.git.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":"pull.726.v2.git.1611954543.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] pretty.c: capture invalid trailer argument","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-29T21:09:02Z","receivedAt":"2021-01-29T21:10:07Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nAs we would like to use this same logic in ref-filter, it's nice to\nget invalid trailer argument. This will allow us to print precise\nerror message, while using `format_set_trailers_options()` in\nref-filter.\n\nMentored-by: Christian Couder \nMentored-by: Heba Waly \nSigned-off-by: Hariom Verma \n---\n pretty.c | 18 ++++++++++++++----\n pretty.h | 3 ++-\n 2 files changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex bb6a3c634ac..b5fa7944389 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1152,12 +1152,17 @@ 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\tstruct strbuf *kvsepbuf,\n-\t\t\t\tconst char **arg)\n+\t\t\t\tconst char **arg,\n+\t\t\t\tchar **invalid_arg)\n {\n \tfor (;;) {\n \t\tconst char *argval;\n \t\tsize_t arglen;\n \n+\t\tif(**arg == ')') {\n+\t\t\tbreak;\n+\t\t}\n+\n \t\tif (match_placeholder_arg_value(*arg, \"key\", arg, &argval, &arglen)) {\n \t\t\tuintptr_t len = arglen;\n \n@@ -1186,8 +1191,11 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n \t\t} else if (!match_placeholder_bool_arg(*arg, \"only\", arg, &opts->only_trailers) &&\n \t\t\t !match_placeholder_bool_arg(*arg, \"unfold\", arg, &opts->unfold) &&\n \t\t\t !match_placeholder_bool_arg(*arg, \"keyonly\", arg, &opts->key_only) &&\n-\t\t\t !match_placeholder_bool_arg(*arg, \"valueonly\", arg, &opts->value_only))\n-\t\t\tbreak;\n+\t\t\t !match_placeholder_bool_arg(*arg, \"valueonly\", arg, &opts->value_only)) {\n+\t\t\tsize_t invalid_arg_len = strcspn(*arg, \",)\");\n+\t\t\t*invalid_arg = xstrndup(*arg, invalid_arg_len);\n+\t\t\treturn 1;\n+\t\t}\n \t}\n \treturn 0;\n }\n@@ -1464,12 +1472,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\tstruct strbuf sepbuf = STRBUF_INIT;\n \t\tstruct strbuf kvsepbuf = STRBUF_INIT;\n \t\tsize_t ret = 0;\n+\t\tchar *unused = NULL;\n \n \t\topts.no_divider = 1;\n \n \t\tif (*arg == ':') {\n \t\t\targ++;\n-\t\t\tif (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg))\n+\t\t\tif (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg, &unused))\n \t\t\t\tgoto trailer_out;\n \t\t}\n \t\tif (*arg == ')') {\n@@ -1479,6 +1488,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 7369cf7e148..d902cdd70a9 100644\n--- a/pretty.h\n+++ b/pretty.h\n@@ -151,6 +151,7 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n \t\t\tstruct string_list *filter_list,\n \t\t\tstruct strbuf *sepbuf,\n \t\t\tstruct strbuf *kvsepbuf,\n-\t\t\tconst char **arg);\n+\t\t\tconst char **arg,\n+\t\t\tchar **invalid_arg);\n \n #endif /* PRETTY_H */\n-- \ngitgitgadget\n\n"},{"id":"415599","messageId":"7b8cfb2721c349f2bcebec98f84291b1cffd3b49.1611954543.git.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":"pull.726.v2.git.1611954543.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] ref-filter: use pretty.c logic for trailers","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-29T21:09:03Z","receivedAt":"2021-01-29T21:10:09Z","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 :key_value_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 | 39 ++++++++--\n ref-filter.c | 36 +++++----\n t/t6300-for-each-ref.sh | 119 +++++++++++++++++++++++++----\n 3 files changed, 160 insertions(+), 34 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 2962f85a502..ea1f8417176 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -260,11 +260,40 @@ 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. To use comma as separator one must use\n+ `%x2C` as it would otherwise be parsed as next option. If separator\n+ option is given multiple times only the last one is used.\n+ E.g., `%(trailers:key=Ticket,separator=%x2C)` shows all trailer lines\n+ whose key is \"Ticket\" separated by a comma.\n+** 'key_value_separator=': specify a separator inserted between\n+ key and value. The string SEP may contain the literal formatting codes.\n+ E.g., `%(trailers:key=Ticket,key_value_separator=%x2C)` shows all trailer\n+ lines whose key is \"Ticket\" with key and value separated by a comma.\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 ee337df232a..2b1c61eadf4 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -67,6 +67,12 @@ 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+\tstruct strbuf kvsepbuf;\n+} ref_trailer_buf;\n+\n static struct expand_data {\n \tstruct object_id oid;\n \tenum object_type type;\n@@ -313,28 +319,26 @@ 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\tchar *invalid_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&ref_trailer_buf.kvsepbuf,\n+\t\t\t&argbuf, &invalid_arg)) {\n+\t\t\tif (!invalid_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\"), invalid_arg);\n+\t\t\tfree((char *)invalid_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 ca62e764b58..a8835b13915 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -825,14 +825,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/main >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/main >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/main >actual &&\n+\tgit for-each-ref --format=\"%(trailers:only=no)\" refs/heads/main >actual &&\n \ttest_cmp expect actual &&\n-\tgit for-each-ref --format=\"%(contents:trailers:only)\" refs/heads/main >actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:only=no)\" refs/heads/main >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -851,17 +869,92 @@ 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/main >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/main >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/main >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:key=Acked-by)\" refs/heads/main >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/main >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:key=Signed-Off-by,unfold)\" refs/heads/main >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/main >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:key=Signed-off-by,only=no)\" refs/heads/main >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+test_trailer_option '%(trailers:key_value_separator) changes key-value separator' \\\n+\t'trailers:key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor \\nSigned-off-by,A U Thor \\n'\n+test_trailer_option '%(trailers:separator,key_value_separator) changes both separators' \\\n+\t'trailers:separator=%x2C,key_value_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/main 2>actual &&\n+\t\ttest_i18ncmp expect actual &&\n+\t\ttest_must_fail git for-each-ref --format=\"%(contents:$option)\" refs/heads/main 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 colon is missing' '\n \tcat >expect <<-EOF &&\n \tfatal: unrecognized %(contents) argument: trailersonly\n-- \ngitgitgadget\n"},{"id":"415606","messageId":"CAP8UFD00sdiaFYUvgzQmXKCQSyrNKG82_xXvRGRaqdkbqKu7UQ@mail.gmail.com","threadId":"54195","inReplyTo":"245e48eb6835cae4e61f65af780b766d990d4b5f.1611954543.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] pretty.c: capture invalid trailer argument","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-01-29T22:28:23Z","receivedAt":"2021-01-29T22:29:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jan 29, 2021 at 10:15 PM Hariom Verma via GitGitGadget\n wrote:\n>\n> From: Hariom Verma \n>\n> As we would like to use this same logic in ref-filter, it's nice to\n> get invalid trailer argument. This will allow us to print precise\n> error message, while using `format_set_trailers_options()` in\n> ref-filter.\n\nThanks for continuing to work on this!\n\n> {\n> for (;;) {\n> const char *argval;\n> size_t arglen;\n>\n> + if(**arg == ')') {\n> + break;\n> + }\n\nA space char is missing between \"if\" and \"(\". Also no need for \"{\" and\n\"}\". It could just be:\n\n> + if (**arg == ')')\n> + break;\n"},{"id":"415619","messageId":"xmqqwnvvqp5o.fsf@gitster.c.googlers.com","threadId":"54195","inReplyTo":"fc5fd5217dfc105f3e03a9800a35209a985775a4.1611954543.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] pretty.c: refactor trailer logic to `format_set_trailers_options()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-29T23:49:55Z","receivedAt":"2021-01-29T23:51:08Z","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> Refactored trailers formatting logic inside pretty.c to a new function\n> `format_set_trailers_options()`. This change will allow us to reuse\n> the same logic in other places.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Heba Waly \n> Signed-off-by: Hariom Verma \n> ---\n> pretty.c | 85 ++++++++++++++++++++++++++++++--------------------------\n> pretty.h | 11 ++++++++\n> 2 files changed, 57 insertions(+), 39 deletions(-)\n>\n> diff --git a/pretty.c b/pretty.c\n> index 3922f6f9f24..bb6a3c634ac 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1148,6 +1148,50 @@ 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\tstruct strbuf *kvsepbuf,\n> +\t\t\t\tconst char **arg)\n> +{\n> +\tfor (;;) {\n> +\t\tconst char *argval;\n> +\t\tsize_t arglen;\n> +\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\nThe convention in this codebase is to signal unusual/error return\nwith a negative value, especially when a successful exit is signaled\nby returning 0. Perhaps return -1 from here instead?\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;\n> +\t\t\tfmt = 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_arg_value(*arg, \"key_value_separator\", arg, &argval, &arglen)) {\n> +\t\t\tchar *fmt;\n> +\t\t\tfmt = xstrndup(argval, arglen);\n> +\t\t\tstrbuf_expand(kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);\n> +\t\t\tfree(fmt);\n> +\t\t\topts->key_value_separator = kvsepbuf;\n\nIn these two else-if clauses, the original code clears sepbuf and\nkvsepbuf before calling strbuf_expand(), but this one does not.\n\nAs strbuf_expand() is an appending function, this distinction would\nmatter if the for(;;) loop causes these two else-if clauses to be\nentered twice. The original code will implement the \"last one wins\"\nsemantics, and this new one acumulates what it sees.\n\nIntended? If so, the reason why we want the accumulating semantics,\ninstead of the last-one-wins we've been using, needs to be explained\nin the log message.\n\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\nHere you can see that the original clears what was in sepbuf before\nreading the separator anew.\n\nThanks.\n"},{"id":"415622","messageId":"xmqqsg6jqomc.fsf@gitster.c.googlers.com","threadId":"54195","inReplyTo":"245e48eb6835cae4e61f65af780b766d990d4b5f.1611954543.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] pretty.c: capture invalid trailer argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-30T00:01:31Z","receivedAt":"2021-01-30T00:02:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Hariom Verma via GitGitGadget\" writes:\n\n> +\t\t\tsize_t invalid_arg_len = strcspn(*arg, \",)\");\n> +\t\t\t*invalid_arg = xstrndup(*arg, invalid_arg_len);\n> +\t\t\treturn 1;\n\nHow about doing this only when invalid_arg is not NULL, i.e.\n\n\t} else if (!match_placeholder_bool_arg(....) &&\n\t\t ...\n \t !match_placeholder_bool_arg(....)) {\n\t\tif (invalid_arg) {\n\t\t\tsize_t len = strcspn(*arg, \",)\");\n\t\t\t*invalid_arg = xstrndup(*arg, len);\n\t\t}\n\t\treturn -1;\n\t}\n\nNote that I just used 'len'; when the scope of a variable is so\nshort, it is clear the length of what thing it refers to from the\ncontext, and there is no point in using a variable name that long.\n\nIn any case, by doing so, the callers that are not interested in the\nreport can just pass NULL, which means ...\n\n> @@ -1464,12 +1472,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n> \t\tstruct strbuf sepbuf = STRBUF_INIT;\n> \t\tstruct strbuf kvsepbuf = STRBUF_INIT;\n> \t\tsize_t ret = 0;\n> +\t\tchar *unused = NULL;\n\n... this will become unneeded, and ...\n\n> \t\topts.no_divider = 1;\n> \n> \t\tif (*arg == ':') {\n> \t\t\targ++;\n> -\t\t\tif (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg))\n> +\t\t\tif (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg, &unused))\n> \t\t\t\tgoto trailer_out;\n\n... this will pass NULL, and ...\n\n> \t\t}\n> \t\tif (*arg == ')') {\n> @@ -1479,6 +1488,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\n... this will become unneeded.\n\n> \t\treturn ret;\n> \t}\n> \n> diff --git a/pretty.h b/pretty.h\n> index 7369cf7e148..d902cdd70a9 100644\n> --- a/pretty.h\n> +++ b/pretty.h\n> @@ -151,6 +151,7 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n> \t\t\tstruct string_list *filter_list,\n> \t\t\tstruct strbuf *sepbuf,\n> \t\t\tstruct strbuf *kvsepbuf,\n> -\t\t\tconst char **arg);\n> +\t\t\tconst char **arg,\n> +\t\t\tchar **invalid_arg);\n> \n> #endif /* PRETTY_H */\n"},{"id":"415623","messageId":"xmqqo8h7qoci.fsf@gitster.c.googlers.com","threadId":"54195","inReplyTo":"245e48eb6835cae4e61f65af780b766d990d4b5f.1611954543.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] pretty.c: capture invalid trailer argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-30T00:07:25Z","receivedAt":"2021-01-30T00:08:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Hariom Verma via GitGitGadget\" writes:\n\n> \t\t} else if (!match_placeholder_bool_arg(*arg, \"only\", arg, &opts->only_trailers) &&\n> \t\t\t !match_placeholder_bool_arg(*arg, \"unfold\", arg, &opts->unfold) &&\n> \t\t\t !match_placeholder_bool_arg(*arg, \"keyonly\", arg, &opts->key_only) &&\n> -\t\t\t !match_placeholder_bool_arg(*arg, \"valueonly\", arg, &opts->value_only))\n> -\t\t\tbreak;\n> +\t\t\t !match_placeholder_bool_arg(*arg, \"valueonly\", arg, &opts->value_only)) {\n> +\t\t\tsize_t invalid_arg_len = strcspn(*arg, \",)\");\n> +\t\t\t*invalid_arg = xstrndup(*arg, invalid_arg_len);\n> +\t\t\treturn 1;\n> +\t\t}\n> \t}\n> \treturn 0;\n> }\n\nWould the existing caller be OK with this change?\n\nIt used to be that this parsing code simply _ignored_ unrecognised\ntrailer keyword because the very original just did a \"break\" and\nfell though, but now because this returns non-zero, it causes the\ncaller rewritten in the patch [v2 1/3] to \"goto trailer_out\".\n\nIt is not clear from your proposed log message if this would result\nin behaviour change, and if so if that behaviour change was intended.\n\nI suspect that the behaviour change the code implements may be OK,\nbut the log message needs to discuss why it is OK.\n\nThanks.\n"},{"id":"415624","messageId":"xmqqk0rvql3a.fsf@gitster.c.googlers.com","threadId":"54195","inReplyTo":"pull.726.v2.git.1611954543.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] Unify trailers formatting logic for pretty.c and ref-filter.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-30T01:17:45Z","receivedAt":"2021-01-30T01:22:02Z","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 separate logic for %(trailers) in \"pretty.{c,h}\"\n> and \"ref-filter.{c,h}\". Both are actually doing the same thing, why not use\n> the same code for both of them?\n>\n> This is the 2nd version of the patch series that I sent few months back. It\n> is focused on unifying the \"%(trailers)\" logic for both 'pretty.{c,h}' and\n> 'ref-filter.{c,h}'. So, we can have one logic for trailers.\n>\n> v2 changes:\n>\n> * Contains Improvements as suggested by \"René Scharfe\" l.s.r@web.de 1\n> [https://public-inbox.org/git/bf4423d5-c0ee-6bef-59ff-fcde003ec463@web.de/]\n> * A new trailer option was introduced to pretty.c when I was absent i.e\n> \"key_value_separator\". Updated the patch series with latest changes.\n\nref-filter.c:74:3: error: symbol 'ref_trailer_buf' was not declared. Should it be static?\n SP refs/packed-backend.c\n SP refs/ref-cache.c\n\nFor now, I've queued this on the tip of the topic while queuing;\nrunning \"make SPARSE_FLAGS=-Wsparse-error sparse\" before sending\nyour series out may reduce embarrassment in the future.\n\nThanks.\n\n ref-filter.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 2b1c61eadf..0e414765c1 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -67,7 +67,7 @@ struct refname_atom {\n \tint lstrip, rstrip;\n };\n \n-struct ref_trailer_buf {\n+static struct ref_trailer_buf {\n \tstruct string_list filter_list;\n \tstruct strbuf sepbuf;\n \tstruct strbuf kvsepbuf;\n-- \n2.30.0-540-g714779256f\n\n"},{"id":"415625","messageId":"xmqqft2jqkli.fsf@gitster.c.googlers.com","threadId":"54195","inReplyTo":"pull.726.v2.git.1611954543.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] Unify trailers formatting logic for pretty.c and ref-filter.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-30T01:28:25Z","receivedAt":"2021-01-30T01:33:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"With this queued directly on top of master, I am getting these\nfailures.\n\nt6300-for-each-ref.sh (Wstat: 256 Tests: 301 Failed: 6)\n Failed tests: 277-280, 285, 287\n Non-zero exit status: 1\n\n"},{"id":"415661","messageId":"CA+CkUQ-_HQG4UtdZGGc3mpXB3OnsZ0kzAPZfLLP2sV1HY9tw6A@mail.gmail.com","threadId":"54195","inReplyTo":"xmqqsg6jqomc.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] pretty.c: capture invalid trailer argument","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2021-01-30T19:00:56Z","receivedAt":"2021-01-30T19:02:06Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi,\n\nOn Sat, Jan 30, 2021 at 5:31 AM Junio C Hamano wrote:\n>\n> \"Hariom Verma via GitGitGadget\" writes:\n>\n> > + size_t invalid_arg_len = strcspn(*arg, \",)\");\n> > + *invalid_arg = xstrndup(*arg, invalid_arg_len);\n> > + return 1;\n>\n> How about doing this only when invalid_arg is not NULL, i.e.\n>\n> } else if (!match_placeholder_bool_arg(....) &&\n> ...\n> !match_placeholder_bool_arg(....)) {\n> if (invalid_arg) {\n> size_t len = strcspn(*arg, \",)\");\n> *invalid_arg = xstrndup(*arg, len);\n> }\n> return -1;\n> }\n\nSounds like an improvement to the current version. Will change.\n\n> Note that I just used 'len'; when the scope of a variable is so\n> short, it is clear the length of what thing it refers to from the\n> context, and there is no point in using a variable name that long.\n>\n> In any case, by doing so, the callers that are not interested in the\n> report can just pass NULL, which means ...\n>\n> > @@ -1464,12 +1472,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n> > struct strbuf sepbuf = STRBUF_INIT;\n> > struct strbuf kvsepbuf = STRBUF_INIT;\n> > size_t ret = 0;\n> > + char *unused = NULL;\n>\n> ... this will become unneeded, and ...\n>\n> > opts.no_divider = 1;\n> >\n> > if (*arg == ':') {\n> > arg++;\n> > - if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg))\n> > + if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg, &unused))\n> > goto trailer_out;\n>\n> ... this will pass NULL, and ...\n>\n> > }\n> > if (*arg == ')') {\n> > @@ -1479,6 +1488,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n> > trailer_out:\n> > string_list_clear(&filter_list, 0);\n> > strbuf_release(&sepbuf);\n> > + free((char *)unused);\n>\n> ... this will become unneeded.\n>\n\nThanks,\nHariom Verma\n"},{"id":"415662","messageId":"CA+CkUQ_tyJMsxwopU=g-8keyK=Y1nzE1wgXnohQy3h8h_px4Nw@mail.gmail.com","threadId":"54195","inReplyTo":"xmqqo8h7qoci.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/3] pretty.c: capture invalid trailer argument","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2021-01-30T19:06:55Z","receivedAt":"2021-01-30T19:08:09Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi,\n\nOn Sat, Jan 30, 2021 at 5:37 AM Junio C Hamano wrote:\n>\n> \"Hariom Verma via GitGitGadget\" writes:\n>\n> > } else if (!match_placeholder_bool_arg(*arg, \"only\", arg, &opts->only_trailers) &&\n> > !match_placeholder_bool_arg(*arg, \"unfold\", arg, &opts->unfold) &&\n> > !match_placeholder_bool_arg(*arg, \"keyonly\", arg, &opts->key_only) &&\n> > - !match_placeholder_bool_arg(*arg, \"valueonly\", arg, &opts->value_only))\n> > - break;\n> > + !match_placeholder_bool_arg(*arg, \"valueonly\", arg, &opts->value_only)) {\n> > + size_t invalid_arg_len = strcspn(*arg, \",)\");\n> > + *invalid_arg = xstrndup(*arg, invalid_arg_len);\n> > + return 1;\n> > + }\n> > }\n> > return 0;\n> > }\n>\n> Would the existing caller be OK with this change?\n\nYes, I made sure of that.\n\n> It used to be that this parsing code simply _ignored_ unrecognised\n> trailer keyword because the very original just did a \"break\" and\n> fell though, but now because this returns non-zero, it causes the\n> caller rewritten in the patch [v2 1/3] to \"goto trailer_out\".\n>\n> It is not clear from your proposed log message if this would result\n> in behaviour change, and if so if that behaviour change was intended.\n>\n> I suspect that the behaviour change the code implements may be OK,\n> but the log message needs to discuss why it is OK.\n\nI should have included that change in behaviour in the commit message.\nWill change that too.\n\nThanks,\nHariom Verma\n"},{"id":"415663","messageId":"CA+CkUQ93OOd0PTu0N4dgwvm1kb7WP8GLFvGTXZvh=4=1KuE6=g@mail.gmail.com","threadId":"54195","inReplyTo":"xmqqft2jqkli.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/3] Unify trailers formatting logic for pretty.c and ref-filter.c","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2021-01-30T19:15:15Z","receivedAt":"2021-01-30T19:16:24Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi,\n\nOn Sat, Jan 30, 2021 at 6:58 AM Junio C Hamano wrote:\n>\n> With this queued directly on top of master, I am getting these\n> failures.\n>\n> t6300-for-each-ref.sh (Wstat: 256 Tests: 301 Failed: 6)\n> Failed tests: 277-280, 285, 287\n> Non-zero exit status: 1\n>\n\nThat's strange!\n\nAll 301 tests seem to work fine in my system.\n\nI'm clueless about these failures and can't think of any possible reason too.\n\nIs there any way I can re-generate this or can trace the part of code\nwhich caused these failures?\n\nThanks,\nHariom.\n"},{"id":"415664","messageId":"CA+CkUQ9sKyWJYahnZqfy1OfxxA+ukv148SCxjbGaOBzkCH0kbg@mail.gmail.com","threadId":"54195","inReplyTo":"CAP8UFD00sdiaFYUvgzQmXKCQSyrNKG82_xXvRGRaqdkbqKu7UQ@mail.gmail.com","subject":"Re: [PATCH v2 2/3] pretty.c: capture invalid trailer argument","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2021-01-30T19:16:56Z","receivedAt":"2021-01-30T19:18:08Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi Christian,\n\nOn Sat, Jan 30, 2021 at 3:58 AM Christian Couder\n wrote:\n>\n> On Fri, Jan 29, 2021 at 10:15 PM Hariom Verma via GitGitGadget\n> wrote:\n> >\n> > From: Hariom Verma \n> >\n> > As we would like to use this same logic in ref-filter, it's nice to\n> > get invalid trailer argument. This will allow us to print precise\n> > error message, while using `format_set_trailers_options()` in\n> > ref-filter.\n>\n> Thanks for continuing to work on this!\n>\n> > {\n> > for (;;) {\n> > const char *argval;\n> > size_t arglen;\n> >\n> > + if(**arg == ')') {\n> > + break;\n> > + }\n>\n> A space char is missing between \"if\" and \"(\". Also no need for \"{\" and\n> \"}\". It could just be:\n>\n> > + if (**arg == ')')\n> > + break;\n\nThanks for pointing this out. Will fix it.\n"},{"id":"415665","messageId":"xmqq35yip45z.fsf@gitster.c.googlers.com","threadId":"54195","inReplyTo":"xmqqft2jqkli.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/3] Unify trailers formatting logic for pretty.c and ref-filter.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-30T20:20:56Z","receivedAt":"2021-01-30T20:24:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> With this queued directly on top of master, I am getting these\n> failures.\n>\n> t6300-for-each-ref.sh (Wstat: 256 Tests: 301 Failed: 6)\n> Failed tests: 277-280, 285, 287\n> Non-zero exit status: 1\n\nJudging from the way the first failure happens, it appears that\nsome code depends on a kind of shell portability issues?\n\nThe failure is under GNU bash, version 5.1.4(1)-release (x86_64-pc-linux-gnu)\n\nAh, I think I know what is going on.\n\ntest_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/main >actual &&\n\t\ttest_cmp expect actual &&\n\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/main >actual &&\n\t\ttest_cmp expect actual\n\t'\n}\n\nNot quoting \"$expect\" given to 'echo' inside double-quote is\ncriminal, but I do not think that contributes to this particular\nfailure. The problem is in the callers.\n\ntest_trailer_option '%(trailers:key=foo) shows that trailer' \\\n\t'trailers:key=Signed-off-by' 'Signed-off-by: A U Thor \\n'\n\n\nDoing\n\n\texpect='foo\\n'\n\techo $expect\n\nand expecting that somebody would magically turn \\n to a true\nnewline is the bug. Not everybody's builtin \"echo\" works that way.\n\nHere is a quick fix-up. I didn't check which parts are to be blamed\non which patch of yours, if any, so you might need to split them up\ninto multiple pieces (i.e. a preliminary clean-up patch, and a patch\neach to be applied to part N/3 (1 <= N <= 3) of your patch).\n\n\n t/t6300-for-each-ref.sh | 22 +++++++++++++++-------\n 1 file changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git c/t/t6300-for-each-ref.sh w/t/t6300-for-each-ref.sh\nindex a8835b1391..0278cb9924 100755\n--- c/t/t6300-for-each-ref.sh\n+++ w/t/t6300-for-each-ref.sh\n@@ -874,7 +874,7 @@ test_trailer_option() {\n \toption=\"$2\"\n \texpect=\"$3\"\n \ttest_expect_success \"$title\" '\n-\t\techo $expect >expect &&\n+\t\techo \"$expect\" >expect &&\n \t\tgit for-each-ref --format=\"%($option)\" refs/heads/main >actual &&\n \t\ttest_cmp expect actual &&\n \t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/main >actual &&\n@@ -883,13 +883,18 @@ test_trailer_option() {\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+\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+\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+\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+\t'trailers:key=Reviewed-by:,key=Signed-off-by' 'Reviewed-by: A U Thor \n+Signed-off-by: A U Thor \n+'\n test_trailer_option '%(trailers:key=nonexistent) becomes empty' \\\n \t'trailers:key=Shined-off-by:' ''\n \n@@ -928,11 +933,14 @@ test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes non\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+\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 test_trailer_option '%(trailers:key_value_separator) changes key-value separator' \\\n-\t'trailers:key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor \\nSigned-off-by,A U Thor \\n'\n+\t'trailers:key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor \n+Signed-off-by,A U Thor \n+'\n test_trailer_option '%(trailers:separator,key_value_separator) changes both separators' \\\n \t'trailers:separator=%x2C,key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor ,Signed-off-by,A U Thor '\n \n\n\n\n"},{"id":"415666","messageId":"875z3ep30j.fsf@evledraar.gmail.com","threadId":"54195","inReplyTo":"7b8cfb2721c349f2bcebec98f84291b1cffd3b49.1611954543.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] ref-filter: use pretty.c logic for trailers","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-01-30T20:45:48Z","receivedAt":"2021-01-30T20:46:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jan 29 2021, Hariom Verma via GitGitGadget wrote:\n\n> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> index 2962f85a502..ea1f8417176 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -260,11 +260,40 @@ 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. To use comma as separator one must use\n> + `%x2C` as it would otherwise be parsed as next option. If separator\n> + option is given multiple times only the last one is used.\n> + E.g., `%(trailers:key=Ticket,separator=%x2C)` shows all trailer lines\n> + whose key is \"Ticket\" separated by a comma.\n> +** 'key_value_separator=': specify a separator inserted between\n> + key and value. The string SEP may contain the literal formatting codes.\n> + E.g., `%(trailers:key=Ticket,key_value_separator=%x2C)` shows all trailer\n> + lines whose key is \"Ticket\" with key and value separated by a comma.\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\nGiven that the goal of this series is to unify this parsing logic\nbetween log/for-each-ref, why do we need to then copy/paste the exact\nsame docs we have in pretty-formats.txt?\n\nAt the very least we should move this to pretty-formats-trailers.txt or\nsomething, and just include it in both places, or better yet just refer\nto the relevan parts of \"git log\"'s man page, no?\n\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index ca62e764b58..a8835b13915 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -825,14 +825,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 +\t\t\techo\n> +\t\t} >expect &&\n> +\t\tgit for-each-ref --format=\"%($option)\" refs/heads/main >actual &&\n> +\t\ttest_cmp expect actual &&\n> +\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/main >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 +\t\tcat trailers &&\n> \t\techo\n> \t} >expect &&\n> -\tgit for-each-ref --format=\"%(trailers:only)\" refs/heads/main >actual &&\n> +\tgit for-each-ref --format=\"%(trailers:only=no)\" refs/heads/main >actual &&\n> \ttest_cmp expect actual &&\n> -\tgit for-each-ref --format=\"%(contents:trailers:only)\" refs/heads/main >actual &&\n> +\tgit for-each-ref --format=\"%(contents:trailers:only=no)\" refs/heads/main >actual &&\n> \ttest_cmp expect actual\n> '\n> \n> @@ -851,17 +869,92 @@ 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/main >actual &&\n> +\t\ttest_cmp expect actual &&\n> +\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/main >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 +\t\techo\n> +\t} >expect &&\n> +\tgit for-each-ref --format=\"%(trailers:key=Acked-by)\" refs/heads/main >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit for-each-ref --format=\"%(contents:trailers:key=Acked-by)\" refs/heads/main >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '\n> +\t{\n> +\t\tunfold +\t\techo\n> +\t} >expect &&\n> +\tgit for-each-ref --format=\"%(trailers:key=Signed-Off-by,unfold)\" refs/heads/main >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit for-each-ref --format=\"%(contents:trailers:key=Signed-Off-by,unfold)\" refs/heads/main >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 +\t\techo\n> +\t} >expect &&\n> +\tgit for-each-ref --format=\"%(trailers:key=Signed-off-by,only=no)\" refs/heads/main >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit for-each-ref --format=\"%(contents:trailers:key=Signed-off-by,only=no)\" refs/heads/main >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 '7\n> +test_trailer_option '%(trailers:key_value_separator) changes key-value separator' \\\n> +\t'trailers:key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor \\nSigned-off-by,A U Thor \\n'\n> +test_trailer_option '%(trailers:separator,key_value_separator) changes both separators' \\\n> +\t'trailers:separator=%x2C,key_value_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/main 2>actual &&\n> +\t\ttest_i18ncmp expect actual &&\n> +\t\ttest_must_fail git for-each-ref --format=\"%(contents:$option)\" refs/heads/main 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 colon is missing' '\n> \tcat >expect <<-EOF &&\n> \tfatal: unrecognized %(contents) argument: trailersonly\n\nAnd similarly, here we have now mostly duplicated tests for this between\nhere and t/t4205-log-pretty-formats.sh.\n\nI think the right thing to do is to start by moving the tests that are\nnow in t/t4205-log-pretty-formats.sh relevant to this formatting into\nits own file or something.\n\nThen instead of duplicating the tests here, just prepare them to be\nchanged so that we can add both \"git log\" and a \"git for-each-ref\"\ninvocation to some for-loop, so we'll test both.\n"},{"id":"416162","messageId":"CA+CkUQ_YDxF+fphzyQRD1OkFh7NGEmHUABvRiAjL-H52MHyH3Q@mail.gmail.com","threadId":"54195","inReplyTo":"875z3ep30j.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 3/3] ref-filter: use pretty.c logic for trailers","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2021-02-04T18:46:41Z","receivedAt":"2021-02-04T18:48:21Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi,\n\nOn Sun, Jan 31, 2021 at 2:15 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> Given that the goal of this series is to unify this parsing logic\n> between log/for-each-ref, why do we need to then copy/paste the exact\n> same docs we have in pretty-formats.txt?\n>\n> At the very least we should move this to pretty-formats-trailers.txt or\n> something, and just include it in both places, or better yet just refer\n> to the relevan parts of \"git log\"'s man page, no?\n\nOk. I will refer to the trailers part of \"pretty-formats\"'s man page\nin \"git-for-each-ref\"'s man page.\n\n> And similarly, here we have now mostly duplicated tests for this between\n> here and t/t4205-log-pretty-formats.sh.\n>\n> I think the right thing to do is to start by moving the tests that are\n> now in t/t4205-log-pretty-formats.sh relevant to this formatting into\n> its own file or something.\n>\n> Then instead of duplicating the tests here, just prepare them to be\n> changed so that we can add both \"git log\" and a \"git for-each-ref\"\n> invocation to some for-loop, so we'll test both.\n\nWith this unified trailer logic, \"git log\" and \"git for-each-ref\"\nstill behave differently.\nFor e.g.: \"git log\" does nothing for unknown/incorrect trailer option,\nwhereas \"git for-each-ref\" stops.\n\nEven if we move trailer related tests for both into a new file, I\nguess we still need to test trailers for both \"git log\" and \"git\nfor-each-ref\" separately?\n\nThanks,\nHariom.\n"},{"id":"416195","messageId":"87sg6bd06k.fsf@evledraar.gmail.com","threadId":"54195","inReplyTo":"CA+CkUQ_YDxF+fphzyQRD1OkFh7NGEmHUABvRiAjL-H52MHyH3Q@mail.gmail.com","subject":"Re: [PATCH v2 3/3] ref-filter: use pretty.c logic for trailers","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-04T20:53:39Z","receivedAt":"2021-02-04T20:54:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 04 2021, Hariom verma wrote:\n\n> Hi,\n>\n> On Sun, Jan 31, 2021 at 2:15 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>> Given that the goal of this series is to unify this parsing logic\n>> between log/for-each-ref, why do we need to then copy/paste the exact\n>> same docs we have in pretty-formats.txt?\n>>\n>> At the very least we should move this to pretty-formats-trailers.txt or\n>> something, and just include it in both places, or better yet just refer\n>> to the relevan parts of \"git log\"'s man page, no?\n>\n> Ok. I will refer to the trailers part of \"pretty-formats\"'s man page\n> in \"git-for-each-ref\"'s man page.\n\nSure, FWIW you can also (not saying it has to be this) include the same\nsection in both, maybe with some blurb on the top saying it's not\ndifferent between the two...\n\n>> And similarly, here we have now mostly duplicated tests for this between\n>> here and t/t4205-log-pretty-formats.sh.\n>>\n>> I think the right thing to do is to start by moving the tests that are\n>> now in t/t4205-log-pretty-formats.sh relevant to this formatting into\n>> its own file or something.\n>>\n>> Then instead of duplicating the tests here, just prepare them to be\n>> changed so that we can add both \"git log\" and a \"git for-each-ref\"\n>> invocation to some for-loop, so we'll test both.\n>\n> With this unified trailer logic, \"git log\" and \"git for-each-ref\"\n> still behave differently.\n> For e.g.: \"git log\" does nothing for unknown/incorrect trailer option,\n> whereas \"git for-each-ref\" stops.\n>\n> Even if we move trailer related tests for both into a new file, I\n> guess we still need to test trailers for both \"git log\" and \"git\n> for-each-ref\" separately?\n\nWe have a few tests that define a test function to test these sorts of\ncases, t/t3070-wildmatch.sh is one, t/t3800-mktag.sh another.\n\nSo you can just do:\n\n test_trailers A '%(trailers:keyonly)' 'Signed-off-by' 'ERR: error from for-each-ref' # (or whatever)\n\nAnd make the \"test_trailers\" function do the common setup, have both\n\"log\" and \"for-each-ref\" look at the \"A\" tag and assert what their\noutput is, respectively (or an error, or whatever).\n\nI think that's especially valuable in cases where you have similar\ncodepaths, because it makes it easy for both the author and reviewers to\neyeball intended an unintended differences.\n"},{"id":"416334","messageId":"pull.726.v3.git.1612602945.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":"pull.726.v2.git.1611954543.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] Unify trailers formatting logic for pretty.c and ref-filter.c","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-06T09:15:41Z","receivedAt":"2021-02-06T09:17:11Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"Currently, there exists a separate logic for %(trailers) in \"pretty.{c,h}\"\nand \"ref-filter.{c,h}\". Both are actually doing the same thing, why not use\nthe same code for both of them?\n\nThis is the 3rd version of the patch series. It is focused on unifying the\n\"%(trailers)\" logic for both 'pretty.{c,h}' and 'ref-filter.{c,h}'. So, we\ncan have one logic for trailers.\n\nv3 changes:\n\n * replaced echo with printf in the tests failing in previous version for\n consistent behaviour.\n * strbuf_reset() is back in format_set_trailers_options().\n * made struct ref_trailer_buf static.\n * initialised structure variable ref_trailer_buf . so we may not encounter\n any problem while doing strbuf_reset().\n * refer to the trailers part of \"git-log\"'s man page in\n \"git-for-each-ref\"'s man page.\n * improved commit messages.\n\n/* TODO */\n\nAs suggested by Ævar Arnfjörð Bjarmason avarab@gmail.com here\n[https://public-inbox.org/git/875z3ep30j.fsf@evledraar.gmail.com/], I plan\nto unify the trailers related tests for \"git log\" and \"git for-each-ref\" in\nnew file. Maybe on top of this patch series?\n\nHariom Verma (3):\n pretty.c: refactor trailer logic to `format_set_trailers_options()`\n pretty.c: capture invalid trailer argument\n ref-filter: use pretty.c logic for trailers\n\n Documentation/git-for-each-ref.txt | 8 +-\n pretty.c | 98 ++++++++++++++----------\n pretty.h | 12 +++\n ref-filter.c | 36 +++++----\n t/t6300-for-each-ref.sh | 119 +++++++++++++++++++++++++----\n 5 files changed, 200 insertions(+), 73 deletions(-)\n\n\nbase-commit: e6362826a0409539642a5738db61827e5978e2e4\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-726%2Fharry-hov%2Funify-trailers-logic-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-726/harry-hov/unify-trailers-logic-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/726\n\nRange-diff vs v2:\n\n 1: fc5fd5217dfc ! 1: 81030f00b11b pretty.c: refactor trailer logic to `format_set_trailers_options()`\n @@ Commit message\n pretty.c: refactor trailer logic to `format_set_trailers_options()`\n \n Refactored trailers formatting logic inside pretty.c to a new function\n - `format_set_trailers_options()`. This change will allow us to reuse\n - the same logic in other places.\n + `format_set_trailers_options()`. This new function returns the non-zero\n + in case of unusual. The caller handles the non-zero by \"goto trailers_out\".\n +\n + This change will allow us to reuse the same logic in other places.\n \n Mentored-by: Christian Couder \n Mentored-by: Heba Waly \n @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)\n +\t\t\tuintptr_t len = arglen;\n +\n +\t\t\tif (!argval)\n -+\t\t\t\treturn 1;\n ++\t\t\t\treturn -1;\n +\n +\t\t\tif (len && argval[len - 1] == ':')\n +\t\t\t\tlen--;\n @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)\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;\n ++\n ++\t\t\tstrbuf_reset(sepbuf);\n +\t\t\tfmt = 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_arg_value(*arg, \"key_value_separator\", arg, &argval, &arglen)) {\n +\t\t\tchar *fmt;\n ++\n ++\t\t\tstrbuf_reset(kvsepbuf);\n +\t\t\tfmt = xstrndup(argval, arglen);\n +\t\t\tstrbuf_expand(kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);\n +\t\t\tfree(fmt);\n 2: 245e48eb6835 ! 2: f4a6b2df1444 pretty.c: capture invalid trailer argument\n @@ Metadata\n ## Commit message ##\n pretty.c: capture invalid trailer argument\n \n - As we would like to use this same logic in ref-filter, it's nice to\n - get invalid trailer argument. This will allow us to print precise\n - error message, while using `format_set_trailers_options()` in\n + As we would like to use this trailers logic in the ref-filter, it's\n + nice to get an invalid trailer argument. This will allow us to print\n + precise error message while using `format_set_trailers_options()` in\n ref-filter.\n \n + For capturing the invalid argument, we changed the working of\n + `format_set_trailers_options()` a little bit.\n + Original logic does \"break\" and fell through in mainly 2 cases -\n + 1. unknown/invalid argument\n + 2. end of the arg string\n +\n + But now instead of \"break\", we capture invalid argument and return\n + non-zero. And non-zero is handled by the caller.\n + (We prepared the caller to handle non-zero in the previous commit).\n +\n + Capturing invalid arguments this way will also affects the working\n + of current logic. As at the end of the arg string it will return non-zero.\n + So in order to make things correct, introduced an additional conditional\n + statement i.e if encounter \")\", do 'break'.\n +\n Mentored-by: Christian Couder \n Mentored-by: Heba Waly \n Signed-off-by: Hariom Verma \n @@ pretty.c: int format_set_trailers_options(struct process_trailer_options *opts,\n \t\tconst char *argval;\n \t\tsize_t arglen;\n \n -+\t\tif(**arg == ')') {\n ++\t\tif (**arg == ')')\n +\t\t\tbreak;\n -+\t\t}\n +\n \t\tif (match_placeholder_arg_value(*arg, \"key\", arg, &argval, &arglen)) {\n \t\t\tuintptr_t len = arglen;\n @@ pretty.c: int format_set_trailers_options(struct process_trailer_options *opts,\n -\t\t\t !match_placeholder_bool_arg(*arg, \"valueonly\", arg, &opts->value_only))\n -\t\t\tbreak;\n +\t\t\t !match_placeholder_bool_arg(*arg, \"valueonly\", arg, &opts->value_only)) {\n -+\t\t\tsize_t invalid_arg_len = strcspn(*arg, \",)\");\n -+\t\t\t*invalid_arg = xstrndup(*arg, invalid_arg_len);\n -+\t\t\treturn 1;\n ++\t\t\tif (invalid_arg) {\n ++\t\t\t\tsize_t len = strcspn(*arg, \",)\");\n ++\t\t\t\t*invalid_arg = xstrndup(*arg, len);\n ++\t\t\t}\n ++\t\t\treturn -1;\n +\t\t}\n \t}\n \treturn 0;\n }\n @@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n - \t\tstruct strbuf sepbuf = STRBUF_INIT;\n - \t\tstruct strbuf kvsepbuf = STRBUF_INIT;\n - \t\tsize_t ret = 0;\n -+\t\tchar *unused = NULL;\n - \n - \t\topts.no_divider = 1;\n \n \t\tif (*arg == ':') {\n \t\t\targ++;\n -\t\t\tif (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg))\n -+\t\t\tif (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg, &unused))\n ++\t\t\tif (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg, NULL))\n \t\t\t\tgoto trailer_out;\n \t\t}\n \t\tif (*arg == ')') {\n -@@ pretty.c: 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 - \n \n ## pretty.h ##\n @@ pretty.h: int format_set_trailers_options(struct process_trailer_options *opts,\n 3: 7b8cfb2721c3 ! 3: 47d89f872314 ref-filter: use pretty.c logic for trailers\n @@ Commit message\n :key= - only show trailers with specified key.\n :valueonly[=val] - only show the value part.\n :separator= - inserted between trailer lines.\n - :key_value_separator= - inserted between trailer lines\n + :key_value_separator= - inserted between key and value in trailer lines\n \n Enhancement to existing options(now can take value and its optional):\n :only[=val]\n @@ Documentation/git-for-each-ref.txt: contents:lines=N::\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. To use comma as separator one must use\n -+ `%x2C` as it would otherwise be parsed as next option. If separator\n -+ option is given multiple times only the last one is used.\n -+ E.g., `%(trailers:key=Ticket,separator=%x2C)` shows all trailer lines\n -+ whose key is \"Ticket\" separated by a comma.\n -+** 'key_value_separator=': specify a separator inserted between\n -+ key and value. The string SEP may contain the literal formatting codes.\n -+ E.g., `%(trailers:key=Ticket,key_value_separator=%x2C)` shows all trailer\n -+ lines whose key is \"Ticket\" with key and value separated by a comma.\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 ++`contents:trailers[:options]`). For valid [:option] values see `trailers`\n ++section of linkgit:git-log[1].\n \n For sorting purposes, fields with numeric values sort in numeric order\n (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`).\n @@ ref-filter.c: struct refname_atom {\n \tint lstrip, rstrip;\n };\n \n -+struct ref_trailer_buf {\n ++static struct ref_trailer_buf {\n +\tstruct string_list filter_list;\n +\tstruct strbuf sepbuf;\n +\tstruct strbuf kvsepbuf;\n -+} ref_trailer_buf;\n ++} ref_trailer_buf = {STRING_LIST_INIT_NODUP, STRBUF_INIT, STRBUF_INIT};\n +\n static struct expand_data {\n \tstruct object_id oid;\n @@ ref-filter.c: static int subject_atom_parser(const struct ref_format *format, st\n +\t\tchar *invalid_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&ref_trailer_buf.kvsepbuf,\n -+\t\t\t&argbuf, &invalid_arg)) {\n ++\t\t &ref_trailer_buf.filter_list,\n ++\t\t &ref_trailer_buf.sepbuf,\n ++\t\t &ref_trailer_buf.kvsepbuf,\n ++\t\t &argbuf, &invalid_arg)) {\n +\t\t\tif (!invalid_arg)\n +\t\t\t\tstrbuf_addf(err, _(\"expected %%(trailers:key=)\"));\n +\t\t\telse\n @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un\n +\toption=\"$2\"\n +\texpect=\"$3\"\n +\ttest_expect_success \"$title\" '\n -+\t\techo $expect >expect &&\n ++\t\tprintf \"$expect\\n\" >expect &&\n +\t\tgit for-each-ref --format=\"%($option)\" refs/heads/main >actual &&\n +\t\ttest_cmp expect actual &&\n +\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/main >actual &&\n\n-- \ngitgitgadget\n"},{"id":"416335","messageId":"81030f00b11b39d7fa665c71d3bbe61203e84b54.1612602945.git.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":"pull.726.v3.git.1612602945.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] pretty.c: refactor trailer logic to `format_set_trailers_options()`","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-06T09:15:42Z","receivedAt":"2021-02-06T09:17:32Z","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()`. This new function returns the non-zero\nin case of unusual. The caller handles the non-zero by \"goto trailers_out\".\n\nThis change will allow us to reuse the same logic in other places.\n\nMentored-by: Christian Couder \nMentored-by: Heba Waly \nSigned-off-by: Hariom Verma \n---\n pretty.c | 89 +++++++++++++++++++++++++++++++-------------------------\n pretty.h | 11 +++++++\n 2 files changed, 61 insertions(+), 39 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 3922f6f9f249..59cefdddf674 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1148,6 +1148,54 @@ 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\tstruct strbuf *kvsepbuf,\n+\t\t\t\tconst char **arg)\n+{\n+\tfor (;;) {\n+\t\tconst char *argval;\n+\t\tsize_t arglen;\n+\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;\n+\n+\t\t\tstrbuf_reset(sepbuf);\n+\t\t\tfmt = 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_arg_value(*arg, \"key_value_separator\", arg, &argval, &arglen)) {\n+\t\t\tchar *fmt;\n+\n+\t\t\tstrbuf_reset(kvsepbuf);\n+\t\t\tfmt = xstrndup(argval, arglen);\n+\t\t\tstrbuf_expand(kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);\n+\t\t\tfree(fmt);\n+\t\t\topts->key_value_separator = kvsepbuf;\n+\t\t} else if (!match_placeholder_bool_arg(*arg, \"only\", arg, &opts->only_trailers) &&\n+\t\t\t !match_placeholder_bool_arg(*arg, \"unfold\", arg, &opts->unfold) &&\n+\t\t\t !match_placeholder_bool_arg(*arg, \"keyonly\", arg, &opts->key_only) &&\n+\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@@ -1425,45 +1473,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\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_arg_value(arg, \"key_value_separator\", &arg, &argval, &arglen)) {\n-\t\t\t\t\tchar *fmt;\n-\n-\t\t\t\t\tstrbuf_reset(&kvsepbuf);\n-\t\t\t\t\tfmt = xstrndup(argval, arglen);\n-\t\t\t\t\tstrbuf_expand(&kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);\n-\t\t\t\t\tfree(fmt);\n-\t\t\t\t\topts.key_value_separator = &kvsepbuf;\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, \"keyonly\", &arg, &opts.key_only) &&\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, &kvsepbuf, &arg))\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);\ndiff --git a/pretty.h b/pretty.h\nindex 7ce6c0b437b4..7369cf7e1484 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@@ -142,4 +143,14 @@ int commit_format_is_empty(enum cmit_fmt);\n /* Make subject of commit message suitable for filename */\n void format_sanitized_subject(struct strbuf *sb, const char *msg, size_t len);\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\tstruct string_list *filter_list,\n+\t\t\tstruct strbuf *sepbuf,\n+\t\t\tstruct strbuf *kvsepbuf,\n+\t\t\tconst char **arg);\n+\n #endif /* PRETTY_H */\n-- \ngitgitgadget\n\n"},{"id":"416336","messageId":"f4a6b2df14443e9a010b86066c6dad043966ab44.1612602945.git.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":"pull.726.v3.git.1612602945.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] pretty.c: capture invalid trailer argument","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-06T09:15:43Z","receivedAt":"2021-02-06T09:17:32Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nAs we would like to use this trailers logic in the ref-filter, it's\nnice to get an invalid trailer argument. This will allow us to print\nprecise error message while using `format_set_trailers_options()` in\nref-filter.\n\nFor capturing the invalid argument, we changed the working of\n`format_set_trailers_options()` a little bit.\nOriginal logic does \"break\" and fell through in mainly 2 cases -\n 1. unknown/invalid argument\n 2. end of the arg string\n\nBut now instead of \"break\", we capture invalid argument and return\nnon-zero. And non-zero is handled by the caller.\n(We prepared the caller to handle non-zero in the previous commit).\n\nCapturing invalid arguments this way will also affects the working\nof current logic. As at the end of the arg string it will return non-zero.\nSo in order to make things correct, introduced an additional conditional\nstatement i.e if encounter \")\", do 'break'.\n\nMentored-by: Christian Couder \nMentored-by: Heba Waly \nSigned-off-by: Hariom Verma \n---\n pretty.c | 17 +++++++++++++----\n pretty.h | 3 ++-\n 2 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 59cefdddf674..ed16b32df922 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1152,12 +1152,16 @@ 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\tstruct strbuf *kvsepbuf,\n-\t\t\t\tconst char **arg)\n+\t\t\t\tconst char **arg,\n+\t\t\t\tchar **invalid_arg)\n {\n \tfor (;;) {\n \t\tconst char *argval;\n \t\tsize_t arglen;\n \n+\t\tif (**arg == ')')\n+\t\t\tbreak;\n+\n \t\tif (match_placeholder_arg_value(*arg, \"key\", arg, &argval, &arglen)) {\n \t\t\tuintptr_t len = arglen;\n \n@@ -1190,8 +1194,13 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n \t\t} else if (!match_placeholder_bool_arg(*arg, \"only\", arg, &opts->only_trailers) &&\n \t\t\t !match_placeholder_bool_arg(*arg, \"unfold\", arg, &opts->unfold) &&\n \t\t\t !match_placeholder_bool_arg(*arg, \"keyonly\", arg, &opts->key_only) &&\n-\t\t\t !match_placeholder_bool_arg(*arg, \"valueonly\", arg, &opts->value_only))\n-\t\t\tbreak;\n+\t\t\t !match_placeholder_bool_arg(*arg, \"valueonly\", arg, &opts->value_only)) {\n+\t\t\tif (invalid_arg) {\n+\t\t\t\tsize_t len = strcspn(*arg, \",)\");\n+\t\t\t\t*invalid_arg = xstrndup(*arg, len);\n+\t\t\t}\n+\t\t\treturn -1;\n+\t\t}\n \t}\n \treturn 0;\n }\n@@ -1473,7 +1482,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \n \t\tif (*arg == ':') {\n \t\t\targ++;\n-\t\t\tif (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg))\n+\t\t\tif (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg, NULL))\n \t\t\t\tgoto trailer_out;\n \t\t}\n \t\tif (*arg == ')') {\ndiff --git a/pretty.h b/pretty.h\nindex 7369cf7e1484..d902cdd70a95 100644\n--- a/pretty.h\n+++ b/pretty.h\n@@ -151,6 +151,7 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n \t\t\tstruct string_list *filter_list,\n \t\t\tstruct strbuf *sepbuf,\n \t\t\tstruct strbuf *kvsepbuf,\n-\t\t\tconst char **arg);\n+\t\t\tconst char **arg,\n+\t\t\tchar **invalid_arg);\n \n #endif /* PRETTY_H */\n-- \ngitgitgadget\n\n"},{"id":"416337","messageId":"47d89f872314cad6dc6010ff3c8ade43a70bc540.1612602945.git.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":"pull.726.v3.git.1612602945.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] ref-filter: use pretty.c logic for trailers","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-06T09:15:44Z","receivedAt":"2021-02-06T09:17:32Z","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 :key_value_separator= - inserted between key and value in 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 | 8 +-\n ref-filter.c | 36 +++++----\n t/t6300-for-each-ref.sh | 119 +++++++++++++++++++++++++----\n 3 files changed, 129 insertions(+), 34 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 2962f85a502a..2ae2478de706 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -260,11 +260,9 @@ 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]`). For valid [:option] values see `trailers`\n+section of linkgit:git-log[1].\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 ee337df232a5..4dc4882cc768 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -67,6 +67,12 @@ struct refname_atom {\n \tint lstrip, rstrip;\n };\n \n+static struct ref_trailer_buf {\n+\tstruct string_list filter_list;\n+\tstruct strbuf sepbuf;\n+\tstruct strbuf kvsepbuf;\n+} ref_trailer_buf = {STRING_LIST_INIT_NODUP, STRBUF_INIT, STRBUF_INIT};\n+\n static struct expand_data {\n \tstruct object_id oid;\n \tenum object_type type;\n@@ -313,28 +319,26 @@ 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\tchar *invalid_arg = NULL;\n+\n+\t\tif (format_set_trailers_options(&atom->u.contents.trailer_opts,\n+\t\t &ref_trailer_buf.filter_list,\n+\t\t &ref_trailer_buf.sepbuf,\n+\t\t &ref_trailer_buf.kvsepbuf,\n+\t\t &argbuf, &invalid_arg)) {\n+\t\t\tif (!invalid_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\"), invalid_arg);\n+\t\t\tfree((char *)invalid_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 ca62e764b586..4b3745839c86 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -825,14 +825,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/main >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/main >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/main >actual &&\n+\tgit for-each-ref --format=\"%(trailers:only=no)\" refs/heads/main >actual &&\n \ttest_cmp expect actual &&\n-\tgit for-each-ref --format=\"%(contents:trailers:only)\" refs/heads/main >actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:only=no)\" refs/heads/main >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -851,17 +869,92 @@ 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\tprintf \"$expect\\n\" >expect &&\n+\t\tgit for-each-ref --format=\"%($option)\" refs/heads/main >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/main >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/main >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:key=Acked-by)\" refs/heads/main >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/main >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:key=Signed-Off-by,unfold)\" refs/heads/main >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/main >actual &&\n+\ttest_cmp expect actual &&\n+\tgit for-each-ref --format=\"%(contents:trailers:key=Signed-off-by,only=no)\" refs/heads/main >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+test_trailer_option '%(trailers:key_value_separator) changes key-value separator' \\\n+\t'trailers:key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor \\nSigned-off-by,A U Thor \\n'\n+test_trailer_option '%(trailers:separator,key_value_separator) changes both separators' \\\n+\t'trailers:separator=%x2C,key_value_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/main 2>actual &&\n+\t\ttest_i18ncmp expect actual &&\n+\t\ttest_must_fail git for-each-ref --format=\"%(contents:$option)\" refs/heads/main 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 colon is missing' '\n \tcat >expect <<-EOF &&\n \tfatal: unrecognized %(contents) argument: trailersonly\n-- \ngitgitgadget\n"},{"id":"416345","messageId":"xmqqim74a6x1.fsf@gitster.c.googlers.com","threadId":"54195","inReplyTo":"pull.726.v3.git.1612602945.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/3] Unify trailers formatting logic for pretty.c and ref-filter.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-07T03:33:14Z","receivedAt":"2021-02-07T03:34:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Hariom Verma via GitGitGadget\" writes:\n\n> @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un\n> +\toption=\"$2\"\n> +\texpect=\"$3\"\n> +\ttest_expect_success \"$title\" '\n> -+\t\techo $expect >expect &&\n> ++\t\tprintf \"$expect\\n\" >expect &&\n\nAre we sure that \"$expect\" would not ever have any '%' in it, to\nconfuse printf? To be future-proof and safe, it would be prudent to\ninstead use\n\n\tprintf \"%s\\n\" \"$expect\"\n\nto make sure that whatever is passed in $3 gets output LITERALLY.\n\nThe callers need to adopt the change I gave you in the review of the\nprevious round so that they do not assume backslash-en will by\nchanged to LF by somebody---instead if they mean LF, they just say\nLF.\n\nThanks.\n\n\n\n\n\n\t\n"},{"id":"416346","messageId":"xmqqy2g08o0r.fsf@gitster.c.googlers.com","threadId":"54195","inReplyTo":"xmqqim74a6x1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 0/3] Unify trailers formatting logic for pretty.c and ref-filter.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-07T05:06:44Z","receivedAt":"2021-02-07T05:08:03Z","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>> @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un\n>> +\toption=\"$2\"\n>> +\texpect=\"$3\"\n>> +\ttest_expect_success \"$title\" '\n>> -+\t\techo $expect >expect &&\n>> ++\t\tprintf \"$expect\\n\" >expect &&\n>\n> Are we sure that \"$expect\" would not ever have any '%' in it, to\n> confuse printf?\n\nJust to make sure we won't waste your time in useless roundtrip(s),\nlet me say that possible unacceptable answers are:\n\n - no, right now nobody passes a % in it\n - no, I do not expect anybody needs to pass a % in it\n - when somebody really needs to pass a %, they can write it as %%\n\nThe last one is the worst one, by the way.\n\nThe point of adding a test_trailer_option HELPER function is to HELP\nthe developers who write tests, now and in the future. There are\nsome things they MUST know to use the helper successfully, like it\ntakes three parameters, the first one being the test title, the\nsecond one is the string you'd give as the \"--format=\"\noption to the for-each-ref command, and the third one is the\nexpected output.\n\nForcing them to know any more than that is *not* helping them.\n\nThe shell programming language is perfectly capable of passing an\nargument that happens to be a multi-line string to functions and\nexternal commands, and the developers who are writing test knows\nthat already (the last argument to test_expect_success used\neverywhere in the test suite, that is a multi-line code snippet, is\na good example). When they need to write an expected output that is\ntwo lines, they expect to be able to write things like\n\n\ttest_trailer_option title format-string \\\n\t'expected line #1\n\texpected line #2'\n\nwithout having to worry about the need for special formatting that\nis applicable *only* when passing argument to this helper. They do\nnot need to know that they cannot pass backslash-en literally, and\nhave to say '\\\\n' instead, or they have to double a per-cent sign,\nonly when using this helper but not other helper functions.\n\nIn the message I am replying to, I used\n\n\tprintf \"%s\\n\" \"$expect\"\n\nfor a reason. We expect that trailer options output are complete\nlines, so it is annoying to force the caller to write the final\nnewline, especially if many of the callers have only one line of\nexpected output. So\n\n test_trailer_option title format-string 'expected output'\n\nwould end up doing\n\n\tprintf \"%s\\n\" \"expected output\" >expect\n\nto write a complete line, i.e. a caller does not have to say any of\n\n test_trailer_option title format-string 'expected output\\n'\n\n test_trailer_option title format-string 'expected output\n '\n\n lf='\n '\n test_trailer_option title format-string \"expected output$lf\"\n\nIf we do not need to extend test_trailer_option with further\n\"features\" (like \"check output case insensitively this time\" or\n\"allow output lines in any order\"), we can make it even nicer and\neasier to use for callers, by the way.\n\nFor example, with this (by the way, make sure there are SP on both\nsides around \"()\", that's our house style):\n\n\ttest_trailer_option () {\n\t\ttitle=$1 option=$2\n\t\tshift 2\n\t\tif test $# != 0\n\t\tthen\n\t\t\tprintf \"%s\\n\" \"$@\"\n\t\tfi >expect\n\t\ttest_expect_success \"$title\" '\n\t\t\t... >actual &&\n\t\t\ttest_cmp expect actual &&\n\t\t\t... >actual &&\n\t\t\ttest_cmp expect actual\n\t\t'\n\t}\n\nthe caller can do\n\n\ttest_trailer_option 'single line output' \\\n\t\t'trailers:key=Signed-off-by' \\\n\t\t'Signed-off-by: A U Thor '\n\n\ttest_trailer_option 'expect two lines' \\\n\t\t'trailers:key=Reviewed-by' \\\n\t\t'Reviewed-by: A U Thor ' \\\n\t\t'Reviewed-by: R E Viewer '\n\n\ttest_trailer_option 'no output expected' 'trailers:key=no-such:' ''\n\nThat is, instead of \"the first arg is title, the second is format\nand the third is the entire expected output\", the helper's manual\ncan say \"give title and format as the first and the second argument.\nEach argument after that is an expected output, one line per arg.\"\n\nAnother possibility is to feed the expected output from the standard\ninput of the helper, e.g.\n\n\ttest_trailer_option () {\n\t\ttitle=$1 option=$2\n\t\tcat >expect\n\t\ttest_expect_success \"$title\" '\n\t\t\t... >actual &&\n\t\t\ttest_cmp expect actual &&\n\t\t\t... >actual &&\n\t\t\ttest_cmp expect actual\n\t\t'\n\t}\n\nAnd the caller can now do:\n\n\ttest_trailer_option 'expect two lines' 'trailers:key=Reviewed-by' <<-\\EOF\n Reviewed-by: A U Thor \n Reviewed-by: R E Viewer \n\tEOF\n\nIt is a bit cumbersome when the expected output is a single line:\n\n\ttest_trailer_option 'single line output' 'trailers:key=Signed-off-by' <<-\\EOF\n\tSigned-off-by: A U Thor \n\tEOF\n\nbut the contrast between the \"two-line expected\" case and this one\nwould be easy to see when reading the tests. The pattern to write\n\"expect no output\" would be quite simple, too:\n\n\ttest_trailer_option 'no output expected' 'trailers:key=no-such:' writes:\n\n> +test_trailer_option() {\n> +\ttitle=\"$1\"\n> +\toption=\"$2\"\n> +\texpect=\"$3\"\n> +\ttest_expect_success \"$title\" '\n> +\t\tprintf \"$expect\\n\" >expect &&\n> +\t\tgit for-each-ref --format=\"%($option)\" refs/heads/main >actual &&\n> +\t\ttest_cmp expect actual &&\n> +\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/main >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\nThis is *not* an issue about the test script and its helper\nfunction, but I just noticed that --format=\"%(trailers:key=)\"\nis expected to write the matching trailers *AND* an empty line, and\nI wonder if that is a sensible thing to expect.\n\nThe \"--pretty\" side does not give such an extra blank line after the\noutput, though.\n\n $ git show -s --pretty=format:\"%(trailers:key=Signed-off-by:)\" \\\n js/range-diff-wo-dotdot\n Signed-off-by: Johannes Schindelin \n Signed-off-by: Junio C Hamano \n $ git show -s --pretty=format:\"%(trailers:key=None:)\" \\\n js/range-diff-wo-dotdot\n $ exit\n\nUnlike the above, when there is no matching trailer lines, the\n\"for-each-ref\" in this series shows zero lines, and when there is\none matching trailer line, it gives that single line plus an empty\nline, two lines in total. The inconsistency is a bit disturbing.\n\nIs the extra blank line given on purpose? I don't see why we would\nwant it. Or is it a bug we did not catch during the previous two\nrounds of reviews?\n\nThanks.\n"},{"id":"416350","messageId":"CA+CkUQ9-OCiEkMDRTpyF3rp-g1mSSzn4s9MgqJZ2BJY=XJCoEw@mail.gmail.com","threadId":"54195","inReplyTo":"xmqqpn1c8m7u.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 3/3] ref-filter: use pretty.c logic for trailers","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2021-02-07T12:06:45Z","receivedAt":"2021-02-07T12:08:09Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi,\n\nOn Sun, Feb 7, 2021 at 11:15 AM Junio C Hamano wrote:\n>\n> \"Hariom Verma via GitGitGadget\" writes:\n>\n> > +test_trailer_option() {\n> > + title=\"$1\"\n> > + option=\"$2\"\n> > + expect=\"$3\"\n> > + test_expect_success \"$title\" '\n> > + printf \"$expect\\n\" >expect &&\n> > + git for-each-ref --format=\"%($option)\" refs/heads/main >actual &&\n> > + test_cmp expect actual &&\n> > + git for-each-ref --format=\"%(contents:$option)\" refs/heads/main >actual &&\n> > + test_cmp expect actual\n> > + '\n> > +}\n> > +\n> > +test_trailer_option '%(trailers:key=foo) shows that trailer' \\\n> > + 'trailers:key=Signed-off-by' 'Signed-off-by: A U Thor \\n'\n>\n> This is *not* an issue about the test script and its helper\n> function, but I just noticed that --format=\"%(trailers:key=)\"\n> is expected to write the matching trailers *AND* an empty line, and\n> I wonder if that is a sensible thing to expect.\n>\n> The \"--pretty\" side does not give such an extra blank line after the\n> output, though.\n>\n> $ git show -s --pretty=format:\"%(trailers:key=Signed-off-by:)\" \\\n> js/range-diff-wo-dotdot\n> Signed-off-by: Johannes Schindelin \n> Signed-off-by: Junio C Hamano \n> $ git show -s --pretty=format:\"%(trailers:key=None:)\" \\\n> js/range-diff-wo-dotdot\n> $ exit\n>\n> Unlike the above, when there is no matching trailer lines, the\n> \"for-each-ref\" in this series shows zero lines, and when there is\n> one matching trailer line, it gives that single line plus an empty\n> line, two lines in total. The inconsistency is a bit disturbing.\n>\n> Is the extra blank line given on purpose? I don't see why we would\n> want it. Or is it a bug we did not catch during the previous two\n> rounds of reviews?\n\nI don't think that \"extra blank line\" is due to this patch series.\nWait. Let me see.\n\nSince \"for-each-ref\"'s original code does not support\n\"trailers:key=\", Let's check original code for \"trailers:only\":\n```\n $ git for-each-ref refs/heads/master --format=\"%(trailers:only)\"\n Signed-off-by: Junio C Hamano \n\n $ exit\n```\nI see. The original code also gives an extra blank line.\n\nNow, let's check for this patch series:\n```\n $ ./bin-wrappers/git for-each-ref refs/heads/master\n--format=\"%(trailers:key=Signed-off-by)\"\n Signed-off-by: Junio C Hamano \n\n $ ./bin-wrappers/git for-each-ref refs/heads/master\n--format=\"%(trailers:key=None)\"\n\n $ exit\n```\nwhen there is no matching trailer lines, the \"for-each-ref\" in this\nseries shows one empty line, and when there is one matching trailer\nline, it gives that single line plus an empty line, two lines in\ntotal. Seems consistent to me.\n\nSo this isn't about the patch series. Question still remains the same.\nWhy extra blank line?\nLet's dig a bit.\nAh. I guess I found the reason. It's due to `putchar('\\n');` in\n`show_ref_array_item() [1]`. It puts a new line after each ref item.\n\nDo you want me to include a patch to get rid of this \"extra blank\nline\" for trailers in \"for-each-ref\"?\n\nThanks,\nHariom.\n\n[1]: https://github.com/git/git/blob/fb7fa4a1fd273f22efcafdd13c7f897814fd1eb9/ref-filter.c#L2435\n"},{"id":"416363","messageId":"xmqqh7mn91w2.fsf@gitster.c.googlers.com","threadId":"54195","inReplyTo":"CA+CkUQ9-OCiEkMDRTpyF3rp-g1mSSzn4s9MgqJZ2BJY=XJCoEw@mail.gmail.com","subject":"Re: [PATCH v3 3/3] ref-filter: use pretty.c logic for trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-07T18:19:25Z","receivedAt":"2021-02-07T18:20:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hariom verma writes:\n\n> So this isn't about the patch series. Question still remains the same.\n\nThanks for digging the history.\n\n> Why extra blank line?\n> Let's dig a bit.\n> Ah. I guess I found the reason. It's due to `putchar('\\n');` in\n> `show_ref_array_item() [1]`. It puts a new line after each ref item.\n>\n> Do you want me to include a patch to get rid of this \"extra blank\n> line\" for trailers in \"for-each-ref\"?\n\nI do not know the answer to the last question, because we haven't\nlearned the original reason why we decided to add the extra blank\nline after the trailer output. Even though I find it unnecessary,\nthe code that adds it must have been written with a good reason to\ndo so, and I do not want to see us remove the \"\\n\" without knowing\nthat reason.\n\nThanks.\n\n"},{"id":"416378","messageId":"CA+CkUQ9kHhbDVMru=pRO90o+k7cc_ykxN9JRFGMvoG3hkeGJpA@mail.gmail.com","threadId":"54195","inReplyTo":"xmqqh7mn91w2.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 3/3] ref-filter: use pretty.c logic for trailers","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2021-02-07T19:38:26Z","receivedAt":"2021-02-07T19:39:22Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi,\n\nOn Sun, Feb 7, 2021 at 11:49 PM Junio C Hamano wrote:\n>\n> Hariom verma writes:\n>\n> > Do you want me to include a patch to get rid of this \"extra blank\n> > line\" for trailers in \"for-each-ref\"?\n>\n> I do not know the answer to the last question, because we haven't\n> learned the original reason why we decided to add the extra blank\n> line after the trailer output. Even though I find it unnecessary,\n> the code that adds it must have been written with a good reason to\n> do so, and I do not want to see us remove the \"\\n\" without knowing\n> that reason.\n\nAs per my understanding it works something like this:\n\nprint a ref item... put newline... print a ref item... put newline..\nprint a ref item... put newline... (so on)\n\nBut the catch is that trailer comes with a newline already included.\nSo it becomes:\n\nprint trailers with newline included... put newline... print trailers\nwith newline included... put newline.. (so on)\n\nSo we end up having 2 new lines in total.\n\nwe just can't directly remove the newline. but we introduce an option\nto skip at will. Something like this?\nhttps://github.com/harry-hov/git/commit/af75f5c9b0325af90831998f56d6f36b6baa928e\n\nSo we can turn off newline(extra) for trailers without disturbing\n\"for-each-ref\"'s working.\n\nThanks,\nHariom.\n"},{"id":"416381","messageId":"xmqqlfbz7i7i.fsf@gitster.c.googlers.com","threadId":"54195","inReplyTo":"CA+CkUQ9kHhbDVMru=pRO90o+k7cc_ykxN9JRFGMvoG3hkeGJpA@mail.gmail.com","subject":"Re: [PATCH v3 3/3] ref-filter: use pretty.c logic for trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-07T20:09:53Z","receivedAt":"2021-02-07T20:10:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hariom verma writes:\n\n> As per my understanding it works something like this:\n>\n> print a ref item... put newline... print a ref item... put newline..\n> print a ref item... put newline... (so on)\n>\n> But the catch is that trailer comes with a newline already included.\n> So it becomes:\n>\n> print trailers with newline included... put newline... print trailers\n> with newline included... put newline.. (so on)\n\nWe know how it happens. The question is if that is a sensible\nbehaviour, and if the trailing blank line was _intended_, or a bug\nthat nobody has complained about so far.\n\n> we just can't directly remove the newline.\n\nIf we agree that the current behaviour is *not* sensible, then we\ncan. On the \"log --pretty\" side, we have \"terminator semantics\" and\n\"separator semantics\" between \"tformat\" and \"format\", when showing\nmore than one commits in a row, the \"terminator semantics\" places\none blank line after each commit we emit, while the \"separator\nsemantics\" gives one blank line between each commit pair. I think\nwe initially (incorrectly) used terminator semantics and our output\nfor two commits looked like \"CommitA CommitB \" before\nwe fixed it to use separator semantics to show \"CommitA CommitB\"\nwithout the useless trailing blank line. We can apply the same principle\nwhen \"fixing\" this issue to show a block of trailer lines (that is, the\nchange in behaviour to remove the trailing blank line turns out to\nbe a \"fix\").\n"},{"id":"416419","messageId":"CA+CkUQ_cdUmuP+_yUeCytn=6cc8SjMBE1aTLzWJL-U_V01uzog@mail.gmail.com","threadId":"54195","inReplyTo":"xmqqlfbz7i7i.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 3/3] ref-filter: use pretty.c logic for trailers","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2021-02-08T17:07:38Z","receivedAt":"2021-02-08T17:10:35Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi,\n\nOn Mon, Feb 8, 2021 at 1:39 AM Junio C Hamano wrote:\n>\n> If we agree that the current behaviour is *not* sensible, then we\n> can. On the \"log --pretty\" side, we have \"terminator semantics\" and\n> \"separator semantics\" between \"tformat\" and \"format\", when showing\n> more than one commits in a row, the \"terminator semantics\" places\n> one blank line after each commit we emit, while the \"separator\n> semantics\" gives one blank line between each commit pair. I think\n> we initially (incorrectly) used terminator semantics and our output\n> for two commits looked like \"CommitA CommitB \" before\n> we fixed it to use separator semantics to show \"CommitA CommitB\"\n> without the useless trailing blank line. We can apply the same principle\n> when \"fixing\" this issue to show a block of trailer lines (that is, the\n> change in behaviour to remove the trailing blank line turns out to\n> be a \"fix\").\n\nI suspect that \"fix\" for \"log --pretty\" isn't going to work here.\n\nEven if we apply the same \"log --pretty\"'s fix here. I think we still\nend up having an empty blank line between each ref item.\n\nThank,\nHariom\n"},{"id":"416422","messageId":"xmqqv9b25s7f.fsf@gitster.c.googlers.com","threadId":"54195","inReplyTo":"CA+CkUQ_cdUmuP+_yUeCytn=6cc8SjMBE1aTLzWJL-U_V01uzog@mail.gmail.com","subject":"Re: [PATCH v3 3/3] ref-filter: use pretty.c logic for trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-08T18:29:08Z","receivedAt":"2021-02-08T18:30:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hariom verma writes:\n\n> I suspect that \"fix\" for \"log --pretty\" isn't going to work here.\n>\n> Even if we apply the same \"log --pretty\"'s fix here. I think we still\n> end up having an empty blank line between each ref item.\n\nAfter sleeping on it and seeing a result of an experiment like\nthis one, I think that might be unavoidable.\n\n $ git for-each-ref \\\n\t--format=\"One%0a%(trailers:key=Signed-off-by:)Two%0a\" \\\n\trefs/heads/js/range-diff-wo-dotdot\n One\n Signed-off-by: Johannes Schindelin \n Signed-off-by: Junio C Hamano \n Two\n $ exit\n\nPeople who write such \"Two\" without prefixing it with a newline\n\"%0a\" themselves may view such a \"fix\" a regression.\n\nIt is sad that this %(trailers) itself is relatively a new thing,\nand I had thought that all the other ingredients are designed to\nstrip the trailing newline, e.g. try this:\n\n $ git for-each-ref \\\n\t--format=\"%(subject)%0a%(trailers:key=Signed-off-by:)\" \\\n\trefs/heads/js/range-diff-wo-dotdot\n range-diff(docs): explain how to specify commit ranges\n Signed-off-by: Johannes Schindelin \n Signed-off-by: Junio C Hamano \n\nNotice that %(subject) is followed explicitly by %0a. I think\n%(author:date), etc. would do the same. But %(trailers) behave\ndifferently, and that is because it expects to be multi-line and\nperhaps to mimic %(body)? In any case, it may be too late to change\nits behaviour. At least I do not think of a good waoy to do so.\n\nBy the way, when merged to 'seen' (you can try the above that shows\n%(subject) followed by %(trailers) with the tip of 'seen'), it dies\nlike this:\n\n $ git for-each-ref \\\n\t--format=\"%(subject)%0a%(trailers:key=Signed-off-by:)\" \\\n\trefs/heads/js/range-diff-wo-dotdot\n free(): double free detected in tcache 2\n Aborted\n\nThere must be some interaction with another topic but I didn't dig\ndeeper.\n\nThanks.\n\n"},{"id":"416461","messageId":"YCHMhYLuFYZBWjQM@camp.crustytoothpaste.net","threadId":"54195","inReplyTo":"xmqqlfby5o9h.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 3/3] ref-filter: use pretty.c logic for trailers","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-02-08T23:43:01Z","receivedAt":"2021-02-08T23:44:19Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-02-08 at 19:54:18, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > By the way, when merged to 'seen' (you can try the above that shows\n> > %(subject) followed by %(trailers) with the tip of 'seen'), it dies\n> > like this:\n> >\n> > $ git for-each-ref \\\n> > \t--format=\"%(subject)%0a%(trailers:key=Signed-off-by:)\" \\\n> > \trefs/heads/js/range-diff-wo-dotdot\n> > free(): double free detected in tcache 2\n> > Aborted\n> >\n> > There must be some interaction with another topic but I didn't dig\n> > deeper.\n> \n> It seems brian's bc/signed-objects-with-both-hashes topic alone has\n> the double-free issue, without the \"trailers\" topic.\n> \n> $ git checkout --detach bc/signed-objects-with-both-hashes\n> $ make git\n> $ ./git for-each-ref --format='%(subject)%(body)' refs/heads/maint\n> free(): double free detected in tcache 2\n> Aborted\n> \n> So for now, you do not have to worry about it in your topic. Of\n> course, you are very much welcome to help debugging and fixing it\n> ;-)\n\nI'll take a look. Thanks for the heads up.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"416468","messageId":"YCH71+ck1Wmk1Css@camp.crustytoothpaste.net","threadId":"54195","inReplyTo":"xmqqlfby5o9h.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 3/3] ref-filter: use pretty.c logic for trailers","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-02-09T03:04:55Z","receivedAt":"2021-02-09T03:08:29Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-02-08 at 19:54:18, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > By the way, when merged to 'seen' (you can try the above that shows\n> > %(subject) followed by %(trailers) with the tip of 'seen'), it dies\n> > like this:\n> >\n> > $ git for-each-ref \\\n> > \t--format=\"%(subject)%0a%(trailers:key=Signed-off-by:)\" \\\n> > \trefs/heads/js/range-diff-wo-dotdot\n> > free(): double free detected in tcache 2\n> > Aborted\n> >\n> > There must be some interaction with another topic but I didn't dig\n> > deeper.\n> \n> It seems brian's bc/signed-objects-with-both-hashes topic alone has\n> the double-free issue, without the \"trailers\" topic.\n> \n> $ git checkout --detach bc/signed-objects-with-both-hashes\n> $ make git\n> $ ./git for-each-ref --format='%(subject)%(body)' refs/heads/maint\n> free(): double free detected in tcache 2\n> Aborted\n> \n> So for now, you do not have to worry about it in your topic. Of\n> course, you are very much welcome to help debugging and fixing it\n> ;-)\n\nI'll send out a fixed patch tomorrow, but for the moment, here's the\ngist of the change if you want to an immediate fix to squash in:\n\n------- %< ---------\ndiff --git a/ref-filter.c b/ref-filter.c\nindex e6c8106377..5f8a443be5 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1344,8 +1344,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)\n \t\t} else if (atom->u.contents.option == C_BARE)\n \t\t\tv->s = xstrdup(subpos);\n \n-\t\tfree((void *)sigpos);\n \t}\n+\tfree((void *)sigpos);\n }\n \n /*\n------- %< ---------\n\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"416559","messageId":"xmqqh7ml0xop.fsf@gitster.c.googlers.com","threadId":"54195","inReplyTo":"YCH71+ck1Wmk1Css@camp.crustytoothpaste.net","subject":"Re: [PATCH v3 3/3] ref-filter: use pretty.c logic for trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-09T20:54:14Z","receivedAt":"2021-02-09T21:43:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> I'll send out a fixed patch tomorrow, but for the moment, here's the\n> gist of the change if you want to an immediate fix to squash in:\n>\n> ------- %< ---------\n> diff --git a/ref-filter.c b/ref-filter.c\n> index e6c8106377..5f8a443be5 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1344,8 +1344,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)\n> \t\t} else if (atom->u.contents.option == C_BARE)\n> \t\t\tv->s = xstrdup(subpos);\n> \n> -\t\tfree((void *)sigpos);\n> \t}\n> +\tfree((void *)sigpos);\n> }\n\nAh, I see. find_subpos() will only called once to find the subject\nand signature in the loop, and the finding will have to live even\nthe current iteration of the loop is done, only to be released after\neverything is done.\n\nMakes sense.\n"},{"id":"416827","messageId":"410b02dbad20c77662dd4581f9985783ca08fc70.1613181163.git.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":"pull.726.v4.git.1613181163.gitgitgadget@gmail.com","subject":"[PATCH v4 1/4] t6300: use function to test trailer options","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-13T01:52:40Z","receivedAt":"2021-02-13T01:53:44Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nAdd a function to test trailer options. This will make tests look cleaner,\nas well as will make it easier to add new tests for trailers in the future.\n\nMentored-by: Christian Couder \nMentored-by: Heba Waly \nSigned-off-by: Hariom Verma \n---\n t/t6300-for-each-ref.sh | 90 +++++++++++++++++++++--------------------\n 1 file changed, 47 insertions(+), 43 deletions(-)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex ca62e764b586..a8faddd18a9b 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -814,53 +814,57 @@ test_expect_success 'set up trailers for next test' '\n \tEOF\n '\n \n-test_expect_success '%(trailers:unfold) unfolds trailers' '\n-\t{\n-\t\tunfold expect &&\n-\tgit for-each-ref --format=\"%(trailers:unfold)\" refs/heads/main >actual &&\n-\ttest_cmp expect actual &&\n-\tgit for-each-ref --format=\"%(contents:trailers:unfold)\" refs/heads/main >actual &&\n-\ttest_cmp expect actual\n-'\n+test_trailer_option () {\n+\ttitle=$1 option=$2\n+\tcat >expect\n+\ttest_expect_success \"$title\" '\n+\t\tgit for-each-ref --format=\"%($option)\" refs/heads/main >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/main >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+}\n \n-test_expect_success '%(trailers:only) shows only \"key: value\" trailers' '\n-\t{\n-\t\tgrep -v patch.description expect &&\n-\tgit for-each-ref --format=\"%(trailers:only)\" refs/heads/main >actual &&\n-\ttest_cmp expect actual &&\n-\tgit for-each-ref --format=\"%(contents:trailers:only)\" refs/heads/main >actual &&\n-\ttest_cmp expect actual\n-'\n+test_trailer_option '%(trailers:unfold) unfolds trailers' \\\n+\t'trailers:unfold' <<-EOF\n+\t$(unfold expect &&\n-\tgit for-each-ref --format=\"%(trailers:only,unfold)\" refs/heads/main >actual &&\n-\ttest_cmp expect actual &&\n-\tgit for-each-ref --format=\"%(trailers:unfold,only)\" refs/heads/main >actual &&\n-\ttest_cmp actual actual &&\n-\tgit for-each-ref --format=\"%(contents:trailers:only,unfold)\" refs/heads/main >actual &&\n-\ttest_cmp expect actual &&\n-\tgit for-each-ref --format=\"%(contents:trailers:unfold,only)\" refs/heads/main >actual &&\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+\tEOF\n+\n+test_trailer_option '%(trailers:only) shows only \"key: value\" trailers' \\\n+\t'trailers:only' <<-EOF\n+\t$(grep -v patch.description expect\n+\ttest_expect_success \"$title\" '\n+\t\t# error message cannot be checked under i18n\n+\t\ttest_must_fail git for-each-ref --format=\"%($option)\" refs/heads/main 2>actual &&\n+\t\ttest_i18ncmp expect actual &&\n+\t\ttest_must_fail git for-each-ref --format=\"%(contents:$option)\" refs/heads/main 2>actual &&\n+\t\ttest_i18ncmp expect actual\n+\t'\n+}\n+\n+test_failing_trailer_option '%(trailers) rejects unknown trailers arguments' \\\n+\t'trailers:unsupported' <<-\\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-'\n \n test_expect_success 'if arguments, %(contents:trailers) shows error if colon is missing' '\n \tcat >expect <<-EOF &&\n-- \ngitgitgadget\n\n"},{"id":"416828","messageId":"pull.726.v4.git.1613181163.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":"pull.726.v3.git.1612602945.gitgitgadget@gmail.com","subject":"[PATCH v4 0/4] Unify trailers formatting logic for pretty.c and ref-filter.c","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-13T01:52:39Z","receivedAt":"2021-02-13T01:53:44Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"Currently, there exists a separate logic for %(trailers) in \"pretty.{c,h}\"\nand \"ref-filter.{c,h}\". Both are actually doing the same thing, why not use\nthe same code for both of them?\n\nThis is the 4th version of the patch series focused on unifying the\n\"%(trailers)\" logic for both 'pretty.{c,h}' and 'ref-filter.{c,h}'. So, we\ncan have one logic for trailers.\n\nv4 changes:\n\n * improved tests\n\nHariom Verma (4):\n t6300: use function to test trailer options\n pretty.c: refactor trailer logic to `format_set_trailers_options()`\n pretty.c: capture invalid trailer argument\n ref-filter: use pretty.c logic for trailers\n\n Documentation/git-for-each-ref.txt | 8 +-\n pretty.c | 98 +++++++++------\n pretty.h | 12 ++\n ref-filter.c | 36 +++---\n t/t6300-for-each-ref.sh | 185 ++++++++++++++++++++++-------\n 5 files changed, 236 insertions(+), 103 deletions(-)\n\n\nbase-commit: 328c10930387d301560f7cbcd3351cc485a13381\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-726%2Fharry-hov%2Funify-trailers-logic-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-726/harry-hov/unify-trailers-logic-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/726\n\nRange-diff vs v3:\n\n -: ------------ > 1: 410b02dbad20 t6300: use function to test trailer options\n 1: 81030f00b11b = 2: fd275fed8347 pretty.c: refactor trailer logic to `format_set_trailers_options()`\n 2: f4a6b2df1444 = 3: 073c75dc4494 pretty.c: capture invalid trailer argument\n 3: 47d89f872314 ! 4: 9ec989176993 ref-filter: use pretty.c logic for trailers\n @@ ref-filter.c: static int subject_atom_parser(const struct ref_format *format, st\n \n \n ## t/t6300-for-each-ref.sh ##\n -@@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:unfold) unfolds trailers' '\n - \ttest_cmp expect actual\n - '\n +@@ t/t6300-for-each-ref.sh: test_trailer_option '%(trailers:only) shows only \"key: value\" trailers' \\\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/main >actual &&\n -+\t\ttest_cmp expect actual &&\n -+\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/main >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/main >actual &&\n -+\tgit for-each-ref --format=\"%(trailers:only=no)\" refs/heads/main >actual &&\n - \ttest_cmp expect actual &&\n --\tgit for-each-ref --format=\"%(contents:trailers:only)\" refs/heads/main >actual &&\n -+\tgit for-each-ref --format=\"%(contents:trailers:only=no)\" refs/heads/main >actual &&\n - \ttest_cmp expect actual\n - '\n + \tEOF\n \n -@@ t/t6300-for-each-ref.sh: 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\tprintf \"$expect\\n\" >expect &&\n -+\t\tgit for-each-ref --format=\"%($option)\" refs/heads/main >actual &&\n -+\t\ttest_cmp expect actual &&\n -+\t\tgit for-each-ref --format=\"%(contents:$option)\" refs/heads/main >actual &&\n -+\t\ttest_cmp expect actual\n -+\t'\n -+}\n ++test_trailer_option '%(trailers:only=no,only=true) shows only \"key: value\" trailers' \\\n ++\t'trailers:only=no,only=true' <<-EOF\n ++\t$(grep -v patch.description \\n'\n ++\t'trailers:key=Signed-off-by' <<-EOF\n ++\tSigned-off-by: A U Thor \n ++\n ++\tEOF\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 ++\t'trailers:key=SiGned-oFf-bY' <<-EOF\n ++\tSigned-off-by: A U Thor \n ++\n ++\tEOF\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 ++\t'trailers:key=Signed-off-by:' <<-EOF\n ++\tSigned-off-by: A U Thor \n ++\n ++\tEOF\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 ++\t'trailers:key=Reviewed-by:,key=Signed-off-by' <<-EOF\n ++\tReviewed-by: A U Thor \n ++\tSigned-off-by: A U Thor \n ++\n ++\tEOF\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/main >actual &&\n -+\ttest_cmp expect actual &&\n -+\tgit for-each-ref --format=\"%(contents:trailers:key=Acked-by)\" refs/heads/main >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/main >actual &&\n -+\ttest_cmp expect actual &&\n -+\tgit for-each-ref --format=\"%(contents:trailers:key=Signed-Off-by,unfold)\" refs/heads/main >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/main >actual &&\n -+\ttest_cmp expect actual &&\n -+\tgit for-each-ref --format=\"%(contents:trailers:key=Signed-off-by,only=no)\" refs/heads/main >actual &&\n -+\ttest_cmp expect actual\n -+'\n ++\t'trailers:key=Shined-off-by:' <<-EOF\n ++\n ++\tEOF\n ++\n ++test_trailer_option '%(trailers:key=foo) handles multiple lines even if folded' \\\n ++\t'trailers:key=Acked-by' <<-EOF\n ++\t$(grep -v patch.description \n ++\t$(grep patch.description \\n'\n ++\t'trailers:key=Signed-off-by,valueonly' <<-EOF\n ++\tA U Thor \n ++\n ++\tEOF\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 ++\t'trailers:separator=%x2C,key=Reviewed-by,key=Signed-off-by:' <<-EOF\n ++\tReviewed-by: A U Thor ,Signed-off-by: A U Thor \n ++\tEOF\n ++\n +test_trailer_option '%(trailers:key_value_separator) changes key-value separator' \\\n -+\t'trailers:key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor \\nSigned-off-by,A U Thor \\n'\n ++\t'trailers:key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' <<-EOF\n ++\tReviewed-by,A U Thor \n ++\tSigned-off-by,A U Thor \n ++\n ++\tEOF\n ++\n +test_trailer_option '%(trailers:separator,key_value_separator) changes both separators' \\\n -+\t'trailers:separator=%x2C,key_value_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/main 2>actual &&\n -+\t\ttest_i18ncmp expect actual &&\n -+\t\ttest_must_fail git for-each-ref --format=\"%(contents:$option)\" refs/heads/main 2>actual &&\n -+\t\ttest_i18ncmp expect actual\n -+\t'\n -+}\n ++\t'trailers:separator=%x2C,key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' <<-EOF\n ++\tReviewed-by,A U Thor ,Signed-off-by,A U Thor \n ++\tEOF\n +\n + test_failing_trailer_option () {\n + \ttitle=$1 option=$2\n + \tcat >expect\n +@@ t/t6300-for-each-ref.sh: test_failing_trailer_option '%(trailers) rejects unknown trailers arguments' \\\n + \tfatal: unknown %(trailers) argument: unsupported\n + \tEOF\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 ++\t'trailers:key' <<-\\EOF\n ++\tfatal: expected %(trailers:key=)\n ++\tEOF\n +\n test_expect_success 'if arguments, %(contents:trailers) shows error if colon is missing' '\n \tcat >expect <<-EOF &&\n\n-- \ngitgitgadget\n"},{"id":"416829","messageId":"fd275fed834780514a7026ee64468b31496d72cc.1613181163.git.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":"pull.726.v4.git.1613181163.gitgitgadget@gmail.com","subject":"[PATCH v4 2/4] pretty.c: refactor trailer logic to `format_set_trailers_options()`","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-13T01:52:41Z","receivedAt":"2021-02-13T01:53:45Z","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()`. This new function returns the non-zero\nin case of unusual. The caller handles the non-zero by \"goto trailers_out\".\n\nThis change will allow us to reuse the same logic in other places.\n\nMentored-by: Christian Couder \nMentored-by: Heba Waly \nSigned-off-by: Hariom Verma \n---\n pretty.c | 89 +++++++++++++++++++++++++++++++-------------------------\n pretty.h | 11 +++++++\n 2 files changed, 61 insertions(+), 39 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex b4ff3f602f9b..304b73068bc4 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1149,6 +1149,54 @@ 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\tstruct strbuf *kvsepbuf,\n+\t\t\t\tconst char **arg)\n+{\n+\tfor (;;) {\n+\t\tconst char *argval;\n+\t\tsize_t arglen;\n+\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;\n+\n+\t\t\tstrbuf_reset(sepbuf);\n+\t\t\tfmt = 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_arg_value(*arg, \"key_value_separator\", arg, &argval, &arglen)) {\n+\t\t\tchar *fmt;\n+\n+\t\t\tstrbuf_reset(kvsepbuf);\n+\t\t\tfmt = xstrndup(argval, arglen);\n+\t\t\tstrbuf_expand(kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);\n+\t\t\tfree(fmt);\n+\t\t\topts->key_value_separator = kvsepbuf;\n+\t\t} else if (!match_placeholder_bool_arg(*arg, \"only\", arg, &opts->only_trailers) &&\n+\t\t\t !match_placeholder_bool_arg(*arg, \"unfold\", arg, &opts->unfold) &&\n+\t\t\t !match_placeholder_bool_arg(*arg, \"keyonly\", arg, &opts->key_only) &&\n+\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@@ -1429,45 +1477,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\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_arg_value(arg, \"key_value_separator\", &arg, &argval, &arglen)) {\n-\t\t\t\t\tchar *fmt;\n-\n-\t\t\t\t\tstrbuf_reset(&kvsepbuf);\n-\t\t\t\t\tfmt = xstrndup(argval, arglen);\n-\t\t\t\t\tstrbuf_expand(&kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);\n-\t\t\t\t\tfree(fmt);\n-\t\t\t\t\topts.key_value_separator = &kvsepbuf;\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, \"keyonly\", &arg, &opts.key_only) &&\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, &kvsepbuf, &arg))\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);\ndiff --git a/pretty.h b/pretty.h\nindex 7ce6c0b437b4..7369cf7e1484 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@@ -142,4 +143,14 @@ int commit_format_is_empty(enum cmit_fmt);\n /* Make subject of commit message suitable for filename */\n void format_sanitized_subject(struct strbuf *sb, const char *msg, size_t len);\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\tstruct string_list *filter_list,\n+\t\t\tstruct strbuf *sepbuf,\n+\t\t\tstruct strbuf *kvsepbuf,\n+\t\t\tconst char **arg);\n+\n #endif /* PRETTY_H */\n-- \ngitgitgadget\n\n"},{"id":"416830","messageId":"073c75dc4494db3074e426a751595ea83467fece.1613181163.git.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":"pull.726.v4.git.1613181163.gitgitgadget@gmail.com","subject":"[PATCH v4 3/4] pretty.c: capture invalid trailer argument","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-13T01:52:42Z","receivedAt":"2021-02-13T01:53:45Z","isPatch":true,"sender":{"key":"name:Hariom Verma","avatar":null},"body":"From: Hariom Verma \n\nAs we would like to use this trailers logic in the ref-filter, it's\nnice to get an invalid trailer argument. This will allow us to print\nprecise error message while using `format_set_trailers_options()` in\nref-filter.\n\nFor capturing the invalid argument, we changed the working of\n`format_set_trailers_options()` a little bit.\nOriginal logic does \"break\" and fell through in mainly 2 cases -\n 1. unknown/invalid argument\n 2. end of the arg string\n\nBut now instead of \"break\", we capture invalid argument and return\nnon-zero. And non-zero is handled by the caller.\n(We prepared the caller to handle non-zero in the previous commit).\n\nCapturing invalid arguments this way will also affects the working\nof current logic. As at the end of the arg string it will return non-zero.\nSo in order to make things correct, introduced an additional conditional\nstatement i.e if encounter \")\", do 'break'.\n\nMentored-by: Christian Couder \nMentored-by: Heba Waly \nSigned-off-by: Hariom Verma \n---\n pretty.c | 17 +++++++++++++----\n pretty.h | 3 ++-\n 2 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 304b73068bc4..c5f5ecc40d3f 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1153,12 +1153,16 @@ 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\tstruct strbuf *kvsepbuf,\n-\t\t\t\tconst char **arg)\n+\t\t\t\tconst char **arg,\n+\t\t\t\tchar **invalid_arg)\n {\n \tfor (;;) {\n \t\tconst char *argval;\n \t\tsize_t arglen;\n \n+\t\tif (**arg == ')')\n+\t\t\tbreak;\n+\n \t\tif (match_placeholder_arg_value(*arg, \"key\", arg, &argval, &arglen)) {\n \t\t\tuintptr_t len = arglen;\n \n@@ -1191,8 +1195,13 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n \t\t} else if (!match_placeholder_bool_arg(*arg, \"only\", arg, &opts->only_trailers) &&\n \t\t\t !match_placeholder_bool_arg(*arg, \"unfold\", arg, &opts->unfold) &&\n \t\t\t !match_placeholder_bool_arg(*arg, \"keyonly\", arg, &opts->key_only) &&\n-\t\t\t !match_placeholder_bool_arg(*arg, \"valueonly\", arg, &opts->value_only))\n-\t\t\tbreak;\n+\t\t\t !match_placeholder_bool_arg(*arg, \"valueonly\", arg, &opts->value_only)) {\n+\t\t\tif (invalid_arg) {\n+\t\t\t\tsize_t len = strcspn(*arg, \",)\");\n+\t\t\t\t*invalid_arg = xstrndup(*arg, len);\n+\t\t\t}\n+\t\t\treturn -1;\n+\t\t}\n \t}\n \treturn 0;\n }\n@@ -1477,7 +1486,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \n \t\tif (*arg == ':') {\n \t\t\targ++;\n-\t\t\tif (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg))\n+\t\t\tif (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg, NULL))\n \t\t\t\tgoto trailer_out;\n \t\t}\n \t\tif (*arg == ')') {\ndiff --git a/pretty.h b/pretty.h\nindex 7369cf7e1484..d902cdd70a95 100644\n--- a/pretty.h\n+++ b/pretty.h\n@@ -151,6 +151,7 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n \t\t\tstruct string_list *filter_list,\n \t\t\tstruct strbuf *sepbuf,\n \t\t\tstruct strbuf *kvsepbuf,\n-\t\t\tconst char **arg);\n+\t\t\tconst char **arg,\n+\t\t\tchar **invalid_arg);\n \n #endif /* PRETTY_H */\n-- \ngitgitgadget\n\n"},{"id":"416831","messageId":"9ec98917699382c2a43b29e1c552d3685f63f29a.1613181163.git.gitgitgadget@gmail.com","threadId":"54195","inReplyTo":"pull.726.v4.git.1613181163.gitgitgadget@gmail.com","subject":"[PATCH v4 4/4] ref-filter: use pretty.c logic for trailers","fromName":"Hariom Verma via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-02-13T01:52:43Z","receivedAt":"2021-02-13T01:53:45Z","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 :key_value_separator= - inserted between key and value in 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 | 8 +--\n ref-filter.c | 36 ++++++-----\n t/t6300-for-each-ref.sh | 95 ++++++++++++++++++++++++++++++\n 3 files changed, 118 insertions(+), 21 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 2962f85a502a..2ae2478de706 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -260,11 +260,9 @@ 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]`). For valid [:option] values see `trailers`\n+section of linkgit:git-log[1].\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 fd994e18744c..5224037d3da4 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -67,6 +67,12 @@ struct refname_atom {\n \tint lstrip, rstrip;\n };\n \n+static struct ref_trailer_buf {\n+\tstruct string_list filter_list;\n+\tstruct strbuf sepbuf;\n+\tstruct strbuf kvsepbuf;\n+} ref_trailer_buf = {STRING_LIST_INIT_NODUP, STRBUF_INIT, STRBUF_INIT};\n+\n static struct expand_data {\n \tstruct object_id oid;\n \tenum object_type type;\n@@ -313,28 +319,26 @@ 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\tchar *invalid_arg = NULL;\n+\n+\t\tif (format_set_trailers_options(&atom->u.contents.trailer_opts,\n+\t\t &ref_trailer_buf.filter_list,\n+\t\t &ref_trailer_buf.sepbuf,\n+\t\t &ref_trailer_buf.kvsepbuf,\n+\t\t &argbuf, &invalid_arg)) {\n+\t\t\tif (!invalid_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\"), invalid_arg);\n+\t\t\tfree((char *)invalid_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 a8faddd18a9b..cac7f443d004 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -837,6 +837,24 @@ test_trailer_option '%(trailers:only) shows only \"key: value\" trailers' \\\n \n \tEOF\n \n+test_trailer_option '%(trailers:only=no,only=true) shows only \"key: value\" trailers' \\\n+\t'trailers:only=no,only=true' <<-EOF\n+\t$(grep -v patch.description \n+\n+\tEOF\n+\n+test_trailer_option '%(trailers:key=foo) is case insensitive' \\\n+\t'trailers:key=SiGned-oFf-bY' <<-EOF\n+\tSigned-off-by: A U Thor \n+\n+\tEOF\n+\n+test_trailer_option '%(trailers:key=foo:) trailing colon also works' \\\n+\t'trailers:key=Signed-off-by:' <<-EOF\n+\tSigned-off-by: A U Thor \n+\n+\tEOF\n+\n+test_trailer_option '%(trailers:key=foo) multiple keys' \\\n+\t'trailers:key=Reviewed-by:,key=Signed-off-by' <<-EOF\n+\tReviewed-by: A U Thor \n+\tSigned-off-by: A U Thor \n+\n+\tEOF\n+\n+test_trailer_option '%(trailers:key=nonexistent) becomes empty' \\\n+\t'trailers:key=Shined-off-by:' <<-EOF\n+\n+\tEOF\n+\n+test_trailer_option '%(trailers:key=foo) handles multiple lines even if folded' \\\n+\t'trailers:key=Acked-by' <<-EOF\n+\t$(grep -v patch.description \n+\t$(grep patch.description \n+\n+\tEOF\n+\n+test_trailer_option '%(trailers:separator) changes separator' \\\n+\t'trailers:separator=%x2C,key=Reviewed-by,key=Signed-off-by:' <<-EOF\n+\tReviewed-by: A U Thor ,Signed-off-by: A U Thor \n+\tEOF\n+\n+test_trailer_option '%(trailers:key_value_separator) changes key-value separator' \\\n+\t'trailers:key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' <<-EOF\n+\tReviewed-by,A U Thor \n+\tSigned-off-by,A U Thor \n+\n+\tEOF\n+\n+test_trailer_option '%(trailers:separator,key_value_separator) changes both separators' \\\n+\t'trailers:separator=%x2C,key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' <<-EOF\n+\tReviewed-by,A U Thor ,Signed-off-by,A U Thor \n+\tEOF\n+\n test_failing_trailer_option () {\n \ttitle=$1 option=$2\n \tcat >expect\n@@ -866,6 +956,11 @@ test_failing_trailer_option '%(trailers) rejects unknown trailers arguments' \\\n \tfatal: unknown %(trailers) argument: unsupported\n \tEOF\n \n+test_failing_trailer_option '%(trailers:key) without value is error' \\\n+\t'trailers:key' <<-\\EOF\n+\tfatal: expected %(trailers:key=)\n+\tEOF\n+\n test_expect_success 'if arguments, %(contents:trailers) shows error if colon is missing' '\n \tcat >expect <<-EOF &&\n \tfatal: unrecognized %(contents) argument: trailersonly\n-- \ngitgitgadget\n"}]}