[PATCH 00/21] trailer fixes

[PATCH 00/21] trailer fixes

[Anders Waldenborg]

This patch series contains a bunch fo trailer related changes. Sparked
from this thread:
  https://public-inbox.org/git/87blk0rjob.fsf@0x63.nu/T/#r3dc3e4fa67b6fba95e4b2ea2c1cf1672af55a9ee

Most commits are refactors preparing for the others, the actual user
visible changes are:
 * Allow using aliases in pretty formatting '%(trailer:key=foo)`
 * Fixes related to matching prefix rather than full trailer
 * Tighten up "canonicalization" of trailers
 * Add --(no-)canonicalize

Anders Waldenborg (21):
  trailer: change token_{from,matches}_item into taking conf_info
  trailer: don't use 'struct arg_item' for storing config
  doc: mention canonicalization in git i-t manual
  pretty: allow using aliases in %(trailer:key=xyz)
  trailer: rename 'free_all' to 'free_all_trailer_items'
  t4205: add test for trailer in log with nonstandard separator
  trailer: simplify 'arg_item' lifetime
  trailer: keep track of conf in trailer_item
  trailer: refactor print_tok_val into taking item
  trailer: move trailer token canonicalization print time
  trailer: remember separator used in input
  trailer: handle configured nondefault separators explicitly
  trailer: add option to make canonicalization optional
  trailer: move skipping of blank lines to own loop when finding trailer
  trailer: factor out classify_trailer_line
  t7513: add failing test for configured trailing line classification
  trailer: don't treat line with prefix of known trailer as known
  trailer: factor out config lookup to separate function
  trailer: move config lookup out of parse_trailer
  trailer: add failing tests for matching trailers against input
  trailer: only do prefix matching for configured trailers on
    commandline

 Documentation/git-interpret-trailers.txt |  10 +-
 Documentation/pretty-formats.txt         |   4 +-
 builtin/interpret-trailers.c             |   3 +
 pretty.c                                 |   5 +-
 t/t4205-log-pretty-formats.sh            |  18 ++
 t/t7513-interpret-trailers.sh            | 120 ++++++++
 trailer.c                                | 374 ++++++++++++++---------
 trailer.h                                |   3 +-
 8 files changed, 386 insertions(+), 151 deletions(-)

-- 
2.25.1

[PATCH 01/21] trailer: change token_{from,matches}_item into taking conf_info

[Anders Waldenborg]

); SAEximRunCond expanded to false

These functions don't use anything from the arg_item except the conf,
so make them take conf as argument instead. This will allow them to be
used on other things that has a conf_info.

No functional change intended.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 trailer.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/trailer.c b/trailer.c
index 3f7391d793..efb88c2008 100644
--- a/trailer.c
+++ b/trailer.c
@@ -574,20 +574,20 @@ static void ensure_configured(void)
 	configured = 1;
 }
 
-static const char *token_from_item(struct arg_item *item, char *tok)
+static const char *token_from_conf(const struct conf_info *conf, char *tok)
 {
-	if (item->conf.key)
-		return item->conf.key;
+	if (conf->key)
+		return conf->key;
 	if (tok)
 		return tok;
-	return item->conf.name;
+	return conf->name;
 }
 
-static int token_matches_item(const char *tok, struct arg_item *item, size_t tok_len)
+static int token_matches_conf(const char *tok, const struct conf_info *conf, size_t tok_len)
 {
-	if (!strncasecmp(tok, item->conf.name, tok_len))
+	if (!strncasecmp(tok, conf->name, tok_len))
 		return 1;
-	return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;
+	return conf->key ? !strncasecmp(tok, conf->key, tok_len) : 0;
 }
 
 /*
@@ -650,11 +650,11 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,
 		*conf = &default_conf_info;
 	list_for_each(pos, &conf_head) {
 		item = list_entry(pos, struct arg_item, list);
-		if (token_matches_item(tok->buf, item, tok_len)) {
+		if (token_matches_conf(tok->buf, &item->conf, tok_len)) {
 			char *tok_buf = strbuf_detach(tok, NULL);
 			if (conf)
 				*conf = &item->conf;
-			strbuf_addstr(tok, token_from_item(item, tok_buf));
+			strbuf_addstr(tok, token_from_conf(&item->conf, tok_buf));
 			free(tok_buf);
 			break;
 		}
@@ -710,7 +710,7 @@ static void process_command_line_args(struct list_head *arg_head,
 		item = list_entry(pos, struct arg_item, list);
 		if (item->conf.command)
 			add_arg_item(arg_head,
-				     xstrdup(token_from_item(item, NULL)),
+				     xstrdup(token_from_conf(&item->conf, NULL)),
 				     xstrdup(""),
 				     &item->conf, NULL);
 	}
@@ -879,7 +879,7 @@ static size_t find_trailer_start(const char *buf, size_t len)
 			list_for_each(pos, &conf_head) {
 				struct arg_item *item;
 				item = list_entry(pos, struct arg_item, list);
-				if (token_matches_item(bol, item,
+				if (token_matches_conf(bol, &item->conf,
 						       separator_pos)) {
 					recognized_prefix = 1;
 					break;
-- 
2.25.1

Re: [PATCH 01/21] trailer: change token_{from,matches}_item into taking conf_info

[Christian Couder]

On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg <anders@0x63.nu> wrote:
>
> ); SAEximRunCond expanded to false

Do you have a way to avoid the above line? It shouldn't be in the
commit message after applying this.

> These functions don't use anything from the arg_item except the conf,
> so make them take conf as argument instead. This will allow them to be
> used on other things that has a conf_info.

s/has/have/

[PATCH 02/21] trailer: don't use 'struct arg_item' for storing config

[Anders Waldenborg]

); SAEximRunCond expanded to false

The '--trailer' options given to 'git interpret-trailers' are store in
the suitably named 'struct arg_item'.

The configuration done in 'trailer.<name>.xyz' was also stored in that
struct. Even though it only needs the "conf_info" part of it.

This commit creates a separate struct for conf_info_item

No functional change intended.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 trailer.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/trailer.c b/trailer.c
index efb88c2008..ca7a823af6 100644
--- a/trailer.c
+++ b/trailer.c
@@ -19,6 +19,11 @@ struct conf_info {
 	enum trailer_if_missing if_missing;
 };
 
+struct conf_info_item {
+	struct list_head list;
+	struct conf_info conf;
+};
+
 static struct conf_info default_conf_info;
 
 struct trailer_item {
@@ -432,16 +437,16 @@ static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
 	dst->command = xstrdup_or_null(src->command);
 }
 
-static struct arg_item *get_conf_item(const char *name)
+static struct conf_info *get_conf_item(const char *name)
 {
 	struct list_head *pos;
-	struct arg_item *item;
+	struct conf_info_item *item;
 
 	/* Look up item with same name */
 	list_for_each(pos, &conf_head) {
-		item = list_entry(pos, struct arg_item, list);
+		item = list_entry(pos, struct conf_info_item, list);
 		if (!strcasecmp(item->conf.name, name))
-			return item;
+			return &item->conf;
 	}
 
 	/* Item does not already exists, create it */
@@ -451,7 +456,7 @@ static struct arg_item *get_conf_item(const char *name)
 
 	list_add_tail(&item->list, &conf_head);
 
-	return item;
+	return &item->conf;
 }
 
 enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_WHERE,
@@ -502,7 +507,6 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v
 static int git_trailer_config(const char *conf_key, const char *value, void *cb)
 {
 	const char *trailer_item, *variable_name;
-	struct arg_item *item;
 	struct conf_info *conf;
 	char *name = NULL;
 	enum trailer_info_type type;
@@ -527,8 +531,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
 	if (!name)
 		return 0;
 
-	item = get_conf_item(name);
-	conf = &item->conf;
+	conf = get_conf_item(name);
 	free(name);
 
 	switch (type) {
@@ -630,7 +633,7 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,
 			 const struct conf_info **conf, const char *trailer,
 			 ssize_t separator_pos)
 {
-	struct arg_item *item;
+	struct conf_info_item *item;
 	size_t tok_len;
 	struct list_head *pos;
 
@@ -649,7 +652,7 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,
 	if (conf)
 		*conf = &default_conf_info;
 	list_for_each(pos, &conf_head) {
-		item = list_entry(pos, struct arg_item, list);
+		item = list_entry(pos, struct conf_info_item, list);
 		if (token_matches_conf(tok->buf, &item->conf, tok_len)) {
 			char *tok_buf = strbuf_detach(tok, NULL);
 			if (conf)
@@ -693,7 +696,7 @@ static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
 static void process_command_line_args(struct list_head *arg_head,
 				      struct list_head *new_trailer_head)
 {
-	struct arg_item *item;
+	struct conf_info_item *item;
 	struct strbuf tok = STRBUF_INIT;
 	struct strbuf val = STRBUF_INIT;
 	const struct conf_info *conf;
@@ -707,7 +710,7 @@ static void process_command_line_args(struct list_head *arg_head,
 
 	/* Add an arg item for each configured trailer with a command */
 	list_for_each(pos, &conf_head) {
-		item = list_entry(pos, struct arg_item, list);
+		item = list_entry(pos, struct conf_info_item, list);
 		if (item->conf.command)
 			add_arg_item(arg_head,
 				     xstrdup(token_from_conf(&item->conf, NULL)),
@@ -877,8 +880,8 @@ static size_t find_trailer_start(const char *buf, size_t len)
 			if (recognized_prefix)
 				continue;
 			list_for_each(pos, &conf_head) {
-				struct arg_item *item;
-				item = list_entry(pos, struct arg_item, list);
+				struct conf_info_item *item;
+				item = list_entry(pos, struct conf_info_item, list);
 				if (token_matches_conf(bol, &item->conf,
 						       separator_pos)) {
 					recognized_prefix = 1;
-- 
2.25.1

[PATCH 03/21] doc: mention canonicalization in git i-t manual

[Anders Waldenborg]

); SAEximRunCond expanded to false

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/git-interpret-trailers.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 96ec6499f0..a4be8aed66 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -25,6 +25,11 @@ Otherwise, this command applies the arguments passed using the
 `--trailer` option, if any, to the commit message part of each input
 file. The result is emitted on the standard output.
 
+When trailers read from input they will be changed into "canonical"
+form if the trailer has a corresponding 'trailer.<token>.key'
+configuration value. This means that it will use the exact spelling
+(upper case vs lower case and separator) defined in configuration.
+
 Some configuration variables control the way the `--trailer` arguments
 are applied to each commit message and the way any existing trailer in
 the commit message is changed. They also make it possible to
-- 
2.25.1

Re: [PATCH 03/21] doc: mention canonicalization in git i-t manual

[Christian Couder]

On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg <anders@0x63.nu> wrote:

> diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
> index 96ec6499f0..a4be8aed66 100644
> --- a/Documentation/git-interpret-trailers.txt
> +++ b/Documentation/git-interpret-trailers.txt
> @@ -25,6 +25,11 @@ Otherwise, this command applies the arguments passed using the
>  `--trailer` option, if any, to the commit message part of each input
>  file. The result is emitted on the standard output.
>
> +When trailers read from input they will be changed into "canonical"

Do you mean "When trailers are read from standard input"?

> +form if the trailer has a corresponding 'trailer.<token>.key'
> +configuration value.

This doesn't explain what the canonical form is. So maybe something like:

"When there is a 'trailer.<token>.key' configuration value defined,
this value becomes the canonical form of the <token> trailer, so when
a trailer matching <token> is read from standard input, it is changed
to this canonical value."

> This means that it will use the exact spelling
> +(upper case vs lower case and separator) defined in configuration.

Maybe:

"This means that the key part of the trailer will use the exact
spelling (upper case vs lower case and separator) defined in the
configuration."

[PATCH 04/21] pretty: allow using aliases in %(trailer:key=xyz)

[Anders Waldenborg]

); SAEximRunCond expanded to false

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/pretty-formats.txt | 4 +++-
 pretty.c                         | 5 ++++-
 t/t4205-log-pretty-formats.sh    | 6 ++++++
 trailer.c                        | 7 +++++--
 trailer.h                        | 2 +-
 5 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 84bbc7439a..1714fa447d 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -256,7 +256,9 @@ endif::git-rev-list[]
 ** 'key=<K>': only show trailers with specified key. Matching is done
    case-insensitively and trailing colon is optional. If option is
    given multiple times trailer lines matching any of the keys are
-   shown. This option automatically enables the `only` option so that
+   shown. If `trailer.<token>.key` configuration option is set 'token'
+   can be used as an alias for showing trailers with the value in
+   key. This option automatically enables the `only` option so that
    non-trailer lines in the trailer block are hidden. If that is not
    desired it can be disabled with `only=false`.  E.g.,
    `%(trailers:key=Reviewed-by)` shows trailer lines with key
diff --git a/pretty.c b/pretty.c
index 7a7708a0ea..3c374abffe 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1135,7 +1135,7 @@ static int match_placeholder_bool_arg(const char *to_parse, const char *candidat
 	return 1;
 }
 
-static int format_trailer_match_cb(const struct strbuf *key, void *ud)
+static int format_trailer_match_cb(const struct strbuf *key, const char *alias, void *ud)
 {
 	const struct string_list *list = ud;
 	const struct string_list_item *item;
@@ -1144,6 +1144,9 @@ static int format_trailer_match_cb(const struct strbuf *key, void *ud)
 		if (key->len == (uintptr_t)item->util &&
 		    !strncasecmp(item->string, key->buf, key->len))
 			return 1;
+		if (alias && strlen(alias) == (uintptr_t)item->util &&
+		    !strncasecmp(item->string, alias, (uintptr_t)item->util))
+			return 1;
 	}
 	return 0;
 }
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 204c149d5a..757575d3f6 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -676,6 +676,12 @@ test_expect_success 'pretty format %(trailers:key=foo) multiple keys' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:key=foo) alias in config' '
+	git -c trailer.ab.key=Acked-by log --no-walk --pretty="format:%(trailers:key=ab)" >actual &&
+	echo "Acked-by: A U Thor <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers:key=nonexistent) becomes empty' '
 	git log --no-walk --pretty="x%(trailers:key=Nacked-by)x" >actual &&
 	echo "xx" >expect &&
diff --git a/trailer.c b/trailer.c
index ca7a823af6..8c0687a529 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1148,8 +1148,11 @@ static void format_trailer_info(struct strbuf *out,
 			struct strbuf tok = STRBUF_INIT;
 			struct strbuf val = STRBUF_INIT;
 
-			parse_trailer(&tok, &val, NULL, trailer, separator_pos);
-			if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
+			const struct conf_info *conf;
+
+			parse_trailer(&tok, &val, &conf, trailer, separator_pos);
+			if (!opts->filter ||
+			    opts->filter(&tok, conf ? conf->name : NULL, opts->filter_data)) {
 				if (opts->unfold)
 					unfold_value(&val);
 
diff --git a/trailer.h b/trailer.h
index cd93e7ddea..b362b0d44d 100644
--- a/trailer.h
+++ b/trailer.h
@@ -73,7 +73,7 @@ struct process_trailer_options {
 	int no_divider;
 	int value_only;
 	const struct strbuf *separator;
-	int (*filter)(const struct strbuf *, void *);
+	int (*filter)(const struct strbuf *, const char *alias, void *);
 	void *filter_data;
 };
 
-- 
2.25.1

Re: [PATCH 04/21] pretty: allow using aliases in %(trailer:key=xyz)

[Christian Couder]

On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg <anders@0x63.nu> wrote:

> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 84bbc7439a..1714fa447d 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -256,7 +256,9 @@ endif::git-rev-list[]
>  ** 'key=<K>': only show trailers with specified key. Matching is done
>     case-insensitively and trailing colon is optional. If option is
>     given multiple times trailer lines matching any of the keys are
> -   shown. This option automatically enables the `only` option so that
> +   shown. If `trailer.<token>.key` configuration option is set 'token'

s/If `trailer.<token>.key`/If the `trailer.<token>.key`/
s/is set 'token'/is set, '<token>'/

> +   can be used as an alias for showing trailers with the value in
> +   key.

Maybe:

"... for showing trailers case insensitively matching the value of
this configuration option."

[PATCH 05/21] trailer: rename 'free_all' to 'free_all_trailer_items'

[Anders Waldenborg]

); SAEximRunCond expanded to false

No functional change intended.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 trailer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/trailer.c b/trailer.c
index 8c0687a529..227df1c0ef 100644
--- a/trailer.c
+++ b/trailer.c
@@ -990,7 +990,7 @@ static size_t process_input_file(FILE *outfile,
 	return info.trailer_end - str;
 }
 
-static void free_all(struct list_head *head)
+static void free_all_trailer_items(struct list_head *head)
 {
 	struct list_head *pos, *p;
 	list_for_each_safe(pos, p, head) {
@@ -1057,7 +1057,7 @@ void process_trailers(const char *file,
 
 	print_all(outfile, &head, opts);
 
-	free_all(&head);
+	free_all_trailer_items(&head);
 
 	/* Print the lines after the trailers as is */
 	if (!opts->only_trailers)
-- 
2.25.1

Re: [PATCH 05/21] trailer: rename 'free_all' to 'free_all_trailer_items'

[Christian Couder]

On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg <anders@0x63.nu> wrote:
>
> ); SAEximRunCond expanded to false

As already mentioned, please find a way to remove the above line in
all your patches.

> No functional change intended.

This doesn't explain much why renaming 'free_all' to
'free_all_trailer_items' is a good idea. Is the function specific to
trailer items or is it generic enough to be useful on other 'struct
list_head *head'?

> Signed-off-by: Anders Waldenborg <anders@0x63.nu>

Re: [PATCH 05/21] trailer: rename 'free_all' to 'free_all_trailer_items'

[Jeff King]

On Mon, Oct 26, 2020 at 01:42:25PM +0100, Christian Couder wrote:

> > No functional change intended.
> 
> This doesn't explain much why renaming 'free_all' to
> 'free_all_trailer_items' is a good idea. Is the function specific to
> trailer items or is it generic enough to be useful on other 'struct
> list_head *head'?

It can't be generic, because list_head needs to use the expected
containing type to find the containing pointer. So free_all() is quite a
bad name, even within trailer.c, because the compiler won't even tell
you if you pass a different list to it.

I do agree this should be spelled out in the commit message, though. :)

-Peff

[PATCH 06/21] t4205: add test for trailer in log with nonstandard separator

[Anders Waldenborg]

); SAEximRunCond expanded to false

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 t/t4205-log-pretty-formats.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 757575d3f6..42544fb07a 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -757,6 +757,18 @@ test_expect_success 'pretty format %(trailers) combining separator/key/valueonly
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers) with nonstandard separator' '
+	git commit --allow-empty -F - <<-\EOF &&
+	Some fix
+
+	Closes #1234
+	EOF
+
+	git -c "trailer.separators=:#" log --no-walk --pretty="format:%s% (trailers:key=Closes)"  >actual &&
+	echo "Some fix Closes: 1234" >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'trailer parsing not fooled by --- line' '
 	git commit --allow-empty -F - <<-\EOF &&
 	this is the subject
-- 
2.25.1

Re: [PATCH 06/21] t4205: add test for trailer in log with nonstandard separator

[Christian Couder]

On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg <anders@0x63.nu> wrote:
>
> ); SAEximRunCond expanded to false
>
> Signed-off-by: Anders Waldenborg <anders@0x63.nu>

Why is this new test important?

Re: [PATCH 06/21] t4205: add test for trailer in log with nonstandard separator

[Anders Waldenborg]

Christian Couder writes:

> On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg <anders@0x63.nu> wrote:
>>
>> ); SAEximRunCond expanded to false

Please disregard this line. It is an unfortunate and most embarrassing
artifact of messed up git send-email and stmp forwarding over ssh. Which
hopefully have been sorted so it doesn't happen next time. It obviously
shouldn't be part of the commit massage in any of the patches in the
series.

>> Signed-off-by: Anders Waldenborg <anders@0x63.nu>
>
> Why is this new test important?

The test that checks that 'git log --pretty=format:%(trailers)' shows
the output in the form "Closes: 1234" even if input was "Closes #1234"
is interesting both because it checks that this behavior is kept intact
in the patches later in the series which modifies handling of separator
and because it is a behavior that can be surprising and not well defined
in documentation and those tend to be the ones that are easiest to
accidentally break. Maybe the addition of the test should come later in
the series where the changes that potentially could break it happen.


It seems like you stopped reviewing my patch series at patch 06/21. That
is IMHO just before it starts to get interesting :)  Now I don't know if
rest of it was rubbish or uninteresting or just there was no time to
look at it.

I've updated according to the suggestions, but not sure if I should
repost the series with just such small adjustments.

Re: [PATCH 06/21] t4205: add test for trailer in log with nonstandard separator

[Christian Couder]

On Mon, Nov 9, 2020 at 11:12 PM Anders Waldenborg <anders@0x63.nu> wrote:
>
>
> Christian Couder writes:
>
> > On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg <anders@0x63.nu> wrote:
> >>
> >> ); SAEximRunCond expanded to false
>
> Please disregard this line. It is an unfortunate and most embarrassing
> artifact of messed up git send-email and stmp forwarding over ssh. Which
> hopefully have been sorted so it doesn't happen next time. It obviously
> shouldn't be part of the commit massage in any of the patches in the
> series.

Ok.

> >> Signed-off-by: Anders Waldenborg <anders@0x63.nu>
> >
> > Why is this new test important?
>
> The test that checks that 'git log --pretty=format:%(trailers)' shows
> the output in the form "Closes: 1234" even if input was "Closes #1234"
> is interesting both because it checks that this behavior is kept intact
> in the patches later in the series which modifies handling of separator
> and because it is a behavior that can be surprising and not well defined
> in documentation and those tend to be the ones that are easiest to
> accidentally break.

Ok, I would suggest adding some of the above in the commit message of
the next version of the patch.

> Maybe the addition of the test should come later in
> the series where the changes that potentially could break it happen.

Maybe. I found the series a bit confusing because it seemed to me that
the cover letter wasn't explaining very well what it does. I just
commented on the cover letter. Hopefully in the next version it will
be better, and it will then be easier to see if patches should be
moved around.

> It seems like you stopped reviewing my patch series at patch 06/21. That
> is IMHO just before it starts to get interesting :)  Now I don't know if
> rest of it was rubbish or uninteresting or just there was no time to
> look at it.

It was a combination of not much time and the cover letter not making
it easy to understand the whole series. I was hoping that the next
version would have more explanations in the cover letter and also in
some commit messages.

> I've updated according to the suggestions, but not sure if I should
> repost the series with just such small adjustments.

I think it's worth reposting with an improved cover letter and other
small adjustments.

Thanks,
Christian.

Re: [PATCH 06/21] t4205: add test for trailer in log with nonstandard separator

[Jeff King]

On Mon, Nov 09, 2020 at 11:12:14PM +0100, Anders Waldenborg wrote:

> > Why is this new test important?
> 
> The test that checks that 'git log --pretty=format:%(trailers)' shows
> the output in the form "Closes: 1234" even if input was "Closes #1234"
> is interesting both because it checks that this behavior is kept intact
> in the patches later in the series which modifies handling of separator
> and because it is a behavior that can be surprising and not well defined
> in documentation and those tend to be the ones that are easiest to
> accidentally break. Maybe the addition of the test should come later in
> the series where the changes that potentially could break it happen.

That makes sense, but should be in the commit message.

I also found the expected output confusing. I thought at first we were
mis-parsing to include part of the subject in the trailer, but it is
just that we put "%s" into the format argument.

> It seems like you stopped reviewing my patch series at patch 06/21. That
> is IMHO just before it starts to get interesting :)  Now I don't know if
> rest of it was rubbish or uninteresting or just there was no time to
> look at it.
> 
> I've updated according to the suggestions, but not sure if I should
> repost the series with just such small adjustments.

Reviewing this has been on my todo list, but I'd just as soon do it from
your latest version. Since it has been a while, it may make sense to
just repost with the fixes, and note in the cover letter that it didn't
get a lot of review yet.

-Peff

[PATCH 07/21] trailer: simplify 'arg_item' lifetime

[Anders Waldenborg]

); SAEximRunCond expanded to false

'struct arg_item' are created from config and '--trailers' arguments
in 'git interpret-trailers'.

Then they were freed as they were processed. This made it harder to
reason about and ensure that all of them were properly freed in all
cases.

This commit extends the lifetime by not doing any freeing during
processing but rather freeing the whole list afterwards. This make it
clearer and will allow keeping a reference to the config stored in the
arg item.

The drawback is that there is extra memory allocation as previously
the strings could be donated to the trailer_item when that is
created. Now they have to be copied.

No functional change intended.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 trailer.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/trailer.c b/trailer.c
index 227df1c0ef..047781463a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -177,13 +177,11 @@ static void print_all(FILE *outfile, struct list_head *head,
 	}
 }
 
-static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
+static struct trailer_item *trailer_from_arg(const struct arg_item *arg_tok)
 {
 	struct trailer_item *new_item = xcalloc(sizeof(*new_item), 1);
-	new_item->token = arg_tok->token;
-	new_item->value = arg_tok->value;
-	arg_tok->token = arg_tok->value = NULL;
-	free_arg_item(arg_tok);
+	new_item->token = xstrdup(arg_tok->token);
+	new_item->value = xstrdup(arg_tok->value);
 	return new_item;
 }
 
@@ -274,7 +272,6 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
 {
 	switch (arg_tok->conf.if_exists) {
 	case EXISTS_DO_NOTHING:
-		free_arg_item(arg_tok);
 		break;
 	case EXISTS_REPLACE:
 		apply_item_command(in_tok, arg_tok);
@@ -290,15 +287,11 @@ static void apply_arg_if_exists(struct trailer_item *in_tok,
 		apply_item_command(in_tok, arg_tok);
 		if (check_if_different(in_tok, arg_tok, 1, head))
 			add_arg_to_input_list(on_tok, arg_tok);
-		else
-			free_arg_item(arg_tok);
 		break;
 	case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
 		apply_item_command(in_tok, arg_tok);
 		if (check_if_different(on_tok, arg_tok, 0, head))
 			add_arg_to_input_list(on_tok, arg_tok);
-		else
-			free_arg_item(arg_tok);
 		break;
 	default:
 		BUG("trailer.c: unhandled value %d",
@@ -314,7 +307,6 @@ static void apply_arg_if_missing(struct list_head *head,
 
 	switch (arg_tok->conf.if_missing) {
 	case MISSING_DO_NOTHING:
-		free_arg_item(arg_tok);
 		break;
 	case MISSING_ADD:
 		where = arg_tok->conf.where;
@@ -364,15 +356,13 @@ static int find_same_and_apply_arg(struct list_head *head,
 static void process_trailers_lists(struct list_head *head,
 				   struct list_head *arg_head)
 {
-	struct list_head *pos, *p;
+	struct list_head *pos;
 	struct arg_item *arg_tok;
 
-	list_for_each_safe(pos, p, arg_head) {
+	list_for_each(pos, arg_head) {
 		int applied = 0;
 		arg_tok = list_entry(pos, struct arg_item, list);
 
-		list_del(pos);
-
 		applied = find_same_and_apply_arg(head, arg_tok);
 
 		if (!applied)
@@ -999,6 +989,15 @@ static void free_all_trailer_items(struct list_head *head)
 	}
 }
 
+static void free_all_arg_items(struct list_head *head)
+{
+	struct list_head *pos, *p;
+	list_for_each_safe(pos, p, head) {
+		list_del(pos);
+		free_arg_item(list_entry(pos, struct arg_item, list));
+	}
+}
+
 static struct tempfile *trailers_tempfile;
 
 static FILE *create_in_place_tempfile(const char *file)
@@ -1035,6 +1034,7 @@ void process_trailers(const char *file,
 		      struct list_head *new_trailer_head)
 {
 	LIST_HEAD(head);
+	LIST_HEAD(arg_head);
 	struct strbuf sb = STRBUF_INIT;
 	size_t trailer_end;
 	FILE *outfile = stdout;
@@ -1050,7 +1050,6 @@ void process_trailers(const char *file,
 	trailer_end = process_input_file(outfile, sb.buf, &head, opts);
 
 	if (!opts->only_input) {
-		LIST_HEAD(arg_head);
 		process_command_line_args(&arg_head, new_trailer_head);
 		process_trailers_lists(&head, &arg_head);
 	}
@@ -1058,6 +1057,7 @@ void process_trailers(const char *file,
 	print_all(outfile, &head, opts);
 
 	free_all_trailer_items(&head);
+	free_all_arg_items(&arg_head);
 
 	/* Print the lines after the trailers as is */
 	if (!opts->only_trailers)
-- 
2.25.1

[PATCH 08/21] trailer: keep track of conf in trailer_item

[Anders Waldenborg]

); SAEximRunCond expanded to false

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 trailer.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/trailer.c b/trailer.c
index 047781463a..0986d4267e 100644
--- a/trailer.c
+++ b/trailer.c
@@ -34,6 +34,7 @@ struct trailer_item {
 	 */
 	char *token;
 	char *value;
+	const struct conf_info *conf;
 };
 
 struct arg_item {
@@ -182,6 +183,7 @@ static struct trailer_item *trailer_from_arg(const struct arg_item *arg_tok)
 	struct trailer_item *new_item = xcalloc(sizeof(*new_item), 1);
 	new_item->token = xstrdup(arg_tok->token);
 	new_item->value = xstrdup(arg_tok->value);
+	new_item->conf = &arg_tok->conf;
 	return new_item;
 }
 
@@ -655,11 +657,12 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,
 }
 
 static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
-					     char *val)
+					     char *val, const struct conf_info *conf)
 {
 	struct trailer_item *new_item = xcalloc(sizeof(*new_item), 1);
 	new_item->token = tok;
 	new_item->value = val;
+	new_item->conf = conf;
 	list_add_tail(&new_item->list, head);
 	return new_item;
 }
@@ -959,19 +962,22 @@ static size_t process_input_file(FILE *outfile,
 			continue;
 		separator_pos = find_separator(trailer, separators);
 		if (separator_pos >= 1) {
-			parse_trailer(&tok, &val, NULL, trailer,
+			const struct conf_info *conf;
+			parse_trailer(&tok, &val, &conf, trailer,
 				      separator_pos);
 			if (opts->unfold)
 				unfold_value(&val);
 			add_trailer_item(head,
 					 strbuf_detach(&tok, NULL),
-					 strbuf_detach(&val, NULL));
+					 strbuf_detach(&val, NULL),
+					 conf);
 		} else if (!opts->only_trailers) {
 			strbuf_addstr(&val, trailer);
 			strbuf_strip_suffix(&val, "\n");
 			add_trailer_item(head,
 					 NULL,
-					 strbuf_detach(&val, NULL));
+					 strbuf_detach(&val, NULL),
+					 NULL);
 		}
 	}
 
-- 
2.25.1

Re: [PATCH 08/21] trailer: keep track of conf in trailer_item

[Jeff King]

On Sun, Oct 25, 2020 at 10:26:39PM +0100, Anders Waldenborg wrote:

> Signed-off-by: Anders Waldenborg <anders@0x63.nu>

For refactoring commits like this, it can help to even give one or two
sentences saying why this moves us in a good direction.

Probably it is obvious to you, who wrote the later commits already, but
in isolation it's hard to say if it is good move direction or not. And
maybe it would become apparent by the time I get to the end, but leading
the reviewer along the string of refactorings is a good way to help them
understand the end state, too. :)

-Peff

[PATCH 09/21] trailer: refactor print_tok_val into taking item

[Anders Waldenborg]

); SAEximRunCond expanded to false

No functional change intended.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 trailer.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/trailer.c b/trailer.c
index 0986d4267e..71921e70ce 100644
--- a/trailer.c
+++ b/trailer.c
@@ -147,22 +147,22 @@ static char last_non_space_char(const char *s)
 	return '\0';
 }
 
-static void print_tok_val(FILE *outfile, const char *tok, const char *val)
+static void print_item(FILE *outfile, const struct trailer_item *item)
 {
 	char c;
 
-	if (!tok) {
-		fprintf(outfile, "%s\n", val);
+	if (!item->token) {
+		fprintf(outfile, "%s\n", item->value);
 		return;
 	}
 
-	c = last_non_space_char(tok);
+	c = last_non_space_char(item->token);
 	if (!c)
 		return;
 	if (strchr(separators, c))
-		fprintf(outfile, "%s%s\n", tok, val);
+		fprintf(outfile, "%s%s\n", item->token, item->value);
 	else
-		fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
+		fprintf(outfile, "%s%c %s\n", item->token, separators[0], item->value);
 }
 
 static void print_all(FILE *outfile, struct list_head *head,
@@ -174,7 +174,7 @@ static void print_all(FILE *outfile, struct list_head *head,
 		item = list_entry(pos, struct trailer_item, list);
 		if ((!opts->trim_empty || strlen(item->value) > 0) &&
 		    (!opts->only_trailers || item->token))
-			print_tok_val(outfile, item->token, item->value);
+			print_item(outfile, item);
 	}
 }
 
-- 
2.25.1

[PATCH 10/21] trailer: move trailer token canonicalization print time

[Anders Waldenborg]

); SAEximRunCond expanded to false

Now that config is stored on the trailer_item it can easily be
accessed print time and the changing of the token into the
configured (canonical) spelling can be done print time instead.

No functional change intended.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 trailer.c | 42 +++++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

diff --git a/trailer.c b/trailer.c
index 71921e70ce..d6882155be 100644
--- a/trailer.c
+++ b/trailer.c
@@ -149,20 +149,24 @@ static char last_non_space_char(const char *s)
 
 static void print_item(FILE *outfile, const struct trailer_item *item)
 {
-	char c;
-
-	if (!item->token) {
-		fprintf(outfile, "%s\n", item->value);
-		return;
+	if (item->token) {
+		const char *tok = item->token;
+		const struct conf_info *conf = item->conf;
+		char c;
+
+		if (conf && conf->key)
+			tok = conf->key;
+
+		c = last_non_space_char(tok);
+		if (!c)
+			return;
+		if (strchr(separators, c))
+			fputs(tok, outfile);
+		else
+			fprintf(outfile, "%s%c ", tok, separators[0]);
 	}
 
-	c = last_non_space_char(item->token);
-	if (!c)
-		return;
-	if (strchr(separators, c))
-		fprintf(outfile, "%s%s\n", item->token, item->value);
-	else
-		fprintf(outfile, "%s%c %s\n", item->token, separators[0], item->value);
+	fprintf(outfile, "%s\n", item->value);
 }
 
 static void print_all(FILE *outfile, struct list_head *head,
@@ -569,15 +573,6 @@ static void ensure_configured(void)
 	configured = 1;
 }
 
-static const char *token_from_conf(const struct conf_info *conf, char *tok)
-{
-	if (conf->key)
-		return conf->key;
-	if (tok)
-		return tok;
-	return conf->name;
-}
-
 static int token_matches_conf(const char *tok, const struct conf_info *conf, size_t tok_len)
 {
 	if (!strncasecmp(tok, conf->name, tok_len))
@@ -646,11 +641,8 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,
 	list_for_each(pos, &conf_head) {
 		item = list_entry(pos, struct conf_info_item, list);
 		if (token_matches_conf(tok->buf, &item->conf, tok_len)) {
-			char *tok_buf = strbuf_detach(tok, NULL);
 			if (conf)
 				*conf = &item->conf;
-			strbuf_addstr(tok, token_from_conf(&item->conf, tok_buf));
-			free(tok_buf);
 			break;
 		}
 	}
@@ -706,7 +698,7 @@ static void process_command_line_args(struct list_head *arg_head,
 		item = list_entry(pos, struct conf_info_item, list);
 		if (item->conf.command)
 			add_arg_item(arg_head,
-				     xstrdup(token_from_conf(&item->conf, NULL)),
+				     xstrdup(item->conf.name),
 				     xstrdup(""),
 				     &item->conf, NULL);
 	}
-- 
2.25.1

[PATCH 11/21] trailer: remember separator used in input

[Anders Waldenborg]

); SAEximRunCond expanded to false

This will in later commits make it easier to allow configuration to
decide if separator should be canonicalized or displayed as it was in
input.

No functional change intended.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 trailer.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/trailer.c b/trailer.c
index d6882155be..1592e6c998 100644
--- a/trailer.c
+++ b/trailer.c
@@ -34,6 +34,7 @@ struct trailer_item {
 	 */
 	char *token;
 	char *value;
+	char *used_separator;
 	const struct conf_info *conf;
 };
 
@@ -125,6 +126,7 @@ static void free_trailer_item(struct trailer_item *item)
 {
 	free(item->token);
 	free(item->value);
+	free(item->used_separator);
 	free(item);
 }
 
@@ -616,18 +618,26 @@ static ssize_t find_separator(const char *line, const char *separators)
  *
  * If separator_pos is -1, interpret the whole trailer as a token.
  */
-static void parse_trailer(struct strbuf *tok, struct strbuf *val,
+static void parse_trailer(struct strbuf *tok, struct strbuf *val, struct strbuf *sep,
 			 const struct conf_info **conf, const char *trailer,
 			 ssize_t separator_pos)
 {
 	struct conf_info_item *item;
-	size_t tok_len;
 	struct list_head *pos;
 
 	if (separator_pos != -1) {
-		strbuf_add(tok, trailer, separator_pos);
-		strbuf_trim(tok);
-		strbuf_addstr(val, trailer + separator_pos + 1);
+		size_t sep_spacing_begin = separator_pos;
+		size_t sep_spacing_end = separator_pos + 1;
+
+		while (sep_spacing_begin > 0 && trailer[sep_spacing_begin - 1] == ' ')
+			sep_spacing_begin--;
+		while (trailer[sep_spacing_end] == ' ')
+			sep_spacing_end++;
+
+		strbuf_add(tok, trailer, sep_spacing_begin);
+		if (sep)
+			strbuf_add(sep, trailer + sep_spacing_begin, sep_spacing_end - sep_spacing_begin);
+		strbuf_addstr(val, trailer + sep_spacing_end);
 		strbuf_trim(val);
 	} else {
 		strbuf_addstr(tok, trailer);
@@ -635,12 +645,11 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,
 	}
 
 	/* Lookup if the token matches something in the config */
-	tok_len = token_len_without_separator(tok->buf, tok->len);
 	if (conf)
 		*conf = &default_conf_info;
 	list_for_each(pos, &conf_head) {
 		item = list_entry(pos, struct conf_info_item, list);
-		if (token_matches_conf(tok->buf, &item->conf, tok_len)) {
+		if (token_matches_conf(tok->buf, &item->conf, tok->len)) {
 			if (conf)
 				*conf = &item->conf;
 			break;
@@ -649,11 +658,12 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val,
 }
 
 static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
-					     char *val, const struct conf_info *conf)
+					     char *val, char *separator, const struct conf_info *conf)
 {
 	struct trailer_item *new_item = xcalloc(sizeof(*new_item), 1);
 	new_item->token = tok;
 	new_item->value = val;
+	new_item->used_separator = separator;
 	new_item->conf = conf;
 	list_add_tail(&new_item->list, head);
 	return new_item;
@@ -717,7 +727,7 @@ static void process_command_line_args(struct list_head *arg_head,
 			      (int) sb.len, sb.buf);
 			strbuf_release(&sb);
 		} else {
-			parse_trailer(&tok, &val, &conf, tr->text,
+			parse_trailer(&tok, &val, NULL, &conf, tr->text,
 				      separator_pos);
 			add_arg_item(arg_head,
 				     strbuf_detach(&tok, NULL),
@@ -936,6 +946,7 @@ static size_t process_input_file(FILE *outfile,
 	struct trailer_info info;
 	struct strbuf tok = STRBUF_INIT;
 	struct strbuf val = STRBUF_INIT;
+	struct strbuf sep = STRBUF_INIT;
 	size_t i;
 
 	trailer_info_get(&info, str, opts);
@@ -955,13 +966,14 @@ static size_t process_input_file(FILE *outfile,
 		separator_pos = find_separator(trailer, separators);
 		if (separator_pos >= 1) {
 			const struct conf_info *conf;
-			parse_trailer(&tok, &val, &conf, trailer,
+			parse_trailer(&tok, &val, &sep, &conf, trailer,
 				      separator_pos);
 			if (opts->unfold)
 				unfold_value(&val);
 			add_trailer_item(head,
 					 strbuf_detach(&tok, NULL),
 					 strbuf_detach(&val, NULL),
+					 strbuf_detach(&sep, NULL),
 					 conf);
 		} else if (!opts->only_trailers) {
 			strbuf_addstr(&val, trailer);
@@ -969,6 +981,7 @@ static size_t process_input_file(FILE *outfile,
 			add_trailer_item(head,
 					 NULL,
 					 strbuf_detach(&val, NULL),
+					 NULL,
 					 NULL);
 		}
 	}
@@ -1148,7 +1161,7 @@ static void format_trailer_info(struct strbuf *out,
 
 			const struct conf_info *conf;
 
-			parse_trailer(&tok, &val, &conf, trailer, separator_pos);
+			parse_trailer(&tok, &val, NULL, &conf, trailer, separator_pos);
 			if (!opts->filter ||
 			    opts->filter(&tok, conf ? conf->name : NULL, opts->filter_data)) {
 				if (opts->unfold)
@@ -1209,7 +1222,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
 
 		strbuf_reset(&iter->key);
 		strbuf_reset(&iter->val);
-		parse_trailer(&iter->key, &iter->val, NULL,
+		parse_trailer(&iter->key, &iter->val, NULL, NULL,
 			      trailer, separator_pos);
 		unfold_value(&iter->val);
 		return 1;
-- 
2.25.1

[PATCH 12/21] trailer: handle configured nondefault separators explicitly

[Anders Waldenborg]

); SAEximRunCond expanded to false

Instead of parsing out separator from configuration when it is
printed, do this parsing when reading the configuration so it can be
stored separately and "conf->key" will contain the actual key only.

No functional change intended.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 trailer.c | 59 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 20 deletions(-)

diff --git a/trailer.c b/trailer.c
index 1592e6c998..102eca0127 100644
--- a/trailer.c
+++ b/trailer.c
@@ -13,6 +13,7 @@
 struct conf_info {
 	char *name;
 	char *key;
+	char *nondefault_separator;
 	char *command;
 	enum trailer_where where;
 	enum trailer_if_exists if_exists;
@@ -140,32 +141,21 @@ static void free_arg_item(struct arg_item *item)
 	free(item);
 }
 
-static char last_non_space_char(const char *s)
-{
-	int i;
-	for (i = strlen(s) - 1; i >= 0; i--)
-		if (!isspace(s[i]))
-			return s[i];
-	return '\0';
-}
-
 static void print_item(FILE *outfile, const struct trailer_item *item)
 {
 	if (item->token) {
 		const char *tok = item->token;
+		const char *sep = (char []){separators[0], ' ', '\0'};
 		const struct conf_info *conf = item->conf;
-		char c;
 
-		if (conf && conf->key)
-			tok = conf->key;
+		if (conf) {
+			if (conf->key)
+				tok = conf->key;
+			if (conf->nondefault_separator)
+				sep = conf->nondefault_separator;
+		}
 
-		c = last_non_space_char(tok);
-		if (!c)
-			return;
-		if (strchr(separators, c))
-			fputs(tok, outfile);
-		else
-			fprintf(outfile, "%s%c ", tok, separators[0]);
+		fprintf(outfile, "%s%s", tok, sep);
 	}
 
 	fprintf(outfile, "%s\n", item->value);
@@ -502,6 +492,34 @@ static int git_trailer_default_config(const char *conf_key, const char *value, v
 	return 0;
 }
 
+static void git_trailer_config_key(const char *conf_key, const char *value, struct conf_info *conf)
+{
+	const char *end = value + strlen(value) - 1;
+
+	while (end > value && isspace(*end))
+		end--;
+
+	if (end == value) {
+		warning(_("Ignoring empty token for key '%s'"), conf_key);
+		return;
+	}
+
+	if (strchr(separators, *end)) {
+		const char *token_end = end - 1;
+		while (token_end > value && isspace(*token_end))
+			token_end--;
+		if (token_end == value) {
+			warning(_("Ignoring empty token for key '%s'"), conf_key);
+			return;
+		}
+
+		conf->key = xstrndup(value, token_end - value + 1);
+		conf->nondefault_separator = xstrdup(token_end + 1);
+	} else {
+		conf->key = xstrdup(value);
+	}
+}
+
 static int git_trailer_config(const char *conf_key, const char *value, void *cb)
 {
 	const char *trailer_item, *variable_name;
@@ -536,7 +554,8 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
 	case TRAILER_KEY:
 		if (conf->key)
 			warning(_("more than one %s"), conf_key);
-		conf->key = xstrdup(value);
+
+		git_trailer_config_key (conf_key, value, conf);
 		break;
 	case TRAILER_COMMAND:
 		if (conf->command)
-- 
2.25.1

Re: [PATCH 12/21] trailer: handle configured nondefault separators explicitly

[Jeff King]

On Sun, Oct 25, 2020 at 10:26:43PM +0100, Anders Waldenborg wrote:

>  static void print_item(FILE *outfile, const struct trailer_item *item)
>  {
>  	if (item->token) {
>  		const char *tok = item->token;
> +		const char *sep = (char []){separators[0], ' ', '\0'};

I don't think this syntax is likely to be sufficiently portable, as
you're defining a variable length array implicitly. I think:

  char orig_sep[] = { separators[0], ' ', '\0' };
  const char *sep = orig_sep;

would work. Though I suspect that just making this:

> -		c = last_non_space_char(tok);
> -		if (!c)
> -			return;
> -		if (strchr(separators, c))
> -			fputs(tok, outfile);
> -		else
> -			fprintf(outfile, "%s%c ", tok, separators[0]);
> +		fprintf(outfile, "%s%s", tok, sep);

into:

  fprintf(outfile, "%s", tok);
  if (conf && conf->nondefault_separator)
	fprintf(outfile, "%s", conf->nondefault_separator);
  else
	fprintf(outfile, "%c ", separators[0]);

might be simpler for a reader to follow, even though it's a little more
verbose.

-Peff

[PATCH 13/21] trailer: add option to make canonicalization optional

[Anders Waldenborg]

); SAEximRunCond expanded to false

Adds a new `--(no-)canonicalize` option to interpret-trailers. By
default it is on unless `--parse` option is given.

When option is on trailer tokens and separators get canonicalized to
the form they have in config (if there is any config for that
trailer). This is same behavior as before this patch, which allows
this behavior to be disabled with `--no-canonicalize`. `--parse` now
also implies `--no-canonicalize`, if previous behavior with
canonicalization also in parse mode is wanted it needs to be combined
with `--parse --canonicalize`

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 Documentation/git-interpret-trailers.txt |  5 ++-
 builtin/interpret-trailers.c             |  3 ++
 t/t7513-interpret-trailers.sh            | 52 ++++++++++++++++++++++++
 trailer.c                                | 10 +++--
 trailer.h                                |  1 +
 5 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index a4be8aed66..a9e6816525 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -129,13 +129,16 @@ OPTIONS
 
 --parse::
 	A convenience alias for `--only-trailers --only-input
-	--unfold`.
+	--unfold --no-canonicalize`.
 
 --no-divider::
 	Do not treat `---` as the end of the commit message. Use this
 	when you know your input contains just the commit message itself
 	(and not an email or the output of `git format-patch`).
 
+--no-canonicalize::
+	Disable canonicalization of input trailers.
+
 CONFIGURATION VARIABLES
 -----------------------
 
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 84748eafc0..51678657a3 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -81,6 +81,7 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
 	v->only_trailers = 1;
 	v->only_input = 1;
 	v->unfold = 1;
+	v->canonicalize = 0;
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 	return 0;
@@ -105,6 +106,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "only-trailers", &opts.only_trailers, N_("output only the trailers")),
 		OPT_BOOL(0, "only-input", &opts.only_input, N_("do not apply config rules")),
 		OPT_BOOL(0, "unfold", &opts.unfold, N_("join whitespace-continued values")),
+		OPT_BOOL(0, "canonicalize", &opts.canonicalize, N_("canonicalize spelling for trailers with config")),
 		OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("set parsing options"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
 		OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat --- specially")),
@@ -112,6 +114,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 				N_("trailer(s) to add"), option_parse_trailer),
 		OPT_END()
 	};
+	opts.canonicalize = 1;
 
 	git_config(git_default_config, NULL);
 
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 6602790b5f..4b3a2484b5 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -99,6 +99,58 @@ test_expect_success 'with config option on the command line' '
 	test_cmp expected actual
 '
 
+test_expect_success 'spelling and separators are canonicalized from configs with key' '
+	cat >patch <<-\EOF &&
+		non-trailer-line
+
+		ReviEweD-bY :abc
+		ReviEwEd-bY) rst
+		ReviEweD-BY ; xyz
+		aCked-bY) only separator gets normalized
+	EOF
+	cat >expected <<-\EOF &&
+		Reviewed-By: abc
+		Reviewed-By: rst
+		Reviewed-By: xyz
+		aCked-bY: only separator gets normalized
+	EOF
+	git \
+		-c "trailer.separators=:);" \
+		-c "trailer.rb.key=Reviewed-By" \
+		-c "trailer.Acked-By.ifmissing=doNothing" \
+		interpret-trailers --only-trailers --only-input patch >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'spelling and separators are not canonicalized with --parse or --no-canonicalize' '
+	cat >patch <<-\EOF &&
+		non-trailer-line
+
+		ReviEweD-bY :abc
+		ReviEwEd-bY) rst
+		ReviEweD-BY ; xyz
+		aCked-bY) not normalized
+	EOF
+	cat >expected <<-\EOF &&
+		ReviEweD-bY :abc
+		ReviEwEd-bY) rst
+		ReviEweD-BY ; xyz
+		aCked-bY) not normalized
+	EOF
+	git \
+		-c "trailer.separators=:);" \
+		-c "trailer.rb.key=Reviewed-By" \
+		-c "trailer.Acked-By.ifmissing=doNothing" \
+		interpret-trailers --parse patch >actual &&
+	test_cmp expected actual &&
+	git \
+		-c "trailer.separators=:);" \
+		-c "trailer.rb.key=Reviewed-By" \
+		-c "trailer.Acked-By.ifmissing=doNothing" \
+		interpret-trailers --only-trailers --only-input --no-canonicalize patch >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'with only a title in the message' '
 	cat >expected <<-\EOF &&
 		area: change
diff --git a/trailer.c b/trailer.c
index 102eca0127..110d3ed226 100644
--- a/trailer.c
+++ b/trailer.c
@@ -141,14 +141,18 @@ static void free_arg_item(struct arg_item *item)
 	free(item);
 }
 
-static void print_item(FILE *outfile, const struct trailer_item *item)
+static void print_item(FILE *outfile, const struct trailer_item *item,
+		       const struct process_trailer_options *opts)
 {
 	if (item->token) {
 		const char *tok = item->token;
 		const char *sep = (char []){separators[0], ' ', '\0'};
 		const struct conf_info *conf = item->conf;
 
-		if (conf) {
+		if (!opts->canonicalize && item->used_separator)
+			sep = item->used_separator;
+
+		if (opts->canonicalize && conf) {
 			if (conf->key)
 				tok = conf->key;
 			if (conf->nondefault_separator)
@@ -170,7 +174,7 @@ static void print_all(FILE *outfile, struct list_head *head,
 		item = list_entry(pos, struct trailer_item, list);
 		if ((!opts->trim_empty || strlen(item->value) > 0) &&
 		    (!opts->only_trailers || item->token))
-			print_item(outfile, item);
+			print_item(outfile, item, opts);
 	}
 }
 
diff --git a/trailer.h b/trailer.h
index b362b0d44d..aad856da8c 100644
--- a/trailer.h
+++ b/trailer.h
@@ -72,6 +72,7 @@ struct process_trailer_options {
 	int unfold;
 	int no_divider;
 	int value_only;
+	int canonicalize;
 	const struct strbuf *separator;
 	int (*filter)(const struct strbuf *, const char *alias, void *);
 	void *filter_data;
-- 
2.25.1

Re: [PATCH 13/21] trailer: add option to make canonicalization optional

[Jeff King]

On Sun, Oct 25, 2020 at 10:26:44PM +0100, Anders Waldenborg wrote:

> Adds a new `--(no-)canonicalize` option to interpret-trailers. By
> default it is on unless `--parse` option is given.
> 
> When option is on trailer tokens and separators get canonicalized to
> the form they have in config (if there is any config for that
> trailer). This is same behavior as before this patch, which allows
> this behavior to be disabled with `--no-canonicalize`. `--parse` now
> also implies `--no-canonicalize`, if previous behavior with
> canonicalization also in parse mode is wanted it needs to be combined
> with `--parse --canonicalize`

I'm not sure if this should be tied to --parse or not. The idea of
--parse is that you'd normalize syntactic issues to make it easy to
parse the result. But wouldn't normalizing names around spelling or
capitalization be what you'd usually want there?

So it sounds like you'd want it to _always_ be on, but leave
"--no-canonicalize" as an escape hatch for somebody who's researching
spelling variants.

Or maybe I'm misunderstanding what it does, since...

> --- a/Documentation/git-interpret-trailers.txt
> +++ b/Documentation/git-interpret-trailers.txt
> @@ -129,13 +129,16 @@ OPTIONS
>  
>  --parse::
>  	A convenience alias for `--only-trailers --only-input
> -	--unfold`.
> +	--unfold --no-canonicalize`.
>  
>  --no-divider::
>  	Do not treat `---` as the end of the commit message. Use this
>  	when you know your input contains just the commit message itself
>  	(and not an email or the output of `git format-patch`).
>  
> +--no-canonicalize::
> +	Disable canonicalization of input trailers.

I think this needs to define "canonicalization" here.

-Peff

[PATCH 14/21] trailer: move skipping of blank lines to own loop when finding trailer

[Anders Waldenborg]

); SAEximRunCond expanded to false

No functional change intended.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 trailer.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/trailer.c b/trailer.c
index 110d3ed226..937cf1edeb 100644
--- a/trailer.c
+++ b/trailer.c
@@ -829,7 +829,6 @@ static size_t find_trailer_start(const char *buf, size_t len)
 {
 	const char *s;
 	ssize_t end_of_title, l;
-	int only_spaces = 1;
 	int recognized_prefix = 0, trailer_lines = 0, non_trailer_lines = 0;
 	/*
 	 * Number of possible continuation lines encountered. This will be
@@ -856,6 +855,12 @@ static size_t find_trailer_start(const char *buf, size_t len)
 	 * consists of at least 25% trailers.
 	 */
 	for (l = last_line(buf, len);
+	     l >= end_of_title;
+	     l = last_line(buf, l)) {
+		if (!is_blank_line(buf + l) && buf[l] != comment_line_char)
+			break;
+	}
+	for (;
 	     l >= end_of_title;
 	     l = last_line(buf, l)) {
 		const char *bol = buf + l;
@@ -868,8 +873,6 @@ static size_t find_trailer_start(const char *buf, size_t len)
 			continue;
 		}
 		if (is_blank_line(bol)) {
-			if (only_spaces)
-				continue;
 			non_trailer_lines += possible_continuation_lines;
 			if (recognized_prefix &&
 			    trailer_lines * 3 >= non_trailer_lines)
@@ -878,7 +881,6 @@ static size_t find_trailer_start(const char *buf, size_t len)
 				return next_line(bol) - buf;
 			return len;
 		}
-		only_spaces = 0;
 
 		for (p = git_generated_prefixes; *p; p++) {
 			if (starts_with(bol, *p)) {
-- 
2.25.1

[PATCH 15/21] trailer: factor out classify_trailer_line

[Anders Waldenborg]

); SAEximRunCond expanded to false

No functional change intended.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 trailer.c | 123 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 74 insertions(+), 49 deletions(-)

diff --git a/trailer.c b/trailer.c
index 937cf1edeb..21877e4c06 100644
--- a/trailer.c
+++ b/trailer.c
@@ -821,6 +821,53 @@ static size_t find_patch_start(const char *str)
 	return s - str;
 }
 
+enum trailer_classification {
+	GIT_GENERATED_PREFIX,
+	CONFIGURED_TRAILER,
+	TRAILER,
+	CONTINUATION,
+	NON_TRAILER,
+	COMMENT,
+	BLANK,
+};
+
+static enum trailer_classification classify_trailer_line(const char *line)
+{
+	const char **p;
+	ssize_t separator_pos;
+
+	if (line[0] == comment_line_char)
+		return COMMENT;
+
+	if (is_blank_line(line))
+		return BLANK;
+
+	if (isspace(line[0]))
+		return CONTINUATION;
+
+	for (p = git_generated_prefixes; *p; p++)
+		if (starts_with(line, *p))
+			return GIT_GENERATED_PREFIX;
+
+
+	separator_pos = find_separator(line, separators);
+	if (separator_pos >= 1) {
+		struct list_head *pos;
+
+		list_for_each(pos, &conf_head) {
+			struct conf_info_item *item;
+			item = list_entry(pos, struct conf_info_item, list);
+			if (token_matches_conf(line, &item->conf,
+			                       separator_pos))
+				return CONFIGURED_TRAILER;
+		}
+
+		return TRAILER;
+	}
+
+	return NON_TRAILER;
+}
+
 /*
  * Return the position of the first trailer line or len if there are no
  * trailers.
@@ -864,59 +911,37 @@ static size_t find_trailer_start(const char *buf, size_t len)
 	     l >= end_of_title;
 	     l = last_line(buf, l)) {
 		const char *bol = buf + l;
-		const char **p;
-		ssize_t separator_pos;
 
-		if (bol[0] == comment_line_char) {
-			non_trailer_lines += possible_continuation_lines;
-			possible_continuation_lines = 0;
-			continue;
-		}
-		if (is_blank_line(bol)) {
-			non_trailer_lines += possible_continuation_lines;
-			if (recognized_prefix &&
-			    trailer_lines * 3 >= non_trailer_lines)
-				return next_line(bol) - buf;
-			else if (trailer_lines && !non_trailer_lines)
-				return next_line(bol) - buf;
-			return len;
-		}
-
-		for (p = git_generated_prefixes; *p; p++) {
-			if (starts_with(bol, *p)) {
+		switch (classify_trailer_line(bol)) {
+			case GIT_GENERATED_PREFIX:
+			case CONFIGURED_TRAILER:
+				recognized_prefix = 1;
+				/* fallthrough */
+			case TRAILER:
 				trailer_lines++;
 				possible_continuation_lines = 0;
-				recognized_prefix = 1;
-				goto continue_outer_loop;
-			}
-		}
-
-		separator_pos = find_separator(bol, separators);
-		if (separator_pos >= 1 && !isspace(bol[0])) {
-			struct list_head *pos;
-
-			trailer_lines++;
-			possible_continuation_lines = 0;
-			if (recognized_prefix)
-				continue;
-			list_for_each(pos, &conf_head) {
-				struct conf_info_item *item;
-				item = list_entry(pos, struct conf_info_item, list);
-				if (token_matches_conf(bol, &item->conf,
-						       separator_pos)) {
-					recognized_prefix = 1;
-					break;
-				}
-			}
-		} else if (isspace(bol[0]))
-			possible_continuation_lines++;
-		else {
-			non_trailer_lines++;
-			non_trailer_lines += possible_continuation_lines;
-			possible_continuation_lines = 0;
+				break;
+			case CONTINUATION:
+				possible_continuation_lines++;
+				break;
+			case NON_TRAILER:
+				non_trailer_lines++;
+				non_trailer_lines += possible_continuation_lines;
+				possible_continuation_lines = 0;
+				break;
+			case COMMENT:
+				non_trailer_lines += possible_continuation_lines;
+				possible_continuation_lines = 0;
+				break;
+			case BLANK:
+				non_trailer_lines += possible_continuation_lines;
+				if (recognized_prefix &&
+					trailer_lines * 3 >= non_trailer_lines)
+					return next_line(bol) - buf;
+				else if (trailer_lines && !non_trailer_lines)
+					return next_line(bol) - buf;
+				return len;
 		}
-continue_outer_loop:
-		;
 	}
 
 	return len;
-- 
2.25.1

[PATCH 16/21] t7513: add failing test for configured trailing line classification

[Anders Waldenborg]

); SAEximRunCond expanded to false

This testcases shows why prefix matching shouldn't be used when using
configured trailers to classify lines as trailers or not.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 t/t7513-interpret-trailers.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 4b3a2484b5..b1e9a9e6d1 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -239,6 +239,35 @@ test_expect_success 'with non-trailer lines mixed with a configured trailer' '
 	test_cmp expected actual
 '
 
+# This fails because "c:/windows/tmp/stuff/temp.txt" is classified as
+# a trailer line because "c" is a prefix of "Confirmed-By". Therefore
+# the new trailer is appended to that (non-trailer) block rather than
+# creating a new block. It also canonicalize the "trailer" to
+# "Confirmed-By: /windows/tmp/stuff/temp.txt"
+test_expect_failure 'with non-trailer lines mixed with prefix of configured trailer' '
+	cat >patch <<-\EOF &&
+		some subject
+
+		This is clearly not a trailer line. But
+		on next line there is a a windows path
+		c:/windows/tmp/stuff/temp.txt but that
+		should not make this classify as a trailer block
+	EOF
+	cat >expected <<-\EOF &&
+		some subject
+
+		This is clearly not a trailer line. But
+		on next line there is a a windows path
+		c:/windows/tmp/stuff/temp.txt but that
+		should not make this classify as a trailer block
+
+		t: v
+	EOF
+	test_config trailer.confirmedby.key "Confirmed-By" &&
+	git interpret-trailers --trailer "t: v" patch >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'with non-trailer lines mixed with a non-configured trailer' '
 	cat >patch <<-\EOF &&
 
-- 
2.25.1

[PATCH 17/21] trailer: don't treat line with prefix of known trailer as known

[Anders Waldenborg]

); SAEximRunCond expanded to false

E.g if "Closes" is a configured trailer a line starting with "c:"
shouldn't be treated as a recognized trailer when looking for trailer
block.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 t/t7513-interpret-trailers.sh |  7 +------
 trailer.c                     | 28 ++++++++++++++++++----------
 2 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index b1e9a9e6d1..6ddc2f5573 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -239,12 +239,7 @@ test_expect_success 'with non-trailer lines mixed with a configured trailer' '
 	test_cmp expected actual
 '
 
-# This fails because "c:/windows/tmp/stuff/temp.txt" is classified as
-# a trailer line because "c" is a prefix of "Confirmed-By". Therefore
-# the new trailer is appended to that (non-trailer) block rather than
-# creating a new block. It also canonicalize the "trailer" to
-# "Confirmed-By: /windows/tmp/stuff/temp.txt"
-test_expect_failure 'with non-trailer lines mixed with prefix of configured trailer' '
+test_expect_success 'with non-trailer lines mixed with prefix of configured trailer' '
 	cat >patch <<-\EOF &&
 		some subject
 
diff --git a/trailer.c b/trailer.c
index 21877e4c06..d75d240e10 100644
--- a/trailer.c
+++ b/trailer.c
@@ -831,10 +831,20 @@ enum trailer_classification {
 	BLANK,
 };
 
+static int starts_with_separator(const char *buf)
+{
+	while (*buf == ' ' || *buf == '\t')
+		buf++;
+	if (!*buf)
+		return 0;
+	return !!strchr(separators, *buf);
+}
+
 static enum trailer_classification classify_trailer_line(const char *line)
 {
 	const char **p;
 	ssize_t separator_pos;
+	struct list_head *pos;
 
 	if (line[0] == comment_line_char)
 		return COMMENT;
@@ -849,19 +859,17 @@ static enum trailer_classification classify_trailer_line(const char *line)
 		if (starts_with(line, *p))
 			return GIT_GENERATED_PREFIX;
 
-
-	separator_pos = find_separator(line, separators);
-	if (separator_pos >= 1) {
-		struct list_head *pos;
-
-		list_for_each(pos, &conf_head) {
-			struct conf_info_item *item;
-			item = list_entry(pos, struct conf_info_item, list);
-			if (token_matches_conf(line, &item->conf,
-			                       separator_pos))
+	list_for_each(pos, &conf_head) {
+		struct conf_info_item *item = list_entry(pos, struct conf_info_item, list);
+		const char *conftrailer = item->conf.key ? item->conf.key : item->conf.name;
+		if (istarts_with(line, conftrailer)) {
+			if (starts_with_separator (line + strlen (conftrailer)))
 				return CONFIGURED_TRAILER;
 		}
+	}
 
+	separator_pos = find_separator(line, separators);
+	if (separator_pos >= 1) {
 		return TRAILER;
 	}
 
-- 
2.25.1

Re: [PATCH 17/21] trailer: don't treat line with prefix of known trailer as known

[Jeff King]

On Sun, Oct 25, 2020 at 10:26:48PM +0100, Anders Waldenborg wrote:

> E.g if "Closes" is a configured trailer a line starting with "c:"
> shouldn't be treated as a recognized trailer when looking for trailer
> block.

Would this mean that:

  Signed-off:

is no longer an alias for:

  Signed-off-by:

? TBH that seems overall much more sane to me, but I wonder if it is
breaking somebody's expectations.

-Peff

[PATCH 18/21] trailer: factor out config lookup to separate function

[Anders Waldenborg]

); SAEximRunCond expanded to false

No functional change intended.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 trailer.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/trailer.c b/trailer.c
index d75d240e10..02061877b4 100644
--- a/trailer.c
+++ b/trailer.c
@@ -605,6 +605,20 @@ static int token_matches_conf(const char *tok, const struct conf_info *conf, siz
 	return conf->key ? !strncasecmp(tok, conf->key, tok_len) : 0;
 }
 
+static const struct conf_info *lookup_conf_for_tok(const struct strbuf *tok)
+{
+	struct conf_info_item *item;
+	struct list_head *pos;
+
+	list_for_each(pos, &conf_head) {
+		item = list_entry(pos, struct conf_info_item, list);
+		if (token_matches_conf(tok->buf, &item->conf, tok->len)) {
+			return &item->conf;
+		}
+	}
+	return &default_conf_info;
+}
+
 /*
  * If the given line is of the form
  * "<token><optional whitespace><separator>..." or "<separator>...", return the
@@ -645,9 +659,6 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val, struct strbuf
 			 const struct conf_info **conf, const char *trailer,
 			 ssize_t separator_pos)
 {
-	struct conf_info_item *item;
-	struct list_head *pos;
-
 	if (separator_pos != -1) {
 		size_t sep_spacing_begin = separator_pos;
 		size_t sep_spacing_end = separator_pos + 1;
@@ -667,17 +678,8 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val, struct strbuf
 		strbuf_trim(tok);
 	}
 
-	/* Lookup if the token matches something in the config */
 	if (conf)
-		*conf = &default_conf_info;
-	list_for_each(pos, &conf_head) {
-		item = list_entry(pos, struct conf_info_item, list);
-		if (token_matches_conf(tok->buf, &item->conf, tok->len)) {
-			if (conf)
-				*conf = &item->conf;
-			break;
-		}
-	}
+		*conf = lookup_conf_for_tok (tok);
 }
 
 static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
-- 
2.25.1

[PATCH 19/21] trailer: move config lookup out of parse_trailer

[Anders Waldenborg]

); SAEximRunCond expanded to false

This may be seen as making it worse adding code duplication. But will
hopefully make different handling for config lookups easier.

No functional change intended.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 trailer.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/trailer.c b/trailer.c
index 02061877b4..0db3bba3b1 100644
--- a/trailer.c
+++ b/trailer.c
@@ -656,8 +656,7 @@ static ssize_t find_separator(const char *line, const char *separators)
  * If separator_pos is -1, interpret the whole trailer as a token.
  */
 static void parse_trailer(struct strbuf *tok, struct strbuf *val, struct strbuf *sep,
-			 const struct conf_info **conf, const char *trailer,
-			 ssize_t separator_pos)
+			 const char *trailer, ssize_t separator_pos)
 {
 	if (separator_pos != -1) {
 		size_t sep_spacing_begin = separator_pos;
@@ -677,9 +676,6 @@ static void parse_trailer(struct strbuf *tok, struct strbuf *val, struct strbuf
 		strbuf_addstr(tok, trailer);
 		strbuf_trim(tok);
 	}
-
-	if (conf)
-		*conf = lookup_conf_for_tok (tok);
 }
 
 static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
@@ -752,8 +748,9 @@ static void process_command_line_args(struct list_head *arg_head,
 			      (int) sb.len, sb.buf);
 			strbuf_release(&sb);
 		} else {
-			parse_trailer(&tok, &val, NULL, &conf, tr->text,
+			parse_trailer(&tok, &val, NULL, tr->text,
 				      separator_pos);
+			conf = lookup_conf_for_tok(&tok);
 			add_arg_item(arg_head,
 				     strbuf_detach(&tok, NULL),
 				     strbuf_detach(&val, NULL),
@@ -1026,8 +1023,9 @@ static size_t process_input_file(FILE *outfile,
 		separator_pos = find_separator(trailer, separators);
 		if (separator_pos >= 1) {
 			const struct conf_info *conf;
-			parse_trailer(&tok, &val, &sep, &conf, trailer,
+			parse_trailer(&tok, &val, &sep, trailer,
 				      separator_pos);
+			conf = lookup_conf_for_tok(&tok);
 			if (opts->unfold)
 				unfold_value(&val);
 			add_trailer_item(head,
@@ -1221,7 +1219,8 @@ static void format_trailer_info(struct strbuf *out,
 
 			const struct conf_info *conf;
 
-			parse_trailer(&tok, &val, NULL, &conf, trailer, separator_pos);
+			parse_trailer(&tok, &val, NULL, trailer, separator_pos);
+			conf = lookup_conf_for_tok(&tok);
 			if (!opts->filter ||
 			    opts->filter(&tok, conf ? conf->name : NULL, opts->filter_data)) {
 				if (opts->unfold)
@@ -1282,7 +1281,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
 
 		strbuf_reset(&iter->key);
 		strbuf_reset(&iter->val);
-		parse_trailer(&iter->key, &iter->val, NULL, NULL,
+		parse_trailer(&iter->key, &iter->val, NULL,
 			      trailer, separator_pos);
 		unfold_value(&iter->val);
 		return 1;
-- 
2.25.1

[PATCH 20/21] trailer: add failing tests for matching trailers against input

[Anders Waldenborg]

); SAEximRunCond expanded to false

These tests shows problematic cases where input trailers matches
config.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 t/t7513-interpret-trailers.sh | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 6ddc2f5573..a99d6d7e3b 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -151,6 +151,41 @@ test_expect_success 'spelling and separators are not canonicalized with --parse
 	test_cmp expected actual
 '
 
+# Matching currently is prefix matching, causing "This-trailer" to be normalized too
+test_expect_failure 'config option matches exact only' '
+	cat >patch <<-\EOF &&
+
+		This-trailer: a
+		 b
+		This-trailer-exact: b
+		 c
+		This-trailer-exact-plus-some: c
+		 d
+	EOF
+	cat >expected <<-\EOF &&
+		This-trailer: a b
+		THIS-TRAILER-EXACT: b c
+		This-trailer-exact-plus-some: c d
+	EOF
+	git -c "trailer.tte.key=THIS-TRAILER-EXACT" interpret-trailers --only-input --only-trailers --unfold patch >actual &&
+	test_cmp expected actual
+'
+
+# Matching currently uses the config key even if key value is different
+test_expect_failure 'config option matches exact only' '
+	cat >patch <<-\EOF &&
+
+		Ticket: 1234
+		Reference-ticket: 99
+	EOF
+	cat >expected <<-\EOF &&
+		Ticket: 1234
+		Reference-Ticket: 99
+	EOF
+	git -c "trailer.ticket.key=Reference-Ticket" interpret-trailers --only-input --only-trailers patch >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'with only a title in the message' '
 	cat >expected <<-\EOF &&
 		area: change
-- 
2.25.1

[PATCH 21/21] trailer: only do prefix matching for configured trailers on commandline

[Anders Waldenborg]

); SAEximRunCond expanded to false

If there is a trailer "foobar" in configuration a trailer "foo" in
input shouldn't match that, except in `--trailer` arguments as a
shortcut.

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 t/t7513-interpret-trailers.sh | 17 +++++++++++++----
 trailer.c                     | 14 +++++++++-----
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index a99d6d7e3b..9e06fa4454 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -151,8 +151,7 @@ test_expect_success 'spelling and separators are not canonicalized with --parse
 	test_cmp expected actual
 '
 
-# Matching currently is prefix matching, causing "This-trailer" to be normalized too
-test_expect_failure 'config option matches exact only' '
+test_expect_success 'config option matches exact only' '
 	cat >patch <<-\EOF &&
 
 		This-trailer: a
@@ -171,8 +170,7 @@ test_expect_failure 'config option matches exact only' '
 	test_cmp expected actual
 '
 
-# Matching currently uses the config key even if key value is different
-test_expect_failure 'config option matches exact only' '
+test_expect_success 'config option matches exact only' '
 	cat >patch <<-\EOF &&
 
 		Ticket: 1234
@@ -550,6 +548,17 @@ test_expect_success 'with config setup' '
 	test_cmp expected actual
 '
 
+test_expect_success 'trailer on commandline can be prefix of configured' '
+	cat >expected <<-\EOF &&
+
+		Acked-by: 10
+	EOF
+	git interpret-trailers --trailer "A=10" empty >actual &&
+	test_cmp expected actual
+'
+
+
+
 test_expect_success 'with config setup and ":=" as separators' '
 	git config trailer.separators ":=" &&
 	git config trailer.ack.key "Acked-by= " &&
diff --git a/trailer.c b/trailer.c
index 0db3bba3b1..b00b35ea0e 100644
--- a/trailer.c
+++ b/trailer.c
@@ -605,14 +605,18 @@ static int token_matches_conf(const char *tok, const struct conf_info *conf, siz
 	return conf->key ? !strncasecmp(tok, conf->key, tok_len) : 0;
 }
 
-static const struct conf_info *lookup_conf_for_tok(const struct strbuf *tok)
+static const struct conf_info *lookup_conf_for_tok(const struct strbuf *tok, int strict)
 {
 	struct conf_info_item *item;
 	struct list_head *pos;
 
 	list_for_each(pos, &conf_head) {
 		item = list_entry(pos, struct conf_info_item, list);
-		if (token_matches_conf(tok->buf, &item->conf, tok->len)) {
+		if (strict) {
+			const char *match = item->conf.key ? item->conf.key : item->conf.name;
+			if (!strcasecmp(match, tok->buf))
+				return &item->conf;
+		} else if (token_matches_conf(tok->buf, &item->conf, tok->len)) {
 			return &item->conf;
 		}
 	}
@@ -750,7 +754,7 @@ static void process_command_line_args(struct list_head *arg_head,
 		} else {
 			parse_trailer(&tok, &val, NULL, tr->text,
 				      separator_pos);
-			conf = lookup_conf_for_tok(&tok);
+			conf = lookup_conf_for_tok(&tok, 0);
 			add_arg_item(arg_head,
 				     strbuf_detach(&tok, NULL),
 				     strbuf_detach(&val, NULL),
@@ -1025,7 +1029,7 @@ static size_t process_input_file(FILE *outfile,
 			const struct conf_info *conf;
 			parse_trailer(&tok, &val, &sep, trailer,
 				      separator_pos);
-			conf = lookup_conf_for_tok(&tok);
+			conf = lookup_conf_for_tok(&tok, 1);
 			if (opts->unfold)
 				unfold_value(&val);
 			add_trailer_item(head,
@@ -1220,7 +1224,7 @@ static void format_trailer_info(struct strbuf *out,
 			const struct conf_info *conf;
 
 			parse_trailer(&tok, &val, NULL, trailer, separator_pos);
-			conf = lookup_conf_for_tok(&tok);
+			conf = lookup_conf_for_tok(&tok, 1);
 			if (!opts->filter ||
 			    opts->filter(&tok, conf ? conf->name : NULL, opts->filter_data)) {
 				if (opts->unfold)
-- 
2.25.1

Re: [PATCH 00/21] trailer fixes

[Christian Couder]

On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg <anders@0x63.nu> wrote:
>
>
> This patch series contains a bunch fo trailer related changes. Sparked

s/fo/of/

> from this thread:
>   https://public-inbox.org/git/87blk0rjob.fsf@0x63.nu/T/#r3dc3e4fa67b6fba95e4b2ea2c1cf1672af55a9ee

Ok.

> Most commits are refactors preparing for the others, the actual user
> visible changes are:
>  * Allow using aliases in pretty formatting '%(trailer:key=foo)`
>  * Fixes related to matching prefix rather than full trailer
>  * Tighten up "canonicalization" of trailers
>  * Add --(no-)canonicalize

It's not easy to see which patch(es)/commit(s) correspond to which
change. Maybe you could add numbers like 4/21, 5/21, etc after each of
the above visible changes. Or you could say for example "patches 1/21
to 4/21 are doing this, then patch 5/21 is doing this, then patches
6/21 to 9/21 are doing something else" so it would help us have a
better overview of the series.

Thanks for working on this!

[PATCH 0/5] pretty format %(trailers): improve machine readability

[Ævar Arnfjörð Bjarmason]

I started writing this on top of "master", but then saw the
outstanding series of other miscellaneous fixes to this
facility[1]. This is on top of that topic & rebased on master.

Anders, any plans to re-roll yours? Otherwise the conflicts I'd have
on mine are easy to fix, so I can also submit it as a stand-alone.

This series comes out of a discussion at work today (well, yesterday
at this point) where someone wanted to parse %(trailers) output. As
noted in 3/5 doing this is rather tedious now if you're trying to
unambiguously grap trailers as a stream of key-value pairs.

So this series adds a "key_value_separator" and "keyonly" parameters,
and fixes a few bugs I saw along the way.

1. https://lore.kernel.org/git/20201025212652.3003036-1-anders@0x63.nu/

Ævar Arnfjörð Bjarmason (5):
  pretty format %(trailers) test: split a long line
  pretty format %(trailers): avoid needless repetition
  pretty format %(trailers): add a "keyonly"
  pretty-format %(trailers): fix broken standalone "valueonly"
  pretty format %(trailers): add a "key_value_separator"

 Documentation/pretty-formats.txt | 33 ++++++++-------
 pretty.c                         | 12 ++++++
 t/t4205-log-pretty-formats.sh    | 71 +++++++++++++++++++++++++++++++-
 trailer.c                        | 14 +++++--
 trailer.h                        |  3 ++
 5 files changed, 113 insertions(+), 20 deletions(-)

-- 
2.29.2.222.g5d2a92d10f8

Re: [PATCH 0/5] pretty format %(trailers): improve machine readability

[Anders Waldenborg]

Ævar Arnfjörð Bjarmason writes:

> I started writing this on top of "master", but then saw the
> outstanding series of other miscellaneous fixes to this
> facility[1]. This is on top of that topic & rebased on master.
>
> Anders, any plans to re-roll yours? Otherwise the conflicts I'd have
> on mine are easy to fix, so I can also submit it as a stand-alone.

Yes, I have plans to do that. But have yet to carve out the required
time from my copious spare time to actually do it.

So please don't hold your breath waiting for me to do that.

> This series comes out of a discussion at work today (well, yesterday
> at this point) where someone wanted to parse %(trailers) output. As
> noted in 3/5 doing this is rather tedious now if you're trying to
> unambiguously grap trailers as a stream of key-value pairs.
>
> So this series adds a "key_value_separator" and "keyonly" parameters,
> and fixes a few bugs I saw along the way.

Interesting. When adding "valueonly" I never consider it being used
without "key". The trick you are doing with separate keyonly and
valueonly is quite clever.

I've only been doing machine parsing for explicit keys, things like:
"%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)"
(double-NUL to separate field, single-NUL to separate values within field).

But I can't help wonder that if the goal just is to have a nice machine
parsable format maybe it would be easier (both for user and
implementation) to have a separate placeholder for "machine readable
trailers" which by default emits in a format suitable for machine
parsing. Something like a new "%(ztrailers)" (but with a better name)
which simply emits a sequence of "<KEY> NUL <VAL> NUL" for each trailer.

Re: [PATCH 0/5] pretty format %(trailers): improve machine readability

[Ævar Arnfjörð Bjarmason]

On Sat, Dec 05 2020, Anders Waldenborg wrote:

> Ævar Arnfjörð Bjarmason writes:
>
>> I started writing this on top of "master", but then saw the
>> outstanding series of other miscellaneous fixes to this
>> facility[1]. This is on top of that topic & rebased on master.
>>
>> Anders, any plans to re-roll yours? Otherwise the conflicts I'd have
>> on mine are easy to fix, so I can also submit it as a stand-alone.
>
> Yes, I have plans to do that. But have yet to carve out the required
> time from my copious spare time to actually do it.
>
> So please don't hold your breath waiting for me to do that.

Thanks. I sent a v2 of mine yesterday as
https://lore.kernel.org/git/20201206002449.31452-1-avarab@gmail.com/

As noted there the merge conflict with yours is trivial, so hopefully it
won't cause you much hassle if you re-roll while it's outstanding.

>> This series comes out of a discussion at work today (well, yesterday
>> at this point) where someone wanted to parse %(trailers) output. As
>> noted in 3/5 doing this is rather tedious now if you're trying to
>> unambiguously grap trailers as a stream of key-value pairs.
>>
>> So this series adds a "key_value_separator" and "keyonly" parameters,
>> and fixes a few bugs I saw along the way.
>
> Interesting. When adding "valueonly" I never consider it being used
> without "key". The trick you are doing with separate keyonly and
> valueonly is quite clever.
>
> I've only been doing machine parsing for explicit keys, things like:
> "%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)"
> (double-NUL to separate field, single-NUL to separate values within field).
>
> But I can't help wonder that if the goal just is to have a nice machine
> parsable format maybe it would be easier (both for user and
> implementation) to have a separate placeholder for "machine readable
> trailers" which by default emits in a format suitable for machine
> parsing. Something like a new "%(ztrailers)" (but with a better name)
> which simply emits a sequence of "<KEY> NUL <VAL> NUL" for each trailer

I think it's a bit tricky to make something general in the middle of all
the custom format printf-likes in the pretty format. E.g. some users
might want to use \0 as a delimiter for key-values, others \0\0
etc. because they used \0, or the other way around.

Maybe if there's a reason to extend the optimization it could be smarter
about detecting that you only wanted some fixed-string separator and
nothing else custom?

B.t.w. I tried just deleting the optimization for testing and it slowed
down by around 8% on linux.git according to an extended
p4205-log-pretty-formats.sh.

Looking at the code I wonder if there aren't other lower hanging
optimizations, e.g. it seems we call find_separator() on multiple passes
instead of saving it away, e.g. in the format_trailers_from_commit()
entry point if there's any custom options such as "unfold".

I also wonder if memory allocation is a bottleneck in the "git log"
path, but didn't have time to refactor & test it. For each commit the
walking machinery eventually calls the trailer.c code, which allocates &
free()'s internal structures that could be re-used for parsing the next
commit.

[PATCH v2 0/5] pretty format %(trailers): improve machine readability

[Ævar Arnfjörð Bjarmason]

Per Anders's feedback in [1] this is now a stand-alone series based on
"master". The merge conflict with his series is trivial, both add or
change struct member(s) in trailer.h.

v2 changes:

 * Either incorporate all the feedback from Christian Couder on v1, or
   changed things around to e.g. eliminate the relevant code.

 * Drop the whole "have_options" approach. I saw it conflicted with
   Hariom Verma's cleanup of ref-filter.c in a bad way. I also wasn't
   running the for-each-ref tests in v1 (just interpret-trailers &
   log/pretty). Ran every commit in v2 with the whole test suite (as I
   should have done to begin with, sorry).

 * I'd accidentally left DEVOPTS=no-error on in v1 and didn't notice a
   missing struct member warning. Oops.

 * Lots of my own commit message rewording/cleanup/grammar fixes.

Ævar Arnfjörð Bjarmason (5):
  pretty format %(trailers) test: split a long line
  pretty format %(trailers) doc: avoid repetition
  pretty-format %(trailers): fix broken standalone "valueonly"
  pretty format %(trailers): add a "keyonly"
  pretty format %(trailers): add a "key_value_separator"

 Documentation/pretty-formats.txt | 34 ++++++-----
 pretty.c                         | 10 ++++
 t/t4205-log-pretty-formats.sh    | 99 +++++++++++++++++++++++++++++++-
 trailer.c                        | 15 ++++-
 trailer.h                        |  2 +
 5 files changed, 141 insertions(+), 19 deletions(-)

Range-diff:
1:  51a7a6d8cfe ! 1:  4b134a62aec pretty format %(trailers) test: split a long line
    @@ Commit message
     
         Split a very long line in a test introduced in 0b691d86851 (pretty:
         add support for separator option in %(trailers), 2019-01-28). This
    -    makes it easier to read, and it'll be used as a template in follow-up
    -    commits.
    +    makes it easier to read, especially as follow-up commits will copy
    +    this test as a template.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
2:  2340d856a90 ! 2:  0d3fe6daf6c pretty format %(trailers): avoid needless repetition
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    pretty format %(trailers): avoid needless repetition
    +    pretty format %(trailers) doc: avoid repetition
     
         Change the documentation for the various %(trailers) options so it
         isn't repeating part of the documentation for "only" about how boolean
    -    values are handled. Instead let's split the description of that into
    +    values are handled. Instead, let's split the description of that into
         general documentation at the top.
     
         It then suffices to refer to it by listing the options as
    -    "opt[=bool]". I'm also changing it to "[=bool]" from "[=val]". It took
    -    me a couple of readings to realize that while to realize that these
    -    options were referring back to the "only" option's treatment of
    -    boolean values. Let's try to make this more explicit.
    +    "opt[=<BOOL>]". I'm also changing it to upper-case "[=<BOOL>]" from
    +    "[=val]" for consistency with "<SEP>"
    +
    +    It took me a couple of readings to realize that these options were
    +    referring back to the "only" option's treatment of boolean
    +    values. Let's try to make this more explicit, and upper-case "BOOL"
    +    for consistency with the existing "<SEP>" and "<K>".
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ Documentation/pretty-formats.txt: endif::git-rev-list[]
      			  `trailers` string may be followed by a colon
     -			  and zero or more comma-separated options:
     +			  and zero or more comma-separated options.
    ++			  If any option is provided multiple times the
    ++			  last occurance wins.
     ++
    -+The boolean options accept an optional value. The values `true`,
    -+`false`, `on`, `off` etc. are all accepted. See the "boolean"
    ++The boolean options accept an optional value `[=<BOOL>]`. The values
    ++`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
     +sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
    -+option is given with no value it's enabled. If any option is provided
    -+multiple times the last occurance wins.
    ++option is given with no value, it's enabled.
     ++
      ** 'key=<K>': only show trailers with specified key. Matching is done
         case-insensitively and trailing colon is optional. If option is
    @@ Documentation/pretty-formats.txt: endif::git-rev-list[]
     -   `false`, `off`, `no` to show the non-trailer lines. If option is
     -   given without value it is enabled. If given multiple times the last
     -   value is used.
    -+** 'only[=bool]': select whether non-trailer lines from the trailer
    ++** 'only[=BOOL]': select whether non-trailer lines from the trailer
     +   block should be included.
      ** 'separator=<SEP>': specify a separator inserted between trailer
         lines. When this option is not given each trailer line is
    @@ Documentation/pretty-formats.txt: endif::git-rev-list[]
     -** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`
     -   option was given. In same way as to for `only` it can be followed
     -   by an equal sign and explicit value. E.g.,
    -+** 'unfold[=bool]': make it behave as if interpret-trailer's `--unfold`
    ++** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`
     +   option was given. E.g.,
         `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
     -** 'valueonly[=val]': skip over the key part of the trailer line and only
     -   show the value part. Also this optionally allows explicit value.
    -+** 'valueonly[=bool]': skip over the key part of the trailer line and only
    ++** 'valueonly[=BOOL]': skip over the key part of the trailer line and only
     +   show the value part.
      
      NOTE: Some placeholders may depend on other options given to the
4:  e9ca1e8d88c ! 3:  ea44eeff510 pretty-format %(trailers): fix broken standalone "valueonly"
    @@ Commit message
         pretty-format %(trailers): fix broken standalone "valueonly"
     
         Fix %(trailers:valueonly) being a noop due to on overly eager
    -    optimization. When new trailer options were added they needed to be
    -    listed at the start of the format_trailer_info() function. E.g. as was
    -    done in 250bea0c165 (pretty: allow showing specific trailers,
    -    2019-01-28).
    +    optimization in format_trailer_info() which skips custom formatting if
    +    no custom options are given.
     
    -    When d9b936db522 (pretty: add support for "valueonly" option in
    -    %(trailers), 2019-01-28) was added this was omitted by mistake. Thus
    -    %(trailers:valueonly) was a noop, instead of showing only trailer
    -    value. This wasn't caught because the tests for it always combined it
    -    with other options.
    +    When "valueonly" was added in d9b936db522 (pretty: add support for
    +    "valueonly" option in %(trailers), 2019-01-28) we forgot to add it to
    +    the list of options that optimization checks for. See e.g. the
    +    addition of "key" in 250bea0c165 (pretty: allow showing specific
    +    trailers, 2019-01-28) for a similar change where this wasn't missed.
     
    -    Let's fix the bug, and switch away from this pattern requiring us to
    -    remember to add new flags to the start of the function. Instead as
    -    soon as we see the ":" in "%(trailers:" we skip the fast path. That
    -    over-matches for "%(trailers:)", but I think that's OK.
    +    Thus the "valueonly" option in "%(trailers:valueonly)" was a noop and
    +    the output was equivalent to that of a plain "%(trailers)". This
    +    wasn't caught because the tests for it always combined it with other
    +    options.
     
    -    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +    Fix the bug by adding !opts->value_only to the list. I initially
    +    attempted to make this more future-proof by setting a flag if we got
    +    to ":" in "%(trailers:" in format_commit_one() in pretty.c. However,
    +    "%(trailers:" is also parsed in trailers_atom_parser() in
    +    ref-filter.c.
     
    - ## pretty.c ##
    -@@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
    - 		opts.no_divider = 1;
    - 
    - 		if (*arg == ':') {
    -+			/* over-matches on %(trailers:), but that's OK */
    -+			opts.have_options = 1;
    - 			arg++;
    - 			for (;;) {
    - 				const char *argval;
    +    There is an outstanding patch[1] unify those two, and such a fix, or
    +    other future-proofing, such as changing "process_trailer_options"
    +    flags into a bitfield, would conflict with that effort. Let's instead
    +    do the bare minimum here as this aspect of trailers is being actively
    +    worked on by another series.
    +
    +    Let's also test for a plain "valueonly" without any other options, as
    +    well as "separator". All the other existing options on the pretty.c
    +    path had tests where they were the only option provided. I'm also
    +    keeping a sanity test for "%(trailers:)" being the same as
    +    "%(trailers)". There's no reason to suspect it wouldn't be in the
    +    current implementation, but let's keep it in the interest of black box
    +    testing.
    +
    +    1. https://lore.kernel.org/git/pull.726.git.1599335291.gitgitgadget@gmail.com/
    +
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## t/t4205-log-pretty-formats.sh ##
    +@@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers) shows trailers' '
    + 	test_cmp expect actual
    + '
    + 
    ++test_expect_success 'pretty format %(trailers:) enables no options' '
    ++	git log --no-walk --pretty="%(trailers:)" >actual &&
    ++	# "expect" the same as the test above
    ++	test_cmp expect actual
    ++'
    ++
    + test_expect_success '%(trailers:only) shows only "key: value" trailers' '
    + 	git log --no-walk --pretty="%(trailers:only)" >actual &&
    + 	{
     @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
      	test_cmp expect actual
      '
    @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:key=foo,valueonly
     +	test_cmp expect actual
     +'
     +
    - test_expect_success '%(trailers:key=foo,keyonly,valueonly) shows nothing' '
    - 	git log --no-walk --pretty="format:%(trailers:key=Acked-by,keyonly,valueonly)" >actual &&
    - 	echo >expect &&
    + test_expect_success 'pretty format %(trailers:separator) changes separator' '
    ++	git log --no-walk --pretty=format:"X%(trailers:separator=%x00)X" >actual &&
    ++	(
    ++		printf "XSigned-off-by: A U Thor <author@example.com>\0" &&
    ++		printf "Acked-by: A U Thor <author@example.com>\0" &&
    ++		printf "[ v2 updated patch description ]\0" &&
    ++		printf "Signed-off-by: A U Thor\n  <author@example.com>X"
    ++	) >expect &&
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separator' '
    + 	git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual &&
    + 	(
    + 		printf "XSigned-off-by: A U Thor <author@example.com>\0" &&
     
      ## trailer.c ##
     @@ trailer.c: static void format_trailer_info(struct strbuf *out,
    - 	size_t origlen = out->len;
      	size_t i;
      
    --	/* If we want the whole block untouched, we can take the fast path. */
    + 	/* If we want the whole block untouched, we can take the fast path. */
     -	if (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) {
    -+	if (!opts->have_options) {
    ++	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
    ++	    !opts->separator && !opts->value_only) {
      		strbuf_add(out, info->trailer_start,
      			   info->trailer_end - info->trailer_start);
      		return;
    -
    - ## trailer.h ##
    -@@ trailer.h: struct new_trailer_item {
    - };
    - 
    - struct process_trailer_options {
    -+	int have_options;
    - 	int in_place;
    - 	int trim_empty;
    - 	int only_trailers;
3:  b71a700fa9b ! 4:  4fd193fd90c pretty format %(trailers): add a "keyonly"
    @@ Commit message
         change the ": " delimiter to something easily machine-readable (a key
         might contain ": " too). A follow-up change will add support for that.
     
    +    I don't really have a use-case for just "keyonly" myself. I suppose it
    +    would be useful in some cases as "key=*" matches case-insensitively,
    +    so a plain "keyonly" will give you the variants of the keys you
    +    matched. I'm mainly adding it to fix the inconsistency with
    +    "valueonly".
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Documentation/pretty-formats.txt ##
    -@@ Documentation/pretty-formats.txt: multiple times the last occurance wins.
    - ** 'unfold[=bool]': make it behave as if interpret-trailer's `--unfold`
    +@@ Documentation/pretty-formats.txt: option is given with no value, it's enabled.
    + ** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`
         option was given. E.g.,
         `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
    --** 'valueonly[=bool]': skip over the key part of the trailer line and only
    +-** 'valueonly[=BOOL]': skip over the key part of the trailer line and only
     -   show the value part.
    -+** 'keyonly[=bool]': only show the key part of the trailer.
    -+** 'valueonly[=bool]': only show the value part of the trailer.
    ++** 'keyonly[=BOOL]': only show the key part of the trailer.
    ++** 'valueonly[=BOOL]': only show the value part of the trailer.
      
      NOTE: Some placeholders may depend on other options given to the
      revision traversal engine. For example, the `%g*` reflog options will
    @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:key) without valu
      	test_cmp expect actual
      '
      
    -+test_expect_success '%(trailers:key=foo,keyonly) shows only keys' '
    ++test_expect_success '%(trailers:keyonly) shows only keys' '
     +	git log --no-walk --pretty="format:%(trailers:keyonly)" >actual &&
     +	test_write_lines \
     +		"Signed-off-by" \
    @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:key) without valu
      test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
      	git log --no-walk --pretty="format:%(trailers:key=Acked-by,valueonly)" >actual &&
      	echo "A U Thor <author@example.com>" >expect &&
    +@@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:valueonly) shows only values' '
      	test_cmp expect actual
      '
      
    @@ t/t4205-log-pretty-formats.sh: test_expect_success '%(trailers:key) without valu
     +'
     +
      test_expect_success 'pretty format %(trailers:separator) changes separator' '
    - 	git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual &&
    + 	git log --no-walk --pretty=format:"X%(trailers:separator=%x00)X" >actual &&
      	(
    -@@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers:separator) changes separator' '
    +@@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa
      	test_cmp expect actual
      '
      
    @@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers) co
     
      ## trailer.c ##
     @@ trailer.c: static void format_trailer_info(struct strbuf *out,
    + 
    + 	/* If we want the whole block untouched, we can take the fast path. */
    + 	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
    +-	    !opts->separator && !opts->value_only) {
    ++	    !opts->separator && !opts->key_only && !opts->value_only) {
    + 		strbuf_add(out, info->trailer_start,
    + 			   info->trailer_end - info->trailer_start);
    + 		return;
    +@@ trailer.c: static void format_trailer_info(struct strbuf *out,
      				if (opts->separator && out->len != origlen)
      					strbuf_addbuf(out, opts->separator);
      				if (!opts->value_only)
     -					strbuf_addf(out, "%s: ", tok.buf);
     -				strbuf_addbuf(out, &val);
    -+					strbuf_addstr(out, tok.buf);
    ++					strbuf_addbuf(out, &tok);
     +				if (!opts->key_only && !opts->value_only)
     +					strbuf_addstr(out, ": ");
     +				if (!opts->key_only)
    @@ trailer.h: struct process_trailer_options {
      	int no_divider;
     +	int key_only;
      	int value_only;
    - 	int canonicalize;
      	const struct strbuf *separator;
    -+	const struct strbuf *key_value_separator;
    - 	int (*filter)(const struct strbuf *, const char *alias, void *);
    - 	void *filter_data;
    - };
    + 	int (*filter)(const struct strbuf *, void *);
5:  cd4b3b52cf3 ! 5:  6cc6fc79388 pretty format %(trailers): add a "key_value_separator"
    @@ Metadata
      ## Commit message ##
         pretty format %(trailers): add a "key_value_separator"
     
    -    As noted in a previous commit which added "keyonly" it's needlessly
    -    hard to use the "log" machinery to produce machine-readable output for
    -    %(trailers). with the combination of the existing "separator" and this
    -    new "key_value_separator" this becomes trivial, as seen by the test
    -    being added here.
    +    Add a "key_value_separator" option to the "%(trailers)" pretty format,
    +    to go along with the existing "separator" argument. In combination
    +    these two options make it trivial to produce machine-readable (e.g. \0
    +    and \0\0-delimited) format output.
    +
    +    As elaborated on in a previous commit which added "keyonly" it was
    +    needlessly tedious to extract structured data from "%(trailers)"
    +    before the addition of this "key_value_separator" option. As seen by
    +    the test being added here extracting this data now becomes trivial.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Documentation/pretty-formats.txt ##
    -@@ Documentation/pretty-formats.txt: multiple times the last occurance wins.
    +@@ Documentation/pretty-formats.txt: option is given with no value, it's enabled.
         `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
    - ** 'keyonly[=bool]': only show the key part of the trailer.
    - ** 'valueonly[=bool]': only show the value part of the trailer.
    + ** 'keyonly[=BOOL]': only show the key part of the trailer.
    + ** 'valueonly[=BOOL]': only show the value part of the trailer.
     +** 'key_value_separator=<SEP>': specify a separator inserted between
     +   trailer lines. When this option is not given each trailer key-value
    -+   pair separated by ": ". Otherwise it shares the same semantics as 
    -+   'separator=<SEP>' above.
    ++   pair is separated by ": ". Otherwise it shares the same semantics
    ++   as 'separator=<SEP>' above.
      
      NOTE: Some placeholders may depend on other options given to the
      revision traversal engine. For example, the `%g*` reflog options will
    @@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
      					   !match_placeholder_bool_arg(arg, "keyonly", &arg, &opts.key_only) &&
     
      ## t/t4205-log-pretty-formats.sh ##
    -@@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers:separator) changes separator' '
    +@@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa
      	test_cmp expect actual
      '
      
     +test_expect_success 'pretty format %(trailers:key_value_separator) changes key-value separator' '
    ++	git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00)X" >actual &&
    ++	(
    ++		printf "XSigned-off-by\0A U Thor <author@example.com>\n" &&
    ++		printf "Acked-by\0A U Thor <author@example.com>\n" &&
    ++		printf "[ v2 updated patch description ]\n" &&
    ++		printf "Signed-off-by\0A U Thor\n  <author@example.com>\nX"
    ++	) >expect &&
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_success 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' '
     +	git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00,unfold)X" >actual &&
     +	(
     +		printf "XSigned-off-by\0A U Thor <author@example.com>\n" &&
    @@ t/t4205-log-pretty-formats.sh: test_expect_success 'pretty format %(trailers:sep
     
      ## trailer.c ##
     @@ trailer.c: static void format_trailer_info(struct strbuf *out,
    + 
    + 	/* If we want the whole block untouched, we can take the fast path. */
    + 	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
    +-	    !opts->separator && !opts->key_only && !opts->value_only) {
    ++	    !opts->separator && !opts->key_only && !opts->value_only &&
    ++	    !opts->key_value_separator) {
    + 		strbuf_add(out, info->trailer_start,
    + 			   info->trailer_end - info->trailer_start);
    + 		return;
    +@@ trailer.c: static void format_trailer_info(struct strbuf *out,
      					strbuf_addbuf(out, opts->separator);
      				if (!opts->value_only)
    - 					strbuf_addstr(out, tok.buf);
    + 					strbuf_addbuf(out, &tok);
     -				if (!opts->key_only && !opts->value_only)
     -					strbuf_addstr(out, ": ");
     +				if (!opts->key_only && !opts->value_only) {
    @@ trailer.c: static void format_trailer_info(struct strbuf *out,
      				if (!opts->key_only)
      					strbuf_addbuf(out, &val);
      				if (!opts->separator)
    +
    + ## trailer.h ##
    +@@ trailer.h: struct process_trailer_options {
    + 	int key_only;
    + 	int value_only;
    + 	const struct strbuf *separator;
    ++	const struct strbuf *key_value_separator;
    + 	int (*filter)(const struct strbuf *, void *);
    + 	void *filter_data;
    + };
-- 
2.29.2.222.g5d2a92d10f8

[PATCH v3 0/5] pretty format %(trailers): improve machine readability

[Ævar Arnfjörð Bjarmason]

A minor iteration on v2 with a commit message wording change &
s/=BOOL/=<BOOL>/g in the docs, as suggested by Christian
Couder. Range-diff below:

Ævar Arnfjörð Bjarmason (5):
  pretty format %(trailers) test: split a long line
  pretty format %(trailers) doc: avoid repetition
  pretty-format %(trailers): fix broken standalone "valueonly"
  pretty format %(trailers): add a "keyonly"
  pretty format %(trailers): add a "key_value_separator"

 Documentation/pretty-formats.txt | 34 ++++++-----
 pretty.c                         | 10 ++++
 t/t4205-log-pretty-formats.sh    | 99 +++++++++++++++++++++++++++++++-
 trailer.c                        | 15 ++++-
 trailer.h                        |  2 +
 5 files changed, 141 insertions(+), 19 deletions(-)

Range-diff:
1:  4b134a62aec = 1:  584c7580b5b pretty format %(trailers) test: split a long line
2:  0d3fe6daf6c ! 2:  0255a64949b pretty format %(trailers) doc: avoid repetition
    @@ Commit message
     
         It took me a couple of readings to realize that these options were
         referring back to the "only" option's treatment of boolean
    -    values. Let's try to make this more explicit, and upper-case "BOOL"
    -    for consistency with the existing "<SEP>" and "<K>".
    +    values.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ Documentation/pretty-formats.txt: endif::git-rev-list[]
     -   `false`, `off`, `no` to show the non-trailer lines. If option is
     -   given without value it is enabled. If given multiple times the last
     -   value is used.
    -+** 'only[=BOOL]': select whether non-trailer lines from the trailer
    ++** 'only[=<BOOL>]': select whether non-trailer lines from the trailer
     +   block should be included.
      ** 'separator=<SEP>': specify a separator inserted between trailer
         lines. When this option is not given each trailer line is
    @@ Documentation/pretty-formats.txt: endif::git-rev-list[]
     -** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`
     -   option was given. In same way as to for `only` it can be followed
     -   by an equal sign and explicit value. E.g.,
    -+** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`
    ++** 'unfold[=<BOOL>]': make it behave as if interpret-trailer's `--unfold`
     +   option was given. E.g.,
         `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
     -** 'valueonly[=val]': skip over the key part of the trailer line and only
     -   show the value part. Also this optionally allows explicit value.
    -+** 'valueonly[=BOOL]': skip over the key part of the trailer line and only
    ++** 'valueonly[=<BOOL>]': skip over the key part of the trailer line and only
     +   show the value part.
      
      NOTE: Some placeholders may depend on other options given to the
3:  ea44eeff510 = 3:  c2c5513942f pretty-format %(trailers): fix broken standalone "valueonly"
4:  4fd193fd90c ! 4:  574ef0be25f pretty format %(trailers): add a "keyonly"
    @@ Commit message
     
      ## Documentation/pretty-formats.txt ##
     @@ Documentation/pretty-formats.txt: option is given with no value, it's enabled.
    - ** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`
    + ** 'unfold[=<BOOL>]': make it behave as if interpret-trailer's `--unfold`
         option was given. E.g.,
         `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
    --** 'valueonly[=BOOL]': skip over the key part of the trailer line and only
    +-** 'valueonly[=<BOOL>]': skip over the key part of the trailer line and only
     -   show the value part.
    -+** 'keyonly[=BOOL]': only show the key part of the trailer.
    -+** 'valueonly[=BOOL]': only show the value part of the trailer.
    ++** 'keyonly[=<BOOL>]': only show the key part of the trailer.
    ++** 'valueonly[=<BOOL>]': only show the value part of the trailer.
      
      NOTE: Some placeholders may depend on other options given to the
      revision traversal engine. For example, the `%g*` reflog options will
5:  6cc6fc79388 ! 5:  dbc73b951f6 pretty format %(trailers): add a "key_value_separator"
    @@ Commit message
      ## Documentation/pretty-formats.txt ##
     @@ Documentation/pretty-formats.txt: option is given with no value, it's enabled.
         `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
    - ** 'keyonly[=BOOL]': only show the key part of the trailer.
    - ** 'valueonly[=BOOL]': only show the value part of the trailer.
    + ** 'keyonly[=<BOOL>]': only show the key part of the trailer.
    + ** 'valueonly[=<BOOL>]': only show the value part of the trailer.
     +** 'key_value_separator=<SEP>': specify a separator inserted between
     +   trailer lines. When this option is not given each trailer key-value
     +   pair is separated by ": ". Otherwise it shares the same semantics
-- 
2.29.2.222.g5d2a92d10f8

Re: [PATCH v3 0/5] pretty format %(trailers): improve machine readability

[Christian Couder]

On Wed, Dec 9, 2020 at 4:52 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> A minor iteration on v2 with a commit message wording change &
> s/=BOOL/=<BOOL>/g in the docs, as suggested by Christian
> Couder. Range-diff below:

This one looks good to me!

Reviewed-by: Christian Couder <chriscool@tuxfamily.org>

Re: [PATCH v3 0/5] pretty format %(trailers): improve machine readability

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Wed, Dec 9, 2020 at 4:52 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>> A minor iteration on v2 with a commit message wording change &
>> s/=BOOL/=<BOOL>/g in the docs, as suggested by Christian
>> Couder. Range-diff below:
>
> This one looks good to me!
>
> Reviewed-by: Christian Couder <chriscool@tuxfamily.org>

The range-diff looked minimum and didn't introduce anything funny.
The unchanged parts I only skimmed, though.

Thanks, both.

[PATCH v3 1/5] pretty format %(trailers) test: split a long line

[Ævar Arnfjörð Bjarmason]

Split a very long line in a test introduced in 0b691d86851 (pretty:
add support for separator option in %(trailers), 2019-01-28). This
makes it easier to read, especially as follow-up commits will copy
this test as a template.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4205-log-pretty-formats.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 204c149d5a4..5e5452212d2 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -717,7 +717,12 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
 
 test_expect_success 'pretty format %(trailers:separator) changes separator' '
 	git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual &&
-	printf "XSigned-off-by: A U Thor <author@example.com>\0Acked-by: A U Thor <author@example.com>\0[ v2 updated patch description ]\0Signed-off-by: A U Thor <author@example.com>X" >expect &&
+	(
+		printf "XSigned-off-by: A U Thor <author@example.com>\0" &&
+		printf "Acked-by: A U Thor <author@example.com>\0" &&
+		printf "[ v2 updated patch description ]\0" &&
+		printf "Signed-off-by: A U Thor <author@example.com>X"
+	) >expect &&
 	test_cmp expect actual
 '
 
-- 
2.29.2.222.g5d2a92d10f8

[PATCH v3 2/5] pretty format %(trailers) doc: avoid repetition

[Ævar Arnfjörð Bjarmason]

Change the documentation for the various %(trailers) options so it
isn't repeating part of the documentation for "only" about how boolean
values are handled. Instead, let's split the description of that into
general documentation at the top.

It then suffices to refer to it by listing the options as
"opt[=<BOOL>]". I'm also changing it to upper-case "[=<BOOL>]" from
"[=val]" for consistency with "<SEP>"

It took me a couple of readings to realize that these options were
referring back to the "only" option's treatment of boolean
values.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/pretty-formats.txt | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 84bbc7439a6..973b6c7d482 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -252,7 +252,15 @@ endif::git-rev-list[]
 			  interpreted by
 			  linkgit:git-interpret-trailers[1]. The
 			  `trailers` string may be followed by a colon
-			  and zero or more comma-separated options:
+			  and zero or more comma-separated options.
+			  If any option is provided multiple times the
+			  last occurance wins.
++
+The boolean options accept an optional value `[=<BOOL>]`. The values
+`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
+sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
+option is given with no value, it's enabled.
++
 ** 'key=<K>': only show trailers with specified key. Matching is done
    case-insensitively and trailing colon is optional. If option is
    given multiple times trailer lines matching any of the keys are
@@ -261,27 +269,21 @@ endif::git-rev-list[]
    desired it can be disabled with `only=false`.  E.g.,
    `%(trailers:key=Reviewed-by)` shows trailer lines with key
    `Reviewed-by`.
-** 'only[=val]': select whether non-trailer lines from the trailer
-   block should be included. The `only` keyword may optionally be
-   followed by an equal sign and one of `true`, `on`, `yes` to omit or
-   `false`, `off`, `no` to show the non-trailer lines. If option is
-   given without value it is enabled. If given multiple times the last
-   value is used.
+** 'only[=<BOOL>]': select whether non-trailer lines from the trailer
+   block should be included.
 ** 'separator=<SEP>': specify a separator inserted between trailer
    lines. When this option is not given each trailer line is
    terminated with a line feed character. The string SEP may contain
    the literal formatting codes described above. To use comma as
    separator one must use `%x2C` as it would otherwise be parsed as
-   next option. If separator option is given multiple times only the
-   last one is used. E.g., `%(trailers:key=Ticket,separator=%x2C )`
+   next option. E.g., `%(trailers:key=Ticket,separator=%x2C )`
    shows all trailer lines whose key is "Ticket" separated by a comma
    and a space.
-** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`
-   option was given. In same way as to for `only` it can be followed
-   by an equal sign and explicit value. E.g.,
+** 'unfold[=<BOOL>]': make it behave as if interpret-trailer's `--unfold`
+   option was given. E.g.,
    `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
-** 'valueonly[=val]': skip over the key part of the trailer line and only
-   show the value part. Also this optionally allows explicit value.
+** 'valueonly[=<BOOL>]': skip over the key part of the trailer line and only
+   show the value part.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
-- 
2.29.2.222.g5d2a92d10f8

Re: [PATCH v3 2/5] pretty format %(trailers) doc: avoid repetition

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> -			  and zero or more comma-separated options:
> +			  and zero or more comma-separated options.
> +			  If any option is provided multiple times the
> +			  last occurance wins.
> ++
> +The boolean options accept an optional value `[=<BOOL>]`. The values
> +`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
> +sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
> +option is given with no value, it's enabled.

Nicely written.

[PATCH v3 3/5] pretty-format %(trailers): fix broken standalone "valueonly"

[Ævar Arnfjörð Bjarmason]

Fix %(trailers:valueonly) being a noop due to on overly eager
optimization in format_trailer_info() which skips custom formatting if
no custom options are given.

When "valueonly" was added in d9b936db522 (pretty: add support for
"valueonly" option in %(trailers), 2019-01-28) we forgot to add it to
the list of options that optimization checks for. See e.g. the
addition of "key" in 250bea0c165 (pretty: allow showing specific
trailers, 2019-01-28) for a similar change where this wasn't missed.

Thus the "valueonly" option in "%(trailers:valueonly)" was a noop and
the output was equivalent to that of a plain "%(trailers)". This
wasn't caught because the tests for it always combined it with other
options.

Fix the bug by adding !opts->value_only to the list. I initially
attempted to make this more future-proof by setting a flag if we got
to ":" in "%(trailers:" in format_commit_one() in pretty.c. However,
"%(trailers:" is also parsed in trailers_atom_parser() in
ref-filter.c.

There is an outstanding patch[1] unify those two, and such a fix, or
other future-proofing, such as changing "process_trailer_options"
flags into a bitfield, would conflict with that effort. Let's instead
do the bare minimum here as this aspect of trailers is being actively
worked on by another series.

Let's also test for a plain "valueonly" without any other options, as
well as "separator". All the other existing options on the pretty.c
path had tests where they were the only option provided. I'm also
keeping a sanity test for "%(trailers:)" being the same as
"%(trailers)". There's no reason to suspect it wouldn't be in the
current implementation, but let's keep it in the interest of black box
testing.

1. https://lore.kernel.org/git/pull.726.git.1599335291.gitgitgadget@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4205-log-pretty-formats.sh | 28 ++++++++++++++++++++++++++++
 trailer.c                     |  3 ++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 5e5452212d2..cb09a13249e 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -605,6 +605,12 @@ test_expect_success 'pretty format %(trailers) shows trailers' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:) enables no options' '
+	git log --no-walk --pretty="%(trailers:)" >actual &&
+	# "expect" the same as the test above
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers:only) shows only "key: value" trailers' '
 	git log --no-walk --pretty="%(trailers:only)" >actual &&
 	{
@@ -715,7 +721,29 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:valueonly) shows only values' '
+	git log --no-walk --pretty="format:%(trailers:valueonly)" >actual &&
+	test_write_lines \
+		"A U Thor <author@example.com>" \
+		"A U Thor <author@example.com>" \
+		"[ v2 updated patch description ]" \
+		"A U Thor" \
+		"  <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'pretty format %(trailers:separator) changes separator' '
+	git log --no-walk --pretty=format:"X%(trailers:separator=%x00)X" >actual &&
+	(
+		printf "XSigned-off-by: A U Thor <author@example.com>\0" &&
+		printf "Acked-by: A U Thor <author@example.com>\0" &&
+		printf "[ v2 updated patch description ]\0" &&
+		printf "Signed-off-by: A U Thor\n  <author@example.com>X"
+	) >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separator' '
 	git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual &&
 	(
 		printf "XSigned-off-by: A U Thor <author@example.com>\0" &&
diff --git a/trailer.c b/trailer.c
index 3f7391d793c..d2d01015b1d 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1131,7 +1131,8 @@ static void format_trailer_info(struct strbuf *out,
 	size_t i;
 
 	/* If we want the whole block untouched, we can take the fast path. */
-	if (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) {
+	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
+	    !opts->separator && !opts->value_only) {
 		strbuf_add(out, info->trailer_start,
 			   info->trailer_end - info->trailer_start);
 		return;
-- 
2.29.2.222.g5d2a92d10f8

[PATCH v3 4/5] pretty format %(trailers): add a "keyonly"

[Ævar Arnfjörð Bjarmason]

Add support for a "keyonly". This allows for easier parsing out of the
key and value. Before if you didn't want to make assumptions about how
the key was formatted. You'd need to parse it out as e.g.:

    --pretty=format:'%H%x00%(trailers:separator=%x00%x00)' \
                       '%x00%(trailers:separator=%x00%x00,valueonly)'

And then proceed to deduce keys by looking at those two and
subtracting the value plus the hardcoded ": " separator from the
non-valueonly %(trailers) line. Now it's possible to simply do:

    --pretty=format:'%H%x00%(trailers:separator=%x00%x00,keyonly)' \
                    '%x00%(trailers:separator=%x00%x00,valueonly)'

Which at least reduces it to a state machine where you get N keys and
correlate them with N values. Even better would be to have a way to
change the ": " delimiter to something easily machine-readable (a key
might contain ": " too). A follow-up change will add support for that.

I don't really have a use-case for just "keyonly" myself. I suppose it
would be useful in some cases as "key=*" matches case-insensitively,
so a plain "keyonly" will give you the variants of the keys you
matched. I'm mainly adding it to fix the inconsistency with
"valueonly".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/pretty-formats.txt |  4 ++--
 pretty.c                         |  1 +
 t/t4205-log-pretty-formats.sh    | 31 ++++++++++++++++++++++++++++++-
 trailer.c                        |  9 ++++++---
 trailer.h                        |  1 +
 5 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 973b6c7d482..5eac36500d4 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -282,8 +282,8 @@ option is given with no value, it's enabled.
 ** 'unfold[=<BOOL>]': make it behave as if interpret-trailer's `--unfold`
    option was given. E.g.,
    `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
-** 'valueonly[=<BOOL>]': skip over the key part of the trailer line and only
-   show the value part.
+** 'keyonly[=<BOOL>]': only show the key part of the trailer.
+** 'valueonly[=<BOOL>]': only show the value part of the trailer.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index 7a7708a0ea7..1237ee0e45d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1451,6 +1451,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					opts.separator = &sepbuf;
 				} else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
 					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) &&
+					   !match_placeholder_bool_arg(arg, "keyonly", &arg, &opts.key_only) &&
 					   !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only))
 					break;
 			}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index cb09a13249e..4c9f6eb7946 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -715,6 +715,22 @@ test_expect_success '%(trailers:key) without value is error' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:keyonly) shows only keys' '
+	git log --no-walk --pretty="format:%(trailers:keyonly)" >actual &&
+	test_write_lines \
+		"Signed-off-by" \
+		"Acked-by" \
+		"[ v2 updated patch description ]" \
+		"Signed-off-by" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo,keyonly) shows only key' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by,keyonly)" >actual &&
+	echo "Acked-by" >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
 	git log --no-walk --pretty="format:%(trailers:key=Acked-by,valueonly)" >actual &&
 	echo "A U Thor <author@example.com>" >expect &&
@@ -732,6 +748,12 @@ test_expect_success '%(trailers:valueonly) shows only values' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:key=foo,keyonly,valueonly) shows nothing' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by,keyonly,valueonly)" >actual &&
+	echo >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'pretty format %(trailers:separator) changes separator' '
 	git log --no-walk --pretty=format:"X%(trailers:separator=%x00)X" >actual &&
 	(
@@ -754,7 +776,7 @@ test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa
 	test_cmp expect actual
 '
 
-test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' '
+test_expect_success 'pretty format %(trailers) combining separator/key/keyonly/valueonly' '
 	git commit --allow-empty -F - <<-\EOF &&
 	Important fix
 
@@ -781,6 +803,13 @@ test_expect_success 'pretty format %(trailers) combining separator/key/valueonly
 		"Does not close any tickets" \
 		"Another fix #567, #890" \
 		"Important fix #1234" >expect &&
+	test_cmp expect actual &&
+
+	git log --pretty="%s% (trailers:separator=%x2c%x20,key=Closes,keyonly)" HEAD~3.. >actual &&
+	test_write_lines \
+		"Does not close any tickets" \
+		"Another fix Closes, Closes" \
+		"Important fix Closes" >expect &&
 	test_cmp expect actual
 '
 
diff --git a/trailer.c b/trailer.c
index d2d01015b1d..889b419a4f6 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out,
 
 	/* If we want the whole block untouched, we can take the fast path. */
 	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
-	    !opts->separator && !opts->value_only) {
+	    !opts->separator && !opts->key_only && !opts->value_only) {
 		strbuf_add(out, info->trailer_start,
 			   info->trailer_end - info->trailer_start);
 		return;
@@ -1154,8 +1154,11 @@ static void format_trailer_info(struct strbuf *out,
 				if (opts->separator && out->len != origlen)
 					strbuf_addbuf(out, opts->separator);
 				if (!opts->value_only)
-					strbuf_addf(out, "%s: ", tok.buf);
-				strbuf_addbuf(out, &val);
+					strbuf_addbuf(out, &tok);
+				if (!opts->key_only && !opts->value_only)
+					strbuf_addstr(out, ": ");
+				if (!opts->key_only)
+					strbuf_addbuf(out, &val);
 				if (!opts->separator)
 					strbuf_addch(out, '\n');
 			}
diff --git a/trailer.h b/trailer.h
index cd93e7ddea7..d2f28776be6 100644
--- a/trailer.h
+++ b/trailer.h
@@ -71,6 +71,7 @@ struct process_trailer_options {
 	int only_input;
 	int unfold;
 	int no_divider;
+	int key_only;
 	int value_only;
 	const struct strbuf *separator;
 	int (*filter)(const struct strbuf *, void *);
-- 
2.29.2.222.g5d2a92d10f8

[PATCH v3 5/5] pretty format %(trailers): add a "key_value_separator"

[Ævar Arnfjörð Bjarmason]

Add a "key_value_separator" option to the "%(trailers)" pretty format,
to go along with the existing "separator" argument. In combination
these two options make it trivial to produce machine-readable (e.g. \0
and \0\0-delimited) format output.

As elaborated on in a previous commit which added "keyonly" it was
needlessly tedious to extract structured data from "%(trailers)"
before the addition of this "key_value_separator" option. As seen by
the test being added here extracting this data now becomes trivial.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/pretty-formats.txt |  4 ++++
 pretty.c                         |  9 +++++++++
 t/t4205-log-pretty-formats.sh    | 33 ++++++++++++++++++++++++++++++++
 trailer.c                        | 11 ++++++++---
 trailer.h                        |  1 +
 5 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 5eac36500d4..6b59e28d444 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -284,6 +284,10 @@ option is given with no value, it's enabled.
    `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
 ** 'keyonly[=<BOOL>]': only show the key part of the trailer.
 ** 'valueonly[=<BOOL>]': only show the value part of the trailer.
+** 'key_value_separator=<SEP>': specify a separator inserted between
+   trailer lines. When this option is not given each trailer key-value
+   pair is separated by ": ". Otherwise it shares the same semantics
+   as 'separator=<SEP>' above.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index 1237ee0e45d..05eef7fda0b 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1418,6 +1418,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
 		struct string_list filter_list = STRING_LIST_INIT_NODUP;
 		struct strbuf sepbuf = STRBUF_INIT;
+		struct strbuf kvsepbuf = STRBUF_INIT;
 		size_t ret = 0;
 
 		opts.no_divider = 1;
@@ -1449,6 +1450,14 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					strbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL);
 					free(fmt);
 					opts.separator = &sepbuf;
+				} else if (match_placeholder_arg_value(arg, "key_value_separator", &arg, &argval, &arglen)) {
+					char *fmt;
+
+					strbuf_reset(&kvsepbuf);
+					fmt = xstrndup(argval, arglen);
+					strbuf_expand(&kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);
+					free(fmt);
+					opts.key_value_separator = &kvsepbuf;
 				} else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
 					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) &&
 					   !match_placeholder_bool_arg(arg, "keyonly", &arg, &opts.key_only) &&
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 4c9f6eb7946..749bc1431ac 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -776,6 +776,39 @@ test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:key_value_separator) changes key-value separator' '
+	git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00)X" >actual &&
+	(
+		printf "XSigned-off-by\0A U Thor <author@example.com>\n" &&
+		printf "Acked-by\0A U Thor <author@example.com>\n" &&
+		printf "[ v2 updated patch description ]\n" &&
+		printf "Signed-off-by\0A U Thor\n  <author@example.com>\nX"
+	) >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' '
+	git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00,unfold)X" >actual &&
+	(
+		printf "XSigned-off-by\0A U Thor <author@example.com>\n" &&
+		printf "Acked-by\0A U Thor <author@example.com>\n" &&
+		printf "[ v2 updated patch description ]\n" &&
+		printf "Signed-off-by\0A U Thor <author@example.com>\nX"
+	) >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:separator,key_value_separator) changes both separators' '
+	git log --no-walk --pretty=format:"%(trailers:separator=%x00,key_value_separator=%x00%x00,unfold)" >actual &&
+	(
+		printf "Signed-off-by\0\0A U Thor <author@example.com>\0" &&
+		printf "Acked-by\0\0A U Thor <author@example.com>\0" &&
+		printf "[ v2 updated patch description ]\0" &&
+		printf "Signed-off-by\0\0A U Thor <author@example.com>"
+	) >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'pretty format %(trailers) combining separator/key/keyonly/valueonly' '
 	git commit --allow-empty -F - <<-\EOF &&
 	Important fix
diff --git a/trailer.c b/trailer.c
index 889b419a4f6..249ed618ed8 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1132,7 +1132,8 @@ static void format_trailer_info(struct strbuf *out,
 
 	/* If we want the whole block untouched, we can take the fast path. */
 	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
-	    !opts->separator && !opts->key_only && !opts->value_only) {
+	    !opts->separator && !opts->key_only && !opts->value_only &&
+	    !opts->key_value_separator) {
 		strbuf_add(out, info->trailer_start,
 			   info->trailer_end - info->trailer_start);
 		return;
@@ -1155,8 +1156,12 @@ static void format_trailer_info(struct strbuf *out,
 					strbuf_addbuf(out, opts->separator);
 				if (!opts->value_only)
 					strbuf_addbuf(out, &tok);
-				if (!opts->key_only && !opts->value_only)
-					strbuf_addstr(out, ": ");
+				if (!opts->key_only && !opts->value_only) {
+					if (opts->key_value_separator)
+						strbuf_addbuf(out, opts->key_value_separator);
+					else
+						strbuf_addstr(out, ": ");
+				}
 				if (!opts->key_only)
 					strbuf_addbuf(out, &val);
 				if (!opts->separator)
diff --git a/trailer.h b/trailer.h
index d2f28776be6..795d2fccfd9 100644
--- a/trailer.h
+++ b/trailer.h
@@ -74,6 +74,7 @@ struct process_trailer_options {
 	int key_only;
 	int value_only;
 	const struct strbuf *separator;
+	const struct strbuf *key_value_separator;
 	int (*filter)(const struct strbuf *, void *);
 	void *filter_data;
 };
-- 
2.29.2.222.g5d2a92d10f8

[PATCH v2 1/5] pretty format %(trailers) test: split a long line

[Ævar Arnfjörð Bjarmason]

Split a very long line in a test introduced in 0b691d86851 (pretty:
add support for separator option in %(trailers), 2019-01-28). This
makes it easier to read, especially as follow-up commits will copy
this test as a template.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4205-log-pretty-formats.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 204c149d5a4..5e5452212d2 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -717,7 +717,12 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
 
 test_expect_success 'pretty format %(trailers:separator) changes separator' '
 	git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual &&
-	printf "XSigned-off-by: A U Thor <author@example.com>\0Acked-by: A U Thor <author@example.com>\0[ v2 updated patch description ]\0Signed-off-by: A U Thor <author@example.com>X" >expect &&
+	(
+		printf "XSigned-off-by: A U Thor <author@example.com>\0" &&
+		printf "Acked-by: A U Thor <author@example.com>\0" &&
+		printf "[ v2 updated patch description ]\0" &&
+		printf "Signed-off-by: A U Thor <author@example.com>X"
+	) >expect &&
 	test_cmp expect actual
 '
 
-- 
2.29.2.222.g5d2a92d10f8

[PATCH v2 2/5] pretty format %(trailers) doc: avoid repetition

[Ævar Arnfjörð Bjarmason]

Change the documentation for the various %(trailers) options so it
isn't repeating part of the documentation for "only" about how boolean
values are handled. Instead, let's split the description of that into
general documentation at the top.

It then suffices to refer to it by listing the options as
"opt[=<BOOL>]". I'm also changing it to upper-case "[=<BOOL>]" from
"[=val]" for consistency with "<SEP>"

It took me a couple of readings to realize that these options were
referring back to the "only" option's treatment of boolean
values. Let's try to make this more explicit, and upper-case "BOOL"
for consistency with the existing "<SEP>" and "<K>".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/pretty-formats.txt | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 84bbc7439a6..66dfa122361 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -252,7 +252,15 @@ endif::git-rev-list[]
 			  interpreted by
 			  linkgit:git-interpret-trailers[1]. The
 			  `trailers` string may be followed by a colon
-			  and zero or more comma-separated options:
+			  and zero or more comma-separated options.
+			  If any option is provided multiple times the
+			  last occurance wins.
++
+The boolean options accept an optional value `[=<BOOL>]`. The values
+`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
+sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
+option is given with no value, it's enabled.
++
 ** 'key=<K>': only show trailers with specified key. Matching is done
    case-insensitively and trailing colon is optional. If option is
    given multiple times trailer lines matching any of the keys are
@@ -261,27 +269,21 @@ endif::git-rev-list[]
    desired it can be disabled with `only=false`.  E.g.,
    `%(trailers:key=Reviewed-by)` shows trailer lines with key
    `Reviewed-by`.
-** 'only[=val]': select whether non-trailer lines from the trailer
-   block should be included. The `only` keyword may optionally be
-   followed by an equal sign and one of `true`, `on`, `yes` to omit or
-   `false`, `off`, `no` to show the non-trailer lines. If option is
-   given without value it is enabled. If given multiple times the last
-   value is used.
+** 'only[=BOOL]': select whether non-trailer lines from the trailer
+   block should be included.
 ** 'separator=<SEP>': specify a separator inserted between trailer
    lines. When this option is not given each trailer line is
    terminated with a line feed character. The string SEP may contain
    the literal formatting codes described above. To use comma as
    separator one must use `%x2C` as it would otherwise be parsed as
-   next option. If separator option is given multiple times only the
-   last one is used. E.g., `%(trailers:key=Ticket,separator=%x2C )`
+   next option. E.g., `%(trailers:key=Ticket,separator=%x2C )`
    shows all trailer lines whose key is "Ticket" separated by a comma
    and a space.
-** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`
-   option was given. In same way as to for `only` it can be followed
-   by an equal sign and explicit value. E.g.,
+** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`
+   option was given. E.g.,
    `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
-** 'valueonly[=val]': skip over the key part of the trailer line and only
-   show the value part. Also this optionally allows explicit value.
+** 'valueonly[=BOOL]': skip over the key part of the trailer line and only
+   show the value part.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
-- 
2.29.2.222.g5d2a92d10f8

Re: [PATCH v2 2/5] pretty format %(trailers) doc: avoid repetition

[Christian Couder]

On Sun, Dec 6, 2020 at 1:25 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Change the documentation for the various %(trailers) options so it
> isn't repeating part of the documentation for "only" about how boolean
> values are handled. Instead, let's split the description of that into
> general documentation at the top.
>
> It then suffices to refer to it by listing the options as
> "opt[=<BOOL>]". I'm also changing it to upper-case "[=<BOOL>]" from
> "[=val]" for consistency with "<SEP>"

Good...

> It took me a couple of readings to realize that these options were
> referring back to the "only" option's treatment of boolean
> values. Let's try to make this more explicit, and upper-case "BOOL"
> for consistency with the existing "<SEP>" and "<K>".

... but not sure it's worth repeating that we upper-case "BOOL".

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/pretty-formats.txt | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 84bbc7439a6..66dfa122361 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -252,7 +252,15 @@ endif::git-rev-list[]
>                           interpreted by
>                           linkgit:git-interpret-trailers[1]. The
>                           `trailers` string may be followed by a colon
> -                         and zero or more comma-separated options:
> +                         and zero or more comma-separated options.
> +                         If any option is provided multiple times the
> +                         last occurance wins.
> ++
> +The boolean options accept an optional value `[=<BOOL>]`. The values
> +`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
> +sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
> +option is given with no value, it's enabled.

> +** 'only[=BOOL]': select whether non-trailer lines from the trailer

Here it's "[=BOOL]" while above it's "[=<BOOL>]"

> +** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`

Here also.

> +** 'valueonly[=BOOL]': skip over the key part of the trailer line and only

And here too.

> +   show the value part.

[PATCH v2 3/5] pretty-format %(trailers): fix broken standalone "valueonly"

[Ævar Arnfjörð Bjarmason]

Fix %(trailers:valueonly) being a noop due to on overly eager
optimization in format_trailer_info() which skips custom formatting if
no custom options are given.

When "valueonly" was added in d9b936db522 (pretty: add support for
"valueonly" option in %(trailers), 2019-01-28) we forgot to add it to
the list of options that optimization checks for. See e.g. the
addition of "key" in 250bea0c165 (pretty: allow showing specific
trailers, 2019-01-28) for a similar change where this wasn't missed.

Thus the "valueonly" option in "%(trailers:valueonly)" was a noop and
the output was equivalent to that of a plain "%(trailers)". This
wasn't caught because the tests for it always combined it with other
options.

Fix the bug by adding !opts->value_only to the list. I initially
attempted to make this more future-proof by setting a flag if we got
to ":" in "%(trailers:" in format_commit_one() in pretty.c. However,
"%(trailers:" is also parsed in trailers_atom_parser() in
ref-filter.c.

There is an outstanding patch[1] unify those two, and such a fix, or
other future-proofing, such as changing "process_trailer_options"
flags into a bitfield, would conflict with that effort. Let's instead
do the bare minimum here as this aspect of trailers is being actively
worked on by another series.

Let's also test for a plain "valueonly" without any other options, as
well as "separator". All the other existing options on the pretty.c
path had tests where they were the only option provided. I'm also
keeping a sanity test for "%(trailers:)" being the same as
"%(trailers)". There's no reason to suspect it wouldn't be in the
current implementation, but let's keep it in the interest of black box
testing.

1. https://lore.kernel.org/git/pull.726.git.1599335291.gitgitgadget@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4205-log-pretty-formats.sh | 28 ++++++++++++++++++++++++++++
 trailer.c                     |  3 ++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 5e5452212d2..cb09a13249e 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -605,6 +605,12 @@ test_expect_success 'pretty format %(trailers) shows trailers' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:) enables no options' '
+	git log --no-walk --pretty="%(trailers:)" >actual &&
+	# "expect" the same as the test above
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers:only) shows only "key: value" trailers' '
 	git log --no-walk --pretty="%(trailers:only)" >actual &&
 	{
@@ -715,7 +721,29 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:valueonly) shows only values' '
+	git log --no-walk --pretty="format:%(trailers:valueonly)" >actual &&
+	test_write_lines \
+		"A U Thor <author@example.com>" \
+		"A U Thor <author@example.com>" \
+		"[ v2 updated patch description ]" \
+		"A U Thor" \
+		"  <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'pretty format %(trailers:separator) changes separator' '
+	git log --no-walk --pretty=format:"X%(trailers:separator=%x00)X" >actual &&
+	(
+		printf "XSigned-off-by: A U Thor <author@example.com>\0" &&
+		printf "Acked-by: A U Thor <author@example.com>\0" &&
+		printf "[ v2 updated patch description ]\0" &&
+		printf "Signed-off-by: A U Thor\n  <author@example.com>X"
+	) >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separator' '
 	git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual &&
 	(
 		printf "XSigned-off-by: A U Thor <author@example.com>\0" &&
diff --git a/trailer.c b/trailer.c
index 3f7391d793c..d2d01015b1d 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1131,7 +1131,8 @@ static void format_trailer_info(struct strbuf *out,
 	size_t i;
 
 	/* If we want the whole block untouched, we can take the fast path. */
-	if (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) {
+	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
+	    !opts->separator && !opts->value_only) {
 		strbuf_add(out, info->trailer_start,
 			   info->trailer_end - info->trailer_start);
 		return;
-- 
2.29.2.222.g5d2a92d10f8

[PATCH v2 4/5] pretty format %(trailers): add a "keyonly"

[Ævar Arnfjörð Bjarmason]

Add support for a "keyonly". This allows for easier parsing out of the
key and value. Before if you didn't want to make assumptions about how
the key was formatted. You'd need to parse it out as e.g.:

    --pretty=format:'%H%x00%(trailers:separator=%x00%x00)' \
                       '%x00%(trailers:separator=%x00%x00,valueonly)'

And then proceed to deduce keys by looking at those two and
subtracting the value plus the hardcoded ": " separator from the
non-valueonly %(trailers) line. Now it's possible to simply do:

    --pretty=format:'%H%x00%(trailers:separator=%x00%x00,keyonly)' \
                    '%x00%(trailers:separator=%x00%x00,valueonly)'

Which at least reduces it to a state machine where you get N keys and
correlate them with N values. Even better would be to have a way to
change the ": " delimiter to something easily machine-readable (a key
might contain ": " too). A follow-up change will add support for that.

I don't really have a use-case for just "keyonly" myself. I suppose it
would be useful in some cases as "key=*" matches case-insensitively,
so a plain "keyonly" will give you the variants of the keys you
matched. I'm mainly adding it to fix the inconsistency with
"valueonly".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/pretty-formats.txt |  4 ++--
 pretty.c                         |  1 +
 t/t4205-log-pretty-formats.sh    | 31 ++++++++++++++++++++++++++++++-
 trailer.c                        |  9 ++++++---
 trailer.h                        |  1 +
 5 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 66dfa122361..bf35f7cf219 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -282,8 +282,8 @@ option is given with no value, it's enabled.
 ** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`
    option was given. E.g.,
    `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
-** 'valueonly[=BOOL]': skip over the key part of the trailer line and only
-   show the value part.
+** 'keyonly[=BOOL]': only show the key part of the trailer.
+** 'valueonly[=BOOL]': only show the value part of the trailer.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index 7a7708a0ea7..1237ee0e45d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1451,6 +1451,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					opts.separator = &sepbuf;
 				} else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
 					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) &&
+					   !match_placeholder_bool_arg(arg, "keyonly", &arg, &opts.key_only) &&
 					   !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only))
 					break;
 			}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index cb09a13249e..4c9f6eb7946 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -715,6 +715,22 @@ test_expect_success '%(trailers:key) without value is error' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:keyonly) shows only keys' '
+	git log --no-walk --pretty="format:%(trailers:keyonly)" >actual &&
+	test_write_lines \
+		"Signed-off-by" \
+		"Acked-by" \
+		"[ v2 updated patch description ]" \
+		"Signed-off-by" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo,keyonly) shows only key' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by,keyonly)" >actual &&
+	echo "Acked-by" >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
 	git log --no-walk --pretty="format:%(trailers:key=Acked-by,valueonly)" >actual &&
 	echo "A U Thor <author@example.com>" >expect &&
@@ -732,6 +748,12 @@ test_expect_success '%(trailers:valueonly) shows only values' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:key=foo,keyonly,valueonly) shows nothing' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by,keyonly,valueonly)" >actual &&
+	echo >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'pretty format %(trailers:separator) changes separator' '
 	git log --no-walk --pretty=format:"X%(trailers:separator=%x00)X" >actual &&
 	(
@@ -754,7 +776,7 @@ test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa
 	test_cmp expect actual
 '
 
-test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' '
+test_expect_success 'pretty format %(trailers) combining separator/key/keyonly/valueonly' '
 	git commit --allow-empty -F - <<-\EOF &&
 	Important fix
 
@@ -781,6 +803,13 @@ test_expect_success 'pretty format %(trailers) combining separator/key/valueonly
 		"Does not close any tickets" \
 		"Another fix #567, #890" \
 		"Important fix #1234" >expect &&
+	test_cmp expect actual &&
+
+	git log --pretty="%s% (trailers:separator=%x2c%x20,key=Closes,keyonly)" HEAD~3.. >actual &&
+	test_write_lines \
+		"Does not close any tickets" \
+		"Another fix Closes, Closes" \
+		"Important fix Closes" >expect &&
 	test_cmp expect actual
 '
 
diff --git a/trailer.c b/trailer.c
index d2d01015b1d..889b419a4f6 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out,
 
 	/* If we want the whole block untouched, we can take the fast path. */
 	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
-	    !opts->separator && !opts->value_only) {
+	    !opts->separator && !opts->key_only && !opts->value_only) {
 		strbuf_add(out, info->trailer_start,
 			   info->trailer_end - info->trailer_start);
 		return;
@@ -1154,8 +1154,11 @@ static void format_trailer_info(struct strbuf *out,
 				if (opts->separator && out->len != origlen)
 					strbuf_addbuf(out, opts->separator);
 				if (!opts->value_only)
-					strbuf_addf(out, "%s: ", tok.buf);
-				strbuf_addbuf(out, &val);
+					strbuf_addbuf(out, &tok);
+				if (!opts->key_only && !opts->value_only)
+					strbuf_addstr(out, ": ");
+				if (!opts->key_only)
+					strbuf_addbuf(out, &val);
 				if (!opts->separator)
 					strbuf_addch(out, '\n');
 			}
diff --git a/trailer.h b/trailer.h
index cd93e7ddea7..d2f28776be6 100644
--- a/trailer.h
+++ b/trailer.h
@@ -71,6 +71,7 @@ struct process_trailer_options {
 	int only_input;
 	int unfold;
 	int no_divider;
+	int key_only;
 	int value_only;
 	const struct strbuf *separator;
 	int (*filter)(const struct strbuf *, void *);
-- 
2.29.2.222.g5d2a92d10f8

Re: [PATCH v2 4/5] pretty format %(trailers): add a "keyonly"

[Christian Couder]

On Sun, Dec 6, 2020 at 1:25 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -282,8 +282,8 @@ option is given with no value, it's enabled.
>  ** 'unfold[=BOOL]': make it behave as if interpret-trailer's `--unfold`
>     option was given. E.g.,
>     `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
> -** 'valueonly[=BOOL]': skip over the key part of the trailer line and only
> -   show the value part.
> +** 'keyonly[=BOOL]': only show the key part of the trailer.

Here also "[=<BOOL>]" would be more consistent.

> +** 'valueonly[=BOOL]': only show the value part of the trailer.

[PATCH v2 5/5] pretty format %(trailers): add a "key_value_separator"

[Ævar Arnfjörð Bjarmason]

Add a "key_value_separator" option to the "%(trailers)" pretty format,
to go along with the existing "separator" argument. In combination
these two options make it trivial to produce machine-readable (e.g. \0
and \0\0-delimited) format output.

As elaborated on in a previous commit which added "keyonly" it was
needlessly tedious to extract structured data from "%(trailers)"
before the addition of this "key_value_separator" option. As seen by
the test being added here extracting this data now becomes trivial.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/pretty-formats.txt |  4 ++++
 pretty.c                         |  9 +++++++++
 t/t4205-log-pretty-formats.sh    | 33 ++++++++++++++++++++++++++++++++
 trailer.c                        | 11 ++++++++---
 trailer.h                        |  1 +
 5 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index bf35f7cf219..17050a78245 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -284,6 +284,10 @@ option is given with no value, it's enabled.
    `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
 ** 'keyonly[=BOOL]': only show the key part of the trailer.
 ** 'valueonly[=BOOL]': only show the value part of the trailer.
+** 'key_value_separator=<SEP>': specify a separator inserted between
+   trailer lines. When this option is not given each trailer key-value
+   pair is separated by ": ". Otherwise it shares the same semantics
+   as 'separator=<SEP>' above.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index 1237ee0e45d..05eef7fda0b 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1418,6 +1418,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
 		struct string_list filter_list = STRING_LIST_INIT_NODUP;
 		struct strbuf sepbuf = STRBUF_INIT;
+		struct strbuf kvsepbuf = STRBUF_INIT;
 		size_t ret = 0;
 
 		opts.no_divider = 1;
@@ -1449,6 +1450,14 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					strbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL);
 					free(fmt);
 					opts.separator = &sepbuf;
+				} else if (match_placeholder_arg_value(arg, "key_value_separator", &arg, &argval, &arglen)) {
+					char *fmt;
+
+					strbuf_reset(&kvsepbuf);
+					fmt = xstrndup(argval, arglen);
+					strbuf_expand(&kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);
+					free(fmt);
+					opts.key_value_separator = &kvsepbuf;
 				} else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
 					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) &&
 					   !match_placeholder_bool_arg(arg, "keyonly", &arg, &opts.key_only) &&
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 4c9f6eb7946..749bc1431ac 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -776,6 +776,39 @@ test_expect_success 'pretty format %(trailers:separator=X,unfold) changes separa
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:key_value_separator) changes key-value separator' '
+	git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00)X" >actual &&
+	(
+		printf "XSigned-off-by\0A U Thor <author@example.com>\n" &&
+		printf "Acked-by\0A U Thor <author@example.com>\n" &&
+		printf "[ v2 updated patch description ]\n" &&
+		printf "Signed-off-by\0A U Thor\n  <author@example.com>\nX"
+	) >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key_value_separator,unfold) changes key-value separator' '
+	git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00,unfold)X" >actual &&
+	(
+		printf "XSigned-off-by\0A U Thor <author@example.com>\n" &&
+		printf "Acked-by\0A U Thor <author@example.com>\n" &&
+		printf "[ v2 updated patch description ]\n" &&
+		printf "Signed-off-by\0A U Thor <author@example.com>\nX"
+	) >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:separator,key_value_separator) changes both separators' '
+	git log --no-walk --pretty=format:"%(trailers:separator=%x00,key_value_separator=%x00%x00,unfold)" >actual &&
+	(
+		printf "Signed-off-by\0\0A U Thor <author@example.com>\0" &&
+		printf "Acked-by\0\0A U Thor <author@example.com>\0" &&
+		printf "[ v2 updated patch description ]\0" &&
+		printf "Signed-off-by\0\0A U Thor <author@example.com>"
+	) >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'pretty format %(trailers) combining separator/key/keyonly/valueonly' '
 	git commit --allow-empty -F - <<-\EOF &&
 	Important fix
diff --git a/trailer.c b/trailer.c
index 889b419a4f6..249ed618ed8 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1132,7 +1132,8 @@ static void format_trailer_info(struct strbuf *out,
 
 	/* If we want the whole block untouched, we can take the fast path. */
 	if (!opts->only_trailers && !opts->unfold && !opts->filter &&
-	    !opts->separator && !opts->key_only && !opts->value_only) {
+	    !opts->separator && !opts->key_only && !opts->value_only &&
+	    !opts->key_value_separator) {
 		strbuf_add(out, info->trailer_start,
 			   info->trailer_end - info->trailer_start);
 		return;
@@ -1155,8 +1156,12 @@ static void format_trailer_info(struct strbuf *out,
 					strbuf_addbuf(out, opts->separator);
 				if (!opts->value_only)
 					strbuf_addbuf(out, &tok);
-				if (!opts->key_only && !opts->value_only)
-					strbuf_addstr(out, ": ");
+				if (!opts->key_only && !opts->value_only) {
+					if (opts->key_value_separator)
+						strbuf_addbuf(out, opts->key_value_separator);
+					else
+						strbuf_addstr(out, ": ");
+				}
 				if (!opts->key_only)
 					strbuf_addbuf(out, &val);
 				if (!opts->separator)
diff --git a/trailer.h b/trailer.h
index d2f28776be6..795d2fccfd9 100644
--- a/trailer.h
+++ b/trailer.h
@@ -74,6 +74,7 @@ struct process_trailer_options {
 	int key_only;
 	int value_only;
 	const struct strbuf *separator;
+	const struct strbuf *key_value_separator;
 	int (*filter)(const struct strbuf *, void *);
 	void *filter_data;
 };
-- 
2.29.2.222.g5d2a92d10f8

[PATCH 1/5] pretty format %(trailers) test: split a long line

[Ævar Arnfjörð Bjarmason]

Split a very long line in a test introduced in 0b691d86851 (pretty:
add support for separator option in %(trailers), 2019-01-28). This
makes it easier to read, and it'll be used as a template in follow-up
commits.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4205-log-pretty-formats.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 42544fb07a0..bf9b30ff3d6 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -723,7 +723,12 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
 
 test_expect_success 'pretty format %(trailers:separator) changes separator' '
 	git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual &&
-	printf "XSigned-off-by: A U Thor <author@example.com>\0Acked-by: A U Thor <author@example.com>\0[ v2 updated patch description ]\0Signed-off-by: A U Thor <author@example.com>X" >expect &&
+	(
+		printf "XSigned-off-by: A U Thor <author@example.com>\0" &&
+		printf "Acked-by: A U Thor <author@example.com>\0" &&
+		printf "[ v2 updated patch description ]\0" &&
+		printf "Signed-off-by: A U Thor <author@example.com>X"
+	) >expect &&
 	test_cmp expect actual
 '
 
-- 
2.29.2.222.g5d2a92d10f8

[PATCH 2/5] pretty format %(trailers): avoid needless repetition

[Ævar Arnfjörð Bjarmason]

Change the documentation for the various %(trailers) options so it
isn't repeating part of the documentation for "only" about how boolean
values are handled. Instead let's split the description of that into
general documentation at the top.

It then suffices to refer to it by listing the options as
"opt[=bool]". I'm also changing it to "[=bool]" from "[=val]". It took
me a couple of readings to realize that while to realize that these
options were referring back to the "only" option's treatment of
boolean values. Let's try to make this more explicit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/pretty-formats.txt | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 54f793d424f..8e066594624 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -252,7 +252,14 @@ endif::git-rev-list[]
 			  interpreted by
 			  linkgit:git-interpret-trailers[1]. The
 			  `trailers` string may be followed by a colon
-			  and zero or more comma-separated options:
+			  and zero or more comma-separated options.
++
+The boolean options accept an optional value. The values `true`,
+`false`, `on`, `off` etc. are all accepted. See the "boolean"
+sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
+option is given with no value it's enabled. If any option is provided
+multiple times the last occurance wins.
++
 ** 'key=<K>': only show trailers with specified key. Matching is done
    case-insensitively and trailing colon is optional. If option is
    given multiple times trailer lines matching any of the keys are
@@ -264,27 +271,21 @@ endif::git-rev-list[]
    desired it can be disabled with `only=false`.  E.g.,
    `%(trailers:key=Reviewed-by)` shows trailer lines with key
    `Reviewed-by`.
-** 'only[=val]': select whether non-trailer lines from the trailer
-   block should be included. The `only` keyword may optionally be
-   followed by an equal sign and one of `true`, `on`, `yes` to omit or
-   `false`, `off`, `no` to show the non-trailer lines. If option is
-   given without value it is enabled. If given multiple times the last
-   value is used.
+** 'only[=bool]': select whether non-trailer lines from the trailer
+   block should be included.
 ** 'separator=<SEP>': specify a separator inserted between trailer
    lines. When this option is not given each trailer line is
    terminated with a line feed character. The string SEP may contain
    the literal formatting codes described above. To use comma as
    separator one must use `%x2C` as it would otherwise be parsed as
-   next option. If separator option is given multiple times only the
-   last one is used. E.g., `%(trailers:key=Ticket,separator=%x2C )`
+   next option. E.g., `%(trailers:key=Ticket,separator=%x2C )`
    shows all trailer lines whose key is "Ticket" separated by a comma
    and a space.
-** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`
-   option was given. In same way as to for `only` it can be followed
-   by an equal sign and explicit value. E.g.,
+** 'unfold[=bool]': make it behave as if interpret-trailer's `--unfold`
+   option was given. E.g.,
    `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
-** 'valueonly[=val]': skip over the key part of the trailer line and only
-   show the value part. Also this optionally allows explicit value.
+** 'valueonly[=bool]': skip over the key part of the trailer line and only
+   show the value part.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
-- 
2.29.2.222.g5d2a92d10f8

Re: [PATCH 2/5] pretty format %(trailers): avoid needless repetition

[Christian Couder]

On Sat, Dec 5, 2020 at 2:39 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Change the documentation for the various %(trailers) options so it
> isn't repeating part of the documentation for "only" about how boolean
> values are handled. Instead let's split the description of that into
> general documentation at the top.

Great!

> It then suffices to refer to it by listing the options as
> "opt[=bool]". I'm also changing it to "[=bool]" from "[=val]".

Nice! I wonder if "[=<bool>]" or "[=<BOOL>]" might be even better as
we use "=<K>" for key and "=<SEP>" for separator.

> It took
> me a couple of readings to realize that while to realize that these
> options were referring back to the "only" option's treatment of
> boolean values. Let's try to make this more explicit.

Yeah, it's definitely an improvement.

> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -252,7 +252,14 @@ endif::git-rev-list[]
>                           interpreted by
>                           linkgit:git-interpret-trailers[1]. The
>                           `trailers` string may be followed by a colon
> -                         and zero or more comma-separated options:
> +                         and zero or more comma-separated options.
> ++
> +The boolean options accept an optional value. The values `true`,

Maybe: s/an optional value./an optional value `[=<BOOL>]`./

> +`false`, `on`, `off` etc. are all accepted. See the "boolean"
> +sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
> +option is given with no value it's enabled.

s/value it's enabled/value, it's enabled/

> +If any option is provided multiple times the last occurance wins.

It might be better to have this sentence before the above paragraph
about boolean options, as it's more general.

Thanks,
Christian.

[PATCH 3/5] pretty format %(trailers): add a "keyonly"

[Ævar Arnfjörð Bjarmason]

Add support for a "keyonly". This allows for easier parsing out of the
key and value. Before if you didn't want to make assumptions about how
the key was formatted. You'd need to parse it out as e.g.:

    --pretty=format:'%H%x00%(trailers:separator=%x00%x00)' \
                       '%x00%(trailers:separator=%x00%x00,valueonly)'

And then proceed to deduce keys by looking at those two and
subtracting the value plus the hardcoded ": " separator from the
non-valueonly %(trailers) line. Now it's possible to simply do:

    --pretty=format:'%H%x00%(trailers:separator=%x00%x00,keyonly)' \
                    '%x00%(trailers:separator=%x00%x00,valueonly)'

Which at least reduces it to a state machine where you get N keys and
correlate them with N values. Even better would be to have a way to
change the ": " delimiter to something easily machine-readable (a key
might contain ": " too). A follow-up change will add support for that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/pretty-formats.txt |  4 ++--
 pretty.c                         |  1 +
 t/t4205-log-pretty-formats.sh    | 31 ++++++++++++++++++++++++++++++-
 trailer.c                        |  7 +++++--
 trailer.h                        |  2 ++
 5 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 8e066594624..d080f0d2476 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -284,8 +284,8 @@ multiple times the last occurance wins.
 ** 'unfold[=bool]': make it behave as if interpret-trailer's `--unfold`
    option was given. E.g.,
    `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
-** 'valueonly[=bool]': skip over the key part of the trailer line and only
-   show the value part.
+** 'keyonly[=bool]': only show the key part of the trailer.
+** 'valueonly[=bool]': only show the value part of the trailer.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index 3c374abffe5..590f37489f6 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1454,6 +1454,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					opts.separator = &sepbuf;
 				} else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
 					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) &&
+					   !match_placeholder_bool_arg(arg, "keyonly", &arg, &opts.key_only) &&
 					   !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only))
 					break;
 			}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index bf9b30ff3d6..5dd080c19b2 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -715,12 +715,34 @@ test_expect_success '%(trailers:key) without value is error' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:key=foo,keyonly) shows only keys' '
+	git log --no-walk --pretty="format:%(trailers:keyonly)" >actual &&
+	test_write_lines \
+		"Signed-off-by" \
+		"Acked-by" \
+		"[ v2 updated patch description ]" \
+		"Signed-off-by" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo,keyonly) shows only key' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by,keyonly)" >actual &&
+	echo "Acked-by" >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
 	git log --no-walk --pretty="format:%(trailers:key=Acked-by,valueonly)" >actual &&
 	echo "A U Thor <author@example.com>" >expect &&
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:key=foo,keyonly,valueonly) shows nothing' '
+	git log --no-walk --pretty="format:%(trailers:key=Acked-by,keyonly,valueonly)" >actual &&
+	echo >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'pretty format %(trailers:separator) changes separator' '
 	git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual &&
 	(
@@ -732,7 +754,7 @@ test_expect_success 'pretty format %(trailers:separator) changes separator' '
 	test_cmp expect actual
 '
 
-test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' '
+test_expect_success 'pretty format %(trailers) combining separator/key/keyonly/valueonly' '
 	git commit --allow-empty -F - <<-\EOF &&
 	Important fix
 
@@ -759,6 +781,13 @@ test_expect_success 'pretty format %(trailers) combining separator/key/valueonly
 		"Does not close any tickets" \
 		"Another fix #567, #890" \
 		"Important fix #1234" >expect &&
+	test_cmp expect actual &&
+
+	git log --pretty="%s% (trailers:separator=%x2c%x20,key=Closes,keyonly)" HEAD~3.. >actual &&
+	test_write_lines \
+		"Does not close any tickets" \
+		"Another fix Closes, Closes" \
+		"Important fix Closes" >expect &&
 	test_cmp expect actual
 '
 
diff --git a/trailer.c b/trailer.c
index b00b35ea0eb..40f31e4dfc2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1233,8 +1233,11 @@ static void format_trailer_info(struct strbuf *out,
 				if (opts->separator && out->len != origlen)
 					strbuf_addbuf(out, opts->separator);
 				if (!opts->value_only)
-					strbuf_addf(out, "%s: ", tok.buf);
-				strbuf_addbuf(out, &val);
+					strbuf_addstr(out, tok.buf);
+				if (!opts->key_only && !opts->value_only)
+					strbuf_addstr(out, ": ");
+				if (!opts->key_only)
+					strbuf_addbuf(out, &val);
 				if (!opts->separator)
 					strbuf_addch(out, '\n');
 			}
diff --git a/trailer.h b/trailer.h
index aad856da8c1..d4507b4ef2a 100644
--- a/trailer.h
+++ b/trailer.h
@@ -71,9 +71,11 @@ struct process_trailer_options {
 	int only_input;
 	int unfold;
 	int no_divider;
+	int key_only;
 	int value_only;
 	int canonicalize;
 	const struct strbuf *separator;
+	const struct strbuf *key_value_separator;
 	int (*filter)(const struct strbuf *, const char *alias, void *);
 	void *filter_data;
 };
-- 
2.29.2.222.g5d2a92d10f8

Re: [PATCH 3/5] pretty format %(trailers): add a "keyonly"

[Christian Couder]

On Sat, Dec 5, 2020 at 2:39 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Add support for a "keyonly". This allows for easier parsing out of the
> key and value. Before if you didn't want to make assumptions about how
> the key was formatted. You'd need to parse it out as e.g.:
>
>     --pretty=format:'%H%x00%(trailers:separator=%x00%x00)' \
>                        '%x00%(trailers:separator=%x00%x00,valueonly)'
>
> And then proceed to deduce keys by looking at those two and
> subtracting the value plus the hardcoded ": " separator from the
> non-valueonly %(trailers) line. Now it's possible to simply do:
>
>     --pretty=format:'%H%x00%(trailers:separator=%x00%x00,keyonly)' \
>                     '%x00%(trailers:separator=%x00%x00,valueonly)'
>
> Which at least reduces it to a state machine where you get N keys and
> correlate them with N values. Even better would be to have a way to
> change the ": " delimiter to something easily machine-readable (a key
> might contain ": " too). A follow-up change will add support for that.

Well explained.

> diff --git a/trailer.c b/trailer.c
> index b00b35ea0eb..40f31e4dfc2 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1233,8 +1233,11 @@ static void format_trailer_info(struct strbuf *out,
>                                 if (opts->separator && out->len != origlen)
>                                         strbuf_addbuf(out, opts->separator);
>                                 if (!opts->value_only)
> -                                       strbuf_addf(out, "%s: ", tok.buf);
> -                               strbuf_addbuf(out, &val);
> +                                       strbuf_addstr(out, tok.buf);

Maybe `strbuf_addbuf(out, &tok);`

> +                               if (!opts->key_only && !opts->value_only)
> +                                       strbuf_addstr(out, ": ");
> +                               if (!opts->key_only)
> +                                       strbuf_addbuf(out, &val);

The above is probably correct, but it feels strange to write the key
after the separator and the value, and that the key is in a variable
called "val".

Re: [PATCH 3/5] pretty format %(trailers): add a "keyonly"

[Ævar Arnfjörð Bjarmason]

On Sat, Dec 05 2020, Christian Couder wrote:

> On Sat, Dec 5, 2020 at 2:39 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>> Add support for a "keyonly". This allows for easier parsing out of the
>> key and value. Before if you didn't want to make assumptions about how
>> the key was formatted. You'd need to parse it out as e.g.:
>>
>>     --pretty=format:'%H%x00%(trailers:separator=%x00%x00)' \
>>                        '%x00%(trailers:separator=%x00%x00,valueonly)'
>>
>> And then proceed to deduce keys by looking at those two and
>> subtracting the value plus the hardcoded ": " separator from the
>> non-valueonly %(trailers) line. Now it's possible to simply do:
>>
>>     --pretty=format:'%H%x00%(trailers:separator=%x00%x00,keyonly)' \
>>                     '%x00%(trailers:separator=%x00%x00,valueonly)'
>>
>> Which at least reduces it to a state machine where you get N keys and
>> correlate them with N values. Even better would be to have a way to
>> change the ": " delimiter to something easily machine-readable (a key
>> might contain ": " too). A follow-up change will add support for that.
>
> Well explained.
>
>> diff --git a/trailer.c b/trailer.c
>> index b00b35ea0eb..40f31e4dfc2 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -1233,8 +1233,11 @@ static void format_trailer_info(struct strbuf *out,
>>                                 if (opts->separator && out->len != origlen)
>>                                         strbuf_addbuf(out, opts->separator);
>>                                 if (!opts->value_only)
>> -                                       strbuf_addf(out, "%s: ", tok.buf);
>> -                               strbuf_addbuf(out, &val);
>> +                                       strbuf_addstr(out, tok.buf);
>
> Maybe `strbuf_addbuf(out, &tok);`

Much better, thanks.

>> +                               if (!opts->key_only && !opts->value_only)
>> +                                       strbuf_addstr(out, ": ");
>> +                               if (!opts->key_only)
>> +                                       strbuf_addbuf(out, &val);
>
> The above is probably correct, but it feels strange to write the key
> after the separator and the value, and that the key is in a variable
> called "val".

We write them in the order "key -> sep -> value". with the logic of:
    
    if (!no_key)
        write_key();
    if (!no_sep)
        write_sep();
    if (!no_value)
        write_value();

So the &val here really is the value part.
    

[PATCH 4/5] pretty-format %(trailers): fix broken standalone "valueonly"

[Ævar Arnfjörð Bjarmason]

Fix %(trailers:valueonly) being a noop due to on overly eager
optimization. When new trailer options were added they needed to be
listed at the start of the format_trailer_info() function. E.g. as was
done in 250bea0c165 (pretty: allow showing specific trailers,
2019-01-28).

When d9b936db522 (pretty: add support for "valueonly" option in
%(trailers), 2019-01-28) was added this was omitted by mistake. Thus
%(trailers:valueonly) was a noop, instead of showing only trailer
value. This wasn't caught because the tests for it always combined it
with other options.

Let's fix the bug, and switch away from this pattern requiring us to
remember to add new flags to the start of the function. Instead as
soon as we see the ":" in "%(trailers:" we skip the fast path. That
over-matches for "%(trailers:)", but I think that's OK.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 pretty.c                      |  2 ++
 t/t4205-log-pretty-formats.sh | 11 +++++++++++
 trailer.c                     |  3 +--
 trailer.h                     |  1 +
 4 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/pretty.c b/pretty.c
index 590f37489f6..d989a6ae712 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1426,6 +1426,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		opts.no_divider = 1;
 
 		if (*arg == ':') {
+			/* over-matches on %(trailers:), but that's OK */
+			opts.have_options = 1;
 			arg++;
 			for (;;) {
 				const char *argval;
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 5dd080c19b2..e1100082b34 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -737,6 +737,17 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '
 	test_cmp expect actual
 '
 
+test_expect_success '%(trailers:valueonly) shows only values' '
+	git log --no-walk --pretty="format:%(trailers:valueonly)" >actual &&
+	test_write_lines \
+		"A U Thor <author@example.com>" \
+		"A U Thor <author@example.com>" \
+		"[ v2 updated patch description ]" \
+		"A U Thor" \
+		"  <author@example.com>" >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success '%(trailers:key=foo,keyonly,valueonly) shows nothing' '
 	git log --no-walk --pretty="format:%(trailers:key=Acked-by,keyonly,valueonly)" >actual &&
 	echo >expect &&
diff --git a/trailer.c b/trailer.c
index 40f31e4dfc2..da95e1f3c66 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1206,8 +1206,7 @@ static void format_trailer_info(struct strbuf *out,
 	size_t origlen = out->len;
 	size_t i;
 
-	/* If we want the whole block untouched, we can take the fast path. */
-	if (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) {
+	if (!opts->have_options) {
 		strbuf_add(out, info->trailer_start,
 			   info->trailer_end - info->trailer_start);
 		return;
diff --git a/trailer.h b/trailer.h
index d4507b4ef2a..e348c970ce7 100644
--- a/trailer.h
+++ b/trailer.h
@@ -65,6 +65,7 @@ struct new_trailer_item {
 };
 
 struct process_trailer_options {
+	int have_options;
 	int in_place;
 	int trim_empty;
 	int only_trailers;
-- 
2.29.2.222.g5d2a92d10f8

Re: [PATCH 4/5] pretty-format %(trailers): fix broken standalone "valueonly"

[Christian Couder]

On Sat, Dec 5, 2020 at 2:39 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Fix %(trailers:valueonly) being a noop due to on overly eager

s/on/an/

> optimization. When new trailer options were added they needed to be
> listed at the start of the format_trailer_info() function. E.g. as was
> done in 250bea0c165 (pretty: allow showing specific trailers,
> 2019-01-28).

It seems that  you mean this part of the above patch:

       /* If we want the whole block untouched, we can take the fast path. */
-       if (!opts->only_trailers && !opts->unfold) {
+       if (!opts->only_trailers && !opts->unfold && !opts->filter) {

but this could perhaps be clearer with:

"When new trailer options were added, a check that the new option is
not used needed to be added at the start of the format_trailer_info()
function to see if we could take the fast path of writing the whole
trailer as is."

instead of:

"When new trailer options were added they needed to be listed at the
start of the format_trailer_info() function."

> When d9b936db522 (pretty: add support for "valueonly" option in
> %(trailers), 2019-01-28) was added this was omitted by mistake.

Maybe: s/was added this was/was added, this check was/

> Thus
> %(trailers:valueonly) was a noop, instead of showing only trailer
> value. This wasn't caught because the tests for it always combined it
> with other options.
>
> Let's fix the bug, and switch away from this pattern requiring us to
> remember to add new flags to the start of the function.

s/to add new flags/to add a new check for each new option/

> Instead as
> soon as we see the ":" in "%(trailers:" we skip the fast path. That
> over-matches for "%(trailers:)", but I think that's OK.

Yeah, I think so too. I wonder if it is worth checking that
"%(trailers:)" still works in the same way as "%(trailers)" though.

>  struct process_trailer_options {
> +       int have_options;
>         int in_place;
>         int trim_empty;
>         int only_trailers;

If all of these are booleans, we might want to save a few bytes at one
point by using bit fields.

[PATCH 5/5] pretty format %(trailers): add a "key_value_separator"

[Ævar Arnfjörð Bjarmason]

As noted in a previous commit which added "keyonly" it's needlessly
hard to use the "log" machinery to produce machine-readable output for
%(trailers). with the combination of the existing "separator" and this
new "key_value_separator" this becomes trivial, as seen by the test
being added here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/pretty-formats.txt |  4 ++++
 pretty.c                         |  9 +++++++++
 t/t4205-log-pretty-formats.sh    | 22 ++++++++++++++++++++++
 trailer.c                        |  8 ++++++--
 4 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index d080f0d2476..369d243eae9 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -286,6 +286,10 @@ multiple times the last occurance wins.
    `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
 ** 'keyonly[=bool]': only show the key part of the trailer.
 ** 'valueonly[=bool]': only show the value part of the trailer.
+** 'key_value_separator=<SEP>': specify a separator inserted between
+   trailer lines. When this option is not given each trailer key-value
+   pair separated by ": ". Otherwise it shares the same semantics as 
+   'separator=<SEP>' above.
 
 NOTE: Some placeholders may depend on other options given to the
 revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index d989a6ae712..dea77f2621a 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1421,6 +1421,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
 		struct string_list filter_list = STRING_LIST_INIT_NODUP;
 		struct strbuf sepbuf = STRBUF_INIT;
+		struct strbuf kvsepbuf = STRBUF_INIT;
 		size_t ret = 0;
 
 		opts.no_divider = 1;
@@ -1454,6 +1455,14 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 					strbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL);
 					free(fmt);
 					opts.separator = &sepbuf;
+				} else if (match_placeholder_arg_value(arg, "key_value_separator", &arg, &argval, &arglen)) {
+					char *fmt;
+
+					strbuf_reset(&kvsepbuf);
+					fmt = xstrndup(argval, arglen);
+					strbuf_expand(&kvsepbuf, fmt, strbuf_expand_literal_cb, NULL);
+					free(fmt);
+					opts.key_value_separator = &kvsepbuf;
 				} else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) &&
 					   !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) &&
 					   !match_placeholder_bool_arg(arg, "keyonly", &arg, &opts.key_only) &&
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index e1100082b34..47b3f7d67c4 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -765,6 +765,28 @@ test_expect_success 'pretty format %(trailers:separator) changes separator' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pretty format %(trailers:key_value_separator) changes key-value separator' '
+	git log --no-walk --pretty=format:"X%(trailers:key_value_separator=%x00,unfold)X" >actual &&
+	(
+		printf "XSigned-off-by\0A U Thor <author@example.com>\n" &&
+		printf "Acked-by\0A U Thor <author@example.com>\n" &&
+		printf "[ v2 updated patch description ]\n" &&
+		printf "Signed-off-by\0A U Thor <author@example.com>\nX"
+	) >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:separator,key_value_separator) changes both separators' '
+	git log --no-walk --pretty=format:"%(trailers:separator=%x00,key_value_separator=%x00%x00,unfold)" >actual &&
+	(
+		printf "Signed-off-by\0\0A U Thor <author@example.com>\0" &&
+		printf "Acked-by\0\0A U Thor <author@example.com>\0" &&
+		printf "[ v2 updated patch description ]\0" &&
+		printf "Signed-off-by\0\0A U Thor <author@example.com>"
+	) >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'pretty format %(trailers) combining separator/key/keyonly/valueonly' '
 	git commit --allow-empty -F - <<-\EOF &&
 	Important fix
diff --git a/trailer.c b/trailer.c
index da95e1f3c66..70a560647c1 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1233,8 +1233,12 @@ static void format_trailer_info(struct strbuf *out,
 					strbuf_addbuf(out, opts->separator);
 				if (!opts->value_only)
 					strbuf_addstr(out, tok.buf);
-				if (!opts->key_only && !opts->value_only)
-					strbuf_addstr(out, ": ");
+				if (!opts->key_only && !opts->value_only) {
+					if (opts->key_value_separator)
+						strbuf_addbuf(out, opts->key_value_separator);
+					else
+						strbuf_addstr(out, ": ");
+				}
 				if (!opts->key_only)
 					strbuf_addbuf(out, &val);
 				if (!opts->separator)
-- 
2.29.2.222.g5d2a92d10f8

Re: [PATCH 5/5] pretty format %(trailers): add a "key_value_separator"

[Christian Couder]

On Sat, Dec 5, 2020 at 2:39 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> As noted in a previous commit which added "keyonly" it's needlessly

Maybe add a comma after `"keyonly"` and s/"keyonly"/the "keyonly" option/

> hard to use the "log" machinery to produce machine-readable output for
> %(trailers). with the combination of the existing "separator" and this

s/with/With/
s/"separator"/"separator" option/

> new "key_value_separator" this becomes trivial, as seen by the test

s/"key_value_separator"/"key_value_separator" option,/

> being added here.

> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index d080f0d2476..369d243eae9 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -286,6 +286,10 @@ multiple times the last occurance wins.
>     `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.
>  ** 'keyonly[=bool]': only show the key part of the trailer.
>  ** 'valueonly[=bool]': only show the value part of the trailer.
> +** 'key_value_separator=<SEP>': specify a separator inserted between
> +   trailer lines. When this option is not given each trailer key-value
> +   pair separated by ": ". Otherwise it shares the same semantics as
> +   'separator=<SEP>' above.

The above is not very clear to me.

Maybe:

s/between trailer lines/between a key and its associated value/
s/each trailer key-value pair separated by/each key and its associated
value are separated by/

Re: [PATCH 5/5] pretty format %(trailers): add a "key_value_separator"

[Ævar Arnfjörð Bjarmason]

On Sat, Dec 05 2020, Christian Couder wrote:

> On Sat, Dec 5, 2020 at 2:39 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>> As noted in a previous commit which added "keyonly" it's needlessly
>
> Maybe add a comma after `"keyonly"` and s/"keyonly"/the "keyonly" option/

Thanks a lot for all the feedback. I'll work it into a v2 when I send
it.

I'll wait a bit to see if Anders W. pops up again and see if he'd like
to incorporate this into his series or submit his first/after etc.