{"thread":{"id":"54505","subject":"[PATCH 06/21] t4205: add test for trailer in log with nonstandard separator","startedAt":"2020-10-25T22:11:49Z","lastAt":"2020-12-10T19:04:17Z","messageCount":67,"participants":["Anders Waldenborg","Christian Couder","Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":21},"messages":[{"id":"408357","messageId":"20201025212652.3003036-7-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 06/21] t4205: add test for trailer in log with nonstandard separator","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:37Z","receivedAt":"2020-10-25T22:11:49Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nSigned-off-by: Anders Waldenborg \n---\n t/t4205-log-pretty-formats.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 757575d3f6..42544fb07a 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -757,6 +757,18 @@ test_expect_success 'pretty format %(trailers) combining separator/key/valueonly\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers) with nonstandard separator' '\n+\tgit commit --allow-empty -F - <<-\\EOF &&\n+\tSome fix\n+\n+\tCloses #1234\n+\tEOF\n+\n+\tgit -c \"trailer.separators=:#\" log --no-walk --pretty=\"format:%s% (trailers:key=Closes)\" >actual &&\n+\techo \"Some fix Closes: 1234\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'trailer parsing not fooled by --- line' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tthis is the subject\n-- \n2.25.1\n\n"},{"id":"408359","messageId":"20201025212652.3003036-4-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 03/21] doc: mention canonicalization in git i-t manual","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:34Z","receivedAt":"2020-10-25T22:41:47Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/git-interpret-trailers.txt | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt\nindex 96ec6499f0..a4be8aed66 100644\n--- a/Documentation/git-interpret-trailers.txt\n+++ b/Documentation/git-interpret-trailers.txt\n@@ -25,6 +25,11 @@ Otherwise, this command applies the arguments passed using the\n `--trailer` option, if any, to the commit message part of each input\n file. The result is emitted on the standard output.\n \n+When trailers read from input they will be changed into \"canonical\"\n+form if the trailer has a corresponding 'trailer..key'\n+configuration value. This means that it will use the exact spelling\n+(upper case vs lower case and separator) defined in configuration.\n+\n Some configuration variables control the way the `--trailer` arguments\n are applied to each commit message and the way any existing trailer in\n the commit message is changed. They also make it possible to\n-- \n2.25.1\n\n"},{"id":"408360","messageId":"20201025212652.3003036-20-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 19/21] trailer: move config lookup out of parse_trailer","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:50Z","receivedAt":"2020-10-25T22:41:48Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nThis may be seen as making it worse adding code duplication. But will\nhopefully make different handling for config lookups easier.\n\nNo functional change intended.\n\nSigned-off-by: Anders Waldenborg \n---\n trailer.c | 17 ++++++++---------\n 1 file changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 02061877b4..0db3bba3b1 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -656,8 +656,7 @@ static ssize_t find_separator(const char *line, const char *separators)\n * If separator_pos is -1, interpret the whole trailer as a token.\n */\n static void parse_trailer(struct strbuf *tok, struct strbuf *val, struct strbuf *sep,\n-\t\t\t const struct conf_info **conf, const char *trailer,\n-\t\t\t ssize_t separator_pos)\n+\t\t\t const char *trailer, ssize_t separator_pos)\n {\n \tif (separator_pos != -1) {\n \t\tsize_t sep_spacing_begin = separator_pos;\n@@ -677,9 +676,6 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val, struct strbuf\n \t\tstrbuf_addstr(tok, trailer);\n \t\tstrbuf_trim(tok);\n \t}\n-\n-\tif (conf)\n-\t\t*conf = lookup_conf_for_tok (tok);\n }\n \n static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,\n@@ -752,8 +748,9 @@ static void process_command_line_args(struct list_head *arg_head,\n \t\t\t (int) sb.len, sb.buf);\n \t\t\tstrbuf_release(&sb);\n \t\t} else {\n-\t\t\tparse_trailer(&tok, &val, NULL, &conf, tr->text,\n+\t\t\tparse_trailer(&tok, &val, NULL, tr->text,\n \t\t\t\t separator_pos);\n+\t\t\tconf = lookup_conf_for_tok(&tok);\n \t\t\tadd_arg_item(arg_head,\n \t\t\t\t strbuf_detach(&tok, NULL),\n \t\t\t\t strbuf_detach(&val, NULL),\n@@ -1026,8 +1023,9 @@ static size_t process_input_file(FILE *outfile,\n \t\tseparator_pos = find_separator(trailer, separators);\n \t\tif (separator_pos >= 1) {\n \t\t\tconst struct conf_info *conf;\n-\t\t\tparse_trailer(&tok, &val, &sep, &conf, trailer,\n+\t\t\tparse_trailer(&tok, &val, &sep, trailer,\n \t\t\t\t separator_pos);\n+\t\t\tconf = lookup_conf_for_tok(&tok);\n \t\t\tif (opts->unfold)\n \t\t\t\tunfold_value(&val);\n \t\t\tadd_trailer_item(head,\n@@ -1221,7 +1219,8 @@ static void format_trailer_info(struct strbuf *out,\n \n \t\t\tconst struct conf_info *conf;\n \n-\t\t\tparse_trailer(&tok, &val, NULL, &conf, trailer, separator_pos);\n+\t\t\tparse_trailer(&tok, &val, NULL, trailer, separator_pos);\n+\t\t\tconf = lookup_conf_for_tok(&tok);\n \t\t\tif (!opts->filter ||\n \t\t\t opts->filter(&tok, conf ? conf->name : NULL, opts->filter_data)) {\n \t\t\t\tif (opts->unfold)\n@@ -1282,7 +1281,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)\n \n \t\tstrbuf_reset(&iter->key);\n \t\tstrbuf_reset(&iter->val);\n-\t\tparse_trailer(&iter->key, &iter->val, NULL, NULL,\n+\t\tparse_trailer(&iter->key, &iter->val, NULL,\n \t\t\t trailer, separator_pos);\n \t\tunfold_value(&iter->val);\n \t\treturn 1;\n-- \n2.25.1\n\n"},{"id":"408361","messageId":"20201025212652.3003036-21-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 20/21] trailer: add failing tests for matching trailers against input","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:51Z","receivedAt":"2020-10-25T22:41:51Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nThese tests shows problematic cases where input trailers matches\nconfig.\n\nSigned-off-by: Anders Waldenborg \n---\n t/t7513-interpret-trailers.sh | 35 +++++++++++++++++++++++++++++++++++\n 1 file changed, 35 insertions(+)\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 6ddc2f5573..a99d6d7e3b 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -151,6 +151,41 @@ test_expect_success 'spelling and separators are not canonicalized with --parse\n \ttest_cmp expected actual\n '\n \n+# Matching currently is prefix matching, causing \"This-trailer\" to be normalized too\n+test_expect_failure 'config option matches exact only' '\n+\tcat >patch <<-\\EOF &&\n+\n+\t\tThis-trailer: a\n+\t\t b\n+\t\tThis-trailer-exact: b\n+\t\t c\n+\t\tThis-trailer-exact-plus-some: c\n+\t\t d\n+\tEOF\n+\tcat >expected <<-\\EOF &&\n+\t\tThis-trailer: a b\n+\t\tTHIS-TRAILER-EXACT: b c\n+\t\tThis-trailer-exact-plus-some: c d\n+\tEOF\n+\tgit -c \"trailer.tte.key=THIS-TRAILER-EXACT\" interpret-trailers --only-input --only-trailers --unfold patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+# Matching currently uses the config key even if key value is different\n+test_expect_failure 'config option matches exact only' '\n+\tcat >patch <<-\\EOF &&\n+\n+\t\tTicket: 1234\n+\t\tReference-ticket: 99\n+\tEOF\n+\tcat >expected <<-\\EOF &&\n+\t\tTicket: 1234\n+\t\tReference-Ticket: 99\n+\tEOF\n+\tgit -c \"trailer.ticket.key=Reference-Ticket\" interpret-trailers --only-input --only-trailers patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'with only a title in the message' '\n \tcat >expected <<-\\EOF &&\n \t\tarea: change\n-- \n2.25.1\n\n"},{"id":"408362","messageId":"20201025212652.3003036-14-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 13/21] trailer: add option to make canonicalization optional","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:44Z","receivedAt":"2020-10-25T22:41:52Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nAdds a new `--(no-)canonicalize` option to interpret-trailers. By\ndefault it is on unless `--parse` option is given.\n\nWhen option is on trailer tokens and separators get canonicalized to\nthe form they have in config (if there is any config for that\ntrailer). This is same behavior as before this patch, which allows\nthis behavior to be disabled with `--no-canonicalize`. `--parse` now\nalso implies `--no-canonicalize`, if previous behavior with\ncanonicalization also in parse mode is wanted it needs to be combined\nwith `--parse --canonicalize`\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/git-interpret-trailers.txt | 5 ++-\n builtin/interpret-trailers.c | 3 ++\n t/t7513-interpret-trailers.sh | 52 ++++++++++++++++++++++++\n trailer.c | 10 +++--\n trailer.h | 1 +\n 5 files changed, 67 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt\nindex a4be8aed66..a9e6816525 100644\n--- a/Documentation/git-interpret-trailers.txt\n+++ b/Documentation/git-interpret-trailers.txt\n@@ -129,13 +129,16 @@ OPTIONS\n \n --parse::\n \tA convenience alias for `--only-trailers --only-input\n-\t--unfold`.\n+\t--unfold --no-canonicalize`.\n \n --no-divider::\n \tDo not treat `---` as the end of the commit message. Use this\n \twhen you know your input contains just the commit message itself\n \t(and not an email or the output of `git format-patch`).\n \n+--no-canonicalize::\n+\tDisable canonicalization of input trailers.\n+\n CONFIGURATION VARIABLES\n -----------------------\n \ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex 84748eafc0..51678657a3 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -81,6 +81,7 @@ static int parse_opt_parse(const struct option *opt, const char *arg,\n \tv->only_trailers = 1;\n \tv->only_input = 1;\n \tv->unfold = 1;\n+\tv->canonicalize = 0;\n \tBUG_ON_OPT_NEG(unset);\n \tBUG_ON_OPT_ARG(arg);\n \treturn 0;\n@@ -105,6 +106,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"only-trailers\", &opts.only_trailers, N_(\"output only the trailers\")),\n \t\tOPT_BOOL(0, \"only-input\", &opts.only_input, N_(\"do not apply config rules\")),\n \t\tOPT_BOOL(0, \"unfold\", &opts.unfold, N_(\"join whitespace-continued values\")),\n+\t\tOPT_BOOL(0, \"canonicalize\", &opts.canonicalize, N_(\"canonicalize spelling for trailers with config\")),\n \t\tOPT_CALLBACK_F(0, \"parse\", &opts, NULL, N_(\"set parsing options\"),\n \t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),\n \t\tOPT_BOOL(0, \"no-divider\", &opts.no_divider, N_(\"do not treat --- specially\")),\n@@ -112,6 +114,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"trailer(s) to add\"), option_parse_trailer),\n \t\tOPT_END()\n \t};\n+\topts.canonicalize = 1;\n \n \tgit_config(git_default_config, NULL);\n \ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 6602790b5f..4b3a2484b5 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -99,6 +99,58 @@ test_expect_success 'with config option on the command line' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'spelling and separators are canonicalized from configs with key' '\n+\tcat >patch <<-\\EOF &&\n+\t\tnon-trailer-line\n+\n+\t\tReviEweD-bY :abc\n+\t\tReviEwEd-bY) rst\n+\t\tReviEweD-BY ; xyz\n+\t\taCked-bY) only separator gets normalized\n+\tEOF\n+\tcat >expected <<-\\EOF &&\n+\t\tReviewed-By: abc\n+\t\tReviewed-By: rst\n+\t\tReviewed-By: xyz\n+\t\taCked-bY: only separator gets normalized\n+\tEOF\n+\tgit \\\n+\t\t-c \"trailer.separators=:);\" \\\n+\t\t-c \"trailer.rb.key=Reviewed-By\" \\\n+\t\t-c \"trailer.Acked-By.ifmissing=doNothing\" \\\n+\t\tinterpret-trailers --only-trailers --only-input patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'spelling and separators are not canonicalized with --parse or --no-canonicalize' '\n+\tcat >patch <<-\\EOF &&\n+\t\tnon-trailer-line\n+\n+\t\tReviEweD-bY :abc\n+\t\tReviEwEd-bY) rst\n+\t\tReviEweD-BY ; xyz\n+\t\taCked-bY) not normalized\n+\tEOF\n+\tcat >expected <<-\\EOF &&\n+\t\tReviEweD-bY :abc\n+\t\tReviEwEd-bY) rst\n+\t\tReviEweD-BY ; xyz\n+\t\taCked-bY) not normalized\n+\tEOF\n+\tgit \\\n+\t\t-c \"trailer.separators=:);\" \\\n+\t\t-c \"trailer.rb.key=Reviewed-By\" \\\n+\t\t-c \"trailer.Acked-By.ifmissing=doNothing\" \\\n+\t\tinterpret-trailers --parse patch >actual &&\n+\ttest_cmp expected actual &&\n+\tgit \\\n+\t\t-c \"trailer.separators=:);\" \\\n+\t\t-c \"trailer.rb.key=Reviewed-By\" \\\n+\t\t-c \"trailer.Acked-By.ifmissing=doNothing\" \\\n+\t\tinterpret-trailers --only-trailers --only-input --no-canonicalize patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'with only a title in the message' '\n \tcat >expected <<-\\EOF &&\n \t\tarea: change\ndiff --git a/trailer.c b/trailer.c\nindex 102eca0127..110d3ed226 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -141,14 +141,18 @@ static void free_arg_item(struct arg_item *item)\n \tfree(item);\n }\n \n-static void print_item(FILE *outfile, const struct trailer_item *item)\n+static void print_item(FILE *outfile, const struct trailer_item *item,\n+\t\t const struct process_trailer_options *opts)\n {\n \tif (item->token) {\n \t\tconst char *tok = item->token;\n \t\tconst char *sep = (char []){separators[0], ' ', '\\0'};\n \t\tconst struct conf_info *conf = item->conf;\n \n-\t\tif (conf) {\n+\t\tif (!opts->canonicalize && item->used_separator)\n+\t\t\tsep = item->used_separator;\n+\n+\t\tif (opts->canonicalize && conf) {\n \t\t\tif (conf->key)\n \t\t\t\ttok = conf->key;\n \t\t\tif (conf->nondefault_separator)\n@@ -170,7 +174,7 @@ static void print_all(FILE *outfile, struct list_head *head,\n \t\titem = list_entry(pos, struct trailer_item, list);\n \t\tif ((!opts->trim_empty || strlen(item->value) > 0) &&\n \t\t (!opts->only_trailers || item->token))\n-\t\t\tprint_item(outfile, item);\n+\t\t\tprint_item(outfile, item, opts);\n \t}\n }\n \ndiff --git a/trailer.h b/trailer.h\nindex b362b0d44d..aad856da8c 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -72,6 +72,7 @@ struct process_trailer_options {\n \tint unfold;\n \tint no_divider;\n \tint value_only;\n+\tint canonicalize;\n \tconst struct strbuf *separator;\n \tint (*filter)(const struct strbuf *, const char *alias, void *);\n \tvoid *filter_data;\n-- \n2.25.1\n\n"},{"id":"408363","messageId":"20201025212652.3003036-8-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 07/21] trailer: simplify 'arg_item' lifetime","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:38Z","receivedAt":"2020-10-25T22:41:53Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\n'struct arg_item' are created from config and '--trailers' arguments\nin 'git interpret-trailers'.\n\nThen they were freed as they were processed. This made it harder to\nreason about and ensure that all of them were properly freed in all\ncases.\n\nThis commit extends the lifetime by not doing any freeing during\nprocessing but rather freeing the whole list afterwards. This make it\nclearer and will allow keeping a reference to the config stored in the\narg item.\n\nThe drawback is that there is extra memory allocation as previously\nthe strings could be donated to the trailer_item when that is\ncreated. Now they have to be copied.\n\nNo functional change intended.\n\nSigned-off-by: Anders Waldenborg \n---\n trailer.c | 32 ++++++++++++++++----------------\n 1 file changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 227df1c0ef..047781463a 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -177,13 +177,11 @@ static void print_all(FILE *outfile, struct list_head *head,\n \t}\n }\n \n-static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)\n+static struct trailer_item *trailer_from_arg(const struct arg_item *arg_tok)\n {\n \tstruct trailer_item *new_item = xcalloc(sizeof(*new_item), 1);\n-\tnew_item->token = arg_tok->token;\n-\tnew_item->value = arg_tok->value;\n-\targ_tok->token = arg_tok->value = NULL;\n-\tfree_arg_item(arg_tok);\n+\tnew_item->token = xstrdup(arg_tok->token);\n+\tnew_item->value = xstrdup(arg_tok->value);\n \treturn new_item;\n }\n \n@@ -274,7 +272,6 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,\n {\n \tswitch (arg_tok->conf.if_exists) {\n \tcase EXISTS_DO_NOTHING:\n-\t\tfree_arg_item(arg_tok);\n \t\tbreak;\n \tcase EXISTS_REPLACE:\n \t\tapply_item_command(in_tok, arg_tok);\n@@ -290,15 +287,11 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,\n \t\tapply_item_command(in_tok, arg_tok);\n \t\tif (check_if_different(in_tok, arg_tok, 1, head))\n \t\t\tadd_arg_to_input_list(on_tok, arg_tok);\n-\t\telse\n-\t\t\tfree_arg_item(arg_tok);\n \t\tbreak;\n \tcase EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:\n \t\tapply_item_command(in_tok, arg_tok);\n \t\tif (check_if_different(on_tok, arg_tok, 0, head))\n \t\t\tadd_arg_to_input_list(on_tok, arg_tok);\n-\t\telse\n-\t\t\tfree_arg_item(arg_tok);\n \t\tbreak;\n \tdefault:\n \t\tBUG(\"trailer.c: unhandled value %d\",\n@@ -314,7 +307,6 @@ static void apply_arg_if_missing(struct list_head *head,\n \n \tswitch (arg_tok->conf.if_missing) {\n \tcase MISSING_DO_NOTHING:\n-\t\tfree_arg_item(arg_tok);\n \t\tbreak;\n \tcase MISSING_ADD:\n \t\twhere = arg_tok->conf.where;\n@@ -364,15 +356,13 @@ static int find_same_and_apply_arg(struct list_head *head,\n static void process_trailers_lists(struct list_head *head,\n \t\t\t\t struct list_head *arg_head)\n {\n-\tstruct list_head *pos, *p;\n+\tstruct list_head *pos;\n \tstruct arg_item *arg_tok;\n \n-\tlist_for_each_safe(pos, p, arg_head) {\n+\tlist_for_each(pos, arg_head) {\n \t\tint applied = 0;\n \t\targ_tok = list_entry(pos, struct arg_item, list);\n \n-\t\tlist_del(pos);\n-\n \t\tapplied = find_same_and_apply_arg(head, arg_tok);\n \n \t\tif (!applied)\n@@ -999,6 +989,15 @@ static void free_all_trailer_items(struct list_head *head)\n \t}\n }\n \n+static void free_all_arg_items(struct list_head *head)\n+{\n+\tstruct list_head *pos, *p;\n+\tlist_for_each_safe(pos, p, head) {\n+\t\tlist_del(pos);\n+\t\tfree_arg_item(list_entry(pos, struct arg_item, list));\n+\t}\n+}\n+\n static struct tempfile *trailers_tempfile;\n \n static FILE *create_in_place_tempfile(const char *file)\n@@ -1035,6 +1034,7 @@ void process_trailers(const char *file,\n \t\t struct list_head *new_trailer_head)\n {\n \tLIST_HEAD(head);\n+\tLIST_HEAD(arg_head);\n \tstruct strbuf sb = STRBUF_INIT;\n \tsize_t trailer_end;\n \tFILE *outfile = stdout;\n@@ -1050,7 +1050,6 @@ void process_trailers(const char *file,\n \ttrailer_end = process_input_file(outfile, sb.buf, &head, opts);\n \n \tif (!opts->only_input) {\n-\t\tLIST_HEAD(arg_head);\n \t\tprocess_command_line_args(&arg_head, new_trailer_head);\n \t\tprocess_trailers_lists(&head, &arg_head);\n \t}\n@@ -1058,6 +1057,7 @@ void process_trailers(const char *file,\n \tprint_all(outfile, &head, opts);\n \n \tfree_all_trailer_items(&head);\n+\tfree_all_arg_items(&arg_head);\n \n \t/* Print the lines after the trailers as is */\n \tif (!opts->only_trailers)\n-- \n2.25.1\n\n"},{"id":"408364","messageId":"20201025212652.3003036-15-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 14/21] trailer: move skipping of blank lines to own loop when finding trailer","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:45Z","receivedAt":"2020-10-25T22:41:55Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nNo functional change intended.\n\nSigned-off-by: Anders Waldenborg \n---\n trailer.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 110d3ed226..937cf1edeb 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -829,7 +829,6 @@ static size_t find_trailer_start(const char *buf, size_t len)\n {\n \tconst char *s;\n \tssize_t end_of_title, l;\n-\tint only_spaces = 1;\n \tint recognized_prefix = 0, trailer_lines = 0, non_trailer_lines = 0;\n \t/*\n \t * Number of possible continuation lines encountered. This will be\n@@ -856,6 +855,12 @@ static size_t find_trailer_start(const char *buf, size_t len)\n \t * consists of at least 25% trailers.\n \t */\n \tfor (l = last_line(buf, len);\n+\t l >= end_of_title;\n+\t l = last_line(buf, l)) {\n+\t\tif (!is_blank_line(buf + l) && buf[l] != comment_line_char)\n+\t\t\tbreak;\n+\t}\n+\tfor (;\n \t l >= end_of_title;\n \t l = last_line(buf, l)) {\n \t\tconst char *bol = buf + l;\n@@ -868,8 +873,6 @@ static size_t find_trailer_start(const char *buf, size_t len)\n \t\t\tcontinue;\n \t\t}\n \t\tif (is_blank_line(bol)) {\n-\t\t\tif (only_spaces)\n-\t\t\t\tcontinue;\n \t\t\tnon_trailer_lines += possible_continuation_lines;\n \t\t\tif (recognized_prefix &&\n \t\t\t trailer_lines * 3 >= non_trailer_lines)\n@@ -878,7 +881,6 @@ static size_t find_trailer_start(const char *buf, size_t len)\n \t\t\t\treturn next_line(bol) - buf;\n \t\t\treturn len;\n \t\t}\n-\t\tonly_spaces = 0;\n \n \t\tfor (p = git_generated_prefixes; *p; p++) {\n \t\t\tif (starts_with(bol, *p)) {\n-- \n2.25.1\n\n"},{"id":"408365","messageId":"20201025212652.3003036-5-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 04/21] pretty: allow using aliases in %(trailer:key=xyz)","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:35Z","receivedAt":"2020-10-25T22:41:56Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 4 +++-\n pretty.c | 5 ++++-\n t/t4205-log-pretty-formats.sh | 6 ++++++\n trailer.c | 7 +++++--\n trailer.h | 2 +-\n 5 files changed, 19 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 84bbc7439a..1714fa447d 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -256,7 +256,9 @@ endif::git-rev-list[]\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+ shown. If `trailer..key` configuration option is set 'token'\n+ can be used as an alias for showing trailers with the value in\n+ key. 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\ndiff --git a/pretty.c b/pretty.c\nindex 7a7708a0ea..3c374abffe 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1135,7 +1135,7 @@ static int match_placeholder_bool_arg(const char *to_parse, const char *candidat\n \treturn 1;\n }\n \n-static int format_trailer_match_cb(const struct strbuf *key, void *ud)\n+static int format_trailer_match_cb(const struct strbuf *key, const char *alias, void *ud)\n {\n \tconst struct string_list *list = ud;\n \tconst struct string_list_item *item;\n@@ -1144,6 +1144,9 @@ static int format_trailer_match_cb(const struct strbuf *key, void *ud)\n \t\tif (key->len == (uintptr_t)item->util &&\n \t\t !strncasecmp(item->string, key->buf, key->len))\n \t\t\treturn 1;\n+\t\tif (alias && strlen(alias) == (uintptr_t)item->util &&\n+\t\t !strncasecmp(item->string, alias, (uintptr_t)item->util))\n+\t\t\treturn 1;\n \t}\n \treturn 0;\n }\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 204c149d5a..757575d3f6 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -676,6 +676,12 @@ test_expect_success 'pretty format %(trailers:key=foo) multiple keys' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers:key=foo) alias in config' '\n+\tgit -c trailer.ab.key=Acked-by log --no-walk --pretty=\"format:%(trailers:key=ab)\" >actual &&\n+\techo \"Acked-by: A U Thor \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '%(trailers:key=nonexistent) becomes empty' '\n \tgit log --no-walk --pretty=\"x%(trailers:key=Nacked-by)x\" >actual &&\n \techo \"xx\" >expect &&\ndiff --git a/trailer.c b/trailer.c\nindex ca7a823af6..8c0687a529 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1148,8 +1148,11 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tstruct strbuf tok = STRBUF_INIT;\n \t\t\tstruct strbuf val = STRBUF_INIT;\n \n-\t\t\tparse_trailer(&tok, &val, NULL, trailer, separator_pos);\n-\t\t\tif (!opts->filter || opts->filter(&tok, opts->filter_data)) {\n+\t\t\tconst struct conf_info *conf;\n+\n+\t\t\tparse_trailer(&tok, &val, &conf, trailer, separator_pos);\n+\t\t\tif (!opts->filter ||\n+\t\t\t opts->filter(&tok, conf ? conf->name : NULL, opts->filter_data)) {\n \t\t\t\tif (opts->unfold)\n \t\t\t\t\tunfold_value(&val);\n \ndiff --git a/trailer.h b/trailer.h\nindex cd93e7ddea..b362b0d44d 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -73,7 +73,7 @@ struct process_trailer_options {\n \tint no_divider;\n \tint value_only;\n \tconst struct strbuf *separator;\n-\tint (*filter)(const struct strbuf *, void *);\n+\tint (*filter)(const struct strbuf *, const char *alias, void *);\n \tvoid *filter_data;\n };\n \n-- \n2.25.1\n\n"},{"id":"408366","messageId":"20201025212652.3003036-10-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 09/21] trailer: refactor print_tok_val into taking item","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:40Z","receivedAt":"2020-10-25T22:41:59Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nNo functional change intended.\n\nSigned-off-by: Anders Waldenborg \n---\n trailer.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 0986d4267e..71921e70ce 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -147,22 +147,22 @@ static char last_non_space_char(const char *s)\n \treturn '\\0';\n }\n \n-static void print_tok_val(FILE *outfile, const char *tok, const char *val)\n+static void print_item(FILE *outfile, const struct trailer_item *item)\n {\n \tchar c;\n \n-\tif (!tok) {\n-\t\tfprintf(outfile, \"%s\\n\", val);\n+\tif (!item->token) {\n+\t\tfprintf(outfile, \"%s\\n\", item->value);\n \t\treturn;\n \t}\n \n-\tc = last_non_space_char(tok);\n+\tc = last_non_space_char(item->token);\n \tif (!c)\n \t\treturn;\n \tif (strchr(separators, c))\n-\t\tfprintf(outfile, \"%s%s\\n\", tok, val);\n+\t\tfprintf(outfile, \"%s%s\\n\", item->token, item->value);\n \telse\n-\t\tfprintf(outfile, \"%s%c %s\\n\", tok, separators[0], val);\n+\t\tfprintf(outfile, \"%s%c %s\\n\", item->token, separators[0], item->value);\n }\n \n static void print_all(FILE *outfile, struct list_head *head,\n@@ -174,7 +174,7 @@ static void print_all(FILE *outfile, struct list_head *head,\n \t\titem = list_entry(pos, struct trailer_item, list);\n \t\tif ((!opts->trim_empty || strlen(item->value) > 0) &&\n \t\t (!opts->only_trailers || item->token))\n-\t\t\tprint_tok_val(outfile, item->token, item->value);\n+\t\t\tprint_item(outfile, item);\n \t}\n }\n \n-- \n2.25.1\n\n"},{"id":"408367","messageId":"20201025212652.3003036-11-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 10/21] trailer: move trailer token canonicalization print time","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:41Z","receivedAt":"2020-10-25T22:42:01Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nNow that config is stored on the trailer_item it can easily be\naccessed print time and the changing of the token into the\nconfigured (canonical) spelling can be done print time instead.\n\nNo functional change intended.\n\nSigned-off-by: Anders Waldenborg \n---\n trailer.c | 42 +++++++++++++++++-------------------------\n 1 file changed, 17 insertions(+), 25 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 71921e70ce..d6882155be 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -149,20 +149,24 @@ static char last_non_space_char(const char *s)\n \n static void print_item(FILE *outfile, const struct trailer_item *item)\n {\n-\tchar c;\n-\n-\tif (!item->token) {\n-\t\tfprintf(outfile, \"%s\\n\", item->value);\n-\t\treturn;\n+\tif (item->token) {\n+\t\tconst char *tok = item->token;\n+\t\tconst struct conf_info *conf = item->conf;\n+\t\tchar c;\n+\n+\t\tif (conf && conf->key)\n+\t\t\ttok = conf->key;\n+\n+\t\tc = last_non_space_char(tok);\n+\t\tif (!c)\n+\t\t\treturn;\n+\t\tif (strchr(separators, c))\n+\t\t\tfputs(tok, outfile);\n+\t\telse\n+\t\t\tfprintf(outfile, \"%s%c \", tok, separators[0]);\n \t}\n \n-\tc = last_non_space_char(item->token);\n-\tif (!c)\n-\t\treturn;\n-\tif (strchr(separators, c))\n-\t\tfprintf(outfile, \"%s%s\\n\", item->token, item->value);\n-\telse\n-\t\tfprintf(outfile, \"%s%c %s\\n\", item->token, separators[0], item->value);\n+\tfprintf(outfile, \"%s\\n\", item->value);\n }\n \n static void print_all(FILE *outfile, struct list_head *head,\n@@ -569,15 +573,6 @@ static void ensure_configured(void)\n \tconfigured = 1;\n }\n \n-static const char *token_from_conf(const struct conf_info *conf, char *tok)\n-{\n-\tif (conf->key)\n-\t\treturn conf->key;\n-\tif (tok)\n-\t\treturn tok;\n-\treturn conf->name;\n-}\n-\n static int token_matches_conf(const char *tok, const struct conf_info *conf, size_t tok_len)\n {\n \tif (!strncasecmp(tok, conf->name, tok_len))\n@@ -646,11 +641,8 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,\n \tlist_for_each(pos, &conf_head) {\n \t\titem = list_entry(pos, struct conf_info_item, list);\n \t\tif (token_matches_conf(tok->buf, &item->conf, tok_len)) {\n-\t\t\tchar *tok_buf = strbuf_detach(tok, NULL);\n \t\t\tif (conf)\n \t\t\t\t*conf = &item->conf;\n-\t\t\tstrbuf_addstr(tok, token_from_conf(&item->conf, tok_buf));\n-\t\t\tfree(tok_buf);\n \t\t\tbreak;\n \t\t}\n \t}\n@@ -706,7 +698,7 @@ static void process_command_line_args(struct list_head *arg_head,\n \t\titem = list_entry(pos, struct conf_info_item, list);\n \t\tif (item->conf.command)\n \t\t\tadd_arg_item(arg_head,\n-\t\t\t\t xstrdup(token_from_conf(&item->conf, NULL)),\n+\t\t\t\t xstrdup(item->conf.name),\n \t\t\t\t xstrdup(\"\"),\n \t\t\t\t &item->conf, NULL);\n \t}\n-- \n2.25.1\n\n"},{"id":"408368","messageId":"20201025212652.3003036-9-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 08/21] trailer: keep track of conf in trailer_item","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:39Z","receivedAt":"2020-10-25T22:42:01Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nSigned-off-by: Anders Waldenborg \n---\n trailer.c | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 047781463a..0986d4267e 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -34,6 +34,7 @@ struct trailer_item {\n \t */\n \tchar *token;\n \tchar *value;\n+\tconst struct conf_info *conf;\n };\n \n struct arg_item {\n@@ -182,6 +183,7 @@ static struct trailer_item *trailer_from_arg(const struct arg_item *arg_tok)\n \tstruct trailer_item *new_item = xcalloc(sizeof(*new_item), 1);\n \tnew_item->token = xstrdup(arg_tok->token);\n \tnew_item->value = xstrdup(arg_tok->value);\n+\tnew_item->conf = &arg_tok->conf;\n \treturn new_item;\n }\n \n@@ -655,11 +657,12 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,\n }\n \n static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,\n-\t\t\t\t\t char *val)\n+\t\t\t\t\t char *val, const struct conf_info *conf)\n {\n \tstruct trailer_item *new_item = xcalloc(sizeof(*new_item), 1);\n \tnew_item->token = tok;\n \tnew_item->value = val;\n+\tnew_item->conf = conf;\n \tlist_add_tail(&new_item->list, head);\n \treturn new_item;\n }\n@@ -959,19 +962,22 @@ static size_t process_input_file(FILE *outfile,\n \t\t\tcontinue;\n \t\tseparator_pos = find_separator(trailer, separators);\n \t\tif (separator_pos >= 1) {\n-\t\t\tparse_trailer(&tok, &val, NULL, trailer,\n+\t\t\tconst struct conf_info *conf;\n+\t\t\tparse_trailer(&tok, &val, &conf, trailer,\n \t\t\t\t separator_pos);\n \t\t\tif (opts->unfold)\n \t\t\t\tunfold_value(&val);\n \t\t\tadd_trailer_item(head,\n \t\t\t\t\t strbuf_detach(&tok, NULL),\n-\t\t\t\t\t strbuf_detach(&val, NULL));\n+\t\t\t\t\t strbuf_detach(&val, NULL),\n+\t\t\t\t\t conf);\n \t\t} else if (!opts->only_trailers) {\n \t\t\tstrbuf_addstr(&val, trailer);\n \t\t\tstrbuf_strip_suffix(&val, \"\\n\");\n \t\t\tadd_trailer_item(head,\n \t\t\t\t\t NULL,\n-\t\t\t\t\t strbuf_detach(&val, NULL));\n+\t\t\t\t\t strbuf_detach(&val, NULL),\n+\t\t\t\t\t NULL);\n \t\t}\n \t}\n \n-- \n2.25.1\n\n"},{"id":"408371","messageId":"20201025212652.3003036-18-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 17/21] trailer: don't treat line with prefix of known trailer as known","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:48Z","receivedAt":"2020-10-25T22:42:05Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nE.g if \"Closes\" is a configured trailer a line starting with \"c:\"\nshouldn't be treated as a recognized trailer when looking for trailer\nblock.\n\nSigned-off-by: Anders Waldenborg \n---\n t/t7513-interpret-trailers.sh | 7 +------\n trailer.c | 28 ++++++++++++++++++----------\n 2 files changed, 19 insertions(+), 16 deletions(-)\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex b1e9a9e6d1..6ddc2f5573 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -239,12 +239,7 @@ test_expect_success 'with non-trailer lines mixed with a configured trailer' '\n \ttest_cmp expected actual\n '\n \n-# This fails because \"c:/windows/tmp/stuff/temp.txt\" is classified as\n-# a trailer line because \"c\" is a prefix of \"Confirmed-By\". Therefore\n-# the new trailer is appended to that (non-trailer) block rather than\n-# creating a new block. It also canonicalize the \"trailer\" to\n-# \"Confirmed-By: /windows/tmp/stuff/temp.txt\"\n-test_expect_failure 'with non-trailer lines mixed with prefix of configured trailer' '\n+test_expect_success 'with non-trailer lines mixed with prefix of configured trailer' '\n \tcat >patch <<-\\EOF &&\n \t\tsome subject\n \ndiff --git a/trailer.c b/trailer.c\nindex 21877e4c06..d75d240e10 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -831,10 +831,20 @@ enum trailer_classification {\n \tBLANK,\n };\n \n+static int starts_with_separator(const char *buf)\n+{\n+\twhile (*buf == ' ' || *buf == '\\t')\n+\t\tbuf++;\n+\tif (!*buf)\n+\t\treturn 0;\n+\treturn !!strchr(separators, *buf);\n+}\n+\n static enum trailer_classification classify_trailer_line(const char *line)\n {\n \tconst char **p;\n \tssize_t separator_pos;\n+\tstruct list_head *pos;\n \n \tif (line[0] == comment_line_char)\n \t\treturn COMMENT;\n@@ -849,19 +859,17 @@ static enum trailer_classification classify_trailer_line(const char *line)\n \t\tif (starts_with(line, *p))\n \t\t\treturn GIT_GENERATED_PREFIX;\n \n-\n-\tseparator_pos = find_separator(line, separators);\n-\tif (separator_pos >= 1) {\n-\t\tstruct list_head *pos;\n-\n-\t\tlist_for_each(pos, &conf_head) {\n-\t\t\tstruct conf_info_item *item;\n-\t\t\titem = list_entry(pos, struct conf_info_item, list);\n-\t\t\tif (token_matches_conf(line, &item->conf,\n-\t\t\t separator_pos))\n+\tlist_for_each(pos, &conf_head) {\n+\t\tstruct conf_info_item *item = list_entry(pos, struct conf_info_item, list);\n+\t\tconst char *conftrailer = item->conf.key ? item->conf.key : item->conf.name;\n+\t\tif (istarts_with(line, conftrailer)) {\n+\t\t\tif (starts_with_separator (line + strlen (conftrailer)))\n \t\t\t\treturn CONFIGURED_TRAILER;\n \t\t}\n+\t}\n \n+\tseparator_pos = find_separator(line, separators);\n+\tif (separator_pos >= 1) {\n \t\treturn TRAILER;\n \t}\n \n-- \n2.25.1\n\n"},{"id":"408369","messageId":"20201025212652.3003036-17-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 16/21] t7513: add failing test for configured trailing line classification","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:47Z","receivedAt":"2020-10-25T22:42:06Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nThis testcases shows why prefix matching shouldn't be used when using\nconfigured trailers to classify lines as trailers or not.\n\nSigned-off-by: Anders Waldenborg \n---\n t/t7513-interpret-trailers.sh | 29 +++++++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 4b3a2484b5..b1e9a9e6d1 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -239,6 +239,35 @@ test_expect_success 'with non-trailer lines mixed with a configured trailer' '\n \ttest_cmp expected actual\n '\n \n+# This fails because \"c:/windows/tmp/stuff/temp.txt\" is classified as\n+# a trailer line because \"c\" is a prefix of \"Confirmed-By\". Therefore\n+# the new trailer is appended to that (non-trailer) block rather than\n+# creating a new block. It also canonicalize the \"trailer\" to\n+# \"Confirmed-By: /windows/tmp/stuff/temp.txt\"\n+test_expect_failure 'with non-trailer lines mixed with prefix of configured trailer' '\n+\tcat >patch <<-\\EOF &&\n+\t\tsome subject\n+\n+\t\tThis is clearly not a trailer line. But\n+\t\ton next line there is a a windows path\n+\t\tc:/windows/tmp/stuff/temp.txt but that\n+\t\tshould not make this classify as a trailer block\n+\tEOF\n+\tcat >expected <<-\\EOF &&\n+\t\tsome subject\n+\n+\t\tThis is clearly not a trailer line. But\n+\t\ton next line there is a a windows path\n+\t\tc:/windows/tmp/stuff/temp.txt but that\n+\t\tshould not make this classify as a trailer block\n+\n+\t\tt: v\n+\tEOF\n+\ttest_config trailer.confirmedby.key \"Confirmed-By\" &&\n+\tgit interpret-trailers --trailer \"t: v\" patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'with non-trailer lines mixed with a non-configured trailer' '\n \tcat >patch <<-\\EOF &&\n \n-- \n2.25.1\n\n"},{"id":"408370","messageId":"20201025212652.3003036-3-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 02/21] trailer: don't use 'struct arg_item' for storing config","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:33Z","receivedAt":"2020-10-25T22:42:07Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nThe '--trailer' options given to 'git interpret-trailers' are store in\nthe suitably named 'struct arg_item'.\n\nThe configuration done in 'trailer..xyz' was also stored in that\nstruct. Even though it only needs the \"conf_info\" part of it.\n\nThis commit creates a separate struct for conf_info_item\n\nNo functional change intended.\n\nSigned-off-by: Anders Waldenborg \n---\n trailer.c | 31 +++++++++++++++++--------------\n 1 file changed, 17 insertions(+), 14 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex efb88c2008..ca7a823af6 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -19,6 +19,11 @@ struct conf_info {\n \tenum trailer_if_missing if_missing;\n };\n \n+struct conf_info_item {\n+\tstruct list_head list;\n+\tstruct conf_info conf;\n+};\n+\n static struct conf_info default_conf_info;\n \n struct trailer_item {\n@@ -432,16 +437,16 @@ static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)\n \tdst->command = xstrdup_or_null(src->command);\n }\n \n-static struct arg_item *get_conf_item(const char *name)\n+static struct conf_info *get_conf_item(const char *name)\n {\n \tstruct list_head *pos;\n-\tstruct arg_item *item;\n+\tstruct conf_info_item *item;\n \n \t/* Look up item with same name */\n \tlist_for_each(pos, &conf_head) {\n-\t\titem = list_entry(pos, struct arg_item, list);\n+\t\titem = list_entry(pos, struct conf_info_item, list);\n \t\tif (!strcasecmp(item->conf.name, name))\n-\t\t\treturn item;\n+\t\t\treturn &item->conf;\n \t}\n \n \t/* Item does not already exists, create it */\n@@ -451,7 +456,7 @@ static struct arg_item *get_conf_item(const char *name)\n \n \tlist_add_tail(&item->list, &conf_head);\n \n-\treturn item;\n+\treturn &item->conf;\n }\n \n enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_WHERE,\n@@ -502,7 +507,6 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v\n static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n {\n \tconst char *trailer_item, *variable_name;\n-\tstruct arg_item *item;\n \tstruct conf_info *conf;\n \tchar *name = NULL;\n \tenum trailer_info_type type;\n@@ -527,8 +531,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n \tif (!name)\n \t\treturn 0;\n \n-\titem = get_conf_item(name);\n-\tconf = &item->conf;\n+\tconf = get_conf_item(name);\n \tfree(name);\n \n \tswitch (type) {\n@@ -630,7 +633,7 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,\n \t\t\t const struct conf_info **conf, const char *trailer,\n \t\t\t ssize_t separator_pos)\n {\n-\tstruct arg_item *item;\n+\tstruct conf_info_item *item;\n \tsize_t tok_len;\n \tstruct list_head *pos;\n \n@@ -649,7 +652,7 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,\n \tif (conf)\n \t\t*conf = &default_conf_info;\n \tlist_for_each(pos, &conf_head) {\n-\t\titem = list_entry(pos, struct arg_item, list);\n+\t\titem = list_entry(pos, struct conf_info_item, list);\n \t\tif (token_matches_conf(tok->buf, &item->conf, tok_len)) {\n \t\t\tchar *tok_buf = strbuf_detach(tok, NULL);\n \t\t\tif (conf)\n@@ -693,7 +696,7 @@ static void add_arg_item(struct list_head *arg_head, char *tok, char *val,\n static void process_command_line_args(struct list_head *arg_head,\n \t\t\t\t struct list_head *new_trailer_head)\n {\n-\tstruct arg_item *item;\n+\tstruct conf_info_item *item;\n \tstruct strbuf tok = STRBUF_INIT;\n \tstruct strbuf val = STRBUF_INIT;\n \tconst struct conf_info *conf;\n@@ -707,7 +710,7 @@ static void process_command_line_args(struct list_head *arg_head,\n \n \t/* Add an arg item for each configured trailer with a command */\n \tlist_for_each(pos, &conf_head) {\n-\t\titem = list_entry(pos, struct arg_item, list);\n+\t\titem = list_entry(pos, struct conf_info_item, list);\n \t\tif (item->conf.command)\n \t\t\tadd_arg_item(arg_head,\n \t\t\t\t xstrdup(token_from_conf(&item->conf, NULL)),\n@@ -877,8 +880,8 @@ static size_t find_trailer_start(const char *buf, size_t len)\n \t\t\tif (recognized_prefix)\n \t\t\t\tcontinue;\n \t\t\tlist_for_each(pos, &conf_head) {\n-\t\t\t\tstruct arg_item *item;\n-\t\t\t\titem = list_entry(pos, struct arg_item, list);\n+\t\t\t\tstruct conf_info_item *item;\n+\t\t\t\titem = list_entry(pos, struct conf_info_item, list);\n \t\t\t\tif (token_matches_conf(bol, &item->conf,\n \t\t\t\t\t\t separator_pos)) {\n \t\t\t\t\trecognized_prefix = 1;\n-- \n2.25.1\n\n"},{"id":"408372","messageId":"20201025212652.3003036-13-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 12/21] trailer: handle configured nondefault separators explicitly","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:43Z","receivedAt":"2020-10-25T22:42:12Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nInstead of parsing out separator from configuration when it is\nprinted, do this parsing when reading the configuration so it can be\nstored separately and \"conf->key\" will contain the actual key only.\n\nNo functional change intended.\n\nSigned-off-by: Anders Waldenborg \n---\n trailer.c | 59 ++++++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 39 insertions(+), 20 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 1592e6c998..102eca0127 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -13,6 +13,7 @@\n struct conf_info {\n \tchar *name;\n \tchar *key;\n+\tchar *nondefault_separator;\n \tchar *command;\n \tenum trailer_where where;\n \tenum trailer_if_exists if_exists;\n@@ -140,32 +141,21 @@ static void free_arg_item(struct arg_item *item)\n \tfree(item);\n }\n \n-static char last_non_space_char(const char *s)\n-{\n-\tint i;\n-\tfor (i = strlen(s) - 1; i >= 0; i--)\n-\t\tif (!isspace(s[i]))\n-\t\t\treturn s[i];\n-\treturn '\\0';\n-}\n-\n static void print_item(FILE *outfile, const struct trailer_item *item)\n {\n \tif (item->token) {\n \t\tconst char *tok = item->token;\n+\t\tconst char *sep = (char []){separators[0], ' ', '\\0'};\n \t\tconst struct conf_info *conf = item->conf;\n-\t\tchar c;\n \n-\t\tif (conf && conf->key)\n-\t\t\ttok = conf->key;\n+\t\tif (conf) {\n+\t\t\tif (conf->key)\n+\t\t\t\ttok = conf->key;\n+\t\t\tif (conf->nondefault_separator)\n+\t\t\t\tsep = conf->nondefault_separator;\n+\t\t}\n \n-\t\tc = last_non_space_char(tok);\n-\t\tif (!c)\n-\t\t\treturn;\n-\t\tif (strchr(separators, c))\n-\t\t\tfputs(tok, outfile);\n-\t\telse\n-\t\t\tfprintf(outfile, \"%s%c \", tok, separators[0]);\n+\t\tfprintf(outfile, \"%s%s\", tok, sep);\n \t}\n \n \tfprintf(outfile, \"%s\\n\", item->value);\n@@ -502,6 +492,34 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v\n \treturn 0;\n }\n \n+static void git_trailer_config_key(const char *conf_key, const char *value, struct conf_info *conf)\n+{\n+\tconst char *end = value + strlen(value) - 1;\n+\n+\twhile (end > value && isspace(*end))\n+\t\tend--;\n+\n+\tif (end == value) {\n+\t\twarning(_(\"Ignoring empty token for key '%s'\"), conf_key);\n+\t\treturn;\n+\t}\n+\n+\tif (strchr(separators, *end)) {\n+\t\tconst char *token_end = end - 1;\n+\t\twhile (token_end > value && isspace(*token_end))\n+\t\t\ttoken_end--;\n+\t\tif (token_end == value) {\n+\t\t\twarning(_(\"Ignoring empty token for key '%s'\"), conf_key);\n+\t\t\treturn;\n+\t\t}\n+\n+\t\tconf->key = xstrndup(value, token_end - value + 1);\n+\t\tconf->nondefault_separator = xstrdup(token_end + 1);\n+\t} else {\n+\t\tconf->key = xstrdup(value);\n+\t}\n+}\n+\n static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n {\n \tconst char *trailer_item, *variable_name;\n@@ -536,7 +554,8 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n \tcase TRAILER_KEY:\n \t\tif (conf->key)\n \t\t\twarning(_(\"more than one %s\"), conf_key);\n-\t\tconf->key = xstrdup(value);\n+\n+\t\tgit_trailer_config_key (conf_key, value, conf);\n \t\tbreak;\n \tcase TRAILER_COMMAND:\n \t\tif (conf->command)\n-- \n2.25.1\n\n"},{"id":"408373","messageId":"20201025212652.3003036-12-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 11/21] trailer: remember separator used in input","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:42Z","receivedAt":"2020-10-25T22:42:14Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nThis will in later commits make it easier to allow configuration to\ndecide if separator should be canonicalized or displayed as it was in\ninput.\n\nNo functional change intended.\n\nSigned-off-by: Anders Waldenborg \n---\n trailer.c | 37 +++++++++++++++++++++++++------------\n 1 file changed, 25 insertions(+), 12 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex d6882155be..1592e6c998 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -34,6 +34,7 @@ struct trailer_item {\n \t */\n \tchar *token;\n \tchar *value;\n+\tchar *used_separator;\n \tconst struct conf_info *conf;\n };\n \n@@ -125,6 +126,7 @@ static void free_trailer_item(struct trailer_item *item)\n {\n \tfree(item->token);\n \tfree(item->value);\n+\tfree(item->used_separator);\n \tfree(item);\n }\n \n@@ -616,18 +618,26 @@ static ssize_t find_separator(const char *line, const char *separators)\n *\n * If separator_pos is -1, interpret the whole trailer as a token.\n */\n-static void parse_trailer(struct strbuf *tok, struct strbuf *val,\n+static void parse_trailer(struct strbuf *tok, struct strbuf *val, struct strbuf *sep,\n \t\t\t const struct conf_info **conf, const char *trailer,\n \t\t\t ssize_t separator_pos)\n {\n \tstruct conf_info_item *item;\n-\tsize_t tok_len;\n \tstruct list_head *pos;\n \n \tif (separator_pos != -1) {\n-\t\tstrbuf_add(tok, trailer, separator_pos);\n-\t\tstrbuf_trim(tok);\n-\t\tstrbuf_addstr(val, trailer + separator_pos + 1);\n+\t\tsize_t sep_spacing_begin = separator_pos;\n+\t\tsize_t sep_spacing_end = separator_pos + 1;\n+\n+\t\twhile (sep_spacing_begin > 0 && trailer[sep_spacing_begin - 1] == ' ')\n+\t\t\tsep_spacing_begin--;\n+\t\twhile (trailer[sep_spacing_end] == ' ')\n+\t\t\tsep_spacing_end++;\n+\n+\t\tstrbuf_add(tok, trailer, sep_spacing_begin);\n+\t\tif (sep)\n+\t\t\tstrbuf_add(sep, trailer + sep_spacing_begin, sep_spacing_end - sep_spacing_begin);\n+\t\tstrbuf_addstr(val, trailer + sep_spacing_end);\n \t\tstrbuf_trim(val);\n \t} else {\n \t\tstrbuf_addstr(tok, trailer);\n@@ -635,12 +645,11 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,\n \t}\n \n \t/* Lookup if the token matches something in the config */\n-\ttok_len = token_len_without_separator(tok->buf, tok->len);\n \tif (conf)\n \t\t*conf = &default_conf_info;\n \tlist_for_each(pos, &conf_head) {\n \t\titem = list_entry(pos, struct conf_info_item, list);\n-\t\tif (token_matches_conf(tok->buf, &item->conf, tok_len)) {\n+\t\tif (token_matches_conf(tok->buf, &item->conf, tok->len)) {\n \t\t\tif (conf)\n \t\t\t\t*conf = &item->conf;\n \t\t\tbreak;\n@@ -649,11 +658,12 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,\n }\n \n static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,\n-\t\t\t\t\t char *val, const struct conf_info *conf)\n+\t\t\t\t\t char *val, char *separator, const struct conf_info *conf)\n {\n \tstruct trailer_item *new_item = xcalloc(sizeof(*new_item), 1);\n \tnew_item->token = tok;\n \tnew_item->value = val;\n+\tnew_item->used_separator = separator;\n \tnew_item->conf = conf;\n \tlist_add_tail(&new_item->list, head);\n \treturn new_item;\n@@ -717,7 +727,7 @@ static void process_command_line_args(struct list_head *arg_head,\n \t\t\t (int) sb.len, sb.buf);\n \t\t\tstrbuf_release(&sb);\n \t\t} else {\n-\t\t\tparse_trailer(&tok, &val, &conf, tr->text,\n+\t\t\tparse_trailer(&tok, &val, NULL, &conf, tr->text,\n \t\t\t\t separator_pos);\n \t\t\tadd_arg_item(arg_head,\n \t\t\t\t strbuf_detach(&tok, NULL),\n@@ -936,6 +946,7 @@ static size_t process_input_file(FILE *outfile,\n \tstruct trailer_info info;\n \tstruct strbuf tok = STRBUF_INIT;\n \tstruct strbuf val = STRBUF_INIT;\n+\tstruct strbuf sep = STRBUF_INIT;\n \tsize_t i;\n \n \ttrailer_info_get(&info, str, opts);\n@@ -955,13 +966,14 @@ static size_t process_input_file(FILE *outfile,\n \t\tseparator_pos = find_separator(trailer, separators);\n \t\tif (separator_pos >= 1) {\n \t\t\tconst struct conf_info *conf;\n-\t\t\tparse_trailer(&tok, &val, &conf, trailer,\n+\t\t\tparse_trailer(&tok, &val, &sep, &conf, trailer,\n \t\t\t\t separator_pos);\n \t\t\tif (opts->unfold)\n \t\t\t\tunfold_value(&val);\n \t\t\tadd_trailer_item(head,\n \t\t\t\t\t strbuf_detach(&tok, NULL),\n \t\t\t\t\t strbuf_detach(&val, NULL),\n+\t\t\t\t\t strbuf_detach(&sep, NULL),\n \t\t\t\t\t conf);\n \t\t} else if (!opts->only_trailers) {\n \t\t\tstrbuf_addstr(&val, trailer);\n@@ -969,6 +981,7 @@ static size_t process_input_file(FILE *outfile,\n \t\t\tadd_trailer_item(head,\n \t\t\t\t\t NULL,\n \t\t\t\t\t strbuf_detach(&val, NULL),\n+\t\t\t\t\t NULL,\n \t\t\t\t\t NULL);\n \t\t}\n \t}\n@@ -1148,7 +1161,7 @@ static void format_trailer_info(struct strbuf *out,\n \n \t\t\tconst struct conf_info *conf;\n \n-\t\t\tparse_trailer(&tok, &val, &conf, trailer, separator_pos);\n+\t\t\tparse_trailer(&tok, &val, NULL, &conf, trailer, separator_pos);\n \t\t\tif (!opts->filter ||\n \t\t\t opts->filter(&tok, conf ? conf->name : NULL, opts->filter_data)) {\n \t\t\t\tif (opts->unfold)\n@@ -1209,7 +1222,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)\n \n \t\tstrbuf_reset(&iter->key);\n \t\tstrbuf_reset(&iter->val);\n-\t\tparse_trailer(&iter->key, &iter->val, NULL,\n+\t\tparse_trailer(&iter->key, &iter->val, NULL, NULL,\n \t\t\t trailer, separator_pos);\n \t\tunfold_value(&iter->val);\n \t\treturn 1;\n-- \n2.25.1\n\n"},{"id":"408374","messageId":"20201025212652.3003036-19-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 18/21] trailer: factor out config lookup to separate function","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:49Z","receivedAt":"2020-10-25T22:42:15Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nNo functional change intended.\n\nSigned-off-by: Anders Waldenborg \n---\n trailer.c | 28 +++++++++++++++-------------\n 1 file changed, 15 insertions(+), 13 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex d75d240e10..02061877b4 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -605,6 +605,20 @@ static int token_matches_conf(const char *tok, const struct conf_info *conf, siz\n \treturn conf->key ? !strncasecmp(tok, conf->key, tok_len) : 0;\n }\n \n+static const struct conf_info *lookup_conf_for_tok(const struct strbuf *tok)\n+{\n+\tstruct conf_info_item *item;\n+\tstruct list_head *pos;\n+\n+\tlist_for_each(pos, &conf_head) {\n+\t\titem = list_entry(pos, struct conf_info_item, list);\n+\t\tif (token_matches_conf(tok->buf, &item->conf, tok->len)) {\n+\t\t\treturn &item->conf;\n+\t\t}\n+\t}\n+\treturn &default_conf_info;\n+}\n+\n /*\n * If the given line is of the form\n * \"...\" or \"...\", return the\n@@ -645,9 +659,6 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val, struct strbuf\n \t\t\t const struct conf_info **conf, const char *trailer,\n \t\t\t ssize_t separator_pos)\n {\n-\tstruct conf_info_item *item;\n-\tstruct list_head *pos;\n-\n \tif (separator_pos != -1) {\n \t\tsize_t sep_spacing_begin = separator_pos;\n \t\tsize_t sep_spacing_end = separator_pos + 1;\n@@ -667,17 +678,8 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val, struct strbuf\n \t\tstrbuf_trim(tok);\n \t}\n \n-\t/* Lookup if the token matches something in the config */\n \tif (conf)\n-\t\t*conf = &default_conf_info;\n-\tlist_for_each(pos, &conf_head) {\n-\t\titem = list_entry(pos, struct conf_info_item, list);\n-\t\tif (token_matches_conf(tok->buf, &item->conf, tok->len)) {\n-\t\t\tif (conf)\n-\t\t\t\t*conf = &item->conf;\n-\t\t\tbreak;\n-\t\t}\n-\t}\n+\t\t*conf = lookup_conf_for_tok (tok);\n }\n \n static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,\n-- \n2.25.1\n\n"},{"id":"408375","messageId":"20201025212652.3003036-6-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 05/21] trailer: rename 'free_all' to 'free_all_trailer_items'","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:36Z","receivedAt":"2020-10-25T22:42:15Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nNo functional change intended.\n\nSigned-off-by: Anders Waldenborg \n---\n trailer.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 8c0687a529..227df1c0ef 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -990,7 +990,7 @@ static size_t process_input_file(FILE *outfile,\n \treturn info.trailer_end - str;\n }\n \n-static void free_all(struct list_head *head)\n+static void free_all_trailer_items(struct list_head *head)\n {\n \tstruct list_head *pos, *p;\n \tlist_for_each_safe(pos, p, head) {\n@@ -1057,7 +1057,7 @@ void process_trailers(const char *file,\n \n \tprint_all(outfile, &head, opts);\n \n-\tfree_all(&head);\n+\tfree_all_trailer_items(&head);\n \n \t/* Print the lines after the trailers as is */\n \tif (!opts->only_trailers)\n-- \n2.25.1\n\n"},{"id":"408376","messageId":"20201025212652.3003036-2-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 01/21] trailer: change token_{from,matches}_item into taking conf_info","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:32Z","receivedAt":"2020-10-25T22:42:17Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nThese functions don't use anything from the arg_item except the conf,\nso make them take conf as argument instead. This will allow them to be\nused on other things that has a conf_info.\n\nNo functional change intended.\n\nSigned-off-by: Anders Waldenborg \n---\n trailer.c | 22 +++++++++++-----------\n 1 file changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 3f7391d793..efb88c2008 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -574,20 +574,20 @@ static void ensure_configured(void)\n \tconfigured = 1;\n }\n \n-static const char *token_from_item(struct arg_item *item, char *tok)\n+static const char *token_from_conf(const struct conf_info *conf, char *tok)\n {\n-\tif (item->conf.key)\n-\t\treturn item->conf.key;\n+\tif (conf->key)\n+\t\treturn conf->key;\n \tif (tok)\n \t\treturn tok;\n-\treturn item->conf.name;\n+\treturn conf->name;\n }\n \n-static int token_matches_item(const char *tok, struct arg_item *item, size_t tok_len)\n+static int token_matches_conf(const char *tok, const struct conf_info *conf, size_t tok_len)\n {\n-\tif (!strncasecmp(tok, item->conf.name, tok_len))\n+\tif (!strncasecmp(tok, conf->name, tok_len))\n \t\treturn 1;\n-\treturn item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;\n+\treturn conf->key ? !strncasecmp(tok, conf->key, tok_len) : 0;\n }\n \n /*\n@@ -650,11 +650,11 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,\n \t\t*conf = &default_conf_info;\n \tlist_for_each(pos, &conf_head) {\n \t\titem = list_entry(pos, struct arg_item, list);\n-\t\tif (token_matches_item(tok->buf, item, tok_len)) {\n+\t\tif (token_matches_conf(tok->buf, &item->conf, tok_len)) {\n \t\t\tchar *tok_buf = strbuf_detach(tok, NULL);\n \t\t\tif (conf)\n \t\t\t\t*conf = &item->conf;\n-\t\t\tstrbuf_addstr(tok, token_from_item(item, tok_buf));\n+\t\t\tstrbuf_addstr(tok, token_from_conf(&item->conf, tok_buf));\n \t\t\tfree(tok_buf);\n \t\t\tbreak;\n \t\t}\n@@ -710,7 +710,7 @@ static void process_command_line_args(struct list_head *arg_head,\n \t\titem = list_entry(pos, struct arg_item, list);\n \t\tif (item->conf.command)\n \t\t\tadd_arg_item(arg_head,\n-\t\t\t\t xstrdup(token_from_item(item, NULL)),\n+\t\t\t\t xstrdup(token_from_conf(&item->conf, NULL)),\n \t\t\t\t xstrdup(\"\"),\n \t\t\t\t &item->conf, NULL);\n \t}\n@@ -879,7 +879,7 @@ static size_t find_trailer_start(const char *buf, size_t len)\n \t\t\tlist_for_each(pos, &conf_head) {\n \t\t\t\tstruct arg_item *item;\n \t\t\t\titem = list_entry(pos, struct arg_item, list);\n-\t\t\t\tif (token_matches_item(bol, item,\n+\t\t\t\tif (token_matches_conf(bol, &item->conf,\n \t\t\t\t\t\t separator_pos)) {\n \t\t\t\t\trecognized_prefix = 1;\n \t\t\t\t\tbreak;\n-- \n2.25.1\n\n"},{"id":"408377","messageId":"20201025212652.3003036-1-anders@0x63.nu","threadId":"54505","inReplyTo":null,"subject":"[PATCH 00/21] trailer fixes","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:31Z","receivedAt":"2020-10-25T22:42:17Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nThis patch series contains a bunch fo trailer related changes. Sparked\nfrom this thread:\n https://public-inbox.org/git/87blk0rjob.fsf@0x63.nu/T/#r3dc3e4fa67b6fba95e4b2ea2c1cf1672af55a9ee\n\nMost commits are refactors preparing for the others, the actual user\nvisible changes are:\n * Allow using aliases in pretty formatting '%(trailer:key=foo)`\n * Fixes related to matching prefix rather than full trailer\n * Tighten up \"canonicalization\" of trailers\n * Add --(no-)canonicalize\n\nAnders Waldenborg (21):\n trailer: change token_{from,matches}_item into taking conf_info\n trailer: don't use 'struct arg_item' for storing config\n doc: mention canonicalization in git i-t manual\n pretty: allow using aliases in %(trailer:key=xyz)\n trailer: rename 'free_all' to 'free_all_trailer_items'\n t4205: add test for trailer in log with nonstandard separator\n trailer: simplify 'arg_item' lifetime\n trailer: keep track of conf in trailer_item\n trailer: refactor print_tok_val into taking item\n trailer: move trailer token canonicalization print time\n trailer: remember separator used in input\n trailer: handle configured nondefault separators explicitly\n trailer: add option to make canonicalization optional\n trailer: move skipping of blank lines to own loop when finding trailer\n trailer: factor out classify_trailer_line\n t7513: add failing test for configured trailing line classification\n trailer: don't treat line with prefix of known trailer as known\n trailer: factor out config lookup to separate function\n trailer: move config lookup out of parse_trailer\n trailer: add failing tests for matching trailers against input\n trailer: only do prefix matching for configured trailers on\n commandline\n\n Documentation/git-interpret-trailers.txt | 10 +-\n Documentation/pretty-formats.txt | 4 +-\n builtin/interpret-trailers.c | 3 +\n pretty.c | 5 +-\n t/t4205-log-pretty-formats.sh | 18 ++\n t/t7513-interpret-trailers.sh | 120 ++++++++\n trailer.c | 374 ++++++++++++++---------\n trailer.h | 3 +-\n 8 files changed, 386 insertions(+), 151 deletions(-)\n\n-- \n2.25.1\n\n"},{"id":"408378","messageId":"20201025212652.3003036-16-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 15/21] trailer: factor out classify_trailer_line","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:46Z","receivedAt":"2020-10-25T22:42:18Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nNo functional change intended.\n\nSigned-off-by: Anders Waldenborg \n---\n trailer.c | 123 ++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 74 insertions(+), 49 deletions(-)\n\ndiff --git a/trailer.c b/trailer.c\nindex 937cf1edeb..21877e4c06 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -821,6 +821,53 @@ static size_t find_patch_start(const char *str)\n \treturn s - str;\n }\n \n+enum trailer_classification {\n+\tGIT_GENERATED_PREFIX,\n+\tCONFIGURED_TRAILER,\n+\tTRAILER,\n+\tCONTINUATION,\n+\tNON_TRAILER,\n+\tCOMMENT,\n+\tBLANK,\n+};\n+\n+static enum trailer_classification classify_trailer_line(const char *line)\n+{\n+\tconst char **p;\n+\tssize_t separator_pos;\n+\n+\tif (line[0] == comment_line_char)\n+\t\treturn COMMENT;\n+\n+\tif (is_blank_line(line))\n+\t\treturn BLANK;\n+\n+\tif (isspace(line[0]))\n+\t\treturn CONTINUATION;\n+\n+\tfor (p = git_generated_prefixes; *p; p++)\n+\t\tif (starts_with(line, *p))\n+\t\t\treturn GIT_GENERATED_PREFIX;\n+\n+\n+\tseparator_pos = find_separator(line, separators);\n+\tif (separator_pos >= 1) {\n+\t\tstruct list_head *pos;\n+\n+\t\tlist_for_each(pos, &conf_head) {\n+\t\t\tstruct conf_info_item *item;\n+\t\t\titem = list_entry(pos, struct conf_info_item, list);\n+\t\t\tif (token_matches_conf(line, &item->conf,\n+\t\t\t separator_pos))\n+\t\t\t\treturn CONFIGURED_TRAILER;\n+\t\t}\n+\n+\t\treturn TRAILER;\n+\t}\n+\n+\treturn NON_TRAILER;\n+}\n+\n /*\n * Return the position of the first trailer line or len if there are no\n * trailers.\n@@ -864,59 +911,37 @@ static size_t find_trailer_start(const char *buf, size_t len)\n \t l >= end_of_title;\n \t l = last_line(buf, l)) {\n \t\tconst char *bol = buf + l;\n-\t\tconst char **p;\n-\t\tssize_t separator_pos;\n \n-\t\tif (bol[0] == comment_line_char) {\n-\t\t\tnon_trailer_lines += possible_continuation_lines;\n-\t\t\tpossible_continuation_lines = 0;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (is_blank_line(bol)) {\n-\t\t\tnon_trailer_lines += possible_continuation_lines;\n-\t\t\tif (recognized_prefix &&\n-\t\t\t trailer_lines * 3 >= non_trailer_lines)\n-\t\t\t\treturn next_line(bol) - buf;\n-\t\t\telse if (trailer_lines && !non_trailer_lines)\n-\t\t\t\treturn next_line(bol) - buf;\n-\t\t\treturn len;\n-\t\t}\n-\n-\t\tfor (p = git_generated_prefixes; *p; p++) {\n-\t\t\tif (starts_with(bol, *p)) {\n+\t\tswitch (classify_trailer_line(bol)) {\n+\t\t\tcase GIT_GENERATED_PREFIX:\n+\t\t\tcase CONFIGURED_TRAILER:\n+\t\t\t\trecognized_prefix = 1;\n+\t\t\t\t/* fallthrough */\n+\t\t\tcase TRAILER:\n \t\t\t\ttrailer_lines++;\n \t\t\t\tpossible_continuation_lines = 0;\n-\t\t\t\trecognized_prefix = 1;\n-\t\t\t\tgoto continue_outer_loop;\n-\t\t\t}\n-\t\t}\n-\n-\t\tseparator_pos = find_separator(bol, separators);\n-\t\tif (separator_pos >= 1 && !isspace(bol[0])) {\n-\t\t\tstruct list_head *pos;\n-\n-\t\t\ttrailer_lines++;\n-\t\t\tpossible_continuation_lines = 0;\n-\t\t\tif (recognized_prefix)\n-\t\t\t\tcontinue;\n-\t\t\tlist_for_each(pos, &conf_head) {\n-\t\t\t\tstruct conf_info_item *item;\n-\t\t\t\titem = list_entry(pos, struct conf_info_item, list);\n-\t\t\t\tif (token_matches_conf(bol, &item->conf,\n-\t\t\t\t\t\t separator_pos)) {\n-\t\t\t\t\trecognized_prefix = 1;\n-\t\t\t\t\tbreak;\n-\t\t\t\t}\n-\t\t\t}\n-\t\t} else if (isspace(bol[0]))\n-\t\t\tpossible_continuation_lines++;\n-\t\telse {\n-\t\t\tnon_trailer_lines++;\n-\t\t\tnon_trailer_lines += possible_continuation_lines;\n-\t\t\tpossible_continuation_lines = 0;\n+\t\t\t\tbreak;\n+\t\t\tcase CONTINUATION:\n+\t\t\t\tpossible_continuation_lines++;\n+\t\t\t\tbreak;\n+\t\t\tcase NON_TRAILER:\n+\t\t\t\tnon_trailer_lines++;\n+\t\t\t\tnon_trailer_lines += possible_continuation_lines;\n+\t\t\t\tpossible_continuation_lines = 0;\n+\t\t\t\tbreak;\n+\t\t\tcase COMMENT:\n+\t\t\t\tnon_trailer_lines += possible_continuation_lines;\n+\t\t\t\tpossible_continuation_lines = 0;\n+\t\t\t\tbreak;\n+\t\t\tcase BLANK:\n+\t\t\t\tnon_trailer_lines += possible_continuation_lines;\n+\t\t\t\tif (recognized_prefix &&\n+\t\t\t\t\ttrailer_lines * 3 >= non_trailer_lines)\n+\t\t\t\t\treturn next_line(bol) - buf;\n+\t\t\t\telse if (trailer_lines && !non_trailer_lines)\n+\t\t\t\t\treturn next_line(bol) - buf;\n+\t\t\t\treturn len;\n \t\t}\n-continue_outer_loop:\n-\t\t;\n \t}\n \n \treturn len;\n-- \n2.25.1\n\n"},{"id":"408379","messageId":"20201025212652.3003036-22-anders@0x63.nu","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 21/21] trailer: only do prefix matching for configured trailers on commandline","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-10-25T21:26:52Z","receivedAt":"2020-10-25T22:42:20Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"); SAEximRunCond expanded to false\n\nIf there is a trailer \"foobar\" in configuration a trailer \"foo\" in\ninput shouldn't match that, except in `--trailer` arguments as a\nshortcut.\n\nSigned-off-by: Anders Waldenborg \n---\n t/t7513-interpret-trailers.sh | 17 +++++++++++++----\n trailer.c | 14 +++++++++-----\n 2 files changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex a99d6d7e3b..9e06fa4454 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -151,8 +151,7 @@ test_expect_success 'spelling and separators are not canonicalized with --parse\n \ttest_cmp expected actual\n '\n \n-# Matching currently is prefix matching, causing \"This-trailer\" to be normalized too\n-test_expect_failure 'config option matches exact only' '\n+test_expect_success 'config option matches exact only' '\n \tcat >patch <<-\\EOF &&\n \n \t\tThis-trailer: a\n@@ -171,8 +170,7 @@ test_expect_failure 'config option matches exact only' '\n \ttest_cmp expected actual\n '\n \n-# Matching currently uses the config key even if key value is different\n-test_expect_failure 'config option matches exact only' '\n+test_expect_success 'config option matches exact only' '\n \tcat >patch <<-\\EOF &&\n \n \t\tTicket: 1234\n@@ -550,6 +548,17 @@ test_expect_success 'with config setup' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'trailer on commandline can be prefix of configured' '\n+\tcat >expected <<-\\EOF &&\n+\n+\t\tAcked-by: 10\n+\tEOF\n+\tgit interpret-trailers --trailer \"A=10\" empty >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+\n+\n test_expect_success 'with config setup and \":=\" as separators' '\n \tgit config trailer.separators \":=\" &&\n \tgit config trailer.ack.key \"Acked-by= \" &&\ndiff --git a/trailer.c b/trailer.c\nindex 0db3bba3b1..b00b35ea0e 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -605,14 +605,18 @@ static int token_matches_conf(const char *tok, const struct conf_info *conf, siz\n \treturn conf->key ? !strncasecmp(tok, conf->key, tok_len) : 0;\n }\n \n-static const struct conf_info *lookup_conf_for_tok(const struct strbuf *tok)\n+static const struct conf_info *lookup_conf_for_tok(const struct strbuf *tok, int strict)\n {\n \tstruct conf_info_item *item;\n \tstruct list_head *pos;\n \n \tlist_for_each(pos, &conf_head) {\n \t\titem = list_entry(pos, struct conf_info_item, list);\n-\t\tif (token_matches_conf(tok->buf, &item->conf, tok->len)) {\n+\t\tif (strict) {\n+\t\t\tconst char *match = item->conf.key ? item->conf.key : item->conf.name;\n+\t\t\tif (!strcasecmp(match, tok->buf))\n+\t\t\t\treturn &item->conf;\n+\t\t} else if (token_matches_conf(tok->buf, &item->conf, tok->len)) {\n \t\t\treturn &item->conf;\n \t\t}\n \t}\n@@ -750,7 +754,7 @@ static void process_command_line_args(struct list_head *arg_head,\n \t\t} else {\n \t\t\tparse_trailer(&tok, &val, NULL, tr->text,\n \t\t\t\t separator_pos);\n-\t\t\tconf = lookup_conf_for_tok(&tok);\n+\t\t\tconf = lookup_conf_for_tok(&tok, 0);\n \t\t\tadd_arg_item(arg_head,\n \t\t\t\t strbuf_detach(&tok, NULL),\n \t\t\t\t strbuf_detach(&val, NULL),\n@@ -1025,7 +1029,7 @@ static size_t process_input_file(FILE *outfile,\n \t\t\tconst struct conf_info *conf;\n \t\t\tparse_trailer(&tok, &val, &sep, trailer,\n \t\t\t\t separator_pos);\n-\t\t\tconf = lookup_conf_for_tok(&tok);\n+\t\t\tconf = lookup_conf_for_tok(&tok, 1);\n \t\t\tif (opts->unfold)\n \t\t\t\tunfold_value(&val);\n \t\t\tadd_trailer_item(head,\n@@ -1220,7 +1224,7 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tconst struct conf_info *conf;\n \n \t\t\tparse_trailer(&tok, &val, NULL, trailer, separator_pos);\n-\t\t\tconf = lookup_conf_for_tok(&tok);\n+\t\t\tconf = lookup_conf_for_tok(&tok, 1);\n \t\t\tif (!opts->filter ||\n \t\t\t opts->filter(&tok, conf ? conf->name : NULL, opts->filter_data)) {\n \t\t\t\tif (opts->unfold)\n-- \n2.25.1\n\n"},{"id":"408389","messageId":"CAP8UFD0J+hfARYWmaj51wzUT-Y-nD=9RyvyFqrzsUYMd48WKPg@mail.gmail.com","threadId":"54505","inReplyTo":"20201025212652.3003036-2-anders@0x63.nu","subject":"Re: [PATCH 01/21] trailer: change token_{from,matches}_item into taking conf_info","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-10-26T11:56:10Z","receivedAt":"2020-10-26T11:56:25Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg wrote:\n>\n> ); SAEximRunCond expanded to false\n\nDo you have a way to avoid the above line? It shouldn't be in the\ncommit message after applying this.\n\n> These functions don't use anything from the arg_item except the conf,\n> so make them take conf as argument instead. This will allow them to be\n> used on other things that has a conf_info.\n\ns/has/have/\n"},{"id":"408390","messageId":"CAP8UFD3=HLzG=b61DQYQfAErOg+KXAg-8x06MpDLi+1=NcgejQ@mail.gmail.com","threadId":"54505","inReplyTo":"20201025212652.3003036-4-anders@0x63.nu","subject":"Re: [PATCH 03/21] doc: mention canonicalization in git i-t manual","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-10-26T12:14:01Z","receivedAt":"2020-10-26T12:14:20Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg wrote:\n\n> diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt\n> index 96ec6499f0..a4be8aed66 100644\n> --- a/Documentation/git-interpret-trailers.txt\n> +++ b/Documentation/git-interpret-trailers.txt\n> @@ -25,6 +25,11 @@ Otherwise, this command applies the arguments passed using the\n> `--trailer` option, if any, to the commit message part of each input\n> file. The result is emitted on the standard output.\n>\n> +When trailers read from input they will be changed into \"canonical\"\n\nDo you mean \"When trailers are read from standard input\"?\n\n> +form if the trailer has a corresponding 'trailer..key'\n> +configuration value.\n\nThis doesn't explain what the canonical form is. So maybe something like:\n\n\"When there is a 'trailer..key' configuration value defined,\nthis value becomes the canonical form of the trailer, so when\na trailer matching is read from standard input, it is changed\nto this canonical value.\"\n\n> This means that it will use the exact spelling\n> +(upper case vs lower case and separator) defined in configuration.\n\nMaybe:\n\n\"This means that the key part of the trailer will use the exact\nspelling (upper case vs lower case and separator) defined in the\nconfiguration.\"\n"},{"id":"408391","messageId":"CAP8UFD31UwuiQrWmM7te1Am3i9Ryvi8_XgDLM6B1fs5WUn_GxQ@mail.gmail.com","threadId":"54505","inReplyTo":"20201025212652.3003036-5-anders@0x63.nu","subject":"Re: [PATCH 04/21] pretty: allow using aliases in %(trailer:key=xyz)","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-10-26T12:38:10Z","receivedAt":"2020-10-26T12:38:27Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg wrote:\n\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index 84bbc7439a..1714fa447d 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -256,7 +256,9 @@ endif::git-rev-list[]\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> + shown. If `trailer..key` configuration option is set 'token'\n\ns/If `trailer..key`/If the `trailer..key`/\ns/is set 'token'/is set, ''/\n\n> + can be used as an alias for showing trailers with the value in\n> + key.\n\nMaybe:\n\n\"... for showing trailers case insensitively matching the value of\nthis configuration option.\"\n"},{"id":"408392","messageId":"CAP8UFD0Fpkj_xc-UBCeayw2C_4eXCx7Kan90PCvoM-KUMVEGDA@mail.gmail.com","threadId":"54505","inReplyTo":"20201025212652.3003036-6-anders@0x63.nu","subject":"Re: [PATCH 05/21] trailer: rename 'free_all' to 'free_all_trailer_items'","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-10-26T12:42:25Z","receivedAt":"2020-10-26T12:42:39Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg wrote:\n>\n> ); SAEximRunCond expanded to false\n\nAs already mentioned, please find a way to remove the above line in\nall your patches.\n\n> No functional change intended.\n\nThis doesn't explain much why renaming 'free_all' to\n'free_all_trailer_items' is a good idea. Is the function specific to\ntrailer items or is it generic enough to be useful on other 'struct\nlist_head *head'?\n\n> Signed-off-by: Anders Waldenborg \n"},{"id":"408393","messageId":"CAP8UFD1nYgqT1k1Mc=Ea3AZkb-TdhPBzXo+N+4nWgYVxEBxzRA@mail.gmail.com","threadId":"54505","inReplyTo":"20201025212652.3003036-7-anders@0x63.nu","subject":"Re: [PATCH 06/21] t4205: add test for trailer in log with nonstandard separator","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-10-26T12:43:29Z","receivedAt":"2020-10-26T12:43:43Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg wrote:\n>\n> ); SAEximRunCond expanded to false\n>\n> Signed-off-by: Anders Waldenborg \n\nWhy is this new test important?\n"},{"id":"409422","messageId":"87y2jap4ch.fsf@0x63.nu","threadId":"54505","inReplyTo":"CAP8UFD1nYgqT1k1Mc=Ea3AZkb-TdhPBzXo+N+4nWgYVxEBxzRA@mail.gmail.com","subject":"Re: [PATCH 06/21] t4205: add test for trailer in log with nonstandard separator","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-11-09T22:12:14Z","receivedAt":"2020-11-09T22:12:51Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nChristian Couder writes:\n\n> On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg wrote:\n>>\n>> ); SAEximRunCond expanded to false\n\nPlease disregard this line. It is an unfortunate and most embarrassing\nartifact of messed up git send-email and stmp forwarding over ssh. Which\nhopefully have been sorted so it doesn't happen next time. It obviously\nshouldn't be part of the commit massage in any of the patches in the\nseries.\n\n>> Signed-off-by: Anders Waldenborg \n>\n> Why is this new test important?\n\nThe test that checks that 'git log --pretty=format:%(trailers)' shows\nthe output in the form \"Closes: 1234\" even if input was \"Closes #1234\"\nis interesting both because it checks that this behavior is kept intact\nin the patches later in the series which modifies handling of separator\nand because it is a behavior that can be surprising and not well defined\nin documentation and those tend to be the ones that are easiest to\naccidentally break. Maybe the addition of the test should come later in\nthe series where the changes that potentially could break it happen.\n\n\nIt seems like you stopped reviewing my patch series at patch 06/21. That\nis IMHO just before it starts to get interesting :) Now I don't know if\nrest of it was rubbish or uninteresting or just there was no time to\nlook at it.\n\nI've updated according to the suggestions, but not sure if I should\nrepost the series with just such small adjustments.\n"},{"id":"409468","messageId":"CAP8UFD1pnZeoL7KFCTKdO8OQm0zh9cJWzkyk4+4ykhvwj1ZkMA@mail.gmail.com","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"Re: [PATCH 00/21] trailer fixes","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-11-10T07:44:27Z","receivedAt":"2020-11-10T07:44:42Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg wrote:\n>\n>\n> This patch series contains a bunch fo trailer related changes. Sparked\n\ns/fo/of/\n\n> from this thread:\n> https://public-inbox.org/git/87blk0rjob.fsf@0x63.nu/T/#r3dc3e4fa67b6fba95e4b2ea2c1cf1672af55a9ee\n\nOk.\n\n> Most commits are refactors preparing for the others, the actual user\n> visible changes are:\n> * Allow using aliases in pretty formatting '%(trailer:key=foo)`\n> * Fixes related to matching prefix rather than full trailer\n> * Tighten up \"canonicalization\" of trailers\n> * Add --(no-)canonicalize\n\nIt's not easy to see which patch(es)/commit(s) correspond to which\nchange. Maybe you could add numbers like 4/21, 5/21, etc after each of\nthe above visible changes. Or you could say for example \"patches 1/21\nto 4/21 are doing this, then patch 5/21 is doing this, then patches\n6/21 to 9/21 are doing something else\" so it would help us have a\nbetter overview of the series.\n\nThanks for working on this!\n"},{"id":"409469","messageId":"CAP8UFD0SkCHpSeZ0aWO21mj7+DJJ0GRJhkSCeKzbUsgQkwVLRw@mail.gmail.com","threadId":"54505","inReplyTo":"87y2jap4ch.fsf@0x63.nu","subject":"Re: [PATCH 06/21] t4205: add test for trailer in log with nonstandard separator","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-11-10T07:55:42Z","receivedAt":"2020-11-10T07:55:56Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Nov 9, 2020 at 11:12 PM Anders Waldenborg wrote:\n>\n>\n> Christian Couder writes:\n>\n> > On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg wrote:\n> >>\n> >> ); SAEximRunCond expanded to false\n>\n> Please disregard this line. It is an unfortunate and most embarrassing\n> artifact of messed up git send-email and stmp forwarding over ssh. Which\n> hopefully have been sorted so it doesn't happen next time. It obviously\n> shouldn't be part of the commit massage in any of the patches in the\n> series.\n\nOk.\n\n> >> Signed-off-by: Anders Waldenborg \n> >\n> > Why is this new test important?\n>\n> The test that checks that 'git log --pretty=format:%(trailers)' shows\n> the output in the form \"Closes: 1234\" even if input was \"Closes #1234\"\n> is interesting both because it checks that this behavior is kept intact\n> in the patches later in the series which modifies handling of separator\n> and because it is a behavior that can be surprising and not well defined\n> in documentation and those tend to be the ones that are easiest to\n> accidentally break.\n\nOk, I would suggest adding some of the above in the commit message of\nthe next version of the patch.\n\n> Maybe the addition of the test should come later in\n> the series where the changes that potentially could break it happen.\n\nMaybe. I found the series a bit confusing because it seemed to me that\nthe cover letter wasn't explaining very well what it does. I just\ncommented on the cover letter. Hopefully in the next version it will\nbe better, and it will then be easier to see if patches should be\nmoved around.\n\n> It seems like you stopped reviewing my patch series at patch 06/21. That\n> is IMHO just before it starts to get interesting :) Now I don't know if\n> rest of it was rubbish or uninteresting or just there was no time to\n> look at it.\n\nIt was a combination of not much time and the cover letter not making\nit easy to understand the whole series. I was hoping that the next\nversion would have more explanations in the cover letter and also in\nsome commit messages.\n\n> I've updated according to the suggestions, but not sure if I should\n> repost the series with just such small adjustments.\n\nI think it's worth reposting with an improved cover letter and other\nsmall adjustments.\n\nThanks,\nChristian.\n"},{"id":"409511","messageId":"20201110195219.GB1987088@coredump.intra.peff.net","threadId":"54505","inReplyTo":"CAP8UFD0Fpkj_xc-UBCeayw2C_4eXCx7Kan90PCvoM-KUMVEGDA@mail.gmail.com","subject":"Re: [PATCH 05/21] trailer: rename 'free_all' to 'free_all_trailer_items'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-10T19:52:19Z","receivedAt":"2020-11-10T19:52:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 26, 2020 at 01:42:25PM +0100, Christian Couder wrote:\n\n> > No functional change intended.\n> \n> This doesn't explain much why renaming 'free_all' to\n> 'free_all_trailer_items' is a good idea. Is the function specific to\n> trailer items or is it generic enough to be useful on other 'struct\n> list_head *head'?\n\nIt can't be generic, because list_head needs to use the expected\ncontaining type to find the containing pointer. So free_all() is quite a\nbad name, even within trailer.c, because the compiler won't even tell\nyou if you pass a different list to it.\n\nI do agree this should be spelled out in the commit message, though. :)\n\n-Peff\n"},{"id":"409512","messageId":"20201110195439.GC1987088@coredump.intra.peff.net","threadId":"54505","inReplyTo":"87y2jap4ch.fsf@0x63.nu","subject":"Re: [PATCH 06/21] t4205: add test for trailer in log with nonstandard separator","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-10T19:54:39Z","receivedAt":"2020-11-10T19:55:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 09, 2020 at 11:12:14PM +0100, Anders Waldenborg wrote:\n\n> > Why is this new test important?\n> \n> The test that checks that 'git log --pretty=format:%(trailers)' shows\n> the output in the form \"Closes: 1234\" even if input was \"Closes #1234\"\n> is interesting both because it checks that this behavior is kept intact\n> in the patches later in the series which modifies handling of separator\n> and because it is a behavior that can be surprising and not well defined\n> in documentation and those tend to be the ones that are easiest to\n> accidentally break. Maybe the addition of the test should come later in\n> the series where the changes that potentially could break it happen.\n\nThat makes sense, but should be in the commit message.\n\nI also found the expected output confusing. I thought at first we were\nmis-parsing to include part of the subject in the trailer, but it is\njust that we put \"%s\" into the format argument.\n\n> It seems like you stopped reviewing my patch series at patch 06/21. That\n> is IMHO just before it starts to get interesting :) Now I don't know if\n> rest of it was rubbish or uninteresting or just there was no time to\n> look at it.\n> \n> I've updated according to the suggestions, but not sure if I should\n> repost the series with just such small adjustments.\n\nReviewing this has been on my todo list, but I'd just as soon do it from\nyour latest version. Since it has been a while, it may make sense to\njust repost with the fixes, and note in the cover letter that it didn't\nget a lot of review yet.\n\n-Peff\n"},{"id":"409513","messageId":"20201110195840.GD1987088@coredump.intra.peff.net","threadId":"54505","inReplyTo":"20201025212652.3003036-9-anders@0x63.nu","subject":"Re: [PATCH 08/21] trailer: keep track of conf in trailer_item","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-10T19:58:40Z","receivedAt":"2020-11-10T19:58:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 25, 2020 at 10:26:39PM +0100, Anders Waldenborg wrote:\n\n> Signed-off-by: Anders Waldenborg \n\nFor refactoring commits like this, it can help to even give one or two\nsentences saying why this moves us in a good direction.\n\nProbably it is obvious to you, who wrote the later commits already, but\nin isolation it's hard to say if it is good move direction or not. And\nmaybe it would become apparent by the time I get to the end, but leading\nthe reviewer along the string of refactorings is a good way to help them\nunderstand the end state, too. :)\n\n-Peff\n"},{"id":"409515","messageId":"20201110200604.GE1987088@coredump.intra.peff.net","threadId":"54505","inReplyTo":"20201025212652.3003036-13-anders@0x63.nu","subject":"Re: [PATCH 12/21] trailer: handle configured nondefault separators explicitly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-10T20:06:04Z","receivedAt":"2020-11-10T20:06:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 25, 2020 at 10:26:43PM +0100, Anders Waldenborg wrote:\n\n> static void print_item(FILE *outfile, const struct trailer_item *item)\n> {\n> \tif (item->token) {\n> \t\tconst char *tok = item->token;\n> +\t\tconst char *sep = (char []){separators[0], ' ', '\\0'};\n\nI don't think this syntax is likely to be sufficiently portable, as\nyou're defining a variable length array implicitly. I think:\n\n char orig_sep[] = { separators[0], ' ', '\\0' };\n const char *sep = orig_sep;\n\nwould work. Though I suspect that just making this:\n\n> -\t\tc = last_non_space_char(tok);\n> -\t\tif (!c)\n> -\t\t\treturn;\n> -\t\tif (strchr(separators, c))\n> -\t\t\tfputs(tok, outfile);\n> -\t\telse\n> -\t\t\tfprintf(outfile, \"%s%c \", tok, separators[0]);\n> +\t\tfprintf(outfile, \"%s%s\", tok, sep);\n\ninto:\n\n fprintf(outfile, \"%s\", tok);\n if (conf && conf->nondefault_separator)\n\tfprintf(outfile, \"%s\", conf->nondefault_separator);\n else\n\tfprintf(outfile, \"%c \", separators[0]);\n\nmight be simpler for a reader to follow, even though it's a little more\nverbose.\n\n-Peff\n"},{"id":"409517","messageId":"20201110201040.GF1987088@coredump.intra.peff.net","threadId":"54505","inReplyTo":"20201025212652.3003036-14-anders@0x63.nu","subject":"Re: [PATCH 13/21] trailer: add option to make canonicalization optional","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-10T20:10:40Z","receivedAt":"2020-11-10T20:10:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 25, 2020 at 10:26:44PM +0100, Anders Waldenborg wrote:\n\n> Adds a new `--(no-)canonicalize` option to interpret-trailers. By\n> default it is on unless `--parse` option is given.\n> \n> When option is on trailer tokens and separators get canonicalized to\n> the form they have in config (if there is any config for that\n> trailer). This is same behavior as before this patch, which allows\n> this behavior to be disabled with `--no-canonicalize`. `--parse` now\n> also implies `--no-canonicalize`, if previous behavior with\n> canonicalization also in parse mode is wanted it needs to be combined\n> with `--parse --canonicalize`\n\nI'm not sure if this should be tied to --parse or not. The idea of\n--parse is that you'd normalize syntactic issues to make it easy to\nparse the result. But wouldn't normalizing names around spelling or\ncapitalization be what you'd usually want there?\n\nSo it sounds like you'd want it to _always_ be on, but leave\n\"--no-canonicalize\" as an escape hatch for somebody who's researching\nspelling variants.\n\nOr maybe I'm misunderstanding what it does, since...\n\n> --- a/Documentation/git-interpret-trailers.txt\n> +++ b/Documentation/git-interpret-trailers.txt\n> @@ -129,13 +129,16 @@ OPTIONS\n> \n> --parse::\n> \tA convenience alias for `--only-trailers --only-input\n> -\t--unfold`.\n> +\t--unfold --no-canonicalize`.\n> \n> --no-divider::\n> \tDo not treat `---` as the end of the commit message. Use this\n> \twhen you know your input contains just the commit message itself\n> \t(and not an email or the output of `git format-patch`).\n> \n> +--no-canonicalize::\n> +\tDisable canonicalization of input trailers.\n\nI think this needs to define \"canonicalization\" here.\n\n-Peff\n"},{"id":"409518","messageId":"20201110201638.GG1987088@coredump.intra.peff.net","threadId":"54505","inReplyTo":"20201025212652.3003036-18-anders@0x63.nu","subject":"Re: [PATCH 17/21] trailer: don't treat line with prefix of known trailer as known","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-10T20:16:38Z","receivedAt":"2020-11-10T20:16:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 25, 2020 at 10:26:48PM +0100, Anders Waldenborg wrote:\n\n> E.g if \"Closes\" is a configured trailer a line starting with \"c:\"\n> shouldn't be treated as a recognized trailer when looking for trailer\n> block.\n\nWould this mean that:\n\n Signed-off:\n\nis no longer an alias for:\n\n Signed-off-by:\n\n? TBH that seems overall much more sane to me, but I wonder if it is\nbreaking somebody's expectations.\n\n-Peff\n"},{"id":"411445","messageId":"20201205013918.18981-2-avarab@gmail.com","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 1/5] pretty format %(trailers) test: split a long line","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-05T01:39:14Z","receivedAt":"2020-12-05T01:40:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Split a very long line in a test introduced in 0b691d86851 (pretty:\nadd support for separator option in %(trailers), 2019-01-28). This\nmakes it easier to read, and it'll be used as a template in follow-up\ncommits.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t4205-log-pretty-formats.sh | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 42544fb07a0..bf9b30ff3d6 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -723,7 +723,12 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n \n test_expect_success 'pretty format %(trailers:separator) changes separator' '\n \tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00,unfold)X\" >actual &&\n-\tprintf \"XSigned-off-by: A U Thor \\0Acked-by: A U Thor \\0[ v2 updated patch description ]\\0Signed-off-by: A U Thor X\" >expect &&\n+\t(\n+\t\tprintf \"XSigned-off-by: A U Thor \\0\" &&\n+\t\tprintf \"Acked-by: A U Thor \\0\" &&\n+\t\tprintf \"[ v2 updated patch description ]\\0\" &&\n+\t\tprintf \"Signed-off-by: A U Thor X\"\n+\t) >expect &&\n \ttest_cmp expect actual\n '\n \n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411446","messageId":"20201205013918.18981-1-avarab@gmail.com","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 0/5] pretty format %(trailers): improve machine readability","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-05T01:39:13Z","receivedAt":"2020-12-05T01:40:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I started writing this on top of \"master\", but then saw the\noutstanding series of other miscellaneous fixes to this\nfacility[1]. This is on top of that topic & rebased on master.\n\nAnders, any plans to re-roll yours? Otherwise the conflicts I'd have\non mine are easy to fix, so I can also submit it as a stand-alone.\n\nThis series comes out of a discussion at work today (well, yesterday\nat this point) where someone wanted to parse %(trailers) output. As\nnoted in 3/5 doing this is rather tedious now if you're trying to\nunambiguously grap trailers as a stream of key-value pairs.\n\nSo this series adds a \"key_value_separator\" and \"keyonly\" parameters,\nand fixes a few bugs I saw along the way.\n\n1. https://lore.kernel.org/git/20201025212652.3003036-1-anders@0x63.nu/\n\nÆvar Arnfjörð Bjarmason (5):\n pretty format %(trailers) test: split a long line\n pretty format %(trailers): avoid needless repetition\n pretty format %(trailers): add a \"keyonly\"\n pretty-format %(trailers): fix broken standalone \"valueonly\"\n pretty format %(trailers): add a \"key_value_separator\"\n\n Documentation/pretty-formats.txt | 33 ++++++++-------\n pretty.c | 12 ++++++\n t/t4205-log-pretty-formats.sh | 71 +++++++++++++++++++++++++++++++-\n trailer.c | 14 +++++--\n trailer.h | 3 ++\n 5 files changed, 113 insertions(+), 20 deletions(-)\n\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411447","messageId":"20201205013918.18981-4-avarab@gmail.com","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 3/5] pretty format %(trailers): add a \"keyonly\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-05T01:39:16Z","receivedAt":"2020-12-05T01:40:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add support for a \"keyonly\". This allows for easier parsing out of the\nkey and value. Before if you didn't want to make assumptions about how\nthe key was formatted. You'd need to parse it out as e.g.:\n\n --pretty=format:'%H%x00%(trailers:separator=%x00%x00)' \\\n '%x00%(trailers:separator=%x00%x00,valueonly)'\n\nAnd then proceed to deduce keys by looking at those two and\nsubtracting the value plus the hardcoded \": \" separator from the\nnon-valueonly %(trailers) line. Now it's possible to simply do:\n\n --pretty=format:'%H%x00%(trailers:separator=%x00%x00,keyonly)' \\\n '%x00%(trailers:separator=%x00%x00,valueonly)'\n\nWhich at least reduces it to a state machine where you get N keys and\ncorrelate them with N values. Even better would be to have a way to\nchange the \": \" delimiter to something easily machine-readable (a key\nmight contain \": \" too). A follow-up change will add support for that.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/pretty-formats.txt | 4 ++--\n pretty.c | 1 +\n t/t4205-log-pretty-formats.sh | 31 ++++++++++++++++++++++++++++++-\n trailer.c | 7 +++++--\n trailer.h | 2 ++\n 5 files changed, 40 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 8e066594624..d080f0d2476 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -284,8 +284,8 @@ multiple times the last occurance wins.\n ** 'unfold[=bool]': make it behave as if interpret-trailer's `--unfold`\n option was given. E.g.,\n `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n-** 'valueonly[=bool]': skip over the key part of the trailer line and only\n- show the value part.\n+** 'keyonly[=bool]': only show the key part of the trailer.\n+** 'valueonly[=bool]': only show the value part of the trailer.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex 3c374abffe5..590f37489f6 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1454,6 +1454,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\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, \"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}\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex bf9b30ff3d6..5dd080c19b2 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -715,12 +715,34 @@ test_expect_success '%(trailers:key) without value is error' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:key=foo,keyonly) shows only keys' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:keyonly)\" >actual &&\n+\ttest_write_lines \\\n+\t\t\"Signed-off-by\" \\\n+\t\t\"Acked-by\" \\\n+\t\t\"[ v2 updated patch description ]\" \\\n+\t\t\"Signed-off-by\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=foo,keyonly) shows only key' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,keyonly)\" >actual &&\n+\techo \"Acked-by\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n \tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,valueonly)\" >actual &&\n \techo \"A U Thor \" >expect &&\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:key=foo,keyonly,valueonly) shows nothing' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,keyonly,valueonly)\" >actual &&\n+\techo >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'pretty format %(trailers:separator) changes separator' '\n \tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00,unfold)X\" >actual &&\n \t(\n@@ -732,7 +754,7 @@ test_expect_success 'pretty format %(trailers:separator) changes separator' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' '\n+test_expect_success 'pretty format %(trailers) combining separator/key/keyonly/valueonly' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tImportant fix\n \n@@ -759,6 +781,13 @@ test_expect_success 'pretty format %(trailers) combining separator/key/valueonly\n \t\t\"Does not close any tickets\" \\\n \t\t\"Another fix #567, #890\" \\\n \t\t\"Important fix #1234\" >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit log --pretty=\"%s% (trailers:separator=%x2c%x20,key=Closes,keyonly)\" HEAD~3.. >actual &&\n+\ttest_write_lines \\\n+\t\t\"Does not close any tickets\" \\\n+\t\t\"Another fix Closes, Closes\" \\\n+\t\t\"Important fix Closes\" >expect &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/trailer.c b/trailer.c\nindex b00b35ea0eb..40f31e4dfc2 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1233,8 +1233,11 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\t\tif (opts->separator && out->len != origlen)\n \t\t\t\t\tstrbuf_addbuf(out, opts->separator);\n \t\t\t\tif (!opts->value_only)\n-\t\t\t\t\tstrbuf_addf(out, \"%s: \", tok.buf);\n-\t\t\t\tstrbuf_addbuf(out, &val);\n+\t\t\t\t\tstrbuf_addstr(out, tok.buf);\n+\t\t\t\tif (!opts->key_only && !opts->value_only)\n+\t\t\t\t\tstrbuf_addstr(out, \": \");\n+\t\t\t\tif (!opts->key_only)\n+\t\t\t\t\tstrbuf_addbuf(out, &val);\n \t\t\t\tif (!opts->separator)\n \t\t\t\t\tstrbuf_addch(out, '\\n');\n \t\t\t}\ndiff --git a/trailer.h b/trailer.h\nindex aad856da8c1..d4507b4ef2a 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -71,9 +71,11 @@ struct process_trailer_options {\n \tint only_input;\n \tint unfold;\n \tint no_divider;\n+\tint key_only;\n \tint value_only;\n \tint canonicalize;\n \tconst struct strbuf *separator;\n+\tconst struct strbuf *key_value_separator;\n \tint (*filter)(const struct strbuf *, const char *alias, void *);\n \tvoid *filter_data;\n };\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411448","messageId":"20201205013918.18981-3-avarab@gmail.com","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 2/5] pretty format %(trailers): avoid needless repetition","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-05T01:39:15Z","receivedAt":"2020-12-05T01:40:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the documentation for the various %(trailers) options so it\nisn't repeating part of the documentation for \"only\" about how boolean\nvalues are handled. Instead let's split the description of that into\ngeneral documentation at the top.\n\nIt then suffices to refer to it by listing the options as\n\"opt[=bool]\". I'm also changing it to \"[=bool]\" from \"[=val]\". It took\nme a couple of readings to realize that while to realize that these\noptions were referring back to the \"only\" option's treatment of\nboolean values. Let's try to make this more explicit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/pretty-formats.txt | 29 +++++++++++++++--------------\n 1 file changed, 15 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 54f793d424f..8e066594624 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -252,7 +252,14 @@ endif::git-rev-list[]\n \t\t\t interpreted by\n \t\t\t linkgit:git-interpret-trailers[1]. The\n \t\t\t `trailers` string may be followed by a colon\n-\t\t\t and zero or more comma-separated options:\n+\t\t\t and zero or more comma-separated options.\n++\n+The boolean options accept an optional value. The values `true`,\n+`false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n+sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n+option is given with no value it's enabled. If any option is provided\n+multiple times the last occurance wins.\n++\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@@ -264,27 +271,21 @@ endif::git-rev-list[]\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+** 'only[=bool]': select whether non-trailer lines from the trailer\n+ block should be included.\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+ next option. 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+** 'unfold[=bool]': make it behave as if interpret-trailer's `--unfold`\n+ option was given. 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+** 'valueonly[=bool]': skip over the key part of the trailer line and only\n+ show the value part.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411449","messageId":"20201205013918.18981-5-avarab@gmail.com","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 4/5] pretty-format %(trailers): fix broken standalone \"valueonly\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-05T01:39:17Z","receivedAt":"2020-12-05T01:40:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix %(trailers:valueonly) being a noop due to on overly eager\noptimization. When new trailer options were added they needed to be\nlisted at the start of the format_trailer_info() function. E.g. as was\ndone in 250bea0c165 (pretty: allow showing specific trailers,\n2019-01-28).\n\nWhen d9b936db522 (pretty: add support for \"valueonly\" option in\n%(trailers), 2019-01-28) was added this was omitted by mistake. Thus\n%(trailers:valueonly) was a noop, instead of showing only trailer\nvalue. This wasn't caught because the tests for it always combined it\nwith other options.\n\nLet's fix the bug, and switch away from this pattern requiring us to\nremember to add new flags to the start of the function. Instead as\nsoon as we see the \":\" in \"%(trailers:\" we skip the fast path. That\nover-matches for \"%(trailers:)\", but I think that's OK.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n pretty.c | 2 ++\n t/t4205-log-pretty-formats.sh | 11 +++++++++++\n trailer.c | 3 +--\n trailer.h | 1 +\n 4 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 590f37489f6..d989a6ae712 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1426,6 +1426,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\topts.no_divider = 1;\n \n \t\tif (*arg == ':') {\n+\t\t\t/* over-matches on %(trailers:), but that's OK */\n+\t\t\topts.have_options = 1;\n \t\t\targ++;\n \t\t\tfor (;;) {\n \t\t\t\tconst char *argval;\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 5dd080c19b2..e1100082b34 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -737,6 +737,17 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:valueonly) shows only values' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:valueonly)\" >actual &&\n+\ttest_write_lines \\\n+\t\t\"A U Thor \" \\\n+\t\t\"A U Thor \" \\\n+\t\t\"[ v2 updated patch description ]\" \\\n+\t\t\"A U Thor\" \\\n+\t\t\" \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '%(trailers:key=foo,keyonly,valueonly) shows nothing' '\n \tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,keyonly,valueonly)\" >actual &&\n \techo >expect &&\ndiff --git a/trailer.c b/trailer.c\nindex 40f31e4dfc2..da95e1f3c66 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1206,8 +1206,7 @@ static void format_trailer_info(struct strbuf *out,\n \tsize_t origlen = out->len;\n \tsize_t i;\n \n-\t/* If we want the whole block untouched, we can take the fast path. */\n-\tif (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) {\n+\tif (!opts->have_options) {\n \t\tstrbuf_add(out, info->trailer_start,\n \t\t\t info->trailer_end - info->trailer_start);\n \t\treturn;\ndiff --git a/trailer.h b/trailer.h\nindex d4507b4ef2a..e348c970ce7 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -65,6 +65,7 @@ struct new_trailer_item {\n };\n \n struct process_trailer_options {\n+\tint have_options;\n \tint in_place;\n \tint trim_empty;\n \tint only_trailers;\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411450","messageId":"20201205013918.18981-6-avarab@gmail.com","threadId":"54505","inReplyTo":"20201025212652.3003036-1-anders@0x63.nu","subject":"[PATCH 5/5] pretty format %(trailers): add a \"key_value_separator\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-05T01:39:18Z","receivedAt":"2020-12-05T01:41:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As noted in a previous commit which added \"keyonly\" it's needlessly\nhard to use the \"log\" machinery to produce machine-readable output for\n%(trailers). with the combination of the existing \"separator\" and this\nnew \"key_value_separator\" this becomes trivial, as seen by the test\nbeing added here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/pretty-formats.txt | 4 ++++\n pretty.c | 9 +++++++++\n t/t4205-log-pretty-formats.sh | 22 ++++++++++++++++++++++\n trailer.c | 8 ++++++--\n 4 files changed, 41 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex d080f0d2476..369d243eae9 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -286,6 +286,10 @@ multiple times the last occurance wins.\n `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n ** 'keyonly[=bool]': only show the key part of the trailer.\n ** 'valueonly[=bool]': only show the value part of the trailer.\n+** 'key_value_separator=': specify a separator inserted between\n+ trailer lines. When this option is not given each trailer key-value\n+ pair separated by \": \". Otherwise it shares the same semantics as \n+ 'separator=' above.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex d989a6ae712..dea77f2621a 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1421,6 +1421,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n \t\tstruct string_list filter_list = STRING_LIST_INIT_NODUP;\n \t\tstruct strbuf sepbuf = STRBUF_INIT;\n+\t\tstruct strbuf kvsepbuf = STRBUF_INIT;\n \t\tsize_t ret = 0;\n \n \t\topts.no_divider = 1;\n@@ -1454,6 +1455,14 @@ 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) &&\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex e1100082b34..47b3f7d67c4 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -765,6 +765,28 @@ test_expect_success 'pretty format %(trailers:separator) changes separator' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers:key_value_separator) changes key-value separator' '\n+\tgit log --no-walk --pretty=format:\"X%(trailers:key_value_separator=%x00,unfold)X\" >actual &&\n+\t(\n+\t\tprintf \"XSigned-off-by\\0A U Thor \\n\" &&\n+\t\tprintf \"Acked-by\\0A U Thor \\n\" &&\n+\t\tprintf \"[ v2 updated patch description ]\\n\" &&\n+\t\tprintf \"Signed-off-by\\0A U Thor \\nX\"\n+\t) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:separator,key_value_separator) changes both separators' '\n+\tgit log --no-walk --pretty=format:\"%(trailers:separator=%x00,key_value_separator=%x00%x00,unfold)\" >actual &&\n+\t(\n+\t\tprintf \"Signed-off-by\\0\\0A U Thor \\0\" &&\n+\t\tprintf \"Acked-by\\0\\0A U Thor \\0\" &&\n+\t\tprintf \"[ v2 updated patch description ]\\0\" &&\n+\t\tprintf \"Signed-off-by\\0\\0A U Thor \"\n+\t) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'pretty format %(trailers) combining separator/key/keyonly/valueonly' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tImportant fix\ndiff --git a/trailer.c b/trailer.c\nindex da95e1f3c66..70a560647c1 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1233,8 +1233,12 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\t\t\tstrbuf_addbuf(out, opts->separator);\n \t\t\t\tif (!opts->value_only)\n \t\t\t\t\tstrbuf_addstr(out, tok.buf);\n-\t\t\t\tif (!opts->key_only && !opts->value_only)\n-\t\t\t\t\tstrbuf_addstr(out, \": \");\n+\t\t\t\tif (!opts->key_only && !opts->value_only) {\n+\t\t\t\t\tif (opts->key_value_separator)\n+\t\t\t\t\t\tstrbuf_addbuf(out, opts->key_value_separator);\n+\t\t\t\t\telse\n+\t\t\t\t\t\tstrbuf_addstr(out, \": \");\n+\t\t\t\t}\n \t\t\t\tif (!opts->key_only)\n \t\t\t\t\tstrbuf_addbuf(out, &val);\n \t\t\t\tif (!opts->separator)\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411491","messageId":"CAP8UFD2mYqJA5g+y0Q_48VQ7iCe7xVOkCkN77AdV9T6CBv50kA@mail.gmail.com","threadId":"54505","inReplyTo":"20201205013918.18981-3-avarab@gmail.com","subject":"Re: [PATCH 2/5] pretty format %(trailers): avoid needless repetition","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-12-05T05:43:52Z","receivedAt":"2020-12-05T05:44:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Dec 5, 2020 at 2:39 AM Ævar Arnfjörð Bjarmason wrote:\n>\n> Change the documentation for the various %(trailers) options so it\n> isn't repeating part of the documentation for \"only\" about how boolean\n> values are handled. Instead let's split the description of that into\n> general documentation at the top.\n\nGreat!\n\n> It then suffices to refer to it by listing the options as\n> \"opt[=bool]\". I'm also changing it to \"[=bool]\" from \"[=val]\".\n\nNice! I wonder if \"[=]\" or \"[=]\" might be even better as\nwe use \"=\" for key and \"=\" for separator.\n\n> It took\n> me a couple of readings to realize that while to realize that these\n> options were referring back to the \"only\" option's treatment of\n> boolean values. Let's try to make this more explicit.\n\nYeah, it's definitely an improvement.\n\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -252,7 +252,14 @@ endif::git-rev-list[]\n> interpreted by\n> linkgit:git-interpret-trailers[1]. The\n> `trailers` string may be followed by a colon\n> - and zero or more comma-separated options:\n> + and zero or more comma-separated options.\n> ++\n> +The boolean options accept an optional value. The values `true`,\n\nMaybe: s/an optional value./an optional value `[=]`./\n\n> +`false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n> +sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n> +option is given with no value it's enabled.\n\ns/value it's enabled/value, it's enabled/\n\n> +If any option is provided multiple times the last occurance wins.\n\nIt might be better to have this sentence before the above paragraph\nabout boolean options, as it's more general.\n\nThanks,\nChristian.\n"},{"id":"411492","messageId":"CAP8UFD1gTOKLs55ceVwsDW=uSyW4wx_9eF9Wra5KVP8B19Jx_Q@mail.gmail.com","threadId":"54505","inReplyTo":"20201205013918.18981-4-avarab@gmail.com","subject":"Re: [PATCH 3/5] pretty format %(trailers): add a \"keyonly\"","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-12-05T06:11:31Z","receivedAt":"2020-12-05T06:13:03Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Dec 5, 2020 at 2:39 AM Ævar Arnfjörð Bjarmason wrote:\n>\n> Add support for a \"keyonly\". This allows for easier parsing out of the\n> key and value. Before if you didn't want to make assumptions about how\n> the key was formatted. You'd need to parse it out as e.g.:\n>\n> --pretty=format:'%H%x00%(trailers:separator=%x00%x00)' \\\n> '%x00%(trailers:separator=%x00%x00,valueonly)'\n>\n> And then proceed to deduce keys by looking at those two and\n> subtracting the value plus the hardcoded \": \" separator from the\n> non-valueonly %(trailers) line. Now it's possible to simply do:\n>\n> --pretty=format:'%H%x00%(trailers:separator=%x00%x00,keyonly)' \\\n> '%x00%(trailers:separator=%x00%x00,valueonly)'\n>\n> Which at least reduces it to a state machine where you get N keys and\n> correlate them with N values. Even better would be to have a way to\n> change the \": \" delimiter to something easily machine-readable (a key\n> might contain \": \" too). A follow-up change will add support for that.\n\nWell explained.\n\n> diff --git a/trailer.c b/trailer.c\n> index b00b35ea0eb..40f31e4dfc2 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -1233,8 +1233,11 @@ static void format_trailer_info(struct strbuf *out,\n> if (opts->separator && out->len != origlen)\n> strbuf_addbuf(out, opts->separator);\n> if (!opts->value_only)\n> - strbuf_addf(out, \"%s: \", tok.buf);\n> - strbuf_addbuf(out, &val);\n> + strbuf_addstr(out, tok.buf);\n\nMaybe `strbuf_addbuf(out, &tok);`\n\n> + if (!opts->key_only && !opts->value_only)\n> + strbuf_addstr(out, \": \");\n> + if (!opts->key_only)\n> + strbuf_addbuf(out, &val);\n\nThe above is probably correct, but it feels strange to write the key\nafter the separator and the value, and that the key is in a variable\ncalled \"val\".\n"},{"id":"411493","messageId":"CAP8UFD0iXTLRc9yeyT10w8e7s-8ygPh3A645ss=gXvBrK+D5pQ@mail.gmail.com","threadId":"54505","inReplyTo":"20201205013918.18981-5-avarab@gmail.com","subject":"Re: [PATCH 4/5] pretty-format %(trailers): fix broken standalone \"valueonly\"","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-12-05T06:46:09Z","receivedAt":"2020-12-05T06:58:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Dec 5, 2020 at 2:39 AM Ævar Arnfjörð Bjarmason wrote:\n>\n> Fix %(trailers:valueonly) being a noop due to on overly eager\n\ns/on/an/\n\n> optimization. When new trailer options were added they needed to be\n> listed at the start of the format_trailer_info() function. E.g. as was\n> done in 250bea0c165 (pretty: allow showing specific trailers,\n> 2019-01-28).\n\nIt seems that you mean this part of the above patch:\n\n /* If we want the whole block untouched, we can take the fast path. */\n- if (!opts->only_trailers && !opts->unfold) {\n+ if (!opts->only_trailers && !opts->unfold && !opts->filter) {\n\nbut this could perhaps be clearer with:\n\n\"When new trailer options were added, a check that the new option is\nnot used needed to be added at the start of the format_trailer_info()\nfunction to see if we could take the fast path of writing the whole\ntrailer as is.\"\n\ninstead of:\n\n\"When new trailer options were added they needed to be listed at the\nstart of the format_trailer_info() function.\"\n\n> When d9b936db522 (pretty: add support for \"valueonly\" option in\n> %(trailers), 2019-01-28) was added this was omitted by mistake.\n\nMaybe: s/was added this was/was added, this check was/\n\n> Thus\n> %(trailers:valueonly) was a noop, instead of showing only trailer\n> value. This wasn't caught because the tests for it always combined it\n> with other options.\n>\n> Let's fix the bug, and switch away from this pattern requiring us to\n> remember to add new flags to the start of the function.\n\ns/to add new flags/to add a new check for each new option/\n\n> Instead as\n> soon as we see the \":\" in \"%(trailers:\" we skip the fast path. That\n> over-matches for \"%(trailers:)\", but I think that's OK.\n\nYeah, I think so too. I wonder if it is worth checking that\n\"%(trailers:)\" still works in the same way as \"%(trailers)\" though.\n\n> struct process_trailer_options {\n> + int have_options;\n> int in_place;\n> int trim_empty;\n> int only_trailers;\n\nIf all of these are booleans, we might want to save a few bytes at one\npoint by using bit fields.\n"},{"id":"411496","messageId":"CAP8UFD1q7ab5wyhmxknoM8FC5y_QqrF34HCRiS3=MP8YLCx20A@mail.gmail.com","threadId":"54505","inReplyTo":"20201205013918.18981-6-avarab@gmail.com","subject":"Re: [PATCH 5/5] pretty format %(trailers): add a \"key_value_separator\"","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-12-05T07:13:34Z","receivedAt":"2020-12-05T07:14:28Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Dec 5, 2020 at 2:39 AM Ævar Arnfjörð Bjarmason wrote:\n>\n> As noted in a previous commit which added \"keyonly\" it's needlessly\n\nMaybe add a comma after `\"keyonly\"` and s/\"keyonly\"/the \"keyonly\" option/\n\n> hard to use the \"log\" machinery to produce machine-readable output for\n> %(trailers). with the combination of the existing \"separator\" and this\n\ns/with/With/\ns/\"separator\"/\"separator\" option/\n\n> new \"key_value_separator\" this becomes trivial, as seen by the test\n\ns/\"key_value_separator\"/\"key_value_separator\" option,/\n\n> being added here.\n\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index d080f0d2476..369d243eae9 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -286,6 +286,10 @@ multiple times the last occurance wins.\n> `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n> ** 'keyonly[=bool]': only show the key part of the trailer.\n> ** 'valueonly[=bool]': only show the value part of the trailer.\n> +** 'key_value_separator=': specify a separator inserted between\n> + trailer lines. When this option is not given each trailer key-value\n> + pair separated by \": \". Otherwise it shares the same semantics as\n> + 'separator=' above.\n\nThe above is not very clear to me.\n\nMaybe:\n\ns/between trailer lines/between a key and its associated value/\ns/each trailer key-value pair separated by/each key and its associated\nvalue are separated by/\n"},{"id":"411497","messageId":"87pn3oy6wo.fsf@evledraar.gmail.com","threadId":"54505","inReplyTo":"CAP8UFD1q7ab5wyhmxknoM8FC5y_QqrF34HCRiS3=MP8YLCx20A@mail.gmail.com","subject":"Re: [PATCH 5/5] pretty format %(trailers): add a \"key_value_separator\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-05T08:49:11Z","receivedAt":"2020-12-05T08:50:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Dec 05 2020, Christian Couder wrote:\n\n> On Sat, Dec 5, 2020 at 2:39 AM Ævar Arnfjörð Bjarmason wrote:\n>>\n>> As noted in a previous commit which added \"keyonly\" it's needlessly\n>\n> Maybe add a comma after `\"keyonly\"` and s/\"keyonly\"/the \"keyonly\" option/\n\nThanks a lot for all the feedback. I'll work it into a v2 when I send\nit.\n\nI'll wait a bit to see if Anders W. pops up again and see if he'd like\nto incorporate this into his series or submit his first/after etc.\n"},{"id":"411505","messageId":"87mtysxwu6.fsf@evledraar.gmail.com","threadId":"54505","inReplyTo":"CAP8UFD1gTOKLs55ceVwsDW=uSyW4wx_9eF9Wra5KVP8B19Jx_Q@mail.gmail.com","subject":"Re: [PATCH 3/5] pretty format %(trailers): add a \"keyonly\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-05T12:26:41Z","receivedAt":"2020-12-05T17:50:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Dec 05 2020, Christian Couder wrote:\n\n> On Sat, Dec 5, 2020 at 2:39 AM Ævar Arnfjörð Bjarmason wrote:\n>>\n>> Add support for a \"keyonly\". This allows for easier parsing out of the\n>> key and value. Before if you didn't want to make assumptions about how\n>> the key was formatted. You'd need to parse it out as e.g.:\n>>\n>> --pretty=format:'%H%x00%(trailers:separator=%x00%x00)' \\\n>> '%x00%(trailers:separator=%x00%x00,valueonly)'\n>>\n>> And then proceed to deduce keys by looking at those two and\n>> subtracting the value plus the hardcoded \": \" separator from the\n>> non-valueonly %(trailers) line. Now it's possible to simply do:\n>>\n>> --pretty=format:'%H%x00%(trailers:separator=%x00%x00,keyonly)' \\\n>> '%x00%(trailers:separator=%x00%x00,valueonly)'\n>>\n>> Which at least reduces it to a state machine where you get N keys and\n>> correlate them with N values. Even better would be to have a way to\n>> change the \": \" delimiter to something easily machine-readable (a key\n>> might contain \": \" too). A follow-up change will add support for that.\n>\n> Well explained.\n>\n>> diff --git a/trailer.c b/trailer.c\n>> index b00b35ea0eb..40f31e4dfc2 100644\n>> --- a/trailer.c\n>> +++ b/trailer.c\n>> @@ -1233,8 +1233,11 @@ static void format_trailer_info(struct strbuf *out,\n>> if (opts->separator && out->len != origlen)\n>> strbuf_addbuf(out, opts->separator);\n>> if (!opts->value_only)\n>> - strbuf_addf(out, \"%s: \", tok.buf);\n>> - strbuf_addbuf(out, &val);\n>> + strbuf_addstr(out, tok.buf);\n>\n> Maybe `strbuf_addbuf(out, &tok);`\n\nMuch better, thanks.\n\n>> + if (!opts->key_only && !opts->value_only)\n>> + strbuf_addstr(out, \": \");\n>> + if (!opts->key_only)\n>> + strbuf_addbuf(out, &val);\n>\n> The above is probably correct, but it feels strange to write the key\n> after the separator and the value, and that the key is in a variable\n> called \"val\".\n\nWe write them in the order \"key -> sep -> value\". with the logic of:\n \n if (!no_key)\n write_key();\n if (!no_sep)\n write_sep();\n if (!no_value)\n write_value();\n\nSo the &val here really is the value part.\n \n"},{"id":"411506","messageId":"87wnxwp15o.fsf@0x63.nu","threadId":"54505","inReplyTo":"20201205013918.18981-1-avarab@gmail.com","subject":"Re: [PATCH 0/5] pretty format %(trailers): improve machine readability","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-12-05T18:18:11Z","receivedAt":"2020-12-05T18:41:45Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nÆvar Arnfjörð Bjarmason writes:\n\n> I started writing this on top of \"master\", but then saw the\n> outstanding series of other miscellaneous fixes to this\n> facility[1]. This is on top of that topic & rebased on master.\n>\n> Anders, any plans to re-roll yours? Otherwise the conflicts I'd have\n> on mine are easy to fix, so I can also submit it as a stand-alone.\n\nYes, I have plans to do that. But have yet to carve out the required\ntime from my copious spare time to actually do it.\n\nSo please don't hold your breath waiting for me to do that.\n\n> This series comes out of a discussion at work today (well, yesterday\n> at this point) where someone wanted to parse %(trailers) output. As\n> noted in 3/5 doing this is rather tedious now if you're trying to\n> unambiguously grap trailers as a stream of key-value pairs.\n>\n> So this series adds a \"key_value_separator\" and \"keyonly\" parameters,\n> and fixes a few bugs I saw along the way.\n\nInteresting. When adding \"valueonly\" I never consider it being used\nwithout \"key\". The trick you are doing with separate keyonly and\nvalueonly is quite clever.\n\nI've only been doing machine parsing for explicit keys, things like:\n\"%cn%x00%x00%an%x00%x00%(trailers:key=Reviewed-By,valueonly,unfold,separator=%x00)%x00%x00%(trailers:key=Backport-Reviewed-By,valueonly,unfold,separator=%x00)\"\n(double-NUL to separate field, single-NUL to separate values within field).\n\nBut I can't help wonder that if the goal just is to have a nice machine\nparsable format maybe it would be easier (both for user and\nimplementation) to have a separate placeholder for \"machine readable\ntrailers\" which by default emits in a format suitable for machine\nparsing. Something like a new \"%(ztrailers)\" (but with a better name)\nwhich simply emits a sequence of \" NUL NUL\" for each trailer.\n"},{"id":"411538","messageId":"20201206002449.31452-3-avarab@gmail.com","threadId":"54505","inReplyTo":"20201205013918.18981-1-avarab@gmail.com","subject":"[PATCH v2 2/5] pretty format %(trailers) doc: avoid repetition","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-06T00:24:46Z","receivedAt":"2020-12-06T00:26:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the documentation for the various %(trailers) options so it\nisn't repeating part of the documentation for \"only\" about how boolean\nvalues are handled. Instead, let's split the description of that into\ngeneral documentation at the top.\n\nIt then suffices to refer to it by listing the options as\n\"opt[=]\". I'm also changing it to upper-case \"[=]\" from\n\"[=val]\" for consistency with \"\"\n\nIt took me a couple of readings to realize that these options were\nreferring back to the \"only\" option's treatment of boolean\nvalues. Let's try to make this more explicit, and upper-case \"BOOL\"\nfor consistency with the existing \"\" and \"\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/pretty-formats.txt | 30 ++++++++++++++++--------------\n 1 file changed, 16 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 84bbc7439a6..66dfa122361 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -252,7 +252,15 @@ endif::git-rev-list[]\n \t\t\t interpreted by\n \t\t\t linkgit:git-interpret-trailers[1]. The\n \t\t\t `trailers` string may be followed by a colon\n-\t\t\t and zero or more comma-separated options:\n+\t\t\t and zero or more comma-separated options.\n+\t\t\t If any option is provided multiple times the\n+\t\t\t last occurance wins.\n++\n+The boolean options accept an optional value `[=]`. The values\n+`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n+sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n+option is given with no value, it's enabled.\n++\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@@ -261,27 +269,21 @@ endif::git-rev-list[]\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+** 'only[=BOOL]': select whether non-trailer lines from the trailer\n+ block should be included.\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+ next option. 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+** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`\n+ option was given. 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+** 'valueonly[=BOOL]': skip over the key part of the trailer line and only\n+ show the value part.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411539","messageId":"20201206002449.31452-5-avarab@gmail.com","threadId":"54505","inReplyTo":"20201205013918.18981-1-avarab@gmail.com","subject":"[PATCH v2 4/5] pretty format %(trailers): add a \"keyonly\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-06T00:24:48Z","receivedAt":"2020-12-06T00:26:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add support for a \"keyonly\". This allows for easier parsing out of the\nkey and value. Before if you didn't want to make assumptions about how\nthe key was formatted. You'd need to parse it out as e.g.:\n\n --pretty=format:'%H%x00%(trailers:separator=%x00%x00)' \\\n '%x00%(trailers:separator=%x00%x00,valueonly)'\n\nAnd then proceed to deduce keys by looking at those two and\nsubtracting the value plus the hardcoded \": \" separator from the\nnon-valueonly %(trailers) line. Now it's possible to simply do:\n\n --pretty=format:'%H%x00%(trailers:separator=%x00%x00,keyonly)' \\\n '%x00%(trailers:separator=%x00%x00,valueonly)'\n\nWhich at least reduces it to a state machine where you get N keys and\ncorrelate them with N values. Even better would be to have a way to\nchange the \": \" delimiter to something easily machine-readable (a key\nmight contain \": \" too). A follow-up change will add support for that.\n\nI don't really have a use-case for just \"keyonly\" myself. I suppose it\nwould be useful in some cases as \"key=*\" matches case-insensitively,\nso a plain \"keyonly\" will give you the variants of the keys you\nmatched. I'm mainly adding it to fix the inconsistency with\n\"valueonly\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/pretty-formats.txt | 4 ++--\n pretty.c | 1 +\n t/t4205-log-pretty-formats.sh | 31 ++++++++++++++++++++++++++++++-\n trailer.c | 9 ++++++---\n trailer.h | 1 +\n 5 files changed, 40 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 66dfa122361..bf35f7cf219 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -282,8 +282,8 @@ option is given with no value, it's enabled.\n ** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`\n option was given. E.g.,\n `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n-** 'valueonly[=BOOL]': skip over the key part of the trailer line and only\n- show the value part.\n+** 'keyonly[=BOOL]': only show the key part of the trailer.\n+** 'valueonly[=BOOL]': only show the value part of the trailer.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex 7a7708a0ea7..1237ee0e45d 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1451,6 +1451,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\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, \"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}\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex cb09a13249e..4c9f6eb7946 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -715,6 +715,22 @@ test_expect_success '%(trailers:key) without value is error' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:keyonly) shows only keys' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:keyonly)\" >actual &&\n+\ttest_write_lines \\\n+\t\t\"Signed-off-by\" \\\n+\t\t\"Acked-by\" \\\n+\t\t\"[ v2 updated patch description ]\" \\\n+\t\t\"Signed-off-by\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=foo,keyonly) shows only key' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,keyonly)\" >actual &&\n+\techo \"Acked-by\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n \tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,valueonly)\" >actual &&\n \techo \"A U Thor \" >expect &&\n@@ -732,6 +748,12 @@ test_expect_success '%(trailers:valueonly) shows only values' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:key=foo,keyonly,valueonly) shows nothing' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,keyonly,valueonly)\" >actual &&\n+\techo >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'pretty format %(trailers:separator) changes separator' '\n \tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00)X\" >actual &&\n \t(\n@@ -754,7 +776,7 @@ test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' '\n+test_expect_success 'pretty format %(trailers) combining separator/key/keyonly/valueonly' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tImportant fix\n \n@@ -781,6 +803,13 @@ test_expect_success 'pretty format %(trailers) combining separator/key/valueonly\n \t\t\"Does not close any tickets\" \\\n \t\t\"Another fix #567, #890\" \\\n \t\t\"Important fix #1234\" >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit log --pretty=\"%s% (trailers:separator=%x2c%x20,key=Closes,keyonly)\" HEAD~3.. >actual &&\n+\ttest_write_lines \\\n+\t\t\"Does not close any tickets\" \\\n+\t\t\"Another fix Closes, Closes\" \\\n+\t\t\"Important fix Closes\" >expect &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/trailer.c b/trailer.c\nindex d2d01015b1d..889b419a4f6 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out,\n \n \t/* If we want the whole block untouched, we can take the fast path. */\n \tif (!opts->only_trailers && !opts->unfold && !opts->filter &&\n-\t !opts->separator && !opts->value_only) {\n+\t !opts->separator && !opts->key_only && !opts->value_only) {\n \t\tstrbuf_add(out, info->trailer_start,\n \t\t\t info->trailer_end - info->trailer_start);\n \t\treturn;\n@@ -1154,8 +1154,11 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\t\tif (opts->separator && out->len != origlen)\n \t\t\t\t\tstrbuf_addbuf(out, opts->separator);\n \t\t\t\tif (!opts->value_only)\n-\t\t\t\t\tstrbuf_addf(out, \"%s: \", tok.buf);\n-\t\t\t\tstrbuf_addbuf(out, &val);\n+\t\t\t\t\tstrbuf_addbuf(out, &tok);\n+\t\t\t\tif (!opts->key_only && !opts->value_only)\n+\t\t\t\t\tstrbuf_addstr(out, \": \");\n+\t\t\t\tif (!opts->key_only)\n+\t\t\t\t\tstrbuf_addbuf(out, &val);\n \t\t\t\tif (!opts->separator)\n \t\t\t\t\tstrbuf_addch(out, '\\n');\n \t\t\t}\ndiff --git a/trailer.h b/trailer.h\nindex cd93e7ddea7..d2f28776be6 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -71,6 +71,7 @@ struct process_trailer_options {\n \tint only_input;\n \tint unfold;\n \tint no_divider;\n+\tint key_only;\n \tint value_only;\n \tconst struct strbuf *separator;\n \tint (*filter)(const struct strbuf *, void *);\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411540","messageId":"20201206002449.31452-2-avarab@gmail.com","threadId":"54505","inReplyTo":"20201205013918.18981-1-avarab@gmail.com","subject":"[PATCH v2 1/5] pretty format %(trailers) test: split a long line","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-06T00:24:45Z","receivedAt":"2020-12-06T00:26:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Split a very long line in a test introduced in 0b691d86851 (pretty:\nadd support for separator option in %(trailers), 2019-01-28). This\nmakes it easier to read, especially as follow-up commits will copy\nthis test as a template.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t4205-log-pretty-formats.sh | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 204c149d5a4..5e5452212d2 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -717,7 +717,12 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n \n test_expect_success 'pretty format %(trailers:separator) changes separator' '\n \tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00,unfold)X\" >actual &&\n-\tprintf \"XSigned-off-by: A U Thor \\0Acked-by: A U Thor \\0[ v2 updated patch description ]\\0Signed-off-by: A U Thor X\" >expect &&\n+\t(\n+\t\tprintf \"XSigned-off-by: A U Thor \\0\" &&\n+\t\tprintf \"Acked-by: A U Thor \\0\" &&\n+\t\tprintf \"[ v2 updated patch description ]\\0\" &&\n+\t\tprintf \"Signed-off-by: A U Thor X\"\n+\t) >expect &&\n \ttest_cmp expect actual\n '\n \n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411541","messageId":"20201206002449.31452-4-avarab@gmail.com","threadId":"54505","inReplyTo":"20201205013918.18981-1-avarab@gmail.com","subject":"[PATCH v2 3/5] pretty-format %(trailers): fix broken standalone \"valueonly\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-06T00:24:47Z","receivedAt":"2020-12-06T00:26:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix %(trailers:valueonly) being a noop due to on overly eager\noptimization in format_trailer_info() which skips custom formatting if\nno custom options are given.\n\nWhen \"valueonly\" was added in d9b936db522 (pretty: add support for\n\"valueonly\" option in %(trailers), 2019-01-28) we forgot to add it to\nthe list of options that optimization checks for. See e.g. the\naddition of \"key\" in 250bea0c165 (pretty: allow showing specific\ntrailers, 2019-01-28) for a similar change where this wasn't missed.\n\nThus the \"valueonly\" option in \"%(trailers:valueonly)\" was a noop and\nthe output was equivalent to that of a plain \"%(trailers)\". This\nwasn't caught because the tests for it always combined it with other\noptions.\n\nFix the bug by adding !opts->value_only to the list. I initially\nattempted to make this more future-proof by setting a flag if we got\nto \":\" in \"%(trailers:\" in format_commit_one() in pretty.c. However,\n\"%(trailers:\" is also parsed in trailers_atom_parser() in\nref-filter.c.\n\nThere is an outstanding patch[1] unify those two, and such a fix, or\nother future-proofing, such as changing \"process_trailer_options\"\nflags into a bitfield, would conflict with that effort. Let's instead\ndo the bare minimum here as this aspect of trailers is being actively\nworked on by another series.\n\nLet's also test for a plain \"valueonly\" without any other options, as\nwell as \"separator\". All the other existing options on the pretty.c\npath had tests where they were the only option provided. I'm also\nkeeping a sanity test for \"%(trailers:)\" being the same as\n\"%(trailers)\". There's no reason to suspect it wouldn't be in the\ncurrent implementation, but let's keep it in the interest of black box\ntesting.\n\n1. https://lore.kernel.org/git/pull.726.git.1599335291.gitgitgadget@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t4205-log-pretty-formats.sh | 28 ++++++++++++++++++++++++++++\n trailer.c | 3 ++-\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 5e5452212d2..cb09a13249e 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -605,6 +605,12 @@ test_expect_success 'pretty format %(trailers) shows trailers' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers:) enables no options' '\n+\tgit log --no-walk --pretty=\"%(trailers:)\" >actual &&\n+\t# \"expect\" the same as the test above\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '%(trailers:only) shows only \"key: value\" trailers' '\n \tgit log --no-walk --pretty=\"%(trailers:only)\" >actual &&\n \t{\n@@ -715,7 +721,29 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:valueonly) shows only values' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:valueonly)\" >actual &&\n+\ttest_write_lines \\\n+\t\t\"A U Thor \" \\\n+\t\t\"A U Thor \" \\\n+\t\t\"[ v2 updated patch description ]\" \\\n+\t\t\"A U Thor\" \\\n+\t\t\" \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'pretty format %(trailers:separator) changes separator' '\n+\tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00)X\" >actual &&\n+\t(\n+\t\tprintf \"XSigned-off-by: A U Thor \\0\" &&\n+\t\tprintf \"Acked-by: A U Thor \\0\" &&\n+\t\tprintf \"[ v2 updated patch description ]\\0\" &&\n+\t\tprintf \"Signed-off-by: A U Thor\\n X\"\n+\t) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separator' '\n \tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00,unfold)X\" >actual &&\n \t(\n \t\tprintf \"XSigned-off-by: A U Thor \\0\" &&\ndiff --git a/trailer.c b/trailer.c\nindex 3f7391d793c..d2d01015b1d 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1131,7 +1131,8 @@ static void format_trailer_info(struct strbuf *out,\n \tsize_t i;\n \n \t/* If we want the whole block untouched, we can take the fast path. */\n-\tif (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) {\n+\tif (!opts->only_trailers && !opts->unfold && !opts->filter &&\n+\t !opts->separator && !opts->value_only) {\n \t\tstrbuf_add(out, info->trailer_start,\n \t\t\t info->trailer_end - info->trailer_start);\n \t\treturn;\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411542","messageId":"20201206002449.31452-1-avarab@gmail.com","threadId":"54505","inReplyTo":"20201205013918.18981-1-avarab@gmail.com","subject":"[PATCH v2 0/5] pretty format %(trailers): improve machine readability","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-06T00:24:44Z","receivedAt":"2020-12-06T00:26:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Per Anders's feedback in [1] this is now a stand-alone series based on\n\"master\". The merge conflict with his series is trivial, both add or\nchange struct member(s) in trailer.h.\n\nv2 changes:\n\n * Either incorporate all the feedback from Christian Couder on v1, or\n changed things around to e.g. eliminate the relevant code.\n\n * Drop the whole \"have_options\" approach. I saw it conflicted with\n Hariom Verma's cleanup of ref-filter.c in a bad way. I also wasn't\n running the for-each-ref tests in v1 (just interpret-trailers &\n log/pretty). Ran every commit in v2 with the whole test suite (as I\n should have done to begin with, sorry).\n\n * I'd accidentally left DEVOPTS=no-error on in v1 and didn't notice a\n missing struct member warning. Oops.\n\n * Lots of my own commit message rewording/cleanup/grammar fixes.\n\nÆvar Arnfjörð Bjarmason (5):\n pretty format %(trailers) test: split a long line\n pretty format %(trailers) doc: avoid repetition\n pretty-format %(trailers): fix broken standalone \"valueonly\"\n pretty format %(trailers): add a \"keyonly\"\n pretty format %(trailers): add a \"key_value_separator\"\n\n Documentation/pretty-formats.txt | 34 ++++++-----\n pretty.c | 10 ++++\n t/t4205-log-pretty-formats.sh | 99 +++++++++++++++++++++++++++++++-\n trailer.c | 15 ++++-\n trailer.h | 2 +\n 5 files changed, 141 insertions(+), 19 deletions(-)\n\nRange-diff:\n1: 51a7a6d8cfe ! 1: 4b134a62aec pretty format %(trailers) test: split a long line\n @@ Commit message\n \n Split a very long line in a test introduced in 0b691d86851 (pretty:\n add support for separator option in %(trailers), 2019-01-28). This\n - makes it easier to read, and it'll be used as a template in follow-up\n - commits.\n + makes it easier to read, especially as follow-up commits will copy\n + this test as a template.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n2: 2340d856a90 ! 2: 0d3fe6daf6c pretty format %(trailers): avoid needless repetition\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - pretty format %(trailers): avoid needless repetition\n + pretty format %(trailers) doc: avoid repetition\n \n Change the documentation for the various %(trailers) options so it\n isn't repeating part of the documentation for \"only\" about how boolean\n - values are handled. Instead let's split the description of that into\n + values are handled. Instead, let's split the description of that into\n general documentation at the top.\n \n It then suffices to refer to it by listing the options as\n - \"opt[=bool]\". I'm also changing it to \"[=bool]\" from \"[=val]\". It took\n - me a couple of readings to realize that while to realize that these\n - options were referring back to the \"only\" option's treatment of\n - boolean values. Let's try to make this more explicit.\n + \"opt[=]\". I'm also changing it to upper-case \"[=]\" from\n + \"[=val]\" for consistency with \"\"\n +\n + It took me a couple of readings to realize that these options were\n + referring back to the \"only\" option's treatment of boolean\n + values. Let's try to make this more explicit, and upper-case \"BOOL\"\n + for consistency with the existing \"\" and \"\".\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ Documentation/pretty-formats.txt: endif::git-rev-list[]\n \t\t\t `trailers` string may be followed by a colon\n -\t\t\t and zero or more comma-separated options:\n +\t\t\t and zero or more comma-separated options.\n ++\t\t\t If any option is provided multiple times the\n ++\t\t\t last occurance wins.\n ++\n -+The boolean options accept an optional value. The values `true`,\n -+`false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n ++The boolean options accept an optional value `[=]`. The values\n ++`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n +sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n -+option is given with no value it's enabled. If any option is provided\n -+multiple times the last occurance wins.\n ++option is given with no value, it's enabled.\n ++\n ** 'key=': only show trailers with specified key. Matching is done\n case-insensitively and trailing colon is optional. If option is\n @@ Documentation/pretty-formats.txt: endif::git-rev-list[]\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 -+** 'only[=bool]': select whether non-trailer lines from the trailer\n ++** 'only[=BOOL]': select whether non-trailer lines from the trailer\n + block should be included.\n ** 'separator=': specify a separator inserted between trailer\n lines. When this option is not given each trailer line is\n @@ Documentation/pretty-formats.txt: endif::git-rev-list[]\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 -+** 'unfold[=bool]': make it behave as if interpret-trailer's `--unfold`\n ++** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`\n + option was given. 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 -+** 'valueonly[=bool]': skip over the key part of the trailer line and only\n ++** 'valueonly[=BOOL]': skip over the key part of the trailer line and only\n + show the value part.\n \n NOTE: Some placeholders may depend on other options given to the\n4: e9ca1e8d88c ! 3: ea44eeff510 pretty-format %(trailers): fix broken standalone \"valueonly\"\n @@ Commit message\n pretty-format %(trailers): fix broken standalone \"valueonly\"\n \n Fix %(trailers:valueonly) being a noop due to on overly eager\n - optimization. When new trailer options were added they needed to be\n - listed at the start of the format_trailer_info() function. E.g. as was\n - done in 250bea0c165 (pretty: allow showing specific trailers,\n - 2019-01-28).\n + optimization in format_trailer_info() which skips custom formatting if\n + no custom options are given.\n \n - When d9b936db522 (pretty: add support for \"valueonly\" option in\n - %(trailers), 2019-01-28) was added this was omitted by mistake. Thus\n - %(trailers:valueonly) was a noop, instead of showing only trailer\n - value. This wasn't caught because the tests for it always combined it\n - with other options.\n + When \"valueonly\" was added in d9b936db522 (pretty: add support for\n + \"valueonly\" option in %(trailers), 2019-01-28) we forgot to add it to\n + the list of options that optimization checks for. See e.g. the\n + addition of \"key\" in 250bea0c165 (pretty: allow showing specific\n + trailers, 2019-01-28) for a similar change where this wasn't missed.\n \n - Let's fix the bug, and switch away from this pattern requiring us to\n - remember to add new flags to the start of the function. Instead as\n - soon as we see the \":\" in \"%(trailers:\" we skip the fast path. That\n - over-matches for \"%(trailers:)\", but I think that's OK.\n + Thus the \"valueonly\" option in \"%(trailers:valueonly)\" was a noop and\n + the output was equivalent to that of a plain \"%(trailers)\". This\n + wasn't caught because the tests for it always combined it with other\n + options.\n \n - Signed-off-by: Ævar Arnfjörð Bjarmason \n + Fix the bug by adding !opts->value_only to the list. I initially\n + attempted to make this more future-proof by setting a flag if we got\n + to \":\" in \"%(trailers:\" in format_commit_one() in pretty.c. However,\n + \"%(trailers:\" is also parsed in trailers_atom_parser() in\n + ref-filter.c.\n \n - ## pretty.c ##\n -@@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n - \t\topts.no_divider = 1;\n - \n - \t\tif (*arg == ':') {\n -+\t\t\t/* over-matches on %(trailers:), but that's OK */\n -+\t\t\topts.have_options = 1;\n - \t\t\targ++;\n - \t\t\tfor (;;) {\n - \t\t\t\tconst char *argval;\n + There is an outstanding patch[1] unify those two, and such a fix, or\n + other future-proofing, such as changing \"process_trailer_options\"\n + flags into a bitfield, would conflict with that effort. Let's instead\n + do the bare minimum here as this aspect of trailers is being actively\n + worked on by another series.\n +\n + Let's also test for a plain \"valueonly\" without any other options, as\n + well as \"separator\". All the other existing options on the pretty.c\n + path had tests where they were the only option provided. I'm also\n + keeping a sanity test for \"%(trailers:)\" being the same as\n + \"%(trailers)\". There's no reason to suspect it wouldn't be in the\n + current implementation, but let's keep it in the interest of black box\n + testing.\n +\n + 1. https://lore.kernel.org/git/pull.726.git.1599335291.gitgitgadget@gmail.com/\n +\n + Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## t/t4205-log-pretty-formats.sh ##\n +@@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers) shows trailers' '\n + \ttest_cmp expect actual\n + '\n + \n ++test_expect_success 'pretty format %(trailers:) enables no options' '\n ++\tgit log --no-walk --pretty=\"%(trailers:)\" >actual &&\n ++\t# \"expect\" the same as the test above\n ++\ttest_cmp expect actual\n ++'\n ++\n + test_expect_success '%(trailers:only) shows only \"key: value\" trailers' '\n + \tgit log --no-walk --pretty=\"%(trailers:only)\" >actual &&\n + \t{\n @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n \ttest_cmp expect actual\n '\n @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:key=foo,valueonly\n +\ttest_cmp expect actual\n +'\n +\n - test_expect_success '%(trailers:key=foo,keyonly,valueonly) shows nothing' '\n - \tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,keyonly,valueonly)\" >actual &&\n - \techo >expect &&\n + test_expect_success 'pretty format %(trailers:separator) changes separator' '\n ++\tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00)X\" >actual &&\n ++\t(\n ++\t\tprintf \"XSigned-off-by: A U Thor \\0\" &&\n ++\t\tprintf \"Acked-by: A U Thor \\0\" &&\n ++\t\tprintf \"[ v2 updated patch description ]\\0\" &&\n ++\t\tprintf \"Signed-off-by: A U Thor\\n X\"\n ++\t) >expect &&\n ++\ttest_cmp expect actual\n ++'\n ++\n ++test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separator' '\n + \tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00,unfold)X\" >actual &&\n + \t(\n + \t\tprintf \"XSigned-off-by: A U Thor \\0\" &&\n \n ## trailer.c ##\n @@ trailer.c: static void format_trailer_info(struct strbuf *out,\n - \tsize_t origlen = out->len;\n \tsize_t i;\n \n --\t/* If we want the whole block untouched, we can take the fast path. */\n + \t/* If we want the whole block untouched, we can take the fast path. */\n -\tif (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) {\n -+\tif (!opts->have_options) {\n ++\tif (!opts->only_trailers && !opts->unfold && !opts->filter &&\n ++\t !opts->separator && !opts->value_only) {\n \t\tstrbuf_add(out, info->trailer_start,\n \t\t\t info->trailer_end - info->trailer_start);\n \t\treturn;\n -\n - ## trailer.h ##\n -@@ trailer.h: struct new_trailer_item {\n - };\n - \n - struct process_trailer_options {\n -+\tint have_options;\n - \tint in_place;\n - \tint trim_empty;\n - \tint only_trailers;\n3: b71a700fa9b ! 4: 4fd193fd90c pretty format %(trailers): add a \"keyonly\"\n @@ Commit message\n change the \": \" delimiter to something easily machine-readable (a key\n might contain \": \" too). A follow-up change will add support for that.\n \n + I don't really have a use-case for just \"keyonly\" myself. I suppose it\n + would be useful in some cases as \"key=*\" matches case-insensitively,\n + so a plain \"keyonly\" will give you the variants of the keys you\n + matched. I'm mainly adding it to fix the inconsistency with\n + \"valueonly\".\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## Documentation/pretty-formats.txt ##\n -@@ Documentation/pretty-formats.txt: multiple times the last occurance wins.\n - ** 'unfold[=bool]': make it behave as if interpret-trailer's `--unfold`\n +@@ Documentation/pretty-formats.txt: option is given with no value, it's enabled.\n + ** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`\n option was given. E.g.,\n `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n --** 'valueonly[=bool]': skip over the key part of the trailer line and only\n +-** 'valueonly[=BOOL]': skip over the key part of the trailer line and only\n - show the value part.\n -+** 'keyonly[=bool]': only show the key part of the trailer.\n -+** 'valueonly[=bool]': only show the value part of the trailer.\n ++** 'keyonly[=BOOL]': only show the key part of the trailer.\n ++** 'valueonly[=BOOL]': only show the value part of the trailer.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\n @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:key) without valu\n \ttest_cmp expect actual\n '\n \n -+test_expect_success '%(trailers:key=foo,keyonly) shows only keys' '\n ++test_expect_success '%(trailers:keyonly) shows only keys' '\n +\tgit log --no-walk --pretty=\"format:%(trailers:keyonly)\" >actual &&\n +\ttest_write_lines \\\n +\t\t\"Signed-off-by\" \\\n @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:key) without valu\n test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n \tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,valueonly)\" >actual &&\n \techo \"A U Thor \" >expect &&\n +@@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:valueonly) shows only values' '\n \ttest_cmp expect actual\n '\n \n @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:key) without valu\n +'\n +\n test_expect_success 'pretty format %(trailers:separator) changes separator' '\n - \tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00,unfold)X\" >actual &&\n + \tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00)X\" >actual &&\n \t(\n -@@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers:separator) changes separator' '\n +@@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa\n \ttest_cmp expect actual\n '\n \n @@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers) co\n \n ## trailer.c ##\n @@ trailer.c: static void format_trailer_info(struct strbuf *out,\n + \n + \t/* If we want the whole block untouched, we can take the fast path. */\n + \tif (!opts->only_trailers && !opts->unfold && !opts->filter &&\n +-\t !opts->separator && !opts->value_only) {\n ++\t !opts->separator && !opts->key_only && !opts->value_only) {\n + \t\tstrbuf_add(out, info->trailer_start,\n + \t\t\t info->trailer_end - info->trailer_start);\n + \t\treturn;\n +@@ trailer.c: static void format_trailer_info(struct strbuf *out,\n \t\t\t\tif (opts->separator && out->len != origlen)\n \t\t\t\t\tstrbuf_addbuf(out, opts->separator);\n \t\t\t\tif (!opts->value_only)\n -\t\t\t\t\tstrbuf_addf(out, \"%s: \", tok.buf);\n -\t\t\t\tstrbuf_addbuf(out, &val);\n -+\t\t\t\t\tstrbuf_addstr(out, tok.buf);\n ++\t\t\t\t\tstrbuf_addbuf(out, &tok);\n +\t\t\t\tif (!opts->key_only && !opts->value_only)\n +\t\t\t\t\tstrbuf_addstr(out, \": \");\n +\t\t\t\tif (!opts->key_only)\n @@ trailer.h: struct process_trailer_options {\n \tint no_divider;\n +\tint key_only;\n \tint value_only;\n - \tint canonicalize;\n \tconst struct strbuf *separator;\n -+\tconst struct strbuf *key_value_separator;\n - \tint (*filter)(const struct strbuf *, const char *alias, void *);\n - \tvoid *filter_data;\n - };\n + \tint (*filter)(const struct strbuf *, void *);\n5: cd4b3b52cf3 ! 5: 6cc6fc79388 pretty format %(trailers): add a \"key_value_separator\"\n @@ Metadata\n ## Commit message ##\n pretty format %(trailers): add a \"key_value_separator\"\n \n - As noted in a previous commit which added \"keyonly\" it's needlessly\n - hard to use the \"log\" machinery to produce machine-readable output for\n - %(trailers). with the combination of the existing \"separator\" and this\n - new \"key_value_separator\" this becomes trivial, as seen by the test\n - being added here.\n + Add a \"key_value_separator\" option to the \"%(trailers)\" pretty format,\n + to go along with the existing \"separator\" argument. In combination\n + these two options make it trivial to produce machine-readable (e.g. \\0\n + and \\0\\0-delimited) format output.\n +\n + As elaborated on in a previous commit which added \"keyonly\" it was\n + needlessly tedious to extract structured data from \"%(trailers)\"\n + before the addition of this \"key_value_separator\" option. As seen by\n + the test being added here extracting this data now becomes trivial.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## Documentation/pretty-formats.txt ##\n -@@ Documentation/pretty-formats.txt: multiple times the last occurance wins.\n +@@ Documentation/pretty-formats.txt: option is given with no value, it's enabled.\n `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n - ** 'keyonly[=bool]': only show the key part of the trailer.\n - ** 'valueonly[=bool]': only show the value part of the trailer.\n + ** 'keyonly[=BOOL]': only show the key part of the trailer.\n + ** 'valueonly[=BOOL]': only show the value part of the trailer.\n +** 'key_value_separator=': specify a separator inserted between\n + trailer lines. When this option is not given each trailer key-value\n -+ pair separated by \": \". Otherwise it shares the same semantics as \n -+ 'separator=' above.\n ++ pair is separated by \": \". Otherwise it shares the same semantics\n ++ as 'separator=' above.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\n @@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\t\t !match_placeholder_bool_arg(arg, \"keyonly\", &arg, &opts.key_only) &&\n \n ## t/t4205-log-pretty-formats.sh ##\n -@@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers:separator) changes separator' '\n +@@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa\n \ttest_cmp expect actual\n '\n \n +test_expect_success 'pretty format %(trailers:key_value_separator) changes key-value separator' '\n ++\tgit log --no-walk --pretty=format:\"X%(trailers:key_value_separator=%x00)X\" >actual &&\n ++\t(\n ++\t\tprintf \"XSigned-off-by\\0A U Thor \\n\" &&\n ++\t\tprintf \"Acked-by\\0A U Thor \\n\" &&\n ++\t\tprintf \"[ v2 updated patch description ]\\n\" &&\n ++\t\tprintf \"Signed-off-by\\0A U Thor\\n \\nX\"\n ++\t) >expect &&\n ++\ttest_cmp expect actual\n ++'\n ++\n ++test_expect_success 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' '\n +\tgit log --no-walk --pretty=format:\"X%(trailers:key_value_separator=%x00,unfold)X\" >actual &&\n +\t(\n +\t\tprintf \"XSigned-off-by\\0A U Thor \\n\" &&\n @@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers:sep\n \n ## trailer.c ##\n @@ trailer.c: static void format_trailer_info(struct strbuf *out,\n + \n + \t/* If we want the whole block untouched, we can take the fast path. */\n + \tif (!opts->only_trailers && !opts->unfold && !opts->filter &&\n +-\t !opts->separator && !opts->key_only && !opts->value_only) {\n ++\t !opts->separator && !opts->key_only && !opts->value_only &&\n ++\t !opts->key_value_separator) {\n + \t\tstrbuf_add(out, info->trailer_start,\n + \t\t\t info->trailer_end - info->trailer_start);\n + \t\treturn;\n +@@ trailer.c: static void format_trailer_info(struct strbuf *out,\n \t\t\t\t\tstrbuf_addbuf(out, opts->separator);\n \t\t\t\tif (!opts->value_only)\n - \t\t\t\t\tstrbuf_addstr(out, tok.buf);\n + \t\t\t\t\tstrbuf_addbuf(out, &tok);\n -\t\t\t\tif (!opts->key_only && !opts->value_only)\n -\t\t\t\t\tstrbuf_addstr(out, \": \");\n +\t\t\t\tif (!opts->key_only && !opts->value_only) {\n @@ trailer.c: static void format_trailer_info(struct strbuf *out,\n \t\t\t\tif (!opts->key_only)\n \t\t\t\t\tstrbuf_addbuf(out, &val);\n \t\t\t\tif (!opts->separator)\n +\n + ## trailer.h ##\n +@@ trailer.h: struct process_trailer_options {\n + \tint key_only;\n + \tint value_only;\n + \tconst struct strbuf *separator;\n ++\tconst struct strbuf *key_value_separator;\n + \tint (*filter)(const struct strbuf *, void *);\n + \tvoid *filter_data;\n + };\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411543","messageId":"20201206002449.31452-6-avarab@gmail.com","threadId":"54505","inReplyTo":"20201205013918.18981-1-avarab@gmail.com","subject":"[PATCH v2 5/5] pretty format %(trailers): add a \"key_value_separator\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-06T00:24:49Z","receivedAt":"2020-12-06T00:26:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a \"key_value_separator\" option to the \"%(trailers)\" pretty format,\nto go along with the existing \"separator\" argument. In combination\nthese two options make it trivial to produce machine-readable (e.g. \\0\nand \\0\\0-delimited) format output.\n\nAs elaborated on in a previous commit which added \"keyonly\" it was\nneedlessly tedious to extract structured data from \"%(trailers)\"\nbefore the addition of this \"key_value_separator\" option. As seen by\nthe test being added here extracting this data now becomes trivial.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/pretty-formats.txt | 4 ++++\n pretty.c | 9 +++++++++\n t/t4205-log-pretty-formats.sh | 33 ++++++++++++++++++++++++++++++++\n trailer.c | 11 ++++++++---\n trailer.h | 1 +\n 5 files changed, 55 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex bf35f7cf219..17050a78245 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -284,6 +284,10 @@ option is given with no value, it's enabled.\n `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n ** 'keyonly[=BOOL]': only show the key part of the trailer.\n ** 'valueonly[=BOOL]': only show the value part of the trailer.\n+** 'key_value_separator=': specify a separator inserted between\n+ trailer lines. When this option is not given each trailer key-value\n+ pair is separated by \": \". Otherwise it shares the same semantics\n+ as 'separator=' above.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex 1237ee0e45d..05eef7fda0b 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1418,6 +1418,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n \t\tstruct string_list filter_list = STRING_LIST_INIT_NODUP;\n \t\tstruct strbuf sepbuf = STRBUF_INIT;\n+\t\tstruct strbuf kvsepbuf = STRBUF_INIT;\n \t\tsize_t ret = 0;\n \n \t\topts.no_divider = 1;\n@@ -1449,6 +1450,14 @@ 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) &&\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 4c9f6eb7946..749bc1431ac 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -776,6 +776,39 @@ test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers:key_value_separator) changes key-value separator' '\n+\tgit log --no-walk --pretty=format:\"X%(trailers:key_value_separator=%x00)X\" >actual &&\n+\t(\n+\t\tprintf \"XSigned-off-by\\0A U Thor \\n\" &&\n+\t\tprintf \"Acked-by\\0A U Thor \\n\" &&\n+\t\tprintf \"[ v2 updated patch description ]\\n\" &&\n+\t\tprintf \"Signed-off-by\\0A U Thor\\n \\nX\"\n+\t) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' '\n+\tgit log --no-walk --pretty=format:\"X%(trailers:key_value_separator=%x00,unfold)X\" >actual &&\n+\t(\n+\t\tprintf \"XSigned-off-by\\0A U Thor \\n\" &&\n+\t\tprintf \"Acked-by\\0A U Thor \\n\" &&\n+\t\tprintf \"[ v2 updated patch description ]\\n\" &&\n+\t\tprintf \"Signed-off-by\\0A U Thor \\nX\"\n+\t) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:separator,key_value_separator) changes both separators' '\n+\tgit log --no-walk --pretty=format:\"%(trailers:separator=%x00,key_value_separator=%x00%x00,unfold)\" >actual &&\n+\t(\n+\t\tprintf \"Signed-off-by\\0\\0A U Thor \\0\" &&\n+\t\tprintf \"Acked-by\\0\\0A U Thor \\0\" &&\n+\t\tprintf \"[ v2 updated patch description ]\\0\" &&\n+\t\tprintf \"Signed-off-by\\0\\0A U Thor \"\n+\t) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'pretty format %(trailers) combining separator/key/keyonly/valueonly' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tImportant fix\ndiff --git a/trailer.c b/trailer.c\nindex 889b419a4f6..249ed618ed8 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1132,7 +1132,8 @@ static void format_trailer_info(struct strbuf *out,\n \n \t/* If we want the whole block untouched, we can take the fast path. */\n \tif (!opts->only_trailers && !opts->unfold && !opts->filter &&\n-\t !opts->separator && !opts->key_only && !opts->value_only) {\n+\t !opts->separator && !opts->key_only && !opts->value_only &&\n+\t !opts->key_value_separator) {\n \t\tstrbuf_add(out, info->trailer_start,\n \t\t\t info->trailer_end - info->trailer_start);\n \t\treturn;\n@@ -1155,8 +1156,12 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\t\t\tstrbuf_addbuf(out, opts->separator);\n \t\t\t\tif (!opts->value_only)\n \t\t\t\t\tstrbuf_addbuf(out, &tok);\n-\t\t\t\tif (!opts->key_only && !opts->value_only)\n-\t\t\t\t\tstrbuf_addstr(out, \": \");\n+\t\t\t\tif (!opts->key_only && !opts->value_only) {\n+\t\t\t\t\tif (opts->key_value_separator)\n+\t\t\t\t\t\tstrbuf_addbuf(out, opts->key_value_separator);\n+\t\t\t\t\telse\n+\t\t\t\t\t\tstrbuf_addstr(out, \": \");\n+\t\t\t\t}\n \t\t\t\tif (!opts->key_only)\n \t\t\t\t\tstrbuf_addbuf(out, &val);\n \t\t\t\tif (!opts->separator)\ndiff --git a/trailer.h b/trailer.h\nindex d2f28776be6..795d2fccfd9 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -74,6 +74,7 @@ struct process_trailer_options {\n \tint key_only;\n \tint value_only;\n \tconst struct strbuf *separator;\n+\tconst struct strbuf *key_value_separator;\n \tint (*filter)(const struct strbuf *, void *);\n \tvoid *filter_data;\n };\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411593","messageId":"87h7oyxail.fsf@evledraar.gmail.com","threadId":"54505","inReplyTo":"87wnxwp15o.fsf@0x63.nu","subject":"Re: [PATCH 0/5] pretty format %(trailers): improve machine readability","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-07T08:53:22Z","receivedAt":"2020-12-07T08:54:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Dec 05 2020, Anders Waldenborg wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> I started writing this on top of \"master\", but then saw the\n>> outstanding series of other miscellaneous fixes to this\n>> facility[1]. This is on top of that topic & rebased on master.\n>>\n>> Anders, any plans to re-roll yours? Otherwise the conflicts I'd have\n>> on mine are easy to fix, so I can also submit it as a stand-alone.\n>\n> Yes, I have plans to do that. But have yet to carve out the required\n> time from my copious spare time to actually do it.\n>\n> So please don't hold your breath waiting for me to do that.\n\nThanks. I sent a v2 of mine yesterday as\nhttps://lore.kernel.org/git/20201206002449.31452-1-avarab@gmail.com/\n\nAs noted there the merge conflict with yours is trivial, so hopefully it\nwon't cause you much hassle if you re-roll while it's outstanding.\n\n>> This series comes out of a discussion at work today (well, yesterday\n>> at this point) where someone wanted to parse %(trailers) output. As\n>> noted in 3/5 doing this is rather tedious now if you're trying to\n>> unambiguously grap trailers as a stream of key-value pairs.\n>>\n>> So this series adds a \"key_value_separator\" and \"keyonly\" parameters,\n>> and fixes a few bugs I saw along the way.\n>\n> Interesting. When adding \"valueonly\" I never consider it being used\n> without \"key\". The trick you are doing with separate keyonly and\n> valueonly is quite clever.\n>\n> I've only been doing machine parsing for explicit keys, things like:\n> \"%cn%x00%x00%an%x00%x00%(trailers:key=Reviewed-By,valueonly,unfold,separator=%x00)%x00%x00%(trailers:key=Backport-Reviewed-By,valueonly,unfold,separator=%x00)\"\n> (double-NUL to separate field, single-NUL to separate values within field).\n>\n> But I can't help wonder that if the goal just is to have a nice machine\n> parsable format maybe it would be easier (both for user and\n> implementation) to have a separate placeholder for \"machine readable\n> trailers\" which by default emits in a format suitable for machine\n> parsing. Something like a new \"%(ztrailers)\" (but with a better name)\n> which simply emits a sequence of \" NUL NUL\" for each trailer\n\nI think it's a bit tricky to make something general in the middle of all\nthe custom format printf-likes in the pretty format. E.g. some users\nmight want to use \\0 as a delimiter for key-values, others \\0\\0\netc. because they used \\0, or the other way around.\n\nMaybe if there's a reason to extend the optimization it could be smarter\nabout detecting that you only wanted some fixed-string separator and\nnothing else custom?\n\nB.t.w. I tried just deleting the optimization for testing and it slowed\ndown by around 8% on linux.git according to an extended\np4205-log-pretty-formats.sh.\n\nLooking at the code I wonder if there aren't other lower hanging\noptimizations, e.g. it seems we call find_separator() on multiple passes\ninstead of saving it away, e.g. in the format_trailers_from_commit()\nentry point if there's any custom options such as \"unfold\".\n\nI also wonder if memory allocation is a bottleneck in the \"git log\"\npath, but didn't have time to refactor & test it. For each commit the\nwalking machinery eventually calls the trailer.c code, which allocates &\nfree()'s internal structures that could be re-used for parsing the next\ncommit.\n"},{"id":"411594","messageId":"CAP8UFD3CY1TuKh5TYzEqL1w87cfGHbgoavRoSgcx=uwgpBkfaA@mail.gmail.com","threadId":"54505","inReplyTo":"20201206002449.31452-3-avarab@gmail.com","subject":"Re: [PATCH v2 2/5] pretty format %(trailers) doc: avoid repetition","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-12-07T09:09:23Z","receivedAt":"2020-12-07T09:10:17Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Dec 6, 2020 at 1:25 AM Ævar Arnfjörð Bjarmason wrote:\n>\n> Change the documentation for the various %(trailers) options so it\n> isn't repeating part of the documentation for \"only\" about how boolean\n> values are handled. Instead, let's split the description of that into\n> general documentation at the top.\n>\n> It then suffices to refer to it by listing the options as\n> \"opt[=]\". I'm also changing it to upper-case \"[=]\" from\n> \"[=val]\" for consistency with \"\"\n\nGood...\n\n> It took me a couple of readings to realize that these options were\n> referring back to the \"only\" option's treatment of boolean\n> values. Let's try to make this more explicit, and upper-case \"BOOL\"\n> for consistency with the existing \"\" and \"\".\n\n... but not sure it's worth repeating that we upper-case \"BOOL\".\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> Documentation/pretty-formats.txt | 30 ++++++++++++++++--------------\n> 1 file changed, 16 insertions(+), 14 deletions(-)\n>\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index 84bbc7439a6..66dfa122361 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -252,7 +252,15 @@ endif::git-rev-list[]\n> interpreted by\n> linkgit:git-interpret-trailers[1]. The\n> `trailers` string may be followed by a colon\n> - and zero or more comma-separated options:\n> + and zero or more comma-separated options.\n> + If any option is provided multiple times the\n> + last occurance wins.\n> ++\n> +The boolean options accept an optional value `[=]`. The values\n> +`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n> +sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n> +option is given with no value, it's enabled.\n\n> +** 'only[=BOOL]': select whether non-trailer lines from the trailer\n\nHere it's \"[=BOOL]\" while above it's \"[=]\"\n\n> +** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`\n\nHere also.\n\n> +** 'valueonly[=BOOL]': skip over the key part of the trailer line and only\n\nAnd here too.\n\n> + show the value part.\n"},{"id":"411596","messageId":"CAP8UFD3PCwokJegLfVN2naqKh=1vQRrG4drat95jXF=01_p=yw@mail.gmail.com","threadId":"54505","inReplyTo":"20201206002449.31452-5-avarab@gmail.com","subject":"Re: [PATCH v2 4/5] pretty format %(trailers): add a \"keyonly\"","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-12-07T09:17:49Z","receivedAt":"2020-12-07T09:18:55Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Dec 6, 2020 at 1:25 AM Ævar Arnfjörð Bjarmason wrote:\n\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -282,8 +282,8 @@ option is given with no value, it's enabled.\n> ** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`\n> option was given. E.g.,\n> `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n> -** 'valueonly[=BOOL]': skip over the key part of the trailer line and only\n> - show the value part.\n> +** 'keyonly[=BOOL]': only show the key part of the trailer.\n\nHere also \"[=]\" would be more consistent.\n\n> +** 'valueonly[=BOOL]': only show the value part of the trailer.\n"},{"id":"411874","messageId":"20201209155208.17782-2-avarab@gmail.com","threadId":"54505","inReplyTo":"20201206002449.31452-1-avarab@gmail.com","subject":"[PATCH v3 1/5] pretty format %(trailers) test: split a long line","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-09T15:52:04Z","receivedAt":"2020-12-09T15:53:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Split a very long line in a test introduced in 0b691d86851 (pretty:\nadd support for separator option in %(trailers), 2019-01-28). This\nmakes it easier to read, especially as follow-up commits will copy\nthis test as a template.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t4205-log-pretty-formats.sh | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 204c149d5a4..5e5452212d2 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -717,7 +717,12 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n \n test_expect_success 'pretty format %(trailers:separator) changes separator' '\n \tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00,unfold)X\" >actual &&\n-\tprintf \"XSigned-off-by: A U Thor \\0Acked-by: A U Thor \\0[ v2 updated patch description ]\\0Signed-off-by: A U Thor X\" >expect &&\n+\t(\n+\t\tprintf \"XSigned-off-by: A U Thor \\0\" &&\n+\t\tprintf \"Acked-by: A U Thor \\0\" &&\n+\t\tprintf \"[ v2 updated patch description ]\\0\" &&\n+\t\tprintf \"Signed-off-by: A U Thor X\"\n+\t) >expect &&\n \ttest_cmp expect actual\n '\n \n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411875","messageId":"20201209155208.17782-6-avarab@gmail.com","threadId":"54505","inReplyTo":"20201206002449.31452-1-avarab@gmail.com","subject":"[PATCH v3 5/5] pretty format %(trailers): add a \"key_value_separator\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-09T15:52:08Z","receivedAt":"2020-12-09T15:53:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a \"key_value_separator\" option to the \"%(trailers)\" pretty format,\nto go along with the existing \"separator\" argument. In combination\nthese two options make it trivial to produce machine-readable (e.g. \\0\nand \\0\\0-delimited) format output.\n\nAs elaborated on in a previous commit which added \"keyonly\" it was\nneedlessly tedious to extract structured data from \"%(trailers)\"\nbefore the addition of this \"key_value_separator\" option. As seen by\nthe test being added here extracting this data now becomes trivial.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/pretty-formats.txt | 4 ++++\n pretty.c | 9 +++++++++\n t/t4205-log-pretty-formats.sh | 33 ++++++++++++++++++++++++++++++++\n trailer.c | 11 ++++++++---\n trailer.h | 1 +\n 5 files changed, 55 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 5eac36500d4..6b59e28d444 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -284,6 +284,10 @@ option is given with no value, it's enabled.\n `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n ** 'keyonly[=]': only show the key part of the trailer.\n ** 'valueonly[=]': only show the value part of the trailer.\n+** 'key_value_separator=': specify a separator inserted between\n+ trailer lines. When this option is not given each trailer key-value\n+ pair is separated by \": \". Otherwise it shares the same semantics\n+ as 'separator=' above.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex 1237ee0e45d..05eef7fda0b 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1418,6 +1418,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n \t\tstruct string_list filter_list = STRING_LIST_INIT_NODUP;\n \t\tstruct strbuf sepbuf = STRBUF_INIT;\n+\t\tstruct strbuf kvsepbuf = STRBUF_INIT;\n \t\tsize_t ret = 0;\n \n \t\topts.no_divider = 1;\n@@ -1449,6 +1450,14 @@ 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) &&\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 4c9f6eb7946..749bc1431ac 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -776,6 +776,39 @@ test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers:key_value_separator) changes key-value separator' '\n+\tgit log --no-walk --pretty=format:\"X%(trailers:key_value_separator=%x00)X\" >actual &&\n+\t(\n+\t\tprintf \"XSigned-off-by\\0A U Thor \\n\" &&\n+\t\tprintf \"Acked-by\\0A U Thor \\n\" &&\n+\t\tprintf \"[ v2 updated patch description ]\\n\" &&\n+\t\tprintf \"Signed-off-by\\0A U Thor\\n \\nX\"\n+\t) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' '\n+\tgit log --no-walk --pretty=format:\"X%(trailers:key_value_separator=%x00,unfold)X\" >actual &&\n+\t(\n+\t\tprintf \"XSigned-off-by\\0A U Thor \\n\" &&\n+\t\tprintf \"Acked-by\\0A U Thor \\n\" &&\n+\t\tprintf \"[ v2 updated patch description ]\\n\" &&\n+\t\tprintf \"Signed-off-by\\0A U Thor \\nX\"\n+\t) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:separator,key_value_separator) changes both separators' '\n+\tgit log --no-walk --pretty=format:\"%(trailers:separator=%x00,key_value_separator=%x00%x00,unfold)\" >actual &&\n+\t(\n+\t\tprintf \"Signed-off-by\\0\\0A U Thor \\0\" &&\n+\t\tprintf \"Acked-by\\0\\0A U Thor \\0\" &&\n+\t\tprintf \"[ v2 updated patch description ]\\0\" &&\n+\t\tprintf \"Signed-off-by\\0\\0A U Thor \"\n+\t) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'pretty format %(trailers) combining separator/key/keyonly/valueonly' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tImportant fix\ndiff --git a/trailer.c b/trailer.c\nindex 889b419a4f6..249ed618ed8 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1132,7 +1132,8 @@ static void format_trailer_info(struct strbuf *out,\n \n \t/* If we want the whole block untouched, we can take the fast path. */\n \tif (!opts->only_trailers && !opts->unfold && !opts->filter &&\n-\t !opts->separator && !opts->key_only && !opts->value_only) {\n+\t !opts->separator && !opts->key_only && !opts->value_only &&\n+\t !opts->key_value_separator) {\n \t\tstrbuf_add(out, info->trailer_start,\n \t\t\t info->trailer_end - info->trailer_start);\n \t\treturn;\n@@ -1155,8 +1156,12 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\t\t\tstrbuf_addbuf(out, opts->separator);\n \t\t\t\tif (!opts->value_only)\n \t\t\t\t\tstrbuf_addbuf(out, &tok);\n-\t\t\t\tif (!opts->key_only && !opts->value_only)\n-\t\t\t\t\tstrbuf_addstr(out, \": \");\n+\t\t\t\tif (!opts->key_only && !opts->value_only) {\n+\t\t\t\t\tif (opts->key_value_separator)\n+\t\t\t\t\t\tstrbuf_addbuf(out, opts->key_value_separator);\n+\t\t\t\t\telse\n+\t\t\t\t\t\tstrbuf_addstr(out, \": \");\n+\t\t\t\t}\n \t\t\t\tif (!opts->key_only)\n \t\t\t\t\tstrbuf_addbuf(out, &val);\n \t\t\t\tif (!opts->separator)\ndiff --git a/trailer.h b/trailer.h\nindex d2f28776be6..795d2fccfd9 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -74,6 +74,7 @@ struct process_trailer_options {\n \tint key_only;\n \tint value_only;\n \tconst struct strbuf *separator;\n+\tconst struct strbuf *key_value_separator;\n \tint (*filter)(const struct strbuf *, void *);\n \tvoid *filter_data;\n };\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411876","messageId":"20201209155208.17782-5-avarab@gmail.com","threadId":"54505","inReplyTo":"20201206002449.31452-1-avarab@gmail.com","subject":"[PATCH v3 4/5] pretty format %(trailers): add a \"keyonly\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-09T15:52:07Z","receivedAt":"2020-12-09T15:53:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add support for a \"keyonly\". This allows for easier parsing out of the\nkey and value. Before if you didn't want to make assumptions about how\nthe key was formatted. You'd need to parse it out as e.g.:\n\n --pretty=format:'%H%x00%(trailers:separator=%x00%x00)' \\\n '%x00%(trailers:separator=%x00%x00,valueonly)'\n\nAnd then proceed to deduce keys by looking at those two and\nsubtracting the value plus the hardcoded \": \" separator from the\nnon-valueonly %(trailers) line. Now it's possible to simply do:\n\n --pretty=format:'%H%x00%(trailers:separator=%x00%x00,keyonly)' \\\n '%x00%(trailers:separator=%x00%x00,valueonly)'\n\nWhich at least reduces it to a state machine where you get N keys and\ncorrelate them with N values. Even better would be to have a way to\nchange the \": \" delimiter to something easily machine-readable (a key\nmight contain \": \" too). A follow-up change will add support for that.\n\nI don't really have a use-case for just \"keyonly\" myself. I suppose it\nwould be useful in some cases as \"key=*\" matches case-insensitively,\nso a plain \"keyonly\" will give you the variants of the keys you\nmatched. I'm mainly adding it to fix the inconsistency with\n\"valueonly\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/pretty-formats.txt | 4 ++--\n pretty.c | 1 +\n t/t4205-log-pretty-formats.sh | 31 ++++++++++++++++++++++++++++++-\n trailer.c | 9 ++++++---\n trailer.h | 1 +\n 5 files changed, 40 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 973b6c7d482..5eac36500d4 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -282,8 +282,8 @@ option is given with no value, it's enabled.\n ** 'unfold[=]': make it behave as if interpret-trailer's `--unfold`\n option was given. E.g.,\n `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n-** 'valueonly[=]': skip over the key part of the trailer line and only\n- show the value part.\n+** 'keyonly[=]': only show the key part of the trailer.\n+** 'valueonly[=]': only show the value part of the trailer.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex 7a7708a0ea7..1237ee0e45d 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1451,6 +1451,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\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, \"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}\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex cb09a13249e..4c9f6eb7946 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -715,6 +715,22 @@ test_expect_success '%(trailers:key) without value is error' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:keyonly) shows only keys' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:keyonly)\" >actual &&\n+\ttest_write_lines \\\n+\t\t\"Signed-off-by\" \\\n+\t\t\"Acked-by\" \\\n+\t\t\"[ v2 updated patch description ]\" \\\n+\t\t\"Signed-off-by\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=foo,keyonly) shows only key' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,keyonly)\" >actual &&\n+\techo \"Acked-by\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n \tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,valueonly)\" >actual &&\n \techo \"A U Thor \" >expect &&\n@@ -732,6 +748,12 @@ test_expect_success '%(trailers:valueonly) shows only values' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:key=foo,keyonly,valueonly) shows nothing' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,keyonly,valueonly)\" >actual &&\n+\techo >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'pretty format %(trailers:separator) changes separator' '\n \tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00)X\" >actual &&\n \t(\n@@ -754,7 +776,7 @@ test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' '\n+test_expect_success 'pretty format %(trailers) combining separator/key/keyonly/valueonly' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tImportant fix\n \n@@ -781,6 +803,13 @@ test_expect_success 'pretty format %(trailers) combining separator/key/valueonly\n \t\t\"Does not close any tickets\" \\\n \t\t\"Another fix #567, #890\" \\\n \t\t\"Important fix #1234\" >expect &&\n+\ttest_cmp expect actual &&\n+\n+\tgit log --pretty=\"%s% (trailers:separator=%x2c%x20,key=Closes,keyonly)\" HEAD~3.. >actual &&\n+\ttest_write_lines \\\n+\t\t\"Does not close any tickets\" \\\n+\t\t\"Another fix Closes, Closes\" \\\n+\t\t\"Important fix Closes\" >expect &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/trailer.c b/trailer.c\nindex d2d01015b1d..889b419a4f6 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out,\n \n \t/* If we want the whole block untouched, we can take the fast path. */\n \tif (!opts->only_trailers && !opts->unfold && !opts->filter &&\n-\t !opts->separator && !opts->value_only) {\n+\t !opts->separator && !opts->key_only && !opts->value_only) {\n \t\tstrbuf_add(out, info->trailer_start,\n \t\t\t info->trailer_end - info->trailer_start);\n \t\treturn;\n@@ -1154,8 +1154,11 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\t\tif (opts->separator && out->len != origlen)\n \t\t\t\t\tstrbuf_addbuf(out, opts->separator);\n \t\t\t\tif (!opts->value_only)\n-\t\t\t\t\tstrbuf_addf(out, \"%s: \", tok.buf);\n-\t\t\t\tstrbuf_addbuf(out, &val);\n+\t\t\t\t\tstrbuf_addbuf(out, &tok);\n+\t\t\t\tif (!opts->key_only && !opts->value_only)\n+\t\t\t\t\tstrbuf_addstr(out, \": \");\n+\t\t\t\tif (!opts->key_only)\n+\t\t\t\t\tstrbuf_addbuf(out, &val);\n \t\t\t\tif (!opts->separator)\n \t\t\t\t\tstrbuf_addch(out, '\\n');\n \t\t\t}\ndiff --git a/trailer.h b/trailer.h\nindex cd93e7ddea7..d2f28776be6 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -71,6 +71,7 @@ struct process_trailer_options {\n \tint only_input;\n \tint unfold;\n \tint no_divider;\n+\tint key_only;\n \tint value_only;\n \tconst struct strbuf *separator;\n \tint (*filter)(const struct strbuf *, void *);\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411877","messageId":"20201209155208.17782-4-avarab@gmail.com","threadId":"54505","inReplyTo":"20201206002449.31452-1-avarab@gmail.com","subject":"[PATCH v3 3/5] pretty-format %(trailers): fix broken standalone \"valueonly\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-09T15:52:06Z","receivedAt":"2020-12-09T15:53:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix %(trailers:valueonly) being a noop due to on overly eager\noptimization in format_trailer_info() which skips custom formatting if\nno custom options are given.\n\nWhen \"valueonly\" was added in d9b936db522 (pretty: add support for\n\"valueonly\" option in %(trailers), 2019-01-28) we forgot to add it to\nthe list of options that optimization checks for. See e.g. the\naddition of \"key\" in 250bea0c165 (pretty: allow showing specific\ntrailers, 2019-01-28) for a similar change where this wasn't missed.\n\nThus the \"valueonly\" option in \"%(trailers:valueonly)\" was a noop and\nthe output was equivalent to that of a plain \"%(trailers)\". This\nwasn't caught because the tests for it always combined it with other\noptions.\n\nFix the bug by adding !opts->value_only to the list. I initially\nattempted to make this more future-proof by setting a flag if we got\nto \":\" in \"%(trailers:\" in format_commit_one() in pretty.c. However,\n\"%(trailers:\" is also parsed in trailers_atom_parser() in\nref-filter.c.\n\nThere is an outstanding patch[1] unify those two, and such a fix, or\nother future-proofing, such as changing \"process_trailer_options\"\nflags into a bitfield, would conflict with that effort. Let's instead\ndo the bare minimum here as this aspect of trailers is being actively\nworked on by another series.\n\nLet's also test for a plain \"valueonly\" without any other options, as\nwell as \"separator\". All the other existing options on the pretty.c\npath had tests where they were the only option provided. I'm also\nkeeping a sanity test for \"%(trailers:)\" being the same as\n\"%(trailers)\". There's no reason to suspect it wouldn't be in the\ncurrent implementation, but let's keep it in the interest of black box\ntesting.\n\n1. https://lore.kernel.org/git/pull.726.git.1599335291.gitgitgadget@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t4205-log-pretty-formats.sh | 28 ++++++++++++++++++++++++++++\n trailer.c | 3 ++-\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 5e5452212d2..cb09a13249e 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -605,6 +605,12 @@ test_expect_success 'pretty format %(trailers) shows trailers' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers:) enables no options' '\n+\tgit log --no-walk --pretty=\"%(trailers:)\" >actual &&\n+\t# \"expect\" the same as the test above\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '%(trailers:only) shows only \"key: value\" trailers' '\n \tgit log --no-walk --pretty=\"%(trailers:only)\" >actual &&\n \t{\n@@ -715,7 +721,29 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:valueonly) shows only values' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:valueonly)\" >actual &&\n+\ttest_write_lines \\\n+\t\t\"A U Thor \" \\\n+\t\t\"A U Thor \" \\\n+\t\t\"[ v2 updated patch description ]\" \\\n+\t\t\"A U Thor\" \\\n+\t\t\" \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'pretty format %(trailers:separator) changes separator' '\n+\tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00)X\" >actual &&\n+\t(\n+\t\tprintf \"XSigned-off-by: A U Thor \\0\" &&\n+\t\tprintf \"Acked-by: A U Thor \\0\" &&\n+\t\tprintf \"[ v2 updated patch description ]\\0\" &&\n+\t\tprintf \"Signed-off-by: A U Thor\\n X\"\n+\t) >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separator' '\n \tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00,unfold)X\" >actual &&\n \t(\n \t\tprintf \"XSigned-off-by: A U Thor \\0\" &&\ndiff --git a/trailer.c b/trailer.c\nindex 3f7391d793c..d2d01015b1d 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1131,7 +1131,8 @@ static void format_trailer_info(struct strbuf *out,\n \tsize_t i;\n \n \t/* If we want the whole block untouched, we can take the fast path. */\n-\tif (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) {\n+\tif (!opts->only_trailers && !opts->unfold && !opts->filter &&\n+\t !opts->separator && !opts->value_only) {\n \t\tstrbuf_add(out, info->trailer_start,\n \t\t\t info->trailer_end - info->trailer_start);\n \t\treturn;\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411878","messageId":"20201209155208.17782-3-avarab@gmail.com","threadId":"54505","inReplyTo":"20201206002449.31452-1-avarab@gmail.com","subject":"[PATCH v3 2/5] pretty format %(trailers) doc: avoid repetition","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-09T15:52:05Z","receivedAt":"2020-12-09T15:53:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the documentation for the various %(trailers) options so it\nisn't repeating part of the documentation for \"only\" about how boolean\nvalues are handled. Instead, let's split the description of that into\ngeneral documentation at the top.\n\nIt then suffices to refer to it by listing the options as\n\"opt[=]\". I'm also changing it to upper-case \"[=]\" from\n\"[=val]\" for consistency with \"\"\n\nIt took me a couple of readings to realize that these options were\nreferring back to the \"only\" option's treatment of boolean\nvalues.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/pretty-formats.txt | 30 ++++++++++++++++--------------\n 1 file changed, 16 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 84bbc7439a6..973b6c7d482 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -252,7 +252,15 @@ endif::git-rev-list[]\n \t\t\t interpreted by\n \t\t\t linkgit:git-interpret-trailers[1]. The\n \t\t\t `trailers` string may be followed by a colon\n-\t\t\t and zero or more comma-separated options:\n+\t\t\t and zero or more comma-separated options.\n+\t\t\t If any option is provided multiple times the\n+\t\t\t last occurance wins.\n++\n+The boolean options accept an optional value `[=]`. The values\n+`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n+sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n+option is given with no value, it's enabled.\n++\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@@ -261,27 +269,21 @@ endif::git-rev-list[]\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+** 'only[=]': select whether non-trailer lines from the trailer\n+ block should be included.\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+ next option. 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+** 'unfold[=]': make it behave as if interpret-trailer's `--unfold`\n+ option was given. 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+** 'valueonly[=]': skip over the key part of the trailer line and only\n+ show the value part.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411879","messageId":"20201209155208.17782-1-avarab@gmail.com","threadId":"54505","inReplyTo":"20201206002449.31452-1-avarab@gmail.com","subject":"[PATCH v3 0/5] pretty format %(trailers): improve machine readability","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-09T15:52:03Z","receivedAt":"2020-12-09T15:53:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A minor iteration on v2 with a commit message wording change &\ns/=BOOL/=/g in the docs, as suggested by Christian\nCouder. Range-diff below:\n\nÆvar Arnfjörð Bjarmason (5):\n pretty format %(trailers) test: split a long line\n pretty format %(trailers) doc: avoid repetition\n pretty-format %(trailers): fix broken standalone \"valueonly\"\n pretty format %(trailers): add a \"keyonly\"\n pretty format %(trailers): add a \"key_value_separator\"\n\n Documentation/pretty-formats.txt | 34 ++++++-----\n pretty.c | 10 ++++\n t/t4205-log-pretty-formats.sh | 99 +++++++++++++++++++++++++++++++-\n trailer.c | 15 ++++-\n trailer.h | 2 +\n 5 files changed, 141 insertions(+), 19 deletions(-)\n\nRange-diff:\n1: 4b134a62aec = 1: 584c7580b5b pretty format %(trailers) test: split a long line\n2: 0d3fe6daf6c ! 2: 0255a64949b pretty format %(trailers) doc: avoid repetition\n @@ Commit message\n \n It took me a couple of readings to realize that these options were\n referring back to the \"only\" option's treatment of boolean\n - values. Let's try to make this more explicit, and upper-case \"BOOL\"\n - for consistency with the existing \"\" and \"\".\n + values.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ Documentation/pretty-formats.txt: endif::git-rev-list[]\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 -+** 'only[=BOOL]': select whether non-trailer lines from the trailer\n ++** 'only[=]': select whether non-trailer lines from the trailer\n + block should be included.\n ** 'separator=': specify a separator inserted between trailer\n lines. When this option is not given each trailer line is\n @@ Documentation/pretty-formats.txt: endif::git-rev-list[]\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 -+** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`\n ++** 'unfold[=]': make it behave as if interpret-trailer's `--unfold`\n + option was given. 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 -+** 'valueonly[=BOOL]': skip over the key part of the trailer line and only\n ++** 'valueonly[=]': skip over the key part of the trailer line and only\n + show the value part.\n \n NOTE: Some placeholders may depend on other options given to the\n3: ea44eeff510 = 3: c2c5513942f pretty-format %(trailers): fix broken standalone \"valueonly\"\n4: 4fd193fd90c ! 4: 574ef0be25f pretty format %(trailers): add a \"keyonly\"\n @@ Commit message\n \n ## Documentation/pretty-formats.txt ##\n @@ Documentation/pretty-formats.txt: option is given with no value, it's enabled.\n - ** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`\n + ** 'unfold[=]': make it behave as if interpret-trailer's `--unfold`\n option was given. E.g.,\n `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n --** 'valueonly[=BOOL]': skip over the key part of the trailer line and only\n +-** 'valueonly[=]': skip over the key part of the trailer line and only\n - show the value part.\n -+** 'keyonly[=BOOL]': only show the key part of the trailer.\n -+** 'valueonly[=BOOL]': only show the value part of the trailer.\n ++** 'keyonly[=]': only show the key part of the trailer.\n ++** 'valueonly[=]': only show the value part of the trailer.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\n5: 6cc6fc79388 ! 5: dbc73b951f6 pretty format %(trailers): add a \"key_value_separator\"\n @@ Commit message\n ## Documentation/pretty-formats.txt ##\n @@ Documentation/pretty-formats.txt: option is given with no value, it's enabled.\n `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n - ** 'keyonly[=BOOL]': only show the key part of the trailer.\n - ** 'valueonly[=BOOL]': only show the value part of the trailer.\n + ** 'keyonly[=]': only show the key part of the trailer.\n + ** 'valueonly[=]': only show the value part of the trailer.\n +** 'key_value_separator=': specify a separator inserted between\n + trailer lines. When this option is not given each trailer key-value\n + pair is separated by \": \". Otherwise it shares the same semantics\n-- \n2.29.2.222.g5d2a92d10f8\n\n"},{"id":"411991","messageId":"CAP8UFD0A-wLb3eAHWtnkd-kUbiEt=BReP7pKjgHOktcNrtRnTQ@mail.gmail.com","threadId":"54505","inReplyTo":"20201209155208.17782-1-avarab@gmail.com","subject":"Re: [PATCH v3 0/5] pretty format %(trailers): improve machine readability","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-12-10T10:48:01Z","receivedAt":"2020-12-10T10:50:05Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Dec 9, 2020 at 4:52 PM Ævar Arnfjörð Bjarmason wrote:\n>\n> A minor iteration on v2 with a commit message wording change &\n> s/=BOOL/=/g in the docs, as suggested by Christian\n> Couder. Range-diff below:\n\nThis one looks good to me!\n\nReviewed-by: Christian Couder \n"},{"id":"412008","messageId":"xmqqzh2lv63u.fsf@gitster.c.googlers.com","threadId":"54505","inReplyTo":"CAP8UFD0A-wLb3eAHWtnkd-kUbiEt=BReP7pKjgHOktcNrtRnTQ@mail.gmail.com","subject":"Re: [PATCH v3 0/5] pretty format %(trailers): improve machine readability","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-10T19:00:37Z","receivedAt":"2020-12-10T19:01:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Wed, Dec 9, 2020 at 4:52 PM Ævar Arnfjörð Bjarmason wrote:\n>>\n>> A minor iteration on v2 with a commit message wording change &\n>> s/=BOOL/=/g in the docs, as suggested by Christian\n>> Couder. Range-diff below:\n>\n> This one looks good to me!\n>\n> Reviewed-by: Christian Couder \n\nThe range-diff looked minimum and didn't introduce anything funny.\nThe unchanged parts I only skimmed, though.\n\nThanks, both.\n"},{"id":"412009","messageId":"xmqqv9d9v61r.fsf@gitster.c.googlers.com","threadId":"54505","inReplyTo":"20201209155208.17782-3-avarab@gmail.com","subject":"Re: [PATCH v3 2/5] pretty format %(trailers) doc: avoid repetition","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-10T19:01:52Z","receivedAt":"2020-12-10T19:04:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> -\t\t\t and zero or more comma-separated options:\n> +\t\t\t and zero or more comma-separated options.\n> +\t\t\t If any option is provided multiple times the\n> +\t\t\t last occurance wins.\n> ++\n> +The boolean options accept an optional value `[=]`. The values\n> +`true`, `false`, `on`, `off` etc. are all accepted. See the \"boolean\"\n> +sub-section in \"EXAMPLES\" in linkgit:git-config[1]. If a boolean\n> +option is given with no value, it's enabled.\n\nNicely written.\n"}]}