mbox series

[v2,0/3] Unify trailers formatting logic for pretty.c and ref-filter.c

Message ID pull.726.v2.git.1611954543.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series Unify trailers formatting logic for pretty.c and ref-filter.c | expand

Message

Philippe Blain via GitGitGadget Jan. 29, 2021, 9:09 p.m. UTC
Currently, there exists a separate logic for %(trailers) in "pretty.{c,h}"
and "ref-filter.{c,h}". Both are actually doing the same thing, why not use
the same code for both of them?

This is the 2nd version of the patch series that I sent few months back. It
is focused on unifying the "%(trailers)" logic for both 'pretty.{c,h}' and
'ref-filter.{c,h}'. So, we can have one logic for trailers.

v2 changes:

 * Contains Improvements as suggested by "René Scharfe" l.s.r@web.de 1
   [https://public-inbox.org/git/bf4423d5-c0ee-6bef-59ff-fcde003ec463@web.de/]
 * A new trailer option was introduced to pretty.c when I was absent i.e
   "key_value_separator". Updated the patch series with latest changes.

Sorry for taking a long break.

Link to previous version:
https://public-inbox.org/git/pull.726.git.1599335291.gitgitgadget@gmail.com/

Hariom Verma (3):
  pretty.c: refactor trailer logic to `format_set_trailers_options()`
  pretty.c: capture invalid trailer argument
  ref-filter: use pretty.c logic for trailers

 Documentation/git-for-each-ref.txt |  39 ++++++++--
 pretty.c                           |  95 +++++++++++++----------
 pretty.h                           |  12 +++
 ref-filter.c                       |  36 +++++----
 t/t6300-for-each-ref.sh            | 119 +++++++++++++++++++++++++----
 5 files changed, 228 insertions(+), 73 deletions(-)


base-commit: e6362826a0409539642a5738db61827e5978e2e4
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-726%2Fharry-hov%2Funify-trailers-logic-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-726/harry-hov/unify-trailers-logic-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/726

Range-diff vs v1:

 1:  4180f54c1de ! 1:  fc5fd5217df pretty.c: refactor trailer logic to `format_set_trailers_options()`
     @@ Commit message
          pretty.c: refactor trailer logic to `format_set_trailers_options()`
      
          Refactored trailers formatting logic inside pretty.c to a new function
     -    `format_set_trailers_options()`.
     -
     -    Also, introduced a code to get invalid trailer arguments. As we would
     -    like to use same logic in ref-filter, it's nice to get invalid trailer
     -    argument. This will allow us to print accurate error message, while
     -    using `format_set_trailers_options()` in ref-filter.
     +    `format_set_trailers_options()`. This change will allow us to reuse
     +    the same logic in other places.
      
          Mentored-by: Christian Couder <chriscool@tuxfamily.org>
          Mentored-by: Heba Waly <heba.waly@gmail.com>
          Signed-off-by: Hariom Verma <hariom18599@gmail.com>
      
     - ## Hariom Verma via GitGitGadget (new) ##
     -
       ## pretty.c ##
      @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)
       	return 0;
     @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)
      +int format_set_trailers_options(struct process_trailer_options *opts,
      +				struct string_list *filter_list,
      +				struct strbuf *sepbuf,
     -+				const char **arg,
     -+				const char **err)
     ++				struct strbuf *kvsepbuf,
     ++				const char **arg)
      +{
      +	for (;;) {
      +		const char *argval;
      +		size_t arglen;
      +
     -+		if (**arg != ')') {
     -+			size_t vallen = strcspn(*arg, "=,)");
     -+			const char *valstart = xstrndup(*arg, vallen);
     -+			if (strcmp(valstart, "key") &&
     -+				strcmp(valstart, "separator") &&
     -+				strcmp(valstart, "only") &&
     -+				strcmp(valstart, "valueonly") &&
     -+				strcmp(valstart, "unfold")) {
     -+					*err = xstrdup(valstart);
     -+					return 1;
     -+				}
     -+			free((char *)valstart);
     -+		}
      +		if (match_placeholder_arg_value(*arg, "key", arg, &argval, &arglen)) {
      +			uintptr_t len = arglen;
      +
     @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)
      +			opts->filter_data = filter_list;
      +			opts->only_trailers = 1;
      +		} else if (match_placeholder_arg_value(*arg, "separator", arg, &argval, &arglen)) {
     -+			char *fmt = xstrndup(argval, arglen);
     ++			char *fmt;
     ++			fmt = xstrndup(argval, arglen);
      +			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;
     ++			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, "valueonly", arg, &opts->value_only))
     ++			   !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;
      +	}
      +	return 0;
     @@ pretty.c: static int format_trailer_match_cb(const struct strbuf *key, void *ud)
       				const char *placeholder,
       				void *context)
      @@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
     - 		struct string_list filter_list = STRING_LIST_INIT_NODUP;
     - 		struct strbuf sepbuf = STRBUF_INIT;
     - 		size_t ret = 0;
     -+		const char *unused = NULL;
     - 
     - 		opts.no_divider = 1;
       
       		if (*arg == ':') {
       			arg++;
     @@ pretty.c: 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) &&
      -					   !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only))
      -					break;
      -			}
     -+			if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &arg, &unused))
     ++			if (format_set_trailers_options(&opts, &filter_list, &sepbuf, &kvsepbuf, &arg))
      +				goto trailer_out;
       		}
       		if (*arg == ')') {
       			format_trailers_from_commit(sb, msg + c->subject_off, &opts);
     -@@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
     - 	trailer_out:
     - 		string_list_clear(&filter_list, 0);
     - 		strbuf_release(&sepbuf);
     -+		free((char *)unused);
     - 		return ret;
     - 	}
     - 
      
       ## pretty.h ##
      @@
     @@ pretty.h
       
       /* Commit formats */
       enum cmit_fmt {
     -@@ pretty.h: const char *format_subject(struct strbuf *sb, const char *msg,
     - /* Check if "cmit_fmt" will produce an empty output. */
     - int commit_format_is_empty(enum cmit_fmt);
     +@@ pretty.h: int commit_format_is_empty(enum cmit_fmt);
     + /* Make subject of commit message suitable for filename */
     + void format_sanitized_subject(struct strbuf *sb, const char *msg, size_t len);
       
      +/*
      + * Set values of fields in "struct process_trailer_options"
      + * according to trailers arguments.
      + */
      +int format_set_trailers_options(struct process_trailer_options *opts,
     -+				struct string_list *filter_list,
     -+				struct strbuf *sepbuf,
     -+				const char **arg,
     -+				const char **err);
     ++			struct string_list *filter_list,
     ++			struct strbuf *sepbuf,
     ++			struct strbuf *kvsepbuf,
     ++			const char **arg);
      +
       #endif /* PRETTY_H */
 -:  ----------- > 2:  245e48eb683 pretty.c: capture invalid trailer argument
 2:  a3e16298262 ! 3:  7b8cfb2721c ref-filter: using pretty.c logic for trailers
     @@ Metadata
      Author: Hariom Verma <hariom18599@gmail.com>
      
       ## Commit message ##
     -    ref-filter: using pretty.c logic for trailers
     +    ref-filter: use pretty.c logic for trailers
      
          Now, ref-filter is using pretty.c logic for setting trailer options.
      
          New to ref-filter:
            :key=<K> - only show trailers with specified key.
            :valueonly[=val] - only show the value part.
     -      :separator=<SEP> - inserted between trailer lines
     +      :separator=<SEP> - inserted between trailer lines.
     +      :key_value_separator=<SEP> - inserted between trailer lines
      
          Enhancement to existing options(now can take value and its optional):
            :only[=val]
     @@ Documentation/git-for-each-ref.txt: contents:lines=N::
      +** '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 )`
     -+   shows all trailer lines whose key is "Ticket" separated by a comma
     -+   and a space.
     ++   the literal formatting codes. 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)` shows all trailer lines
     ++   whose key is "Ticket" separated by a comma.
     ++** 'key_value_separator=<SEP>': specify a separator inserted between
     ++   key and value. The string SEP may contain the literal formatting codes.
     ++   E.g., `%(trailers:key=Ticket,key_value_separator=%x2C)` shows all trailer
     ++   lines whose key is "Ticket" with key and value separated by a comma.
      +** '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.,
     @@ ref-filter.c: struct refname_atom {
      +struct ref_trailer_buf {
      +	struct string_list filter_list;
      +	struct strbuf sepbuf;
     ++	struct strbuf kvsepbuf;
      +} ref_trailer_buf;
      +
       static struct expand_data {
     @@ ref-filter.c: static int subject_atom_parser(const struct ref_format *format, st
      -	int i;
      -
       	atom->u.contents.trailer_opts.no_divider = 1;
     --
     + 
       	if (arg) {
      -		string_list_split(&params, arg, ',', -1);
      -		for (i = 0; i < params.nr; i++) {
     @@ ref-filter.c: static int subject_atom_parser(const struct ref_format *format, st
      -				return -1;
      -			}
      +		const char *argbuf = xstrfmt("%s)", arg);
     -+		const char *err_arg = NULL;
     ++		char *invalid_arg = NULL;
      +
      +		if (format_set_trailers_options(&atom->u.contents.trailer_opts,
      +			&ref_trailer_buf.filter_list,
      +			&ref_trailer_buf.sepbuf,
     -+			&argbuf, &err_arg)) {
     -+			if (!err_arg)
     ++			&ref_trailer_buf.kvsepbuf,
     ++			&argbuf, &invalid_arg)) {
     ++			if (!invalid_arg)
      +				strbuf_addf(err, _("expected %%(trailers:key=<value>)"));
      +			else
     -+				strbuf_addf(err, _("unknown %%(trailers) argument: %s"), err_arg);
     -+			free((char *)err_arg);
     ++				strbuf_addf(err, _("unknown %%(trailers) argument: %s"), invalid_arg);
     ++			free((char *)invalid_arg);
      +			return -1;
       		}
       	}
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:unfold) unfolds trailer
      +			grep -v patch.description <trailers &&
      +			echo
      +		} >expect &&
     -+		git for-each-ref --format="%($option)" refs/heads/master >actual &&
     ++		git for-each-ref --format="%($option)" refs/heads/main >actual &&
      +		test_cmp expect actual &&
     -+		git for-each-ref --format="%(contents:$option)" refs/heads/master >actual &&
     ++		git for-each-ref --format="%(contents:$option)" refs/heads/main >actual &&
      +		test_cmp expect actual
      +	'
      +}
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:unfold) unfolds trailer
      +		cat trailers &&
       		echo
       	} >expect &&
     --	git for-each-ref --format="%(trailers:only)" refs/heads/master >actual &&
     -+	git for-each-ref --format="%(trailers:only=no)" refs/heads/master >actual &&
     +-	git for-each-ref --format="%(trailers:only)" refs/heads/main >actual &&
     ++	git for-each-ref --format="%(trailers:only=no)" refs/heads/main >actual &&
       	test_cmp expect actual &&
     --	git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual &&
     -+	git for-each-ref --format="%(contents:trailers:only=no)" refs/heads/master >actual &&
     +-	git for-each-ref --format="%(contents:trailers:only)" refs/heads/main >actual &&
     ++	git for-each-ref --format="%(contents:trailers:only=no)" refs/heads/main >actual &&
       	test_cmp expect actual
       '
       
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un
      +	expect="$3"
      +	test_expect_success "$title" '
      +		echo $expect >expect &&
     -+		git for-each-ref --format="%($option)" refs/heads/master >actual &&
     ++		git for-each-ref --format="%($option)" refs/heads/main >actual &&
      +		test_cmp expect actual &&
     -+		git for-each-ref --format="%(contents:$option)" refs/heads/master >actual &&
     ++		git for-each-ref --format="%(contents:$option)" refs/heads/main >actual &&
      +		test_cmp expect actual
      +	'
      +}
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un
      +		grep -v patch.description <trailers | grep -v Signed-off-by | grep -v Reviewed-by &&
      +		echo
      +	} >expect &&
     -+	git for-each-ref --format="%(trailers:key=Acked-by)" refs/heads/master >actual &&
     ++	git for-each-ref --format="%(trailers:key=Acked-by)" refs/heads/main >actual &&
      +	test_cmp expect actual &&
     -+	git for-each-ref --format="%(contents:trailers:key=Acked-by)" refs/heads/master >actual &&
     ++	git for-each-ref --format="%(contents:trailers:key=Acked-by)" refs/heads/main >actual &&
      +	test_cmp expect actual
      +'
      +
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un
      +		unfold <trailers | grep Signed-off-by &&
      +		echo
      +	} >expect &&
     -+	git for-each-ref --format="%(trailers:key=Signed-Off-by,unfold)" refs/heads/master >actual &&
     ++	git for-each-ref --format="%(trailers:key=Signed-Off-by,unfold)" refs/heads/main >actual &&
      +	test_cmp expect actual &&
     -+	git for-each-ref --format="%(contents:trailers:key=Signed-Off-by,unfold)" refs/heads/master >actual &&
     ++	git for-each-ref --format="%(contents:trailers:key=Signed-Off-by,unfold)" refs/heads/main >actual &&
      +	test_cmp expect actual
       '
       
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un
      +		grep patch.description <trailers &&
      +		echo
      +	} >expect &&
     -+	git for-each-ref --format="%(trailers:key=Signed-off-by,only=no)" refs/heads/master >actual &&
     ++	git for-each-ref --format="%(trailers:key=Signed-off-by,only=no)" refs/heads/main >actual &&
      +	test_cmp expect actual &&
     -+	git for-each-ref --format="%(contents:trailers:key=Signed-off-by,only=no)" refs/heads/master >actual &&
     ++	git for-each-ref --format="%(contents:trailers:key=Signed-off-by,only=no)" refs/heads/main >actual &&
      +	test_cmp expect actual
      +'
      +
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un
      +	'trailers:key=Signed-off-by,valueonly' 'A U Thor <author@example.com>\n'
      +test_trailer_option '%(trailers:separator) changes separator' \
      +	'trailers:separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by: A U Thor <author@example.com>,Signed-off-by: A U Thor <author@example.com>'
     ++test_trailer_option '%(trailers:key_value_separator) changes key-value separator' \
     ++	'trailers:key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor <author@example.com>\nSigned-off-by,A U Thor <author@example.com>\n'
     ++test_trailer_option '%(trailers:separator,key_value_separator) changes both separators' \
     ++	'trailers:separator=%x2C,key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor <author@example.com>,Signed-off-by,A U Thor <author@example.com>'
      +
      +test_failing_trailer_option () {
      +	title="$1"
     @@ t/t6300-for-each-ref.sh: test_expect_success '%(trailers:only) and %(trailers:un
      +	test_expect_success "$title" '
      +		# error message cannot be checked under i18n
      +		echo $error >expect &&
     -+		test_must_fail git for-each-ref --format="%($option)" refs/heads/master 2>actual &&
     ++		test_must_fail git for-each-ref --format="%($option)" refs/heads/main 2>actual &&
      +		test_i18ncmp expect actual &&
     -+		test_must_fail git for-each-ref --format="%(contents:$option)" refs/heads/master 2>actual &&
     ++		test_must_fail git for-each-ref --format="%(contents:$option)" refs/heads/main 2>actual &&
      +		test_i18ncmp expect actual
      +	'
      +}

Comments

Junio C Hamano Jan. 30, 2021, 1:17 a.m. UTC | #1
"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Currently, there exists a separate logic for %(trailers) in "pretty.{c,h}"
> and "ref-filter.{c,h}". Both are actually doing the same thing, why not use
> the same code for both of them?
>
> This is the 2nd version of the patch series that I sent few months back. It
> is focused on unifying the "%(trailers)" logic for both 'pretty.{c,h}' and
> 'ref-filter.{c,h}'. So, we can have one logic for trailers.
>
> v2 changes:
>
>  * Contains Improvements as suggested by "René Scharfe" l.s.r@web.de 1
>    [https://public-inbox.org/git/bf4423d5-c0ee-6bef-59ff-fcde003ec463@web.de/]
>  * A new trailer option was introduced to pretty.c when I was absent i.e
>    "key_value_separator". Updated the patch series with latest changes.

ref-filter.c:74:3: error: symbol 'ref_trailer_buf' was not declared. Should it be static?
    SP refs/packed-backend.c
    SP refs/ref-cache.c

For now, I've queued this on the tip of the topic while queuing;
running "make SPARSE_FLAGS=-Wsparse-error sparse" before sending
your series out may reduce embarrassment in the future.

Thanks.

 ref-filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index 2b1c61eadf..0e414765c1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -67,7 +67,7 @@ struct refname_atom {
 	int lstrip, rstrip;
 };
 
-struct ref_trailer_buf {
+static struct ref_trailer_buf {
 	struct string_list filter_list;
 	struct strbuf sepbuf;
 	struct strbuf kvsepbuf;
Junio C Hamano Jan. 30, 2021, 1:28 a.m. UTC | #2
With this queued directly on top of master, I am getting these
failures.

t6300-for-each-ref.sh                            (Wstat: 256 Tests: 301 Failed: 6)
  Failed tests:  277-280, 285, 287
  Non-zero exit status: 1
Hariom verma Jan. 30, 2021, 7:15 p.m. UTC | #3
Hi,

On Sat, Jan 30, 2021 at 6:58 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> With this queued directly on top of master, I am getting these
> failures.
>
> t6300-for-each-ref.sh                            (Wstat: 256 Tests: 301 Failed: 6)
>   Failed tests:  277-280, 285, 287
>   Non-zero exit status: 1
>

That's strange!

All 301 tests seem to work fine in my system.

I'm clueless about these failures and can't think of any possible reason too.

Is there any way I can re-generate this or can trace the part of code
which caused these failures?

Thanks,
Hariom.
Junio C Hamano Jan. 30, 2021, 8:20 p.m. UTC | #4
Junio C Hamano <gitster@pobox.com> writes:

> With this queued directly on top of master, I am getting these
> failures.
>
> t6300-for-each-ref.sh                            (Wstat: 256 Tests: 301 Failed: 6)
>   Failed tests:  277-280, 285, 287
>   Non-zero exit status: 1

Judging from the way the first failure happens, it appears that
some code depends on a kind of shell portability issues?

The failure is under GNU bash, version 5.1.4(1)-release (x86_64-pc-linux-gnu)

Ah, I think I know what is going on.

test_trailer_option() {
	title="$1"
	option="$2"
	expect="$3"
	test_expect_success "$title" '
		echo $expect >expect &&
		git for-each-ref --format="%($option)" refs/heads/main >actual &&
		test_cmp expect actual &&
		git for-each-ref --format="%(contents:$option)" refs/heads/main >actual &&
		test_cmp expect actual
	'
}

Not quoting "$expect" given to 'echo' inside double-quote is
criminal, but I do not think that contributes to this particular
failure.  The problem is in the callers.

test_trailer_option '%(trailers:key=foo) shows that trailer' \
	'trailers:key=Signed-off-by' 'Signed-off-by: A U Thor <author@example.com>\n'


Doing

	expect='foo\n'
	echo $expect

and expecting that somebody would magically turn \n to a true
newline is the bug.  Not everybody's builtin "echo" works that way.

Here is a quick fix-up.  I didn't check which parts are to be blamed
on which patch of yours, if any, so you might need to split them up
into multiple pieces (i.e. a preliminary clean-up patch, and a patch
each to be applied to part N/3 (1 <= N <= 3) of your patch).


 t/t6300-for-each-ref.sh | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git c/t/t6300-for-each-ref.sh w/t/t6300-for-each-ref.sh
index a8835b1391..0278cb9924 100755
--- c/t/t6300-for-each-ref.sh
+++ w/t/t6300-for-each-ref.sh
@@ -874,7 +874,7 @@ test_trailer_option() {
 	option="$2"
 	expect="$3"
 	test_expect_success "$title" '
-		echo $expect >expect &&
+		echo "$expect" >expect &&
 		git for-each-ref --format="%($option)" refs/heads/main >actual &&
 		test_cmp expect actual &&
 		git for-each-ref --format="%(contents:$option)" refs/heads/main >actual &&
@@ -883,13 +883,18 @@ test_trailer_option() {
 }
 
 test_trailer_option '%(trailers:key=foo) shows that trailer' \
-	'trailers:key=Signed-off-by' 'Signed-off-by: A U Thor <author@example.com>\n'
+	'trailers:key=Signed-off-by' 'Signed-off-by: A U Thor <author@example.com>
+'
 test_trailer_option '%(trailers:key=foo) is case insensitive' \
-	'trailers:key=SiGned-oFf-bY' 'Signed-off-by: A U Thor <author@example.com>\n'
+	'trailers:key=SiGned-oFf-bY' 'Signed-off-by: A U Thor <author@example.com>
+'
 test_trailer_option '%(trailers:key=foo:) trailing colon also works' \
-	'trailers:key=Signed-off-by:' 'Signed-off-by: A U Thor <author@example.com>\n'
+	'trailers:key=Signed-off-by:' 'Signed-off-by: A U Thor <author@example.com>
+'
 test_trailer_option '%(trailers:key=foo) multiple keys' \
-	'trailers:key=Reviewed-by:,key=Signed-off-by' 'Reviewed-by: A U Thor <author@example.com>\nSigned-off-by: A U Thor <author@example.com>\n'
+	'trailers:key=Reviewed-by:,key=Signed-off-by' 'Reviewed-by: A U Thor <author@example.com>
+Signed-off-by: A U Thor <author@example.com>
+'
 test_trailer_option '%(trailers:key=nonexistent) becomes empty' \
 	'trailers:key=Shined-off-by:' ''
 
@@ -928,11 +933,14 @@ test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes non
 '
 
 test_trailer_option '%(trailers:key=foo,valueonly) shows only value' \
-	'trailers:key=Signed-off-by,valueonly' 'A U Thor <author@example.com>\n'
+	'trailers:key=Signed-off-by,valueonly' 'A U Thor <author@example.com>
+'
 test_trailer_option '%(trailers:separator) changes separator' \
 	'trailers:separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by: A U Thor <author@example.com>,Signed-off-by: A U Thor <author@example.com>'
 test_trailer_option '%(trailers:key_value_separator) changes key-value separator' \
-	'trailers:key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor <author@example.com>\nSigned-off-by,A U Thor <author@example.com>\n'
+	'trailers:key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor <author@example.com>
+Signed-off-by,A U Thor <author@example.com>
+'
 test_trailer_option '%(trailers:separator,key_value_separator) changes both separators' \
 	'trailers:separator=%x2C,key_value_separator=%x2C,key=Reviewed-by,key=Signed-off-by:' 'Reviewed-by,A U Thor <author@example.com>,Signed-off-by,A U Thor <author@example.com>'