Questions about trailer configuration semantics

From: Anders Waldenborg <anders@0x63.nu>
Date: Jul 27, 2020, 16:45 UTC
Message-ID: <87blk0rjob.fsf@0x63.nu>

I noticed some undocumented and (at least to me) surprising behavior in trailers.c.

When configuring a value in trailer.<token>.key it causes the trailer to
be normalized to that in "git interpret-trailers --parse".
E.g:
 $ printf '\naCKed: Zz\n' | \
   git -c 'trailer.Acked.key=Acked' interpret-trailers --parse
 will emit: "Acked: Zz"

but only if "key" is used, other config options doesn't cause it to be
normalized.
E.g:
 $ printf '\naCKed: Zz\n' | \
   git -c 'trailer.Acked.ifmissing=doNothing' interpret-trailers --parse
 will emit: "aCKed: Zz" (still lowercase a and uppercase CK)

Then there is the replacement by config "trailer.fix.key=Fixes" which
expands "fix" to "Fixes". This happens when using "--trailer 'fix = 123'"
which seems to be expected and useful behavior (albeit a bit unclear in
documentation). But it also happens when parsing incoming trailers, e.g
with that config
 $ printf "\nFix: 1\n" | git interpret-trailers --parse
 will emit: "Fixes: 1"

(token_from_item prefers order .key, incoming token, .name)

The most surprising thing is that it uses prefix matching when finding they key in configuration. If I have "trailer.reviewed.key=Reviewed-By" it is possible to just '--trailer r=XYZ' and it will find the reviewed-by trailer as "r" is a prefix of reviewedby. This also applies to the "--parse". This in makes it impossible to have trailer keys that are prefix of each other (e.g: "Acked", "Acked-Tests", "Acked-Docs") if there is multiple matching in configuration it will just pick the one that happens to come first.

(token_matches_item uses strncasecmp with token length)

I guess these are the questions for the above observations:

* Should normalization of spelling happen at all?

* If so should it only happen when there is a .key config?

* Should replacement to what is in .key happen also in --parse mode, or
  only for "--trailer"

* The prefix matching gotta be a bug, right?

Here is a patch to the tests showing these things.

From 49a4bb64a7ebf1f2d50897a024deb86b4f8056b1 Mon Sep 17 00:00:00 2001
From: Anders Waldenborg <anders@0x63.nu>
Date: Mon, 27 Jul 2020 18:34:37 +0200
Subject: [PATCH] trailers: add tests for unclear/undocumented behavior

---
 t/t7513-interpret-trailers.sh | 70 +++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 2e6d406edf..d5d19cf89b 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -99,6 +99,64 @@ test_expect_success 'with config option on the command line' '
 	test_cmp expected actual
 '

+test_expect_success 'parse normalizes spelling and separators from configs with key' '
+	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
+'
+
+# 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 --parse 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 --parse patch >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'with only a title in the message' '
 	cat >expected <<-\EOF &&
 		area: change
@@ -473,6 +531,18 @@ test_expect_success 'with config setup' '
 	test_cmp expected actual
 '

+# currently this matches the "Acked-by: " value in ack key set by previous test
+test_expect_failure 'with config setup matches key exactly' '
+	cat >expected <<-\EOF &&
+
+		A: B
+	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= " &&
--
2.25.1