Re: [PATCH 3/5] interpret-trailers: add an option to show only existing trailers

On Wed, Aug 09, 2017 at 11:38:20AM -0700, Jonathan Tan wrote:

quoted

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index e5b0718ef6..525fd53e5b 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh

@@ -1312,4 +1312,19 @@ test_expect_success 'only-trailers omits non-trailer in middle of block' '
 	test_cmp expected actual
 '
 
+test_expect_success 'only existing' '
+	cat >expected <<-\EOF &&
+		existing: existing-value
+	EOF
+	git interpret-trailers \
+		--only-trailers --only-existing >actual <<-\EOF &&
+		my subject
+
+		my body
+
+		existing: existing-value
+	EOF
+	test_cmp expected actual

Would it be worth asserting that the "sign" trailer is configured here
and would normally appear? Maybe through a grep on the output of "git
config".

I'd much rather re-add it than assert it with grep hackery. Note that
its presence is implied in all of the follow-on tests, too (so I had
sort of assumed that its presence in the --only-trailers test would
imply that it was carried through to the others). We can be more
explicit, though, I guess.

quoted

diff --git a/trailer.c b/trailer.c
index a4ff99f98a..88f6efe523 100644
--- a/trailer.c
+++ b/trailer.c

@@ -991,9 +991,10 @@ void process_trailers(const char *file, struct process_trailer_options *opts,
 	trailer_end = process_input_file(opts->only_trailers ? NULL : outfile,
 					 sb.buf, &head);
 
-	process_command_line_args(&arg_head, trailers);
-
-	process_trailers_lists(&head, &arg_head);
+	if (!opts->only_existing) {
+		process_command_line_args(&arg_head, trailers);
+		process_trailers_lists(&head, &arg_head);
+	}

This is a bit confusing, but it is correct, since
process_command_line_args() processes both configured trailers and
command-line trailers.

Yes, it confused me, too. That combination is why "--trailer" is
disallowed with --only-existing (which otherwise could work together). I
didn't think it was worth refactoring for a case that I don't think
anybody would care about.

Having said that, it might be worth declaring LIST_HEAD(arg_head) inside
the if block now.

Agreed.

-Peff