git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] Fixes to trailer test script, help text, and documentation
@ 2023-08-05  4:45 Linus Arver via GitGitGadget
  2023-08-05  4:45 ` [PATCH 1/5] trailer tests: make test cases self-contained Linus Arver via GitGitGadget
                   ` (5 more replies)
  0 siblings, 6 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-05  4:45 UTC (permalink / raw)
  To: git; +Cc: Linus Arver

This series contains various fixes to the trailer code. They pertain to
fixes to the test script, the command line help text for the
interpret-trailers builtin, and the documentation.

Patch 1 is the biggest one, but also perhaps the most important as it does
cleanups in the tests where we used 'git config' in a test case without
cleaning up that state for the next test. This makes the tests
self-contained, making it easier to add new tests anywhere along the script,
without worrying about previously-set implicit state. These test cleanups
exposed lots of cases where the test cases are mutating more configuration
state than is necessary to test the specific behavior in the test; however
such extraneous configurations were not cleaned up to make these patches
easier to review (again, we are not changing any behavior and we are also
not changing what the test cases themselves purport to do).

Note that Patch 1 was originally a 22-commit series, but was squashed
together to make it easier to see the final diff for each test case. You can
see the 22-commit breakdown at
https://github.com/listx/git/tree/backup-trailer-22-commit-breakdown

Patch 3 adds some tests to check the behavior of '--no-if-exists' and
'--no-if-missing', which weren't previously tested. It also adds
similarly-themed test cases for '--no-where' which only had 1 test case for
it.

The other patches aren't as important, but are included here because I think
they are too small to include in a separate series.

Linus Arver (5):
  trailer tests: make test cases self-contained
  trailer test description: this tests --where=after, not --where=before
  trailer: add tests to check defaulting behavior with --no-* flags
  trailer: trailer location is a place, not an action
  trailer --no-divider help: describe usual "---" meaning

 Documentation/git-interpret-trailers.txt |  14 +-
 builtin/interpret-trailers.c             |   4 +-
 t/t7513-interpret-trailers.sh            | 506 +++++++++++++++++++----
 3 files changed, 443 insertions(+), 81 deletions(-)


base-commit: 1b0a5129563ebe720330fdc8f5c6843d27641137
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1564%2Flistx%2Ftrailer-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1564/listx/trailer-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1564
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH 1/5] trailer tests: make test cases self-contained
  2023-08-05  4:45 [PATCH 0/5] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
@ 2023-08-05  4:45 ` Linus Arver via GitGitGadget
  2023-08-07  5:50   ` Linus Arver
  2023-08-05  4:45 ` [PATCH 2/5] trailer test description: this tests --where=after, not --where=before Linus Arver via GitGitGadget
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-05  4:45 UTC (permalink / raw)
  To: git; +Cc: Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

By using "test_config" instead of "git config", we avoid leaking
configuration state across test cases. This in turn helps to make the
tests more self-contained, by explicitly capturing the configuration
setup. It then makes it easier to add tests anywhere in this 1500+ line
file, without worrying about what implicit state was set in some prior
test case defined earlier up in the script.

This commit was created mechanically as follows: we changed the first
occurrence of a particular "git config trailer.*" option, then ran the
tests repeatedly to see which ones broke, adding in the extra
"test_config" equivalents to make them pass again. This was done until
there were no more unbridled "git config" invocations. Some "git config"
invocations still do exist in the script, but they were already cleaned
up properly with

    test_when_finished "git config --remove-section ..."

so they were left alone.

Note that these cleanups result in generally longer test case setups
because the previously hidden state is now being exposed. Although we
could then clean up the test cases' "expected" values to be less
verbose (the verbosity arising from the use of implicit state), we
choose not to do so here, to make sure that this cleanup does not change
any meanings behind the test cases.

Signed-off-by: Linus Arver <linusa@google.com>
---
 t/t7513-interpret-trailers.sh | 374 +++++++++++++++++++++++++++-------
 1 file changed, 300 insertions(+), 74 deletions(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 97f10905d23..5b31896070a 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -489,7 +489,7 @@ test_expect_success 'multiline field treated as atomic for neighbor check' '
 '
 
 test_expect_success 'with config setup' '
-	git config trailer.ack.key "Acked-by: " &&
+	test_config trailer.ack.key "Acked-by: " &&
 	cat >expected <<-\EOF &&
 
 		Acked-by: Peff
@@ -503,8 +503,8 @@ test_expect_success 'with config setup' '
 '
 
 test_expect_success 'with config setup and ":=" as separators' '
-	git config trailer.separators ":=" &&
-	git config trailer.ack.key "Acked-by= " &&
+	test_config trailer.separators ":=" &&
+	test_config trailer.ack.key "Acked-by= " &&
 	cat >expected <<-\EOF &&
 
 		Acked-by= Peff
@@ -518,7 +518,7 @@ test_expect_success 'with config setup and ":=" as separators' '
 '
 
 test_expect_success 'with config setup and "%" as separators' '
-	git config trailer.separators "%" &&
+	test_config trailer.separators "%" &&
 	cat >expected <<-\EOF &&
 
 		bug% 42
@@ -532,6 +532,7 @@ test_expect_success 'with config setup and "%" as separators' '
 '
 
 test_expect_success 'with "%" as separators and a message with trailers' '
+	test_config trailer.separators "%" &&
 	cat >special_message <<-\EOF &&
 		Special Message
 
@@ -553,8 +554,8 @@ test_expect_success 'with "%" as separators and a message with trailers' '
 '
 
 test_expect_success 'with config setup and ":=#" as separators' '
-	git config trailer.separators ":=#" &&
-	git config trailer.bug.key "Bug #" &&
+	test_config trailer.separators ":=#" &&
+	test_config trailer.bug.key "Bug #" &&
 	cat >expected <<-\EOF &&
 
 		Bug #42
@@ -581,6 +582,8 @@ test_expect_success 'with basic patch' '
 '
 
 test_expect_success 'with commit complex message as argument' '
+	test_config trailer.separators ":=" &&
+	test_config trailer.ack.key "Acked-by= " &&
 	cat complex_message_body complex_message_trailers >complex_message &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
@@ -594,6 +597,8 @@ test_expect_success 'with commit complex message as argument' '
 '
 
 test_expect_success 'with 2 files arguments' '
+	test_config trailer.separators ":=" &&
+	test_config trailer.ack.key "Acked-by= " &&
 	cat basic_message >>expected &&
 	echo >>expected &&
 	cat basic_patch >>expected &&
@@ -677,6 +682,9 @@ test_expect_success 'with message that has an old style conflict block' '
 '
 
 test_expect_success 'with commit complex message and trailer args' '
+	test_config trailer.separators ":=#" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -692,6 +700,9 @@ test_expect_success 'with commit complex message and trailer args' '
 '
 
 test_expect_success 'with complex patch, args and --trim-empty' '
+	test_config trailer.separators ":=#" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
 	cat complex_message >complex_patch &&
 	cat basic_patch >>complex_patch &&
 	cat complex_message_body >expected &&
@@ -746,7 +757,10 @@ test_expect_success POSIXPERM,SANITY "in-place editing doesn't clobber original
 '
 
 test_expect_success 'using "where = before"' '
-	git config trailer.bug.where "before" &&
+	test_config trailer.separators ":=#" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -762,7 +776,9 @@ test_expect_success 'using "where = before"' '
 '
 
 test_expect_success 'overriding configuration with "--where after"' '
-	git config trailer.ack.where "before" &&
+	test_config trailer.separators ":=" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "before" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -777,6 +793,11 @@ test_expect_success 'overriding configuration with "--where after"' '
 '
 
 test_expect_success 'using "where = before" with "--no-where"' '
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "before" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -792,7 +813,11 @@ test_expect_success 'using "where = before" with "--no-where"' '
 '
 
 test_expect_success 'using "where = after"' '
-	git config trailer.ack.where "after" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -808,8 +833,11 @@ test_expect_success 'using "where = after"' '
 '
 
 test_expect_success 'using "where = end"' '
-	git config trailer.review.key "Reviewed-by" &&
-	git config trailer.review.where "end" &&
+	test_config trailer.review.key "Reviewed-by" &&
+	test_config trailer.review.where "end" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -827,8 +855,11 @@ test_expect_success 'using "where = end"' '
 '
 
 test_expect_success 'using "where = start"' '
-	git config trailer.review.key "Reviewed-by" &&
-	git config trailer.review.where "start" &&
+	test_config trailer.review.key "Reviewed-by" &&
+	test_config trailer.review.where "start" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Reviewed-by: Johannes
@@ -846,8 +877,13 @@ test_expect_success 'using "where = start"' '
 '
 
 test_expect_success 'using "where = before" for a token in the middle of the message' '
-	git config trailer.review.key "Reviewed-by:" &&
-	git config trailer.review.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.review.where "before" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -864,6 +900,12 @@ test_expect_success 'using "where = before" for a token in the middle of the mes
 '
 
 test_expect_success 'using "where = before" and --trim-empty' '
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	cat >>expected <<-\EOF &&
 		Bug #46
@@ -878,6 +920,13 @@ test_expect_success 'using "where = before" and --trim-empty' '
 '
 
 test_expect_success 'the default is "ifExists = addIfDifferentNeighbor"' '
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.review.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -896,7 +945,13 @@ test_expect_success 'the default is "ifExists = addIfDifferentNeighbor"' '
 '
 
 test_expect_success 'default "ifExists" is now "addIfDifferent"' '
-	git config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -914,8 +969,14 @@ test_expect_success 'default "ifExists" is now "addIfDifferent"' '
 '
 
 test_expect_success 'using "ifExists = addIfDifferent" with "where = end"' '
-	git config trailer.ack.ifExists "addIfDifferent" &&
-	git config trailer.ack.where "end" &&
+	test_config trailer.ack.ifExists "addIfDifferent" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "end" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -932,8 +993,14 @@ test_expect_success 'using "ifExists = addIfDifferent" with "where = end"' '
 '
 
 test_expect_success 'using "ifExists = addIfDifferent" with "where = before"' '
-	git config trailer.ack.ifExists "addIfDifferent" &&
-	git config trailer.ack.where "before" &&
+	test_config trailer.ack.ifExists "addIfDifferent" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "before" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -950,8 +1017,14 @@ test_expect_success 'using "ifExists = addIfDifferent" with "where = before"' '
 '
 
 test_expect_success 'using "ifExists = addIfDifferentNeighbor" with "where = end"' '
-	git config trailer.ack.ifExists "addIfDifferentNeighbor" &&
-	git config trailer.ack.where "end" &&
+	test_config trailer.ack.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "end" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -973,8 +1046,14 @@ test_expect_success 'using "ifExists = addIfDifferentNeighbor" with "where = end
 '
 
 test_expect_success 'using "ifExists = addIfDifferentNeighbor"  with "where = after"' '
-	git config trailer.ack.ifExists "addIfDifferentNeighbor" &&
-	git config trailer.ack.where "after" &&
+	test_config trailer.ack.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -995,7 +1074,11 @@ test_expect_success 'using "ifExists = addIfDifferentNeighbor"  with "where = af
 '
 
 test_expect_success 'using "ifExists = addIfDifferentNeighbor" and --trim-empty' '
-	git config trailer.ack.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.ack.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	cat >>expected <<-\EOF &&
 		Bug #42
@@ -1011,8 +1094,14 @@ test_expect_success 'using "ifExists = addIfDifferentNeighbor" and --trim-empty'
 '
 
 test_expect_success 'using "ifExists = add" with "where = end"' '
-	git config trailer.ack.ifExists "add" &&
-	git config trailer.ack.where "end" &&
+	test_config trailer.ack.ifExists "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "end" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1036,8 +1125,14 @@ test_expect_success 'using "ifExists = add" with "where = end"' '
 '
 
 test_expect_success 'using "ifExists = add" with "where = after"' '
-	git config trailer.ack.ifExists "add" &&
-	git config trailer.ack.where "after" &&
+	test_config trailer.ack.ifExists "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1058,8 +1153,15 @@ test_expect_success 'using "ifExists = add" with "where = after"' '
 '
 
 test_expect_success 'overriding configuration with "--if-exists replace"' '
-	git config trailer.fix.key "Fixes: " &&
-	git config trailer.fix.ifExists "add" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.review.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1075,8 +1177,15 @@ test_expect_success 'overriding configuration with "--if-exists replace"' '
 '
 
 test_expect_success 'using "ifExists = replace"' '
-	git config trailer.fix.key "Fixes: " &&
-	git config trailer.fix.ifExists "replace" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "replace" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1095,7 +1204,16 @@ test_expect_success 'using "ifExists = replace"' '
 '
 
 test_expect_success 'using "ifExists = replace" with "where = after"' '
-	git config trailer.fix.where "after" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "replace" &&
+	test_config trailer.fix.where "after" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1114,7 +1232,15 @@ test_expect_success 'using "ifExists = replace" with "where = after"' '
 '
 
 test_expect_success 'using "ifExists = doNothing"' '
-	git config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1133,8 +1259,17 @@ test_expect_success 'using "ifExists = doNothing"' '
 '
 
 test_expect_success 'the default is "ifMissing = add"' '
-	git config trailer.cc.key "Cc: " &&
-	git config trailer.cc.where "before" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.cc.key "Cc: " &&
+	test_config trailer.cc.where "before" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1154,7 +1289,14 @@ test_expect_success 'the default is "ifMissing = add"' '
 '
 
 test_expect_success 'overriding configuration with "--if-missing doNothing"' '
-	git config trailer.ifmissing "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.ifmissing "add" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -1173,7 +1315,13 @@ test_expect_success 'overriding configuration with "--if-missing doNothing"' '
 '
 
 test_expect_success 'when default "ifMissing" is "doNothing"' '
-	git config trailer.ifmissing "doNothing" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.ifmissing "doNothing" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -1187,14 +1335,21 @@ test_expect_success 'when default "ifMissing" is "doNothing"' '
 		--trailer "cc=Linus" --trailer "ack: Junio" \
 		--trailer "fix=22" --trailer "bug: 42" --trailer "ack: Peff" \
 		<complex_message >actual &&
-	test_cmp expected actual &&
-	git config trailer.ifmissing "add"
+	test_cmp expected actual
 '
 
 test_expect_success 'using "ifMissing = add" with "where = end"' '
-	git config trailer.cc.key "Cc: " &&
-	git config trailer.cc.where "end" &&
-	git config trailer.cc.ifMissing "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.cc.key "Cc: " &&
+	test_config trailer.cc.ifMissing "add" &&
+	test_config trailer.cc.where "end" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1214,9 +1369,17 @@ test_expect_success 'using "ifMissing = add" with "where = end"' '
 '
 
 test_expect_success 'using "ifMissing = add" with "where = before"' '
-	git config trailer.cc.key "Cc: " &&
-	git config trailer.cc.where "before" &&
-	git config trailer.cc.ifMissing "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.cc.key "Cc: " &&
+	test_config trailer.cc.ifMissing "add" &&
+	test_config trailer.cc.where "before" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Cc: Linus
@@ -1236,7 +1399,15 @@ test_expect_success 'using "ifMissing = add" with "where = before"' '
 '
 
 test_expect_success 'using "ifMissing = doNothing"' '
-	git config trailer.cc.ifMissing "doNothing" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.cc.ifMissing "doNothing" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1256,7 +1427,15 @@ test_expect_success 'using "ifMissing = doNothing"' '
 
 test_expect_success 'default "where" is now "after"' '
 	git config trailer.where "after" &&
-	git config --unset trailer.ack.where &&
+	test_config trailer.ack.ifExists "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1280,10 +1459,15 @@ test_expect_success 'default "where" is now "after"' '
 '
 
 test_expect_success 'with simple command' '
-	git config trailer.sign.key "Signed-off-by: " &&
-	git config trailer.sign.where "after" &&
-	git config trailer.sign.ifExists "addIfDifferentNeighbor" &&
-	git config trailer.sign.command "echo \"A U Thor <author@example.com>\"" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"A U Thor <author@example.com>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.sign.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.sign.where "after" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -1298,8 +1482,14 @@ test_expect_success 'with simple command' '
 '
 
 test_expect_success 'with command using committer information' '
-	git config trailer.sign.ifExists "addIfDifferent" &&
-	git config trailer.sign.command "echo \"\$GIT_COMMITTER_NAME <\$GIT_COMMITTER_EMAIL>\"" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"\$GIT_COMMITTER_NAME <\$GIT_COMMITTER_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.sign.ifExists "addIfDifferent" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -1314,10 +1504,15 @@ test_expect_success 'with command using committer information' '
 '
 
 test_expect_success 'with command using author information' '
-	git config trailer.sign.key "Signed-off-by: " &&
-	git config trailer.sign.where "after" &&
-	git config trailer.sign.ifExists "addIfDifferentNeighbor" &&
-	git config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.sign.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.sign.where "after" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -1338,12 +1533,19 @@ test_expect_success 'setup a commit' '
 '
 
 test_expect_success 'cmd takes precedence over command' '
-	test_when_finished "git config --unset trailer.fix.cmd" &&
-	git config trailer.fix.ifExists "replace" &&
-	git config trailer.fix.cmd "test -n \"\$1\" && git log -1 --oneline --format=\"%h (%aN)\" \
-	--abbrev-commit --abbrev=14 \"\$1\" || true" &&
-	git config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" \
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" \
 		--abbrev-commit --abbrev=14 \$ARG" &&
+	test_config trailer.fix.cmd "test -n \"\$1\" && git log -1 --oneline --format=\"%h (%aN)\" \
+	--abbrev-commit --abbrev=14 \"\$1\" || true" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "replace" &&
+	test_config trailer.fix.where "after" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	FIXED=$(git log -1 --oneline --format="%h (%aN)" --abbrev-commit --abbrev=14 HEAD) &&
 	cat complex_message_body >expected2 &&
 	sed -e "s/ Z\$/ /" >>expected2 <<-EOF &&
@@ -1359,8 +1561,16 @@ test_expect_success 'cmd takes precedence over command' '
 '
 
 test_expect_success 'with command using $ARG' '
-	git config trailer.fix.ifExists "replace" &&
-	git config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$ARG" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$ARG" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "replace" &&
+	test_config trailer.fix.where "after" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	FIXED=$(git log -1 --oneline --format="%h (%s)" --abbrev-commit --abbrev=14 HEAD) &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-EOF &&
@@ -1376,8 +1586,16 @@ test_expect_success 'with command using $ARG' '
 '
 
 test_expect_success 'with failing command using $ARG' '
-	git config trailer.fix.ifExists "replace" &&
-	git config trailer.fix.command "false \$ARG" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.command "false \$ARG" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "replace" &&
+	test_config trailer.fix.where "after" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-EOF &&
 		Fixes: Z
@@ -1392,7 +1610,9 @@ test_expect_success 'with failing command using $ARG' '
 '
 
 test_expect_success 'with empty tokens' '
-	git config --unset trailer.fix.command &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.ifexists "addIfDifferent" &&
 	cat >expected <<-EOF &&
 
 		Signed-off-by: A U Thor <author@example.com>
@@ -1403,7 +1623,8 @@ test_expect_success 'with empty tokens' '
 '
 
 test_expect_success 'with command but no key' '
-	git config --unset trailer.sign.key &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.ifexists "addIfDifferent" &&
 	cat >expected <<-EOF &&
 
 		sign: A U Thor <author@example.com>
@@ -1414,7 +1635,9 @@ test_expect_success 'with command but no key' '
 '
 
 test_expect_success 'with no command and no key' '
-	git config --unset trailer.review.key &&
+	test_config trailer.review.where "before" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.ifexists "addIfDifferent" &&
 	cat >expected <<-EOF &&
 
 		review: Junio
@@ -1426,6 +1649,8 @@ test_expect_success 'with no command and no key' '
 '
 
 test_expect_success 'with cut line' '
+	test_config trailer.review.where "before" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
 	cat >expected <<-\EOF &&
 		my subject
 
@@ -1443,7 +1668,8 @@ test_expect_success 'with cut line' '
 '
 
 test_expect_success 'only trailers' '
-	git config trailer.sign.command "echo config-value" &&
+	test_config trailer.sign.command "echo config-value" &&
+	test_config trailer.ifexists "addIfDifferent" &&
 	cat >expected <<-\EOF &&
 		existing: existing-value
 		sign: config-value
@@ -1462,7 +1688,7 @@ test_expect_success 'only trailers' '
 '
 
 test_expect_success 'only-trailers omits non-trailer in middle of block' '
-	git config trailer.sign.command "echo config-value" &&
+	test_config trailer.sign.command "echo config-value" &&
 	cat >expected <<-\EOF &&
 		Signed-off-by: nobody <nobody@nowhere>
 		Signed-off-by: somebody <somebody@somewhere>
@@ -1482,7 +1708,7 @@ test_expect_success 'only-trailers omits non-trailer in middle of block' '
 '
 
 test_expect_success 'only input' '
-	git config trailer.sign.command "echo config-value" &&
+	test_config trailer.sign.command "echo config-value" &&
 	cat >expected <<-\EOF &&
 		existing: existing-value
 	EOF
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH 2/5] trailer test description: this tests --where=after, not --where=before
  2023-08-05  4:45 [PATCH 0/5] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
  2023-08-05  4:45 ` [PATCH 1/5] trailer tests: make test cases self-contained Linus Arver via GitGitGadget
@ 2023-08-05  4:45 ` Linus Arver via GitGitGadget
  2023-08-05  4:45 ` [PATCH 3/5] trailer: add tests to check defaulting behavior with --no-* flags Linus Arver via GitGitGadget
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-05  4:45 UTC (permalink / raw)
  To: git; +Cc: Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

Signed-off-by: Linus Arver <linusa@google.com>
---
 t/t7513-interpret-trailers.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 5b31896070a..ed0fc04bd95 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -792,7 +792,7 @@ test_expect_success 'overriding configuration with "--where after"' '
 	test_cmp expected actual
 '
 
-test_expect_success 'using "where = before" with "--no-where"' '
+test_expect_success 'using "--where after" with "--no-where"' '
 	test_config trailer.ack.key "Acked-by= " &&
 	test_config trailer.ack.where "before" &&
 	test_config trailer.bug.key "Bug #" &&
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH 3/5] trailer: add tests to check defaulting behavior with --no-* flags
  2023-08-05  4:45 [PATCH 0/5] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
  2023-08-05  4:45 ` [PATCH 1/5] trailer tests: make test cases self-contained Linus Arver via GitGitGadget
  2023-08-05  4:45 ` [PATCH 2/5] trailer test description: this tests --where=after, not --where=before Linus Arver via GitGitGadget
@ 2023-08-05  4:45 ` Linus Arver via GitGitGadget
  2023-08-07  1:13   ` Junio C Hamano
  2023-08-05  4:45 ` [PATCH 4/5] trailer: trailer location is a place, not an action Linus Arver via GitGitGadget
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-05  4:45 UTC (permalink / raw)
  To: git; +Cc: Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

While the "--no-where" flag is tested, the "--no-if-exists" and
"--no-if-missing" flags are not, so add tests for them. But also add
tests for all "--no-*" flags to check their effects, both when (1) there
are relevant configuration variables set, and (2) they are not set.

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt |  14 ++-
 t/t7513-interpret-trailers.sh            | 130 +++++++++++++++++++++++
 2 files changed, 140 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 55d89614661..91a4dbc9a72 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -114,8 +114,10 @@ OPTIONS
 	Specify where all new trailers will be added.  A setting
 	provided with '--where' overrides all configuration variables
 	and applies to all '--trailer' options until the next occurrence of
-	'--where' or '--no-where'. Possible values are `after`, `before`,
-	`end` or `start`.
+	'--where' or '--no-where'. Upon encountering '--no-where', clear the
+	effect of any previous use of '--where', such that the relevant configuration
+	variables are no longer overridden. Possible values are `after`,
+	`before`, `end` or `start`.
 
 --if-exists <action>::
 --no-if-exists::
@@ -123,7 +125,9 @@ OPTIONS
 	least one trailer with the same <token> in the input.  A setting
 	provided with '--if-exists' overrides all configuration variables
 	and applies to all '--trailer' options until the next occurrence of
-	'--if-exists' or '--no-if-exists'. Possible actions are `addIfDifferent`,
+	'--if-exists' or '--no-if-exists'. Upon encountering '--no-if-exists, clear the
+	effect of any previous use of '--if-exists, such that the relevant configuration
+	variables are no longer overridden. Possible actions are `addIfDifferent`,
 	`addIfDifferentNeighbor`, `add`, `replace` and `doNothing`.
 
 --if-missing <action>::
@@ -132,7 +136,9 @@ OPTIONS
 	trailer with the same <token> in the input.  A setting
 	provided with '--if-missing' overrides all configuration variables
 	and applies to all '--trailer' options until the next occurrence of
-	'--if-missing' or '--no-if-missing'. Possible actions are `doNothing`
+	'--if-missing' or '--no-if-missing'. Upon encountering '--no-if-missing,
+	clear the effect of any previous use of '--if-missing, such that the relevant
+	configuration variables are no longer overridden. Possible actions are `doNothing`
 	or `add`.
 
 --only-trailers::
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index ed0fc04bd95..832aff06167 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -812,6 +812,53 @@ test_expect_success 'using "--where after" with "--no-where"' '
 	test_cmp expected actual
 '
 
+# Check whether using "--no-where" clears out only the "--where after", such
+# that we still use the configuration in trailer.where (which is different from
+# the hardcoded default (in WHERE_END) assuming the absence of .gitconfig).
+# Here, the "start" setting of trailer.where is respected, so the new "Acked-by"
+# and "Bug" trailers are placed at the beginning, and not at the end which is
+# the harcoded default.
+test_expect_success 'using "--where after" with "--no-where" defaults to configuration' '
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.separators ":=#" &&
+	test_config trailer.where "start" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Bug #42
+		Acked-by= Peff
+		Fixes: Z
+		Acked-by= Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers --where after --no-where --trailer "ack: Peff" \
+		--trailer "bug: 42" complex_message >actual &&
+	test_cmp expected actual
+'
+
+# The "--where after" will only get respected for the trailer that came
+# immediately after it. For the next trailer (Bug #42), we default to using the
+# hardcoded WHERE_END because we don't have any "trailer.where" or
+# "trailer.bug.where" configured.
+test_expect_success 'using "--no-where" defaults to harcoded default if nothing configured' '
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.separators ":=#" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by= Z
+		Acked-by= Peff
+		Reviewed-by: Z
+		Signed-off-by: Z
+		Bug #42
+	EOF
+	git interpret-trailers --where after --trailer "ack: Peff" --no-where \
+		--trailer "bug: 42" complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'using "where = after"' '
 	test_config trailer.ack.key "Acked-by= " &&
 	test_config trailer.ack.where "after" &&
@@ -1176,6 +1223,56 @@ test_expect_success 'overriding configuration with "--if-exists replace"' '
 	test_cmp expected actual
 '
 
+# "trailer.ifexists" is set to "doNothing", so using "--no-if-exists" defaults
+# to this "doNothing" behavior. So the "Fixes: 53" trailer does not get added.
+test_expect_success 'using "--if-exists replace" with "--no-if-exists" defaults to configuration' '
+	test_config trailer.ifexists "doNothing" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by: Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers --if-exists replace --no-if-exists --trailer "Fixes: 53" \
+		<complex_message >actual &&
+	test_cmp expected actual
+'
+
+# No "ifexists" configuration is set, so using "--no-if-exists" makes it default
+# to addIfDifferentNeighbor. Because we do have a different neighbor "Fixes: 53"
+# (because it got added by overriding with "--if-exists replace" earlier in the
+# arguments list), we add "Signed-off-by: addme".
+test_expect_success 'using "--no-if-exists" defaults to hardcoded default if nothing configured' '
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Acked-by: Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+		Fixes: 53
+		Signed-off-by: addme
+	EOF
+	git interpret-trailers --if-exists replace --trailer "Fixes: 53" --no-if-exists \
+		--trailer "Signed-off-by: addme" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+# The second "Fixes: 53" trailer is discarded, because the "--no-if-exists" here
+# makes us default to addIfDifferentNeighbor, and we already added the "Fixes:
+# 53" trailer earlier in the argument list.
+test_expect_success 'using "--no-if-exists" defaults to hardcoded default if nothing configured (no addition)' '
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Acked-by: Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+		Fixes: 53
+	EOF
+	git interpret-trailers --if-exists replace --trailer "Fixes: 53" --no-if-exists \
+		--trailer "Fixes: 53" <complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'using "ifExists = replace"' '
 	test_config trailer.fix.key "Fixes: " &&
 	test_config trailer.fix.ifExists "replace" &&
@@ -1425,6 +1522,39 @@ test_expect_success 'using "ifMissing = doNothing"' '
 	test_cmp expected actual
 '
 
+# Ignore the "IgnoredTrailer" because of "--if-missing doNothing", but also
+# ignore the "StillIgnoredTrailer" because we set "trailer.ifMissing" to
+# "doNothing" in configuration.
+test_expect_success 'using "--no-if-missing" defaults to configuration' '
+	test_config trailer.ifMissing "doNothing" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+			Fixes: Z
+			Acked-by: Z
+			Reviewed-by: Z
+			Signed-off-by: Z
+	EOF
+	git interpret-trailers --if-missing doNothing --trailer "IgnoredTrailer: ignoreme" --no-if-missing \
+			--trailer "StillIgnoredTrailer: ignoreme" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+# Add the "AddedTrailer" because the "--no-if-missing" clears the "--if-missing
+# doNothing" from earlier in the argument list.
+test_expect_success 'using "--no-if-missing" defaults to hardcoded default if nothing configured' '
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+			Fixes: Z
+			Acked-by: Z
+			Reviewed-by: Z
+			Signed-off-by: Z
+			AddedTrailer: addme
+	EOF
+	git interpret-trailers --if-missing doNothing --trailer "IgnoredTrailer: ignoreme" --no-if-missing \
+			--trailer "AddedTrailer: addme" <complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'default "where" is now "after"' '
 	git config trailer.where "after" &&
 	test_config trailer.ack.ifExists "add" &&
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH 4/5] trailer: trailer location is a place, not an action
  2023-08-05  4:45 [PATCH 0/5] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
                   ` (2 preceding siblings ...)
  2023-08-05  4:45 ` [PATCH 3/5] trailer: add tests to check defaulting behavior with --no-* flags Linus Arver via GitGitGadget
@ 2023-08-05  4:45 ` Linus Arver via GitGitGadget
  2023-08-05  4:45 ` [PATCH 5/5] trailer --no-divider help: describe usual "---" meaning Linus Arver via GitGitGadget
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
  5 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-05  4:45 UTC (permalink / raw)
  To: git; +Cc: Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

Fix the help text to say "placement" instead of "action" because the
values are placements, not actions.

While we're at it, tweak the documentation to say "placements" instead
of "values", similar to how the existing language for "--if-exists" uses
the word "action" to describe both the syntax (with the phrase
"--if-exists <action>") and the possible values (with the phrase
"possible actions").

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 2 +-
 builtin/interpret-trailers.c             | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 91a4dbc9a72..c9b1e60251d 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -116,7 +116,7 @@ OPTIONS
 	and applies to all '--trailer' options until the next occurrence of
 	'--where' or '--no-where'. Upon encountering '--no-where', clear the
 	effect of any previous use of '--where', such that the relevant configuration
-	variables are no longer overridden. Possible values are `after`,
+	variables are no longer overridden. Possible placements are `after`,
 	`before`, `end` or `start`.
 
 --if-exists <action>::
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index c5e83452654..cf4f703c4e2 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -97,7 +97,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 
-		OPT_CALLBACK(0, "where", NULL, N_("action"),
+		OPT_CALLBACK(0, "where", NULL, N_("placement"),
 			     N_("where to place the new trailer"), option_parse_where),
 		OPT_CALLBACK(0, "if-exists", NULL, N_("action"),
 			     N_("action if trailer already exists"), option_parse_if_exists),
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH 5/5] trailer --no-divider help: describe usual "---" meaning
  2023-08-05  4:45 [PATCH 0/5] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
                   ` (3 preceding siblings ...)
  2023-08-05  4:45 ` [PATCH 4/5] trailer: trailer location is a place, not an action Linus Arver via GitGitGadget
@ 2023-08-05  4:45 ` Linus Arver via GitGitGadget
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
  5 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-05  4:45 UTC (permalink / raw)
  To: git; +Cc: Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

It's unclear what treating something "specially" means.

Signed-off-by: Linus Arver <linusa@google.com>
---
 builtin/interpret-trailers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index cf4f703c4e2..a7623dbfb2e 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -109,7 +109,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "unfold", &opts.unfold, N_("join whitespace-continued values")),
 		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")),
+		OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
 		OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add"), option_parse_trailer),
 		OPT_END()
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* Re: [PATCH 3/5] trailer: add tests to check defaulting behavior with --no-* flags
  2023-08-05  4:45 ` [PATCH 3/5] trailer: add tests to check defaulting behavior with --no-* flags Linus Arver via GitGitGadget
@ 2023-08-07  1:13   ` Junio C Hamano
  2023-08-07  5:28     ` Linus Arver
  0 siblings, 1 reply; 52+ messages in thread
From: Junio C Hamano @ 2023-08-07  1:13 UTC (permalink / raw)
  To: Linus Arver via GitGitGadget; +Cc: git, Linus Arver

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:

> @@ -114,8 +114,10 @@ OPTIONS
>  	Specify where all new trailers will be added.  A setting
>  	provided with '--where' overrides all configuration variables

Obviously this is not a new issue, but "all configuration variables"
is misleading (the same comment applies to the description of the
"--[no-]if-exists" and the "--[no-]if-missing" options).

If I am reading the code correctly, --where=value overrides the
trailer.where variable and nothing else, and --no-where stops the
overriding of the trailer.where variable.  Ditto for the other two
with their relevant configuration variables.


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH 3/5] trailer: add tests to check defaulting behavior with --no-* flags
  2023-08-07  1:13   ` Junio C Hamano
@ 2023-08-07  5:28     ` Linus Arver
  2023-08-07  5:37       ` Linus Arver
  2023-08-07  6:35       ` Linus Arver
  0 siblings, 2 replies; 52+ messages in thread
From: Linus Arver @ 2023-08-07  5:28 UTC (permalink / raw)
  To: Junio C Hamano, Linus Arver via GitGitGadget; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> @@ -114,8 +114,10 @@ OPTIONS
>>  	Specify where all new trailers will be added.  A setting
>>  	provided with '--where' overrides all configuration variables
>
> Obviously this is not a new issue, but "all configuration variables"
> is misleading (the same comment applies to the description of the
> "--[no-]if-exists" and the "--[no-]if-missing" options).

Agreed.

> If I am reading the code correctly, --where=value overrides the
> trailer.where variable and nothing else, and --no-where stops the
> overriding of the trailer.where variable.  Ditto for the other two
> with their relevant configuration variables.

That is also my understanding. Will update to remove the "all" wording.

On a separate note, I've realized there are more fixes to be done in
this area (as I get more familiar with the codebase). For example, we
have the following language in builtin/interpret-trailers.c inside
cmd_interpret_trailers():

    OPT_BOOL(0, "only-input", &opts.only_input, N_("do not apply config rules")),

which should be fixed in similar style to what you suggested above,
probably with:

    OPT_BOOL(0, "only-input", &opts.only_input, N_("do not apply trailer.* configuration variables")),

When I reroll, I will include these additional fixes so expect the patch
series to grow (probably ~12 patches instead of the ~5).

One more thing. I think the documentation
(Documentation/git-interpret-trailers.txt) uses the word "<token>" in
two different ways. For example, if we have in the input

    subject line

    body text

    Acked-by: Foo

the docs treat the word "Acked-by:" as the <token>. However, it defines
the relevant configuration section like this:

    trailer.<token>.key::
            This `key` will be used instead of <token> in the trailer. At
            the end of this key, a separator can appear and then some
            space characters. By default the only valid separator is ':',
            but this can be changed using the `trailer.separators` config
            variable.
    +
    If there is a separator, then the key will be used instead of both the
    <token> and the default separator when adding the trailer.

So if I configure this like

   git config trailer.ack.key "Acked-by" &&

the <token> is both the longer-form "Acked-by:" (per the meaning so far
in the doc) but also the shorter string "ack" per the
"trailer.<token>.key" configuration section syntax. This secondary
meaning is repeated again in the very start of the doc when we define
the --trailer option syntax as

    SYNOPSIS
    --------
    [verse]
    'git interpret-trailers' [--in-place] [--trim-empty]
                [(--trailer <token>[(=|:)<value>])...]
                [--parse] [<file>...]

because the <token> here could be (using the example above) either
"Acked-by" (as in "--trailer=Acked-by:...") if we did not configure
"trailer.ack.key", or just "ack" (as in "--trailer=ack:...") if we did
configure it. These two scenarios would give identical "Acked-by: ..."
output.

This is confusing and I don't like how we overload this "token" word
(not to mention we already have the word "key" which we don't really use
much in the docs).

I am inclined to replace most uses of the word "<token>" with "<key>"
while leaving the "trailer.<token>.key" configuration syntax intact.
This will result in a large diff but I think the removal of the double
meaning is worth it, and will include this fix also in the next reroll.

The main reason I bring this up is because this means also having to
update our funciton names like "token_len_without_separator" in
trailer.c, to be "key_len_without_separator" if we want the nomenclature
in the trailer.c internals to be consistent with the (updated)
user-facing docs. I am not sure whether we want to do this as part of
the same reroll, or if we should leave it as #leftoverbits for a future
series.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH 3/5] trailer: add tests to check defaulting behavior with --no-* flags
  2023-08-07  5:28     ` Linus Arver
@ 2023-08-07  5:37       ` Linus Arver
  2023-08-07  6:35       ` Linus Arver
  1 sibling, 0 replies; 52+ messages in thread
From: Linus Arver @ 2023-08-07  5:37 UTC (permalink / raw)
  To: Junio C Hamano, Linus Arver via GitGitGadget; +Cc: git

Linus Arver <linusa@google.com> writes:

> So if I configure this like
>
>    git config trailer.ack.key "Acked-by" &&
>

Oops, I meant just

    git config trailer.ack.key "Acked-by"

without the trailing "&&".

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH 1/5] trailer tests: make test cases self-contained
  2023-08-05  4:45 ` [PATCH 1/5] trailer tests: make test cases self-contained Linus Arver via GitGitGadget
@ 2023-08-07  5:50   ` Linus Arver
  0 siblings, 0 replies; 52+ messages in thread
From: Linus Arver @ 2023-08-07  5:50 UTC (permalink / raw)
  To: Linus Arver via GitGitGadget, git

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Linus Arver <linusa@google.com>
> ...
> @@ -1392,7 +1610,9 @@ test_expect_success 'with failing command using $ARG' '
>  '
>  
>  test_expect_success 'with empty tokens' '
> -	git config --unset trailer.fix.command &&
> +	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
> +	test_config trailer.sign.key "Signed-off-by: " &&
> +	test_config trailer.ifexists "addIfDifferent" &&
>  	cat >expected <<-EOF &&
>  
>  		Signed-off-by: A U Thor <author@example.com>

In this test and some other places we get the chance to remove
invocations of "git config --unset ..." (because we don't leak config
state anymore). I will update the commit message accordingly in the next
reroll.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH 3/5] trailer: add tests to check defaulting behavior with --no-* flags
  2023-08-07  5:28     ` Linus Arver
  2023-08-07  5:37       ` Linus Arver
@ 2023-08-07  6:35       ` Linus Arver
  2023-08-07 15:53         ` Junio C Hamano
  1 sibling, 1 reply; 52+ messages in thread
From: Linus Arver @ 2023-08-07  6:35 UTC (permalink / raw)
  To: Junio C Hamano, Linus Arver via GitGitGadget; +Cc: git

Linus Arver <linusa@google.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> @@ -114,8 +114,10 @@ OPTIONS
>>>  	Specify where all new trailers will be added.  A setting
>>>  	provided with '--where' overrides all configuration variables
>>
>> Obviously this is not a new issue, but "all configuration variables"
>> is misleading (the same comment applies to the description of the
>> "--[no-]if-exists" and the "--[no-]if-missing" options).
>
> Agreed.
>
>> If I am reading the code correctly, --where=value overrides the
>> trailer.where variable and nothing else, and --no-where stops the
>> overriding of the trailer.where variable.  Ditto for the other two
>> with their relevant configuration variables.
>
> That is also my understanding. Will update to remove the "all" wording.

Hmph, actually it also overrides any applicable "trailer.<token>.where"
configurations (these <token>-specific configurations override the
"trailer.where" configuration where applicable). Still, the "all
configuration variables" wording should be updated, probably like this:

    ›  Specify where all new trailers will be added.  A setting
    ›  provided with '--where' overrides the `trailer.where` and any
    ›  applicable `trailer.<token>.where` configuration variables
    ›  and applies to all '--trailer' options until the next occurrence of
    ›  '--where' or '--no-where'.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH 3/5] trailer: add tests to check defaulting behavior with --no-* flags
  2023-08-07  6:35       ` Linus Arver
@ 2023-08-07 15:53         ` Junio C Hamano
  0 siblings, 0 replies; 52+ messages in thread
From: Junio C Hamano @ 2023-08-07 15:53 UTC (permalink / raw)
  To: Linus Arver; +Cc: Linus Arver via GitGitGadget, git

Linus Arver <linusa@google.com> writes:

>>> If I am reading the code correctly, --where=value overrides the
>>> trailer.where variable and nothing else, and --no-where stops the
>>> overriding of the trailer.where variable.  Ditto for the other two
>>> with their relevant configuration variables.
>>
>> That is also my understanding. Will update to remove the "all" wording.
>
> Hmph, actually it also overrides any applicable "trailer.<token>.where"
> configurations (these <token>-specific configurations override the
> "trailer.where" configuration where applicable). Still, the "all
> configuration variables" wording should be updated, probably like this:
>
>     ›  Specify where all new trailers will be added.  A setting
>     ›  provided with '--where' overrides the `trailer.where` and any
>     ›  applicable `trailer.<token>.where` configuration variables
>     ›  and applies to all '--trailer' options until the next occurrence of
>     ›  '--where' or '--no-where'.

Yup, that was what I meant.  I found it troubling that the "all"
phrasing felt like it meant all trailer.* configurations, not just
the "where" thing.  Your new description looks good.

Thanks.


^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation
  2023-08-05  4:45 [PATCH 0/5] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
                   ` (4 preceding siblings ...)
  2023-08-05  4:45 ` [PATCH 5/5] trailer --no-divider help: describe usual "---" meaning Linus Arver via GitGitGadget
@ 2023-08-10 21:17 ` Linus Arver via GitGitGadget
  2023-08-10 21:17   ` [PATCH v2 01/13] trailer tests: make test cases self-contained Linus Arver via GitGitGadget
                     ` (14 more replies)
  5 siblings, 15 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-10 21:17 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver

This series contains various fixes to the trailer code. They pertain to
fixes to the test script, the command line help text for the
interpret-trailers builtin, and the documentation.

Patch 1 is the most important as it does cleanups in the tests where we used
'git config' in a test case without cleaning up that state for the next
test. This makes the tests self-contained, making it easier to add new tests
anywhere along the script, without worrying about previously-set implicit
state. These test cleanups exposed lots of cases where the test cases are
mutating more configuration state than is necessary to test the specific
behavior in the test; however such extraneous configurations were not
cleaned up to make these patches easier to review (again, we are not
changing any behavior and we are also not changing what the test cases
themselves purport to do).

Note that Patch 1 was originally a 22-commit series, but was squashed
together to make it easier to see the final diff for each test case. You can
see the 22-commit breakdown at
https://github.com/listx/git/tree/backup-trailer-22-commit-breakdown

Patch 3 adds some tests to check the behavior of '--no-if-exists' and
'--no-if-missing', which weren't previously tested. It also adds
similarly-themed test cases for '--no-where' which only had 1 test case for
it.

The other patches aren't as important, but are included here because I think
they are too small to include in a separate series.


Updates in v2
=============

 * Many additional patches to fix the help text and docs. No changes to any
   of the patches touching the actual tests (that is, Patch 1 and 3 have
   stayed the same, other than a rewording of the commit message for Patch
   1).
 * Of these new patches, I think the last one () is the most important as it
   resolves a longtime ambiguity about what a can be.

Linus Arver (13):
  trailer tests: make test cases self-contained
  trailer test description: this tests --where=after, not --where=before
  trailer: add tests to check defaulting behavior with --no-* flags
  trailer doc: narrow down scope of --where and related flags
  trailer: trailer location is a place, not an action
  trailer --no-divider help: describe usual "---" meaning
  trailer --parse help: expose aliased options
  trailer --only-input: prefer "configuration variables" over "rules"
  trailer --parse docs: add explanation for its usefulness
  trailer --unfold help: prefer "reformat" over "join"
  trailer doc: emphasize the effect of configuration variables
  trailer doc: separator within key suppresses default separator
  trailer doc: <token> is a <key> or <keyAlias>, not both

 Documentation/git-interpret-trailers.txt | 183 ++++----
 builtin/interpret-trailers.c             |  10 +-
 t/t7513-interpret-trailers.sh            | 506 +++++++++++++++++++----
 3 files changed, 544 insertions(+), 155 deletions(-)


base-commit: 1b0a5129563ebe720330fdc8f5c6843d27641137
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1564%2Flistx%2Ftrailer-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1564/listx/trailer-fixes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1564

Range-diff vs v1:

  1:  6d67ae6b1f6 !  1:  1623dd000dd trailer tests: make test cases self-contained
     @@ Commit message
          This commit was created mechanically as follows: we changed the first
          occurrence of a particular "git config trailer.*" option, then ran the
          tests repeatedly to see which ones broke, adding in the extra
     -    "test_config" equivalents to make them pass again. This was done until
     -    there were no more unbridled "git config" invocations. Some "git config"
     -    invocations still do exist in the script, but they were already cleaned
     -    up properly with
     +    "test_config" equivalents to make them pass again. In addition, in some
     +    test cases we removed "git config --unset ..." lines because they were
     +    no longer necessary (as the --unset was being used to clean up leaked
     +    configuration state from earlier test cases).
     +
     +    The process described above was done repeatedly until there were no more
     +    unbridled "git config" invocations. Some "git config" invocations still
     +    do exist in the script, but they were already cleaned up properly with
      
              test_when_finished "git config --remove-section ..."
      
  2:  100a2297fa3 =  2:  f680e76de84 trailer test description: this tests --where=after, not --where=before
  3:  6b427b4b1e8 =  3:  4b5c458ef43 trailer: add tests to check defaulting behavior with --no-* flags
  -:  ----------- >  4:  0df12c5c2dd trailer doc: narrow down scope of --where and related flags
  4:  53adcd9bf14 =  5:  040766861e2 trailer: trailer location is a place, not an action
  5:  68bc89beb2a =  6:  3e58b6f5ea2 trailer --no-divider help: describe usual "---" meaning
  -:  ----------- >  7:  d1780a0127a trailer --parse help: expose aliased options
  -:  ----------- >  8:  5cfff52da8f trailer --only-input: prefer "configuration variables" over "rules"
  -:  ----------- >  9:  ef6b77016cd trailer --parse docs: add explanation for its usefulness
  -:  ----------- > 10:  a08d78618ba trailer --unfold help: prefer "reformat" over "join"
  -:  ----------- > 11:  4db823ac354 trailer doc: emphasize the effect of configuration variables
  -:  ----------- > 12:  66087eaf5bd trailer doc: separator within key suppresses default separator
  -:  ----------- > 13:  7b66cf29d29 trailer doc: <token> is a <key> or <keyAlias>, not both

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH v2 01/13] trailer tests: make test cases self-contained
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
@ 2023-08-10 21:17   ` Linus Arver via GitGitGadget
  2023-08-10 21:17   ` [PATCH v2 02/13] trailer test description: this tests --where=after, not --where=before Linus Arver via GitGitGadget
                     ` (13 subsequent siblings)
  14 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-10 21:17 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

By using "test_config" instead of "git config", we avoid leaking
configuration state across test cases. This in turn helps to make the
tests more self-contained, by explicitly capturing the configuration
setup. It then makes it easier to add tests anywhere in this 1500+ line
file, without worrying about what implicit state was set in some prior
test case defined earlier up in the script.

This commit was created mechanically as follows: we changed the first
occurrence of a particular "git config trailer.*" option, then ran the
tests repeatedly to see which ones broke, adding in the extra
"test_config" equivalents to make them pass again. In addition, in some
test cases we removed "git config --unset ..." lines because they were
no longer necessary (as the --unset was being used to clean up leaked
configuration state from earlier test cases).

The process described above was done repeatedly until there were no more
unbridled "git config" invocations. Some "git config" invocations still
do exist in the script, but they were already cleaned up properly with

    test_when_finished "git config --remove-section ..."

so they were left alone.

Note that these cleanups result in generally longer test case setups
because the previously hidden state is now being exposed. Although we
could then clean up the test cases' "expected" values to be less
verbose (the verbosity arising from the use of implicit state), we
choose not to do so here, to make sure that this cleanup does not change
any meanings behind the test cases.

Signed-off-by: Linus Arver <linusa@google.com>
---
 t/t7513-interpret-trailers.sh | 374 +++++++++++++++++++++++++++-------
 1 file changed, 300 insertions(+), 74 deletions(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 97f10905d23..5b31896070a 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -489,7 +489,7 @@ test_expect_success 'multiline field treated as atomic for neighbor check' '
 '
 
 test_expect_success 'with config setup' '
-	git config trailer.ack.key "Acked-by: " &&
+	test_config trailer.ack.key "Acked-by: " &&
 	cat >expected <<-\EOF &&
 
 		Acked-by: Peff
@@ -503,8 +503,8 @@ test_expect_success 'with config setup' '
 '
 
 test_expect_success 'with config setup and ":=" as separators' '
-	git config trailer.separators ":=" &&
-	git config trailer.ack.key "Acked-by= " &&
+	test_config trailer.separators ":=" &&
+	test_config trailer.ack.key "Acked-by= " &&
 	cat >expected <<-\EOF &&
 
 		Acked-by= Peff
@@ -518,7 +518,7 @@ test_expect_success 'with config setup and ":=" as separators' '
 '
 
 test_expect_success 'with config setup and "%" as separators' '
-	git config trailer.separators "%" &&
+	test_config trailer.separators "%" &&
 	cat >expected <<-\EOF &&
 
 		bug% 42
@@ -532,6 +532,7 @@ test_expect_success 'with config setup and "%" as separators' '
 '
 
 test_expect_success 'with "%" as separators and a message with trailers' '
+	test_config trailer.separators "%" &&
 	cat >special_message <<-\EOF &&
 		Special Message
 
@@ -553,8 +554,8 @@ test_expect_success 'with "%" as separators and a message with trailers' '
 '
 
 test_expect_success 'with config setup and ":=#" as separators' '
-	git config trailer.separators ":=#" &&
-	git config trailer.bug.key "Bug #" &&
+	test_config trailer.separators ":=#" &&
+	test_config trailer.bug.key "Bug #" &&
 	cat >expected <<-\EOF &&
 
 		Bug #42
@@ -581,6 +582,8 @@ test_expect_success 'with basic patch' '
 '
 
 test_expect_success 'with commit complex message as argument' '
+	test_config trailer.separators ":=" &&
+	test_config trailer.ack.key "Acked-by= " &&
 	cat complex_message_body complex_message_trailers >complex_message &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
@@ -594,6 +597,8 @@ test_expect_success 'with commit complex message as argument' '
 '
 
 test_expect_success 'with 2 files arguments' '
+	test_config trailer.separators ":=" &&
+	test_config trailer.ack.key "Acked-by= " &&
 	cat basic_message >>expected &&
 	echo >>expected &&
 	cat basic_patch >>expected &&
@@ -677,6 +682,9 @@ test_expect_success 'with message that has an old style conflict block' '
 '
 
 test_expect_success 'with commit complex message and trailer args' '
+	test_config trailer.separators ":=#" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -692,6 +700,9 @@ test_expect_success 'with commit complex message and trailer args' '
 '
 
 test_expect_success 'with complex patch, args and --trim-empty' '
+	test_config trailer.separators ":=#" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
 	cat complex_message >complex_patch &&
 	cat basic_patch >>complex_patch &&
 	cat complex_message_body >expected &&
@@ -746,7 +757,10 @@ test_expect_success POSIXPERM,SANITY "in-place editing doesn't clobber original
 '
 
 test_expect_success 'using "where = before"' '
-	git config trailer.bug.where "before" &&
+	test_config trailer.separators ":=#" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -762,7 +776,9 @@ test_expect_success 'using "where = before"' '
 '
 
 test_expect_success 'overriding configuration with "--where after"' '
-	git config trailer.ack.where "before" &&
+	test_config trailer.separators ":=" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "before" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -777,6 +793,11 @@ test_expect_success 'overriding configuration with "--where after"' '
 '
 
 test_expect_success 'using "where = before" with "--no-where"' '
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "before" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -792,7 +813,11 @@ test_expect_success 'using "where = before" with "--no-where"' '
 '
 
 test_expect_success 'using "where = after"' '
-	git config trailer.ack.where "after" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -808,8 +833,11 @@ test_expect_success 'using "where = after"' '
 '
 
 test_expect_success 'using "where = end"' '
-	git config trailer.review.key "Reviewed-by" &&
-	git config trailer.review.where "end" &&
+	test_config trailer.review.key "Reviewed-by" &&
+	test_config trailer.review.where "end" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -827,8 +855,11 @@ test_expect_success 'using "where = end"' '
 '
 
 test_expect_success 'using "where = start"' '
-	git config trailer.review.key "Reviewed-by" &&
-	git config trailer.review.where "start" &&
+	test_config trailer.review.key "Reviewed-by" &&
+	test_config trailer.review.where "start" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Reviewed-by: Johannes
@@ -846,8 +877,13 @@ test_expect_success 'using "where = start"' '
 '
 
 test_expect_success 'using "where = before" for a token in the middle of the message' '
-	git config trailer.review.key "Reviewed-by:" &&
-	git config trailer.review.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.review.where "before" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -864,6 +900,12 @@ test_expect_success 'using "where = before" for a token in the middle of the mes
 '
 
 test_expect_success 'using "where = before" and --trim-empty' '
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	cat >>expected <<-\EOF &&
 		Bug #46
@@ -878,6 +920,13 @@ test_expect_success 'using "where = before" and --trim-empty' '
 '
 
 test_expect_success 'the default is "ifExists = addIfDifferentNeighbor"' '
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.review.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -896,7 +945,13 @@ test_expect_success 'the default is "ifExists = addIfDifferentNeighbor"' '
 '
 
 test_expect_success 'default "ifExists" is now "addIfDifferent"' '
-	git config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -914,8 +969,14 @@ test_expect_success 'default "ifExists" is now "addIfDifferent"' '
 '
 
 test_expect_success 'using "ifExists = addIfDifferent" with "where = end"' '
-	git config trailer.ack.ifExists "addIfDifferent" &&
-	git config trailer.ack.where "end" &&
+	test_config trailer.ack.ifExists "addIfDifferent" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "end" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -932,8 +993,14 @@ test_expect_success 'using "ifExists = addIfDifferent" with "where = end"' '
 '
 
 test_expect_success 'using "ifExists = addIfDifferent" with "where = before"' '
-	git config trailer.ack.ifExists "addIfDifferent" &&
-	git config trailer.ack.where "before" &&
+	test_config trailer.ack.ifExists "addIfDifferent" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "before" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -950,8 +1017,14 @@ test_expect_success 'using "ifExists = addIfDifferent" with "where = before"' '
 '
 
 test_expect_success 'using "ifExists = addIfDifferentNeighbor" with "where = end"' '
-	git config trailer.ack.ifExists "addIfDifferentNeighbor" &&
-	git config trailer.ack.where "end" &&
+	test_config trailer.ack.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "end" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -973,8 +1046,14 @@ test_expect_success 'using "ifExists = addIfDifferentNeighbor" with "where = end
 '
 
 test_expect_success 'using "ifExists = addIfDifferentNeighbor"  with "where = after"' '
-	git config trailer.ack.ifExists "addIfDifferentNeighbor" &&
-	git config trailer.ack.where "after" &&
+	test_config trailer.ack.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -995,7 +1074,11 @@ test_expect_success 'using "ifExists = addIfDifferentNeighbor"  with "where = af
 '
 
 test_expect_success 'using "ifExists = addIfDifferentNeighbor" and --trim-empty' '
-	git config trailer.ack.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.ack.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	cat >>expected <<-\EOF &&
 		Bug #42
@@ -1011,8 +1094,14 @@ test_expect_success 'using "ifExists = addIfDifferentNeighbor" and --trim-empty'
 '
 
 test_expect_success 'using "ifExists = add" with "where = end"' '
-	git config trailer.ack.ifExists "add" &&
-	git config trailer.ack.where "end" &&
+	test_config trailer.ack.ifExists "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "end" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1036,8 +1125,14 @@ test_expect_success 'using "ifExists = add" with "where = end"' '
 '
 
 test_expect_success 'using "ifExists = add" with "where = after"' '
-	git config trailer.ack.ifExists "add" &&
-	git config trailer.ack.where "after" &&
+	test_config trailer.ack.ifExists "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1058,8 +1153,15 @@ test_expect_success 'using "ifExists = add" with "where = after"' '
 '
 
 test_expect_success 'overriding configuration with "--if-exists replace"' '
-	git config trailer.fix.key "Fixes: " &&
-	git config trailer.fix.ifExists "add" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.review.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1075,8 +1177,15 @@ test_expect_success 'overriding configuration with "--if-exists replace"' '
 '
 
 test_expect_success 'using "ifExists = replace"' '
-	git config trailer.fix.key "Fixes: " &&
-	git config trailer.fix.ifExists "replace" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "replace" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1095,7 +1204,16 @@ test_expect_success 'using "ifExists = replace"' '
 '
 
 test_expect_success 'using "ifExists = replace" with "where = after"' '
-	git config trailer.fix.where "after" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "replace" &&
+	test_config trailer.fix.where "after" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1114,7 +1232,15 @@ test_expect_success 'using "ifExists = replace" with "where = after"' '
 '
 
 test_expect_success 'using "ifExists = doNothing"' '
-	git config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1133,8 +1259,17 @@ test_expect_success 'using "ifExists = doNothing"' '
 '
 
 test_expect_success 'the default is "ifMissing = add"' '
-	git config trailer.cc.key "Cc: " &&
-	git config trailer.cc.where "before" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.cc.key "Cc: " &&
+	test_config trailer.cc.where "before" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1154,7 +1289,14 @@ test_expect_success 'the default is "ifMissing = add"' '
 '
 
 test_expect_success 'overriding configuration with "--if-missing doNothing"' '
-	git config trailer.ifmissing "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.ifmissing "add" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -1173,7 +1315,13 @@ test_expect_success 'overriding configuration with "--if-missing doNothing"' '
 '
 
 test_expect_success 'when default "ifMissing" is "doNothing"' '
-	git config trailer.ifmissing "doNothing" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.ifmissing "doNothing" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -1187,14 +1335,21 @@ test_expect_success 'when default "ifMissing" is "doNothing"' '
 		--trailer "cc=Linus" --trailer "ack: Junio" \
 		--trailer "fix=22" --trailer "bug: 42" --trailer "ack: Peff" \
 		<complex_message >actual &&
-	test_cmp expected actual &&
-	git config trailer.ifmissing "add"
+	test_cmp expected actual
 '
 
 test_expect_success 'using "ifMissing = add" with "where = end"' '
-	git config trailer.cc.key "Cc: " &&
-	git config trailer.cc.where "end" &&
-	git config trailer.cc.ifMissing "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.cc.key "Cc: " &&
+	test_config trailer.cc.ifMissing "add" &&
+	test_config trailer.cc.where "end" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1214,9 +1369,17 @@ test_expect_success 'using "ifMissing = add" with "where = end"' '
 '
 
 test_expect_success 'using "ifMissing = add" with "where = before"' '
-	git config trailer.cc.key "Cc: " &&
-	git config trailer.cc.where "before" &&
-	git config trailer.cc.ifMissing "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.cc.key "Cc: " &&
+	test_config trailer.cc.ifMissing "add" &&
+	test_config trailer.cc.where "before" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Cc: Linus
@@ -1236,7 +1399,15 @@ test_expect_success 'using "ifMissing = add" with "where = before"' '
 '
 
 test_expect_success 'using "ifMissing = doNothing"' '
-	git config trailer.cc.ifMissing "doNothing" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.cc.ifMissing "doNothing" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1256,7 +1427,15 @@ test_expect_success 'using "ifMissing = doNothing"' '
 
 test_expect_success 'default "where" is now "after"' '
 	git config trailer.where "after" &&
-	git config --unset trailer.ack.where &&
+	test_config trailer.ack.ifExists "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1280,10 +1459,15 @@ test_expect_success 'default "where" is now "after"' '
 '
 
 test_expect_success 'with simple command' '
-	git config trailer.sign.key "Signed-off-by: " &&
-	git config trailer.sign.where "after" &&
-	git config trailer.sign.ifExists "addIfDifferentNeighbor" &&
-	git config trailer.sign.command "echo \"A U Thor <author@example.com>\"" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"A U Thor <author@example.com>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.sign.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.sign.where "after" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -1298,8 +1482,14 @@ test_expect_success 'with simple command' '
 '
 
 test_expect_success 'with command using committer information' '
-	git config trailer.sign.ifExists "addIfDifferent" &&
-	git config trailer.sign.command "echo \"\$GIT_COMMITTER_NAME <\$GIT_COMMITTER_EMAIL>\"" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"\$GIT_COMMITTER_NAME <\$GIT_COMMITTER_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.sign.ifExists "addIfDifferent" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -1314,10 +1504,15 @@ test_expect_success 'with command using committer information' '
 '
 
 test_expect_success 'with command using author information' '
-	git config trailer.sign.key "Signed-off-by: " &&
-	git config trailer.sign.where "after" &&
-	git config trailer.sign.ifExists "addIfDifferentNeighbor" &&
-	git config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.sign.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.sign.where "after" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -1338,12 +1533,19 @@ test_expect_success 'setup a commit' '
 '
 
 test_expect_success 'cmd takes precedence over command' '
-	test_when_finished "git config --unset trailer.fix.cmd" &&
-	git config trailer.fix.ifExists "replace" &&
-	git config trailer.fix.cmd "test -n \"\$1\" && git log -1 --oneline --format=\"%h (%aN)\" \
-	--abbrev-commit --abbrev=14 \"\$1\" || true" &&
-	git config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" \
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" \
 		--abbrev-commit --abbrev=14 \$ARG" &&
+	test_config trailer.fix.cmd "test -n \"\$1\" && git log -1 --oneline --format=\"%h (%aN)\" \
+	--abbrev-commit --abbrev=14 \"\$1\" || true" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "replace" &&
+	test_config trailer.fix.where "after" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	FIXED=$(git log -1 --oneline --format="%h (%aN)" --abbrev-commit --abbrev=14 HEAD) &&
 	cat complex_message_body >expected2 &&
 	sed -e "s/ Z\$/ /" >>expected2 <<-EOF &&
@@ -1359,8 +1561,16 @@ test_expect_success 'cmd takes precedence over command' '
 '
 
 test_expect_success 'with command using $ARG' '
-	git config trailer.fix.ifExists "replace" &&
-	git config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$ARG" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$ARG" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "replace" &&
+	test_config trailer.fix.where "after" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	FIXED=$(git log -1 --oneline --format="%h (%s)" --abbrev-commit --abbrev=14 HEAD) &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-EOF &&
@@ -1376,8 +1586,16 @@ test_expect_success 'with command using $ARG' '
 '
 
 test_expect_success 'with failing command using $ARG' '
-	git config trailer.fix.ifExists "replace" &&
-	git config trailer.fix.command "false \$ARG" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.command "false \$ARG" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "replace" &&
+	test_config trailer.fix.where "after" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-EOF &&
 		Fixes: Z
@@ -1392,7 +1610,9 @@ test_expect_success 'with failing command using $ARG' '
 '
 
 test_expect_success 'with empty tokens' '
-	git config --unset trailer.fix.command &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.ifexists "addIfDifferent" &&
 	cat >expected <<-EOF &&
 
 		Signed-off-by: A U Thor <author@example.com>
@@ -1403,7 +1623,8 @@ test_expect_success 'with empty tokens' '
 '
 
 test_expect_success 'with command but no key' '
-	git config --unset trailer.sign.key &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.ifexists "addIfDifferent" &&
 	cat >expected <<-EOF &&
 
 		sign: A U Thor <author@example.com>
@@ -1414,7 +1635,9 @@ test_expect_success 'with command but no key' '
 '
 
 test_expect_success 'with no command and no key' '
-	git config --unset trailer.review.key &&
+	test_config trailer.review.where "before" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.ifexists "addIfDifferent" &&
 	cat >expected <<-EOF &&
 
 		review: Junio
@@ -1426,6 +1649,8 @@ test_expect_success 'with no command and no key' '
 '
 
 test_expect_success 'with cut line' '
+	test_config trailer.review.where "before" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
 	cat >expected <<-\EOF &&
 		my subject
 
@@ -1443,7 +1668,8 @@ test_expect_success 'with cut line' '
 '
 
 test_expect_success 'only trailers' '
-	git config trailer.sign.command "echo config-value" &&
+	test_config trailer.sign.command "echo config-value" &&
+	test_config trailer.ifexists "addIfDifferent" &&
 	cat >expected <<-\EOF &&
 		existing: existing-value
 		sign: config-value
@@ -1462,7 +1688,7 @@ test_expect_success 'only trailers' '
 '
 
 test_expect_success 'only-trailers omits non-trailer in middle of block' '
-	git config trailer.sign.command "echo config-value" &&
+	test_config trailer.sign.command "echo config-value" &&
 	cat >expected <<-\EOF &&
 		Signed-off-by: nobody <nobody@nowhere>
 		Signed-off-by: somebody <somebody@somewhere>
@@ -1482,7 +1708,7 @@ test_expect_success 'only-trailers omits non-trailer in middle of block' '
 '
 
 test_expect_success 'only input' '
-	git config trailer.sign.command "echo config-value" &&
+	test_config trailer.sign.command "echo config-value" &&
 	cat >expected <<-\EOF &&
 		existing: existing-value
 	EOF
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v2 02/13] trailer test description: this tests --where=after, not --where=before
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
  2023-08-10 21:17   ` [PATCH v2 01/13] trailer tests: make test cases self-contained Linus Arver via GitGitGadget
@ 2023-08-10 21:17   ` Linus Arver via GitGitGadget
  2023-08-10 21:17   ` [PATCH v2 03/13] trailer: add tests to check defaulting behavior with --no-* flags Linus Arver via GitGitGadget
                     ` (12 subsequent siblings)
  14 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-10 21:17 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

Signed-off-by: Linus Arver <linusa@google.com>
---
 t/t7513-interpret-trailers.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 5b31896070a..ed0fc04bd95 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -792,7 +792,7 @@ test_expect_success 'overriding configuration with "--where after"' '
 	test_cmp expected actual
 '
 
-test_expect_success 'using "where = before" with "--no-where"' '
+test_expect_success 'using "--where after" with "--no-where"' '
 	test_config trailer.ack.key "Acked-by= " &&
 	test_config trailer.ack.where "before" &&
 	test_config trailer.bug.key "Bug #" &&
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v2 03/13] trailer: add tests to check defaulting behavior with --no-* flags
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
  2023-08-10 21:17   ` [PATCH v2 01/13] trailer tests: make test cases self-contained Linus Arver via GitGitGadget
  2023-08-10 21:17   ` [PATCH v2 02/13] trailer test description: this tests --where=after, not --where=before Linus Arver via GitGitGadget
@ 2023-08-10 21:17   ` Linus Arver via GitGitGadget
  2023-08-10 21:17   ` [PATCH v2 04/13] trailer doc: narrow down scope of --where and related flags Linus Arver via GitGitGadget
                     ` (11 subsequent siblings)
  14 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-10 21:17 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

While the "--no-where" flag is tested, the "--no-if-exists" and
"--no-if-missing" flags are not, so add tests for them. But also add
tests for all "--no-*" flags to check their effects, both when (1) there
are relevant configuration variables set, and (2) they are not set.

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt |  14 ++-
 t/t7513-interpret-trailers.sh            | 130 +++++++++++++++++++++++
 2 files changed, 140 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 55d89614661..91a4dbc9a72 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -114,8 +114,10 @@ OPTIONS
 	Specify where all new trailers will be added.  A setting
 	provided with '--where' overrides all configuration variables
 	and applies to all '--trailer' options until the next occurrence of
-	'--where' or '--no-where'. Possible values are `after`, `before`,
-	`end` or `start`.
+	'--where' or '--no-where'. Upon encountering '--no-where', clear the
+	effect of any previous use of '--where', such that the relevant configuration
+	variables are no longer overridden. Possible values are `after`,
+	`before`, `end` or `start`.
 
 --if-exists <action>::
 --no-if-exists::
@@ -123,7 +125,9 @@ OPTIONS
 	least one trailer with the same <token> in the input.  A setting
 	provided with '--if-exists' overrides all configuration variables
 	and applies to all '--trailer' options until the next occurrence of
-	'--if-exists' or '--no-if-exists'. Possible actions are `addIfDifferent`,
+	'--if-exists' or '--no-if-exists'. Upon encountering '--no-if-exists, clear the
+	effect of any previous use of '--if-exists, such that the relevant configuration
+	variables are no longer overridden. Possible actions are `addIfDifferent`,
 	`addIfDifferentNeighbor`, `add`, `replace` and `doNothing`.
 
 --if-missing <action>::
@@ -132,7 +136,9 @@ OPTIONS
 	trailer with the same <token> in the input.  A setting
 	provided with '--if-missing' overrides all configuration variables
 	and applies to all '--trailer' options until the next occurrence of
-	'--if-missing' or '--no-if-missing'. Possible actions are `doNothing`
+	'--if-missing' or '--no-if-missing'. Upon encountering '--no-if-missing,
+	clear the effect of any previous use of '--if-missing, such that the relevant
+	configuration variables are no longer overridden. Possible actions are `doNothing`
 	or `add`.
 
 --only-trailers::
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index ed0fc04bd95..832aff06167 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -812,6 +812,53 @@ test_expect_success 'using "--where after" with "--no-where"' '
 	test_cmp expected actual
 '
 
+# Check whether using "--no-where" clears out only the "--where after", such
+# that we still use the configuration in trailer.where (which is different from
+# the hardcoded default (in WHERE_END) assuming the absence of .gitconfig).
+# Here, the "start" setting of trailer.where is respected, so the new "Acked-by"
+# and "Bug" trailers are placed at the beginning, and not at the end which is
+# the harcoded default.
+test_expect_success 'using "--where after" with "--no-where" defaults to configuration' '
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.separators ":=#" &&
+	test_config trailer.where "start" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Bug #42
+		Acked-by= Peff
+		Fixes: Z
+		Acked-by= Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers --where after --no-where --trailer "ack: Peff" \
+		--trailer "bug: 42" complex_message >actual &&
+	test_cmp expected actual
+'
+
+# The "--where after" will only get respected for the trailer that came
+# immediately after it. For the next trailer (Bug #42), we default to using the
+# hardcoded WHERE_END because we don't have any "trailer.where" or
+# "trailer.bug.where" configured.
+test_expect_success 'using "--no-where" defaults to harcoded default if nothing configured' '
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.separators ":=#" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by= Z
+		Acked-by= Peff
+		Reviewed-by: Z
+		Signed-off-by: Z
+		Bug #42
+	EOF
+	git interpret-trailers --where after --trailer "ack: Peff" --no-where \
+		--trailer "bug: 42" complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'using "where = after"' '
 	test_config trailer.ack.key "Acked-by= " &&
 	test_config trailer.ack.where "after" &&
@@ -1176,6 +1223,56 @@ test_expect_success 'overriding configuration with "--if-exists replace"' '
 	test_cmp expected actual
 '
 
+# "trailer.ifexists" is set to "doNothing", so using "--no-if-exists" defaults
+# to this "doNothing" behavior. So the "Fixes: 53" trailer does not get added.
+test_expect_success 'using "--if-exists replace" with "--no-if-exists" defaults to configuration' '
+	test_config trailer.ifexists "doNothing" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by: Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers --if-exists replace --no-if-exists --trailer "Fixes: 53" \
+		<complex_message >actual &&
+	test_cmp expected actual
+'
+
+# No "ifexists" configuration is set, so using "--no-if-exists" makes it default
+# to addIfDifferentNeighbor. Because we do have a different neighbor "Fixes: 53"
+# (because it got added by overriding with "--if-exists replace" earlier in the
+# arguments list), we add "Signed-off-by: addme".
+test_expect_success 'using "--no-if-exists" defaults to hardcoded default if nothing configured' '
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Acked-by: Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+		Fixes: 53
+		Signed-off-by: addme
+	EOF
+	git interpret-trailers --if-exists replace --trailer "Fixes: 53" --no-if-exists \
+		--trailer "Signed-off-by: addme" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+# The second "Fixes: 53" trailer is discarded, because the "--no-if-exists" here
+# makes us default to addIfDifferentNeighbor, and we already added the "Fixes:
+# 53" trailer earlier in the argument list.
+test_expect_success 'using "--no-if-exists" defaults to hardcoded default if nothing configured (no addition)' '
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Acked-by: Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+		Fixes: 53
+	EOF
+	git interpret-trailers --if-exists replace --trailer "Fixes: 53" --no-if-exists \
+		--trailer "Fixes: 53" <complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'using "ifExists = replace"' '
 	test_config trailer.fix.key "Fixes: " &&
 	test_config trailer.fix.ifExists "replace" &&
@@ -1425,6 +1522,39 @@ test_expect_success 'using "ifMissing = doNothing"' '
 	test_cmp expected actual
 '
 
+# Ignore the "IgnoredTrailer" because of "--if-missing doNothing", but also
+# ignore the "StillIgnoredTrailer" because we set "trailer.ifMissing" to
+# "doNothing" in configuration.
+test_expect_success 'using "--no-if-missing" defaults to configuration' '
+	test_config trailer.ifMissing "doNothing" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+			Fixes: Z
+			Acked-by: Z
+			Reviewed-by: Z
+			Signed-off-by: Z
+	EOF
+	git interpret-trailers --if-missing doNothing --trailer "IgnoredTrailer: ignoreme" --no-if-missing \
+			--trailer "StillIgnoredTrailer: ignoreme" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+# Add the "AddedTrailer" because the "--no-if-missing" clears the "--if-missing
+# doNothing" from earlier in the argument list.
+test_expect_success 'using "--no-if-missing" defaults to hardcoded default if nothing configured' '
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+			Fixes: Z
+			Acked-by: Z
+			Reviewed-by: Z
+			Signed-off-by: Z
+			AddedTrailer: addme
+	EOF
+	git interpret-trailers --if-missing doNothing --trailer "IgnoredTrailer: ignoreme" --no-if-missing \
+			--trailer "AddedTrailer: addme" <complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'default "where" is now "after"' '
 	git config trailer.where "after" &&
 	test_config trailer.ack.ifExists "add" &&
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v2 04/13] trailer doc: narrow down scope of --where and related flags
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
                     ` (2 preceding siblings ...)
  2023-08-10 21:17   ` [PATCH v2 03/13] trailer: add tests to check defaulting behavior with --no-* flags Linus Arver via GitGitGadget
@ 2023-08-10 21:17   ` Linus Arver via GitGitGadget
  2023-08-10 21:17   ` [PATCH v2 05/13] trailer: trailer location is a place, not an action Linus Arver via GitGitGadget
                     ` (10 subsequent siblings)
  14 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-10 21:17 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

The wording "all configuration variables" is misleading (the same could
be said to the descriptions of the "--[no-]if-exists" and the
"--[no-]if-missing" options).  Specifying --where=value overrides only
the trailer.where variable and applicable trailer.<token>.where
variables, and --no-where stops the overriding of these variables.
Ditto for the other two with their relevant configuration variables.

Reported-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 91a4dbc9a72..72f5bdb652f 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -112,7 +112,8 @@ OPTIONS
 --where <placement>::
 --no-where::
 	Specify where all new trailers will be added.  A setting
-	provided with '--where' overrides all configuration variables
+	provided with '--where' overrides the `trailer.where` and any
+	applicable `trailer.<token>.where` configuration variables
 	and applies to all '--trailer' options until the next occurrence of
 	'--where' or '--no-where'. Upon encountering '--no-where', clear the
 	effect of any previous use of '--where', such that the relevant configuration
@@ -123,7 +124,8 @@ OPTIONS
 --no-if-exists::
 	Specify what action will be performed when there is already at
 	least one trailer with the same <token> in the input.  A setting
-	provided with '--if-exists' overrides all configuration variables
+	provided with '--if-exists' overrides the `trailer.ifExists` and any
+	applicable `trailer.<token>.ifExists` configuration variables
 	and applies to all '--trailer' options until the next occurrence of
 	'--if-exists' or '--no-if-exists'. Upon encountering '--no-if-exists, clear the
 	effect of any previous use of '--if-exists, such that the relevant configuration
@@ -134,7 +136,8 @@ OPTIONS
 --no-if-missing::
 	Specify what action will be performed when there is no other
 	trailer with the same <token> in the input.  A setting
-	provided with '--if-missing' overrides all configuration variables
+	provided with '--if-missing' overrides the `trailer.ifMissing` and any
+	applicable `trailer.<token>.ifMissing` configuration variables
 	and applies to all '--trailer' options until the next occurrence of
 	'--if-missing' or '--no-if-missing'. Upon encountering '--no-if-missing,
 	clear the effect of any previous use of '--if-missing, such that the relevant
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v2 05/13] trailer: trailer location is a place, not an action
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
                     ` (3 preceding siblings ...)
  2023-08-10 21:17   ` [PATCH v2 04/13] trailer doc: narrow down scope of --where and related flags Linus Arver via GitGitGadget
@ 2023-08-10 21:17   ` Linus Arver via GitGitGadget
  2023-08-10 21:17   ` [PATCH v2 06/13] trailer --no-divider help: describe usual "---" meaning Linus Arver via GitGitGadget
                     ` (9 subsequent siblings)
  14 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-10 21:17 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

Fix the help text to say "placement" instead of "action" because the
values are placements, not actions.

While we're at it, tweak the documentation to say "placements" instead
of "values", similar to how the existing language for "--if-exists" uses
the word "action" to describe both the syntax (with the phrase
"--if-exists <action>") and the possible values (with the phrase
"possible actions").

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 2 +-
 builtin/interpret-trailers.c             | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 72f5bdb652f..b5284c3d33f 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -117,7 +117,7 @@ OPTIONS
 	and applies to all '--trailer' options until the next occurrence of
 	'--where' or '--no-where'. Upon encountering '--no-where', clear the
 	effect of any previous use of '--where', such that the relevant configuration
-	variables are no longer overridden. Possible values are `after`,
+	variables are no longer overridden. Possible placements are `after`,
 	`before`, `end` or `start`.
 
 --if-exists <action>::
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index c5e83452654..cf4f703c4e2 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -97,7 +97,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 
-		OPT_CALLBACK(0, "where", NULL, N_("action"),
+		OPT_CALLBACK(0, "where", NULL, N_("placement"),
 			     N_("where to place the new trailer"), option_parse_where),
 		OPT_CALLBACK(0, "if-exists", NULL, N_("action"),
 			     N_("action if trailer already exists"), option_parse_if_exists),
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v2 06/13] trailer --no-divider help: describe usual "---" meaning
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
                     ` (4 preceding siblings ...)
  2023-08-10 21:17   ` [PATCH v2 05/13] trailer: trailer location is a place, not an action Linus Arver via GitGitGadget
@ 2023-08-10 21:17   ` Linus Arver via GitGitGadget
  2023-08-10 21:17   ` [PATCH v2 07/13] trailer --parse help: expose aliased options Linus Arver via GitGitGadget
                     ` (8 subsequent siblings)
  14 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-10 21:17 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

It's unclear what treating something "specially" means.

Signed-off-by: Linus Arver <linusa@google.com>
---
 builtin/interpret-trailers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index cf4f703c4e2..a7623dbfb2e 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -109,7 +109,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "unfold", &opts.unfold, N_("join whitespace-continued values")),
 		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")),
+		OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
 		OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add"), option_parse_trailer),
 		OPT_END()
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v2 07/13] trailer --parse help: expose aliased options
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
                     ` (5 preceding siblings ...)
  2023-08-10 21:17   ` [PATCH v2 06/13] trailer --no-divider help: describe usual "---" meaning Linus Arver via GitGitGadget
@ 2023-08-10 21:17   ` Linus Arver via GitGitGadget
  2023-08-10 21:17   ` [PATCH v2 08/13] trailer --only-input: prefer "configuration variables" over "rules" Linus Arver via GitGitGadget
                     ` (7 subsequent siblings)
  14 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-10 21:17 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

The existing description "set parsing options" is vague, because
arguably _all_ of the options for interpret-trailers have to do with
parsing to some degree.

Explain what this flag does to match what is in the docs, namely how
it is an alias for "--only-trailers --only-input --unfold".

Signed-off-by: Linus Arver <linusa@google.com>
---
 builtin/interpret-trailers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index a7623dbfb2e..5f3e1a38eee 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -107,7 +107,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_CALLBACK_F(0, "parse", &opts, NULL, N_("set parsing options"),
+		OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("alias for --only-trailers --only-input --unfold"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
 		OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
 		OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v2 08/13] trailer --only-input: prefer "configuration variables" over "rules"
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
                     ` (6 preceding siblings ...)
  2023-08-10 21:17   ` [PATCH v2 07/13] trailer --parse help: expose aliased options Linus Arver via GitGitGadget
@ 2023-08-10 21:17   ` Linus Arver via GitGitGadget
  2023-08-10 21:17   ` [PATCH v2 09/13] trailer --parse docs: add explanation for its usefulness Linus Arver via GitGitGadget
                     ` (6 subsequent siblings)
  14 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-10 21:17 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

Use the phrase "configuration variables" instead of "rules" because

(1) we already say "configuration variables" in multiple
    places in the docs (where the word "rules" is only used for describing
    "--only-input" behavior and for an unrelated case of mentioning how
    the trailers do not follow "rules for RFC 822 headers"), and

(2) this phrase is more specific than just "rules".

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 4 ++--
 builtin/interpret-trailers.c             | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index b5284c3d33f..0eea937c30e 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -149,8 +149,8 @@ OPTIONS
 
 --only-input::
 	Output only trailers that exist in the input; do not add any
-	from the command-line or by following configured `trailer.*`
-	rules.
+	from the command-line or by applying `trailer.*` configuration
+	variables.
 
 --unfold::
 	Remove any whitespace-continuation in trailers, so that each
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 5f3e1a38eee..f70c5df8d4b 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -105,7 +105,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 			     N_("action if trailer is missing"), option_parse_if_missing),
 
 		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, "only-input", &opts.only_input, N_("do not apply trailer.* configuration variables")),
 		OPT_BOOL(0, "unfold", &opts.unfold, N_("join whitespace-continued values")),
 		OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("alias for --only-trailers --only-input --unfold"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v2 09/13] trailer --parse docs: add explanation for its usefulness
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
                     ` (7 preceding siblings ...)
  2023-08-10 21:17   ` [PATCH v2 08/13] trailer --only-input: prefer "configuration variables" over "rules" Linus Arver via GitGitGadget
@ 2023-08-10 21:17   ` Linus Arver via GitGitGadget
  2023-08-10 21:18   ` [PATCH v2 10/13] trailer --unfold help: prefer "reformat" over "join" Linus Arver via GitGitGadget
                     ` (5 subsequent siblings)
  14 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-10 21:17 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

For users who are skimming the docs to go straight to the individual
breakdown of each flag, it may not be clear why --parse is a convenience
alias (without them also looking at the other options that --parse turns
on). To save them the trouble of looking at the other options (and
computing what that would mean), describe a summary of the overall
effect.

Similarly update the area when we first mention --parse near the top of
the doc.

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 0eea937c30e..2d49445b1c3 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -31,7 +31,9 @@ the last two lines starting with "Signed-off-by" are trailers.
 
 This command reads commit messages from either the
 <file> arguments or the standard input if no <file> is specified.
-If `--parse` is specified, the output consists of the parsed trailers.
+If `--parse` is specified, the output consists of the parsed trailers
+coming from the input, without influencing them with any command line
+options or configuration variables.
 Otherwise, this command applies the arguments passed using the
 `--trailer` option, if any, to each input file. The result is emitted on the
 standard output.
@@ -158,7 +160,10 @@ OPTIONS
 
 --parse::
 	A convenience alias for `--only-trailers --only-input
-	--unfold`.
+	--unfold`. This makes it easier to only see the trailers coming from the
+	input without influencing them with any command line options or
+	configuration variables, while also making the output machine-friendly with
+	--unfold.
 
 --no-divider::
 	Do not treat `---` as the end of the commit message. Use this
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v2 10/13] trailer --unfold help: prefer "reformat" over "join"
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
                     ` (8 preceding siblings ...)
  2023-08-10 21:17   ` [PATCH v2 09/13] trailer --parse docs: add explanation for its usefulness Linus Arver via GitGitGadget
@ 2023-08-10 21:18   ` Linus Arver via GitGitGadget
  2023-08-10 21:18   ` [PATCH v2 11/13] trailer doc: emphasize the effect of configuration variables Linus Arver via GitGitGadget
                     ` (4 subsequent siblings)
  14 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-10 21:18 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

The phrase "join whitespace-continued values" requires some additional
context. For example, "whitespace" means newlines (not just space
characters), and "join" means to join only the multiple lines together
for a single trailer (and not that we are joining multiple trailers
together). That is, "join" means to convert

    token: This is a very long value, with spaces and
      newlines in it.

to

    token: This is a very long value, with spaces and newlines in it.

and does not mean to convert

    token: value1
    token: value2

to

    token: value1 value2.

Update the help text to resolve the above ambiguity. While we're add it,
update the docs to use similar language as the change in the help text.

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 4 ++--
 builtin/interpret-trailers.c             | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 2d49445b1c3..62ba2b1232e 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -155,8 +155,8 @@ OPTIONS
 	variables.
 
 --unfold::
-	Remove any whitespace-continuation in trailers, so that each
-	trailer appears on a line by itself with its full content.
+	If a trailer has a value that runs over multiple lines (aka "folded"),
+	reformat the value into a single line.
 
 --parse::
 	A convenience alias for `--only-trailers --only-input
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index f70c5df8d4b..832f86a770a 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -106,7 +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 trailer.* configuration variables")),
-		OPT_BOOL(0, "unfold", &opts.unfold, N_("join whitespace-continued values")),
+		OPT_BOOL(0, "unfold", &opts.unfold, N_("reformat multiline trailer values as single-line values")),
 		OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("alias for --only-trailers --only-input --unfold"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
 		OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v2 11/13] trailer doc: emphasize the effect of configuration variables
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
                     ` (9 preceding siblings ...)
  2023-08-10 21:18   ` [PATCH v2 10/13] trailer --unfold help: prefer "reformat" over "join" Linus Arver via GitGitGadget
@ 2023-08-10 21:18   ` Linus Arver via GitGitGadget
  2023-08-10 21:18   ` [PATCH v2 12/13] trailer doc: separator within key suppresses default separator Linus Arver via GitGitGadget
                     ` (3 subsequent siblings)
  14 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-10 21:18 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

The sentence does not mention the effect of configuration variables at
all, when they are actively used by default (unless --parse is
specified) to potentially add new trailers, without the user having to
always supply --trailer manually.

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 62ba2b1232e..a288ff111cb 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -34,9 +34,12 @@ This command reads commit messages from either the
 If `--parse` is specified, the output consists of the parsed trailers
 coming from the input, without influencing them with any command line
 options or configuration variables.
-Otherwise, this command applies the arguments passed using the
-`--trailer` option, if any, to each input file. The result is emitted on the
-standard output.
+
+Otherwise, this command applies `trailer.*` configuration variables
+(which could potentially add new trailers, as well as reposition them),
+as well as any command line arguments that can override configuration
+variables (such as `--trailer=...` which could also add new trailers),
+to each input file. The result is emitted on the standard output.
 
 This command can also operate on the output of linkgit:git-format-patch[1],
 which is more elaborate than a plain commit message. Namely, such output
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v2 12/13] trailer doc: separator within key suppresses default separator
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
                     ` (10 preceding siblings ...)
  2023-08-10 21:18   ` [PATCH v2 11/13] trailer doc: emphasize the effect of configuration variables Linus Arver via GitGitGadget
@ 2023-08-10 21:18   ` Linus Arver via GitGitGadget
  2023-08-10 21:18   ` [PATCH v2 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both Linus Arver via GitGitGadget
                     ` (2 subsequent siblings)
  14 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-10 21:18 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index a288ff111cb..25433e1a1ff 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -255,8 +255,8 @@ trailer.<token>.key::
 	but this can be changed using the `trailer.separators` config
 	variable.
 +
-If there is a separator, then the key will be used instead of both the
-<token> and the default separator when adding the trailer.
+If there is a separator in the key, then it overrides the default
+separator when adding the trailer.
 
 trailer.<token>.where::
 	This option takes the same values as the 'trailer.where'
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v2 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
                     ` (11 preceding siblings ...)
  2023-08-10 21:18   ` [PATCH v2 12/13] trailer doc: separator within key suppresses default separator Linus Arver via GitGitGadget
@ 2023-08-10 21:18   ` Linus Arver via GitGitGadget
  2023-08-11  1:41   ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Junio C Hamano
  2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
  14 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-08-10 21:18 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

The `--trailer` option takes a "<token>=<value>" argument, for example

    --trailer "Acked-by=Bob"

And in this exampple it is understood that "Acked-by" is the <token>.
However, the user can use a shorter "ack" string by defining
configuration like

    git config trailer.ack.key "Acked-by"

However, in the docs we define the above configuration as

    trailer.<token>.key

so the <token> can mean either the longer "Acked-by" or the shorter
"ack".

Separate the two meanings of <token> into <key> and <keyAlias>, and
update the configuration syntax to say "trailer.<keyAlias>.key".

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 136 +++++++++++++----------
 1 file changed, 76 insertions(+), 60 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 25433e1a1ff..418265f044d 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git interpret-trailers' [--in-place] [--trim-empty]
-			[(--trailer <token>[(=|:)<value>])...]
+			[(--trailer (<key>|<keyAlias>)[(=|:)<value>])...]
 			[--parse] [<file>...]
 
 DESCRIPTION
@@ -53,22 +53,32 @@ are applied to each input and the way any existing trailer in
 the input is changed. They also make it possible to
 automatically add some trailers.
 
-By default, a '<token>=<value>' or '<token>:<value>' argument given
+By default, a '<key>=<value>' or '<key>:<value>' argument given
 using `--trailer` will be appended after the existing trailers only if
-the last trailer has a different (<token>, <value>) pair (or if there
-is no existing trailer). The <token> and <value> parts will be trimmed
+the last trailer has a different (<key>, <value>) pair (or if there
+is no existing trailer). The <key> and <value> parts will be trimmed
 to remove starting and trailing whitespace, and the resulting trimmed
-<token> and <value> will appear in the output like this:
+<key> and <value> will appear in the output like this:
 
 ------------------------------------------------
-token: value
+key: value
 ------------------------------------------------
 
-This means that the trimmed <token> and <value> will be separated by
-`': '` (one colon followed by one space). For convenience, the <token> can be a
-shortened string key (e.g., "sign") instead of the full string which should
-appear before the separator on the output (e.g., "Signed-off-by"). This can be
-configured using the 'trailer.<token>.key' configuration variable.
+This means that the trimmed <key> and <value> will be separated by
+`': '` (one colon followed by one space).
+
+For convenience, a <keyAlias> can be configured to make using `--trailer`
+shorter to type on the command line. This can be configured using the
+'trailer.<keyAlias>.key' configuration variable. The <keyAlias> must be a prefix
+of the full <key> string, although case sensitivity does not matter. For
+example, if you have
+
+------------------------------------------------
+trailer.sign.key "Signed-off-by: "
+------------------------------------------------
+
+in your configuration, you only need to specify `--trailer="sign: foo"`
+on the command line instead of `--trailer="Signed-off-by: foo"`.
 
 By default the new trailer will appear at the end of all the existing
 trailers. If there is no existing trailer, the new trailer will appear
@@ -85,14 +95,14 @@ non-whitespace lines before a line that starts with '---' (followed by a
 space or the end of the line).
 
 When reading trailers, there can be no whitespace before or inside the
-<token>, but any number of regular space and tab characters are allowed
-between the <token> and the separator. There can be whitespaces before,
+<key>, but any number of regular space and tab characters are allowed
+between the <key> and the separator. There can be whitespaces before,
 inside or after the <value>. The <value> may be split over multiple lines
 with each subsequent line starting with at least one whitespace, like
 the "folding" in RFC 822. Example:
 
 ------------------------------------------------
-token: This is a very long value, with spaces and
+key: This is a very long value, with spaces and
   newlines in it.
 ------------------------------------------------
 
@@ -109,8 +119,8 @@ OPTIONS
 	the whole trailer will be removed from the output.
 	This applies to existing trailers as well as new trailers.
 
---trailer <token>[(=|:)<value>]::
-	Specify a (<token>, <value>) pair that should be applied as a
+--trailer <key>[(=|:)<value>]::
+	Specify a (<key>, <value>) pair that should be applied as a
 	trailer to the inputs. See the description of this
 	command.
 
@@ -118,7 +128,7 @@ OPTIONS
 --no-where::
 	Specify where all new trailers will be added.  A setting
 	provided with '--where' overrides the `trailer.where` and any
-	applicable `trailer.<token>.where` configuration variables
+	applicable `trailer.<keyAlias>.where` configuration variables
 	and applies to all '--trailer' options until the next occurrence of
 	'--where' or '--no-where'. Upon encountering '--no-where', clear the
 	effect of any previous use of '--where', such that the relevant configuration
@@ -128,9 +138,9 @@ OPTIONS
 --if-exists <action>::
 --no-if-exists::
 	Specify what action will be performed when there is already at
-	least one trailer with the same <token> in the input.  A setting
+	least one trailer with the same <key> in the input.  A setting
 	provided with '--if-exists' overrides the `trailer.ifExists` and any
-	applicable `trailer.<token>.ifExists` configuration variables
+	applicable `trailer.<keyAlias>.ifExists` configuration variables
 	and applies to all '--trailer' options until the next occurrence of
 	'--if-exists' or '--no-if-exists'. Upon encountering '--no-if-exists, clear the
 	effect of any previous use of '--if-exists, such that the relevant configuration
@@ -140,9 +150,9 @@ OPTIONS
 --if-missing <action>::
 --no-if-missing::
 	Specify what action will be performed when there is no other
-	trailer with the same <token> in the input.  A setting
+	trailer with the same <key> in the input.  A setting
 	provided with '--if-missing' overrides the `trailer.ifMissing` and any
-	applicable `trailer.<token>.ifMissing` configuration variables
+	applicable `trailer.<keyAlias>.ifMissing` configuration variables
 	and applies to all '--trailer' options until the next occurrence of
 	'--if-missing' or '--no-if-missing'. Upon encountering '--no-if-missing,
 	clear the effect of any previous use of '--if-missing, such that the relevant
@@ -187,11 +197,11 @@ used when another separator is not specified in the config for this
 trailer.
 +
 For example, if the value for this option is "%=$", then only lines
-using the format '<token><sep><value>' with <sep> containing '%', '='
+using the format '<key><sep><value>' with <sep> containing '%', '='
 or '$' and then spaces will be considered trailers. And '%' will be
 the default separator used, so by default trailers will appear like:
-'<token>% <value>' (one percent sign and one space will appear between
-the token and the value).
+'<key>% <value>' (one percent sign and one space will appear between
+the key and the value).
 
 trailer.where::
 	This option tells where a new trailer will be added.
@@ -205,41 +215,41 @@ If it is `start`, then each new trailer will appear at the start,
 instead of the end, of the existing trailers.
 +
 If it is `after`, then each new trailer will appear just after the
-last trailer with the same <token>.
+last trailer with the same <key>.
 +
 If it is `before`, then each new trailer will appear just before the
-first trailer with the same <token>.
+first trailer with the same <key>.
 
 trailer.ifexists::
 	This option makes it possible to choose what action will be
 	performed when there is already at least one trailer with the
-	same <token> in the input.
+	same <key> in the input.
 +
 The valid values for this option are: `addIfDifferentNeighbor` (this
 is the default), `addIfDifferent`, `add`, `replace` or `doNothing`.
 +
 With `addIfDifferentNeighbor`, a new trailer will be added only if no
-trailer with the same (<token>, <value>) pair is above or below the line
+trailer with the same (<key>, <value>) pair is above or below the line
 where the new trailer will be added.
 +
 With `addIfDifferent`, a new trailer will be added only if no trailer
-with the same (<token>, <value>) pair is already in the input.
+with the same (<key>, <value>) pair is already in the input.
 +
 With `add`, a new trailer will be added, even if some trailers with
-the same (<token>, <value>) pair are already in the input.
+the same (<key>, <value>) pair are already in the input.
 +
-With `replace`, an existing trailer with the same <token> will be
+With `replace`, an existing trailer with the same <key> will be
 deleted and the new trailer will be added. The deleted trailer will be
-the closest one (with the same <token>) to the place where the new one
+the closest one (with the same <key>) to the place where the new one
 will be added.
 +
 With `doNothing`, nothing will be done; that is no new trailer will be
-added if there is already one with the same <token> in the input.
+added if there is already one with the same <key> in the input.
 
 trailer.ifmissing::
 	This option makes it possible to choose what action will be
 	performed when there is not yet any trailer with the same
-	<token> in the input.
+	<key> in the input.
 +
 The valid values for this option are: `add` (this is the default) and
 `doNothing`.
@@ -248,34 +258,40 @@ With `add`, a new trailer will be added.
 +
 With `doNothing`, nothing will be done.
 
-trailer.<token>.key::
-	This `key` will be used instead of <token> in the trailer. At
-	the end of this key, a separator can appear and then some
-	space characters. By default the only valid separator is ':',
-	but this can be changed using the `trailer.separators` config
-	variable.
+trailer.<keyAlias>.key::
+	Defines a <keyAlias> for the <key>. The <keyAlias> must be a
+	prefix (case does not matter) of the <key>. For example, in `git
+	config trailer.ack.key "Acked-by"` the "Acked-by" is the <key> and
+	the "ack" is the <keyAlias>. This configuration allows the shorter
+	`--trailer "ack:..."` invocation on the command line using the "ack"
+	<keyAlias> instead of the longer `--trailer "Acked-by:..."`.
++
+At the end of the <key>, a separator can appear and then some
+space characters. By default the only valid separator is ':',
+but this can be changed using the `trailer.separators` config
+variable.
 +
 If there is a separator in the key, then it overrides the default
 separator when adding the trailer.
 
-trailer.<token>.where::
+trailer.<keyAlias>.where::
 	This option takes the same values as the 'trailer.where'
 	configuration variable and it overrides what is specified by
-	that option for trailers with the specified <token>.
+	that option for trailers with the specified <keyAlias>.
 
-trailer.<token>.ifexists::
+trailer.<keyAlias>.ifexists::
 	This option takes the same values as the 'trailer.ifexists'
 	configuration variable and it overrides what is specified by
-	that option for trailers with the specified <token>.
+	that option for trailers with the specified <keyAlias>.
 
-trailer.<token>.ifmissing::
+trailer.<keyAlias>.ifmissing::
 	This option takes the same values as the 'trailer.ifmissing'
 	configuration variable and it overrides what is specified by
-	that option for trailers with the specified <token>.
+	that option for trailers with the specified <keyAlias>.
 
-trailer.<token>.command::
-	Deprecated in favor of 'trailer.<token>.cmd'.
-	This option behaves in the same way as 'trailer.<token>.cmd', except
+trailer.<keyAlias>.command::
+	Deprecated in favor of 'trailer.<keyAlias>.cmd'.
+	This option behaves in the same way as 'trailer.<keyAlias>.cmd', except
 	that it doesn't pass anything as argument to the specified command.
 	Instead the first occurrence of substring $ARG is replaced by the
 	<value> that would be passed as argument.
@@ -283,29 +299,29 @@ trailer.<token>.command::
 Note that $ARG in the user's command is
 only replaced once and that the original way of replacing $ARG is not safe.
 +
-When both 'trailer.<token>.cmd' and 'trailer.<token>.command' are given
-for the same <token>, 'trailer.<token>.cmd' is used and
-'trailer.<token>.command' is ignored.
+When both 'trailer.<keyAlias>.cmd' and 'trailer.<keyAlias>.command' are given
+for the same <keyAlias>, 'trailer.<keyAlias>.cmd' is used and
+'trailer.<keyAlias>.command' is ignored.
 
-trailer.<token>.cmd::
+trailer.<keyAlias>.cmd::
 	This option can be used to specify a shell command that will be called
-	once to automatically add a trailer with the specified <token>, and then
-	called each time a '--trailer <token>=<value>' argument is specified to
+	once to automatically add a trailer with the specified <keyAlias>, and then
+	called each time a '--trailer <keyAlias>=<value>' argument is specified to
 	modify the <value> of the trailer that this option would produce.
 +
 When the specified command is first called to add a trailer
-with the specified <token>, the behavior is as if a special
-'--trailer <token>=<value>' argument was added at the beginning
+with the specified <keyAlias>, the behavior is as if a special
+'--trailer <keyAlias>=<value>' argument was added at the beginning
 of the "git interpret-trailers" command, where <value>
 is taken to be the standard output of the command with any
 leading and trailing whitespace trimmed off.
 +
-If some '--trailer <token>=<value>' arguments are also passed
+If some '--trailer <keyAlias>=<value>' arguments are also passed
 on the command line, the command is called again once for each
-of these arguments with the same <token>. And the <value> part
+of these arguments with the same <keyAlias>. And the <value> part
 of these arguments, if any, will be passed to the command as its
 first argument. This way the command can produce a <value> computed
-from the <value> passed in the '--trailer <token>=<value>' argument.
+from the <value> passed in the '--trailer <keyAlias>=<value>' argument.
 
 EXAMPLES
 --------
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* Re: [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
                     ` (12 preceding siblings ...)
  2023-08-10 21:18   ` [PATCH v2 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both Linus Arver via GitGitGadget
@ 2023-08-11  1:41   ` Junio C Hamano
  2023-08-11 17:38     ` Linus Arver
  2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
  14 siblings, 1 reply; 52+ messages in thread
From: Junio C Hamano @ 2023-08-11  1:41 UTC (permalink / raw)
  To: Linus Arver via GitGitGadget; +Cc: git, Christian Couder, Linus Arver

t0450 dies like the attached, probably because the documentation was
updated to say "<key> or <keyAlias>" but a matching update to the
output of "interpret-trailers -h" command help is missing?

A possible trivial fix to 13/13 will be queued on top as "SQUASH???"
patch for now.

 builtin/interpret-trailers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git c/builtin/interpret-trailers.c w/builtin/interpret-trailers.c
index 832f86a770..d2d78fd961 100644
--- c/builtin/interpret-trailers.c
+++ w/builtin/interpret-trailers.c
@@ -14,7 +14,7 @@
 
 static const char * const git_interpret_trailers_usage[] = {
 	N_("git interpret-trailers [--in-place] [--trim-empty]\n"
-	   "                       [(--trailer <token>[(=|:)<value>])...]\n"
+	   "                       [(--trailer (<key>|<keyAlias>)[(=|:)<value>])...]\n"
 	   "                       [--parse] [<file>...]"),
 	NULL
 };





--- txt 2023-08-10 22:31:54.328609532 +0000
+++ help        2023-08-10 22:31:54.332609929 +0000
@@ -1,3 +1,3 @@
 git interpret-trailers [--in-place] [--trim-empty]
-                       [(--trailer (<key>|<keyAlias>)[(=|:)<value>])...]
+                       [(--trailer <token>[(=|:)<value>])...]
                        [--parse] [<file>...]
not ok 302 - interpret-trailers -h output and SYNOPSIS agree
#
#                       t2s="$(txt_to_synopsis "$builtin")" &&
#                       if test "$builtin" = "merge-tree"
#                       then
#                               test_when_finished "rm -f t2s.new" &&
#                               sed -e 's/ (deprecated)$//g' <"$t2s" >t2s.new
#                               t2s=t2s.new
#                       fi &&
#                       h2s="$(help_to_synopsis "$builtin")" &&
#
#                       # The *.txt and -h use different spacing for the
#                       # alignment of continued usage output, normalize it.
#                       align_after_nl "$builtin" <"$t2s" >txt &&
#                       align_after_nl "$builtin" <"$h2s" >help &&
#                       test_cmp txt help
#
1

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* Re: [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation
  2023-08-11  1:41   ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Junio C Hamano
@ 2023-08-11 17:38     ` Linus Arver
  0 siblings, 0 replies; 52+ messages in thread
From: Linus Arver @ 2023-08-11 17:38 UTC (permalink / raw)
  To: Junio C Hamano, Linus Arver via GitGitGadget; +Cc: git, Christian Couder

Junio C Hamano <gitster@pobox.com> writes:

> t0450 dies like the attached, probably because the documentation was
> updated to say "<key> or <keyAlias>" but a matching update to the
> output of "interpret-trailers -h" command help is missing?

Yes, sorry I missed this. I already have made the fix locally.

> A possible trivial fix to 13/13 will be queued on top as "SQUASH???"
> patch for now.

Thanks.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH v3 00/13] Fixes to trailer test script, help text, and documentation
  2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
                     ` (13 preceding siblings ...)
  2023-08-11  1:41   ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Junio C Hamano
@ 2023-09-07 22:19   ` Linus Arver via GitGitGadget
  2023-09-07 22:19     ` [PATCH v3 01/13] trailer tests: make test cases self-contained Linus Arver via GitGitGadget
                       ` (12 more replies)
  14 siblings, 13 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-09-07 22:19 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver

This series contains various fixes to the trailer code. They pertain to
fixes to the test script, the command line help text for the
interpret-trailers builtin, and the documentation.

Patch 1 is the most important as it does cleanups in the tests where we used
'git config' in a test case without cleaning up that state for the next
test. This makes the tests self-contained, making it easier to add new tests
anywhere along the script, without worrying about previously-set implicit
state. These test cleanups exposed lots of cases where the test cases are
mutating more configuration state than is necessary to test the specific
behavior in the test; however such extraneous configurations were not
cleaned up to make these patches easier to review (again, we are not
changing any behavior and we are also not changing what the test cases
themselves purport to do).

Note that Patch 1 was originally a 22-commit series, but was squashed
together to make it easier to see the final diff for each test case. You can
see the 22-commit breakdown at
https://github.com/listx/git/tree/backup-trailer-22-commit-breakdown

Patch 3 adds some tests to check the behavior of '--no-if-exists' and
'--no-if-missing', which weren't previously tested. It also adds
similarly-themed test cases for '--no-where' which only had 1 test case for
it.

The other patches aren't as important, but are included here because I think
they are too small to include in a separate series.


Updates in v3
=============

 * Fix t0450 failure due to mismatch between the updated documentation which
   uses " or " and the help text of the interpret-trailers command.


Updates in v2
=============

 * Many additional patches to fix the help text and docs. No changes to any
   of the patches touching the actual tests (that is, Patch 1 and 3 have
   stayed the same, other than a rewording of the commit message for Patch
   1).
 * Of these new patches, I think the last one (about <keyAlias>) is the most
   important as it resolves a longtime ambiguity about what a <token> can
   be.

Linus Arver (13):
  trailer tests: make test cases self-contained
  trailer test description: this tests --where=after, not --where=before
  trailer: add tests to check defaulting behavior with --no-* flags
  trailer doc: narrow down scope of --where and related flags
  trailer: trailer location is a place, not an action
  trailer --no-divider help: describe usual "---" meaning
  trailer --parse help: expose aliased options
  trailer --only-input: prefer "configuration variables" over "rules"
  trailer --parse docs: add explanation for its usefulness
  trailer --unfold help: prefer "reformat" over "join"
  trailer doc: emphasize the effect of configuration variables
  trailer doc: separator within key suppresses default separator
  trailer doc: <token> is a <key> or <keyAlias>, not both

 Documentation/git-interpret-trailers.txt | 183 ++++----
 builtin/interpret-trailers.c             |  12 +-
 t/t7513-interpret-trailers.sh            | 506 +++++++++++++++++++----
 3 files changed, 545 insertions(+), 156 deletions(-)


base-commit: 1b0a5129563ebe720330fdc8f5c6843d27641137
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1564%2Flistx%2Ftrailer-fixes-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1564/listx/trailer-fixes-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1564

Range-diff vs v2:

  1:  1623dd000dd =  1:  1623dd000dd trailer tests: make test cases self-contained
  2:  f680e76de84 =  2:  f680e76de84 trailer test description: this tests --where=after, not --where=before
  3:  4b5c458ef43 =  3:  4b5c458ef43 trailer: add tests to check defaulting behavior with --no-* flags
  4:  0df12c5c2dd =  4:  0df12c5c2dd trailer doc: narrow down scope of --where and related flags
  5:  040766861e2 =  5:  040766861e2 trailer: trailer location is a place, not an action
  6:  3e58b6f5ea2 =  6:  3e58b6f5ea2 trailer --no-divider help: describe usual "---" meaning
  7:  d1780a0127a =  7:  d1780a0127a trailer --parse help: expose aliased options
  8:  5cfff52da8f =  8:  5cfff52da8f trailer --only-input: prefer "configuration variables" over "rules"
  9:  ef6b77016cd =  9:  ef6b77016cd trailer --parse docs: add explanation for its usefulness
 10:  a08d78618ba = 10:  a08d78618ba trailer --unfold help: prefer "reformat" over "join"
 11:  4db823ac354 = 11:  4db823ac354 trailer doc: emphasize the effect of configuration variables
 12:  66087eaf5bd = 12:  66087eaf5bd trailer doc: separator within key suppresses default separator
 13:  7b66cf29d29 ! 13:  0b9525db5a0 trailer doc: <token> is a <key> or <keyAlias>, not both
     @@ Documentation/git-interpret-trailers.txt: trailer.<token>.command::
       
       EXAMPLES
       --------
     +
     + ## builtin/interpret-trailers.c ##
     +@@
     + 
     + static const char * const git_interpret_trailers_usage[] = {
     + 	N_("git interpret-trailers [--in-place] [--trim-empty]\n"
     +-	   "                       [(--trailer <token>[(=|:)<value>])...]\n"
     ++	   "                       [(--trailer (<key>|<keyAlias>)[(=|:)<value>])...]\n"
     + 	   "                       [--parse] [<file>...]"),
     + 	NULL
     + };

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH v3 01/13] trailer tests: make test cases self-contained
  2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
@ 2023-09-07 22:19     ` Linus Arver via GitGitGadget
  2023-09-07 22:19     ` [PATCH v3 02/13] trailer test description: this tests --where=after, not --where=before Linus Arver via GitGitGadget
                       ` (11 subsequent siblings)
  12 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-09-07 22:19 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

By using "test_config" instead of "git config", we avoid leaking
configuration state across test cases. This in turn helps to make the
tests more self-contained, by explicitly capturing the configuration
setup. It then makes it easier to add tests anywhere in this 1500+ line
file, without worrying about what implicit state was set in some prior
test case defined earlier up in the script.

This commit was created mechanically as follows: we changed the first
occurrence of a particular "git config trailer.*" option, then ran the
tests repeatedly to see which ones broke, adding in the extra
"test_config" equivalents to make them pass again. In addition, in some
test cases we removed "git config --unset ..." lines because they were
no longer necessary (as the --unset was being used to clean up leaked
configuration state from earlier test cases).

The process described above was done repeatedly until there were no more
unbridled "git config" invocations. Some "git config" invocations still
do exist in the script, but they were already cleaned up properly with

    test_when_finished "git config --remove-section ..."

so they were left alone.

Note that these cleanups result in generally longer test case setups
because the previously hidden state is now being exposed. Although we
could then clean up the test cases' "expected" values to be less
verbose (the verbosity arising from the use of implicit state), we
choose not to do so here, to make sure that this cleanup does not change
any meanings behind the test cases.

Signed-off-by: Linus Arver <linusa@google.com>
---
 t/t7513-interpret-trailers.sh | 374 +++++++++++++++++++++++++++-------
 1 file changed, 300 insertions(+), 74 deletions(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 97f10905d23..5b31896070a 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -489,7 +489,7 @@ test_expect_success 'multiline field treated as atomic for neighbor check' '
 '
 
 test_expect_success 'with config setup' '
-	git config trailer.ack.key "Acked-by: " &&
+	test_config trailer.ack.key "Acked-by: " &&
 	cat >expected <<-\EOF &&
 
 		Acked-by: Peff
@@ -503,8 +503,8 @@ test_expect_success 'with config setup' '
 '
 
 test_expect_success 'with config setup and ":=" as separators' '
-	git config trailer.separators ":=" &&
-	git config trailer.ack.key "Acked-by= " &&
+	test_config trailer.separators ":=" &&
+	test_config trailer.ack.key "Acked-by= " &&
 	cat >expected <<-\EOF &&
 
 		Acked-by= Peff
@@ -518,7 +518,7 @@ test_expect_success 'with config setup and ":=" as separators' '
 '
 
 test_expect_success 'with config setup and "%" as separators' '
-	git config trailer.separators "%" &&
+	test_config trailer.separators "%" &&
 	cat >expected <<-\EOF &&
 
 		bug% 42
@@ -532,6 +532,7 @@ test_expect_success 'with config setup and "%" as separators' '
 '
 
 test_expect_success 'with "%" as separators and a message with trailers' '
+	test_config trailer.separators "%" &&
 	cat >special_message <<-\EOF &&
 		Special Message
 
@@ -553,8 +554,8 @@ test_expect_success 'with "%" as separators and a message with trailers' '
 '
 
 test_expect_success 'with config setup and ":=#" as separators' '
-	git config trailer.separators ":=#" &&
-	git config trailer.bug.key "Bug #" &&
+	test_config trailer.separators ":=#" &&
+	test_config trailer.bug.key "Bug #" &&
 	cat >expected <<-\EOF &&
 
 		Bug #42
@@ -581,6 +582,8 @@ test_expect_success 'with basic patch' '
 '
 
 test_expect_success 'with commit complex message as argument' '
+	test_config trailer.separators ":=" &&
+	test_config trailer.ack.key "Acked-by= " &&
 	cat complex_message_body complex_message_trailers >complex_message &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
@@ -594,6 +597,8 @@ test_expect_success 'with commit complex message as argument' '
 '
 
 test_expect_success 'with 2 files arguments' '
+	test_config trailer.separators ":=" &&
+	test_config trailer.ack.key "Acked-by= " &&
 	cat basic_message >>expected &&
 	echo >>expected &&
 	cat basic_patch >>expected &&
@@ -677,6 +682,9 @@ test_expect_success 'with message that has an old style conflict block' '
 '
 
 test_expect_success 'with commit complex message and trailer args' '
+	test_config trailer.separators ":=#" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -692,6 +700,9 @@ test_expect_success 'with commit complex message and trailer args' '
 '
 
 test_expect_success 'with complex patch, args and --trim-empty' '
+	test_config trailer.separators ":=#" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
 	cat complex_message >complex_patch &&
 	cat basic_patch >>complex_patch &&
 	cat complex_message_body >expected &&
@@ -746,7 +757,10 @@ test_expect_success POSIXPERM,SANITY "in-place editing doesn't clobber original
 '
 
 test_expect_success 'using "where = before"' '
-	git config trailer.bug.where "before" &&
+	test_config trailer.separators ":=#" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -762,7 +776,9 @@ test_expect_success 'using "where = before"' '
 '
 
 test_expect_success 'overriding configuration with "--where after"' '
-	git config trailer.ack.where "before" &&
+	test_config trailer.separators ":=" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "before" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -777,6 +793,11 @@ test_expect_success 'overriding configuration with "--where after"' '
 '
 
 test_expect_success 'using "where = before" with "--no-where"' '
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "before" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -792,7 +813,11 @@ test_expect_success 'using "where = before" with "--no-where"' '
 '
 
 test_expect_success 'using "where = after"' '
-	git config trailer.ack.where "after" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -808,8 +833,11 @@ test_expect_success 'using "where = after"' '
 '
 
 test_expect_success 'using "where = end"' '
-	git config trailer.review.key "Reviewed-by" &&
-	git config trailer.review.where "end" &&
+	test_config trailer.review.key "Reviewed-by" &&
+	test_config trailer.review.where "end" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -827,8 +855,11 @@ test_expect_success 'using "where = end"' '
 '
 
 test_expect_success 'using "where = start"' '
-	git config trailer.review.key "Reviewed-by" &&
-	git config trailer.review.where "start" &&
+	test_config trailer.review.key "Reviewed-by" &&
+	test_config trailer.review.where "start" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Reviewed-by: Johannes
@@ -846,8 +877,13 @@ test_expect_success 'using "where = start"' '
 '
 
 test_expect_success 'using "where = before" for a token in the middle of the message' '
-	git config trailer.review.key "Reviewed-by:" &&
-	git config trailer.review.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.review.where "before" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -864,6 +900,12 @@ test_expect_success 'using "where = before" for a token in the middle of the mes
 '
 
 test_expect_success 'using "where = before" and --trim-empty' '
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	cat >>expected <<-\EOF &&
 		Bug #46
@@ -878,6 +920,13 @@ test_expect_success 'using "where = before" and --trim-empty' '
 '
 
 test_expect_success 'the default is "ifExists = addIfDifferentNeighbor"' '
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.review.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -896,7 +945,13 @@ test_expect_success 'the default is "ifExists = addIfDifferentNeighbor"' '
 '
 
 test_expect_success 'default "ifExists" is now "addIfDifferent"' '
-	git config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -914,8 +969,14 @@ test_expect_success 'default "ifExists" is now "addIfDifferent"' '
 '
 
 test_expect_success 'using "ifExists = addIfDifferent" with "where = end"' '
-	git config trailer.ack.ifExists "addIfDifferent" &&
-	git config trailer.ack.where "end" &&
+	test_config trailer.ack.ifExists "addIfDifferent" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "end" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -932,8 +993,14 @@ test_expect_success 'using "ifExists = addIfDifferent" with "where = end"' '
 '
 
 test_expect_success 'using "ifExists = addIfDifferent" with "where = before"' '
-	git config trailer.ack.ifExists "addIfDifferent" &&
-	git config trailer.ack.where "before" &&
+	test_config trailer.ack.ifExists "addIfDifferent" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "before" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -950,8 +1017,14 @@ test_expect_success 'using "ifExists = addIfDifferent" with "where = before"' '
 '
 
 test_expect_success 'using "ifExists = addIfDifferentNeighbor" with "where = end"' '
-	git config trailer.ack.ifExists "addIfDifferentNeighbor" &&
-	git config trailer.ack.where "end" &&
+	test_config trailer.ack.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "end" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -973,8 +1046,14 @@ test_expect_success 'using "ifExists = addIfDifferentNeighbor" with "where = end
 '
 
 test_expect_success 'using "ifExists = addIfDifferentNeighbor"  with "where = after"' '
-	git config trailer.ack.ifExists "addIfDifferentNeighbor" &&
-	git config trailer.ack.where "after" &&
+	test_config trailer.ack.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -995,7 +1074,11 @@ test_expect_success 'using "ifExists = addIfDifferentNeighbor"  with "where = af
 '
 
 test_expect_success 'using "ifExists = addIfDifferentNeighbor" and --trim-empty' '
-	git config trailer.ack.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.ack.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	cat >>expected <<-\EOF &&
 		Bug #42
@@ -1011,8 +1094,14 @@ test_expect_success 'using "ifExists = addIfDifferentNeighbor" and --trim-empty'
 '
 
 test_expect_success 'using "ifExists = add" with "where = end"' '
-	git config trailer.ack.ifExists "add" &&
-	git config trailer.ack.where "end" &&
+	test_config trailer.ack.ifExists "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "end" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1036,8 +1125,14 @@ test_expect_success 'using "ifExists = add" with "where = end"' '
 '
 
 test_expect_success 'using "ifExists = add" with "where = after"' '
-	git config trailer.ack.ifExists "add" &&
-	git config trailer.ack.where "after" &&
+	test_config trailer.ack.ifExists "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1058,8 +1153,15 @@ test_expect_success 'using "ifExists = add" with "where = after"' '
 '
 
 test_expect_success 'overriding configuration with "--if-exists replace"' '
-	git config trailer.fix.key "Fixes: " &&
-	git config trailer.fix.ifExists "add" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.review.where "before" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1075,8 +1177,15 @@ test_expect_success 'overriding configuration with "--if-exists replace"' '
 '
 
 test_expect_success 'using "ifExists = replace"' '
-	git config trailer.fix.key "Fixes: " &&
-	git config trailer.fix.ifExists "replace" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "replace" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1095,7 +1204,16 @@ test_expect_success 'using "ifExists = replace"' '
 '
 
 test_expect_success 'using "ifExists = replace" with "where = after"' '
-	git config trailer.fix.where "after" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "replace" &&
+	test_config trailer.fix.where "after" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1114,7 +1232,15 @@ test_expect_success 'using "ifExists = replace" with "where = after"' '
 '
 
 test_expect_success 'using "ifExists = doNothing"' '
-	git config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1133,8 +1259,17 @@ test_expect_success 'using "ifExists = doNothing"' '
 '
 
 test_expect_success 'the default is "ifMissing = add"' '
-	git config trailer.cc.key "Cc: " &&
-	git config trailer.cc.where "before" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.cc.key "Cc: " &&
+	test_config trailer.cc.where "before" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1154,7 +1289,14 @@ test_expect_success 'the default is "ifMissing = add"' '
 '
 
 test_expect_success 'overriding configuration with "--if-missing doNothing"' '
-	git config trailer.ifmissing "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.ifmissing "add" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -1173,7 +1315,13 @@ test_expect_success 'overriding configuration with "--if-missing doNothing"' '
 '
 
 test_expect_success 'when default "ifMissing" is "doNothing"' '
-	git config trailer.ifmissing "doNothing" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.ifmissing "doNothing" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -1187,14 +1335,21 @@ test_expect_success 'when default "ifMissing" is "doNothing"' '
 		--trailer "cc=Linus" --trailer "ack: Junio" \
 		--trailer "fix=22" --trailer "bug: 42" --trailer "ack: Peff" \
 		<complex_message >actual &&
-	test_cmp expected actual &&
-	git config trailer.ifmissing "add"
+	test_cmp expected actual
 '
 
 test_expect_success 'using "ifMissing = add" with "where = end"' '
-	git config trailer.cc.key "Cc: " &&
-	git config trailer.cc.where "end" &&
-	git config trailer.cc.ifMissing "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.cc.key "Cc: " &&
+	test_config trailer.cc.ifMissing "add" &&
+	test_config trailer.cc.where "end" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1214,9 +1369,17 @@ test_expect_success 'using "ifMissing = add" with "where = end"' '
 '
 
 test_expect_success 'using "ifMissing = add" with "where = before"' '
-	git config trailer.cc.key "Cc: " &&
-	git config trailer.cc.where "before" &&
-	git config trailer.cc.ifMissing "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.cc.key "Cc: " &&
+	test_config trailer.cc.ifMissing "add" &&
+	test_config trailer.cc.where "before" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Cc: Linus
@@ -1236,7 +1399,15 @@ test_expect_success 'using "ifMissing = add" with "where = before"' '
 '
 
 test_expect_success 'using "ifMissing = doNothing"' '
-	git config trailer.cc.ifMissing "doNothing" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.cc.ifMissing "doNothing" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1256,7 +1427,15 @@ test_expect_success 'using "ifMissing = doNothing"' '
 
 test_expect_success 'default "where" is now "after"' '
 	git config trailer.where "after" &&
-	git config --unset trailer.ack.where &&
+	test_config trailer.ack.ifExists "add" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.ack.where "after" &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.bug.where "before" &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=#" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Bug #42
@@ -1280,10 +1459,15 @@ test_expect_success 'default "where" is now "after"' '
 '
 
 test_expect_success 'with simple command' '
-	git config trailer.sign.key "Signed-off-by: " &&
-	git config trailer.sign.where "after" &&
-	git config trailer.sign.ifExists "addIfDifferentNeighbor" &&
-	git config trailer.sign.command "echo \"A U Thor <author@example.com>\"" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"A U Thor <author@example.com>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.sign.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.sign.where "after" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -1298,8 +1482,14 @@ test_expect_success 'with simple command' '
 '
 
 test_expect_success 'with command using committer information' '
-	git config trailer.sign.ifExists "addIfDifferent" &&
-	git config trailer.sign.command "echo \"\$GIT_COMMITTER_NAME <\$GIT_COMMITTER_EMAIL>\"" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"\$GIT_COMMITTER_NAME <\$GIT_COMMITTER_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.sign.ifExists "addIfDifferent" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -1314,10 +1504,15 @@ test_expect_success 'with command using committer information' '
 '
 
 test_expect_success 'with command using author information' '
-	git config trailer.sign.key "Signed-off-by: " &&
-	git config trailer.sign.where "after" &&
-	git config trailer.sign.ifExists "addIfDifferentNeighbor" &&
-	git config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.ifExists "doNothing" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.sign.ifExists "addIfDifferentNeighbor" &&
+	test_config trailer.sign.where "after" &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
 		Fixes: Z
@@ -1338,12 +1533,19 @@ test_expect_success 'setup a commit' '
 '
 
 test_expect_success 'cmd takes precedence over command' '
-	test_when_finished "git config --unset trailer.fix.cmd" &&
-	git config trailer.fix.ifExists "replace" &&
-	git config trailer.fix.cmd "test -n \"\$1\" && git log -1 --oneline --format=\"%h (%aN)\" \
-	--abbrev-commit --abbrev=14 \"\$1\" || true" &&
-	git config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" \
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" \
 		--abbrev-commit --abbrev=14 \$ARG" &&
+	test_config trailer.fix.cmd "test -n \"\$1\" && git log -1 --oneline --format=\"%h (%aN)\" \
+	--abbrev-commit --abbrev=14 \"\$1\" || true" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "replace" &&
+	test_config trailer.fix.where "after" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	FIXED=$(git log -1 --oneline --format="%h (%aN)" --abbrev-commit --abbrev=14 HEAD) &&
 	cat complex_message_body >expected2 &&
 	sed -e "s/ Z\$/ /" >>expected2 <<-EOF &&
@@ -1359,8 +1561,16 @@ test_expect_success 'cmd takes precedence over command' '
 '
 
 test_expect_success 'with command using $ARG' '
-	git config trailer.fix.ifExists "replace" &&
-	git config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$ARG" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$ARG" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "replace" &&
+	test_config trailer.fix.where "after" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	FIXED=$(git log -1 --oneline --format="%h (%s)" --abbrev-commit --abbrev=14 HEAD) &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-EOF &&
@@ -1376,8 +1586,16 @@ test_expect_success 'with command using $ARG' '
 '
 
 test_expect_success 'with failing command using $ARG' '
-	git config trailer.fix.ifExists "replace" &&
-	git config trailer.fix.command "false \$ARG" &&
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.fix.command "false \$ARG" &&
+	test_config trailer.fix.key "Fixes: " &&
+	test_config trailer.fix.ifExists "replace" &&
+	test_config trailer.fix.where "after" &&
+	test_config trailer.review.key "Reviewed-by:" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.ifexists "addIfDifferent" &&
+	test_config trailer.separators ":=" &&
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-EOF &&
 		Fixes: Z
@@ -1392,7 +1610,9 @@ test_expect_success 'with failing command using $ARG' '
 '
 
 test_expect_success 'with empty tokens' '
-	git config --unset trailer.fix.command &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.sign.key "Signed-off-by: " &&
+	test_config trailer.ifexists "addIfDifferent" &&
 	cat >expected <<-EOF &&
 
 		Signed-off-by: A U Thor <author@example.com>
@@ -1403,7 +1623,8 @@ test_expect_success 'with empty tokens' '
 '
 
 test_expect_success 'with command but no key' '
-	git config --unset trailer.sign.key &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.ifexists "addIfDifferent" &&
 	cat >expected <<-EOF &&
 
 		sign: A U Thor <author@example.com>
@@ -1414,7 +1635,9 @@ test_expect_success 'with command but no key' '
 '
 
 test_expect_success 'with no command and no key' '
-	git config --unset trailer.review.key &&
+	test_config trailer.review.where "before" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
+	test_config trailer.ifexists "addIfDifferent" &&
 	cat >expected <<-EOF &&
 
 		review: Junio
@@ -1426,6 +1649,8 @@ test_expect_success 'with no command and no key' '
 '
 
 test_expect_success 'with cut line' '
+	test_config trailer.review.where "before" &&
+	test_config trailer.sign.command "echo \"\$GIT_AUTHOR_NAME <\$GIT_AUTHOR_EMAIL>\"" &&
 	cat >expected <<-\EOF &&
 		my subject
 
@@ -1443,7 +1668,8 @@ test_expect_success 'with cut line' '
 '
 
 test_expect_success 'only trailers' '
-	git config trailer.sign.command "echo config-value" &&
+	test_config trailer.sign.command "echo config-value" &&
+	test_config trailer.ifexists "addIfDifferent" &&
 	cat >expected <<-\EOF &&
 		existing: existing-value
 		sign: config-value
@@ -1462,7 +1688,7 @@ test_expect_success 'only trailers' '
 '
 
 test_expect_success 'only-trailers omits non-trailer in middle of block' '
-	git config trailer.sign.command "echo config-value" &&
+	test_config trailer.sign.command "echo config-value" &&
 	cat >expected <<-\EOF &&
 		Signed-off-by: nobody <nobody@nowhere>
 		Signed-off-by: somebody <somebody@somewhere>
@@ -1482,7 +1708,7 @@ test_expect_success 'only-trailers omits non-trailer in middle of block' '
 '
 
 test_expect_success 'only input' '
-	git config trailer.sign.command "echo config-value" &&
+	test_config trailer.sign.command "echo config-value" &&
 	cat >expected <<-\EOF &&
 		existing: existing-value
 	EOF
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v3 02/13] trailer test description: this tests --where=after, not --where=before
  2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
  2023-09-07 22:19     ` [PATCH v3 01/13] trailer tests: make test cases self-contained Linus Arver via GitGitGadget
@ 2023-09-07 22:19     ` Linus Arver via GitGitGadget
  2023-09-07 22:19     ` [PATCH v3 03/13] trailer: add tests to check defaulting behavior with --no-* flags Linus Arver via GitGitGadget
                       ` (10 subsequent siblings)
  12 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-09-07 22:19 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

Signed-off-by: Linus Arver <linusa@google.com>
---
 t/t7513-interpret-trailers.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 5b31896070a..ed0fc04bd95 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -792,7 +792,7 @@ test_expect_success 'overriding configuration with "--where after"' '
 	test_cmp expected actual
 '
 
-test_expect_success 'using "where = before" with "--no-where"' '
+test_expect_success 'using "--where after" with "--no-where"' '
 	test_config trailer.ack.key "Acked-by= " &&
 	test_config trailer.ack.where "before" &&
 	test_config trailer.bug.key "Bug #" &&
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v3 03/13] trailer: add tests to check defaulting behavior with --no-* flags
  2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
  2023-09-07 22:19     ` [PATCH v3 01/13] trailer tests: make test cases self-contained Linus Arver via GitGitGadget
  2023-09-07 22:19     ` [PATCH v3 02/13] trailer test description: this tests --where=after, not --where=before Linus Arver via GitGitGadget
@ 2023-09-07 22:19     ` Linus Arver via GitGitGadget
  2023-09-08 21:52       ` Junio C Hamano
  2023-09-07 22:20     ` [PATCH v3 04/13] trailer doc: narrow down scope of --where and related flags Linus Arver via GitGitGadget
                       ` (9 subsequent siblings)
  12 siblings, 1 reply; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-09-07 22:19 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

While the "--no-where" flag is tested, the "--no-if-exists" and
"--no-if-missing" flags are not, so add tests for them. But also add
tests for all "--no-*" flags to check their effects, both when (1) there
are relevant configuration variables set, and (2) they are not set.

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt |  14 ++-
 t/t7513-interpret-trailers.sh            | 130 +++++++++++++++++++++++
 2 files changed, 140 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 55d89614661..91a4dbc9a72 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -114,8 +114,10 @@ OPTIONS
 	Specify where all new trailers will be added.  A setting
 	provided with '--where' overrides all configuration variables
 	and applies to all '--trailer' options until the next occurrence of
-	'--where' or '--no-where'. Possible values are `after`, `before`,
-	`end` or `start`.
+	'--where' or '--no-where'. Upon encountering '--no-where', clear the
+	effect of any previous use of '--where', such that the relevant configuration
+	variables are no longer overridden. Possible values are `after`,
+	`before`, `end` or `start`.
 
 --if-exists <action>::
 --no-if-exists::
@@ -123,7 +125,9 @@ OPTIONS
 	least one trailer with the same <token> in the input.  A setting
 	provided with '--if-exists' overrides all configuration variables
 	and applies to all '--trailer' options until the next occurrence of
-	'--if-exists' or '--no-if-exists'. Possible actions are `addIfDifferent`,
+	'--if-exists' or '--no-if-exists'. Upon encountering '--no-if-exists, clear the
+	effect of any previous use of '--if-exists, such that the relevant configuration
+	variables are no longer overridden. Possible actions are `addIfDifferent`,
 	`addIfDifferentNeighbor`, `add`, `replace` and `doNothing`.
 
 --if-missing <action>::
@@ -132,7 +136,9 @@ OPTIONS
 	trailer with the same <token> in the input.  A setting
 	provided with '--if-missing' overrides all configuration variables
 	and applies to all '--trailer' options until the next occurrence of
-	'--if-missing' or '--no-if-missing'. Possible actions are `doNothing`
+	'--if-missing' or '--no-if-missing'. Upon encountering '--no-if-missing,
+	clear the effect of any previous use of '--if-missing, such that the relevant
+	configuration variables are no longer overridden. Possible actions are `doNothing`
 	or `add`.
 
 --only-trailers::
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index ed0fc04bd95..832aff06167 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -812,6 +812,53 @@ test_expect_success 'using "--where after" with "--no-where"' '
 	test_cmp expected actual
 '
 
+# Check whether using "--no-where" clears out only the "--where after", such
+# that we still use the configuration in trailer.where (which is different from
+# the hardcoded default (in WHERE_END) assuming the absence of .gitconfig).
+# Here, the "start" setting of trailer.where is respected, so the new "Acked-by"
+# and "Bug" trailers are placed at the beginning, and not at the end which is
+# the harcoded default.
+test_expect_success 'using "--where after" with "--no-where" defaults to configuration' '
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.separators ":=#" &&
+	test_config trailer.where "start" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Bug #42
+		Acked-by= Peff
+		Fixes: Z
+		Acked-by= Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers --where after --no-where --trailer "ack: Peff" \
+		--trailer "bug: 42" complex_message >actual &&
+	test_cmp expected actual
+'
+
+# The "--where after" will only get respected for the trailer that came
+# immediately after it. For the next trailer (Bug #42), we default to using the
+# hardcoded WHERE_END because we don't have any "trailer.where" or
+# "trailer.bug.where" configured.
+test_expect_success 'using "--no-where" defaults to harcoded default if nothing configured' '
+	test_config trailer.ack.key "Acked-by= " &&
+	test_config trailer.bug.key "Bug #" &&
+	test_config trailer.separators ":=#" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by= Z
+		Acked-by= Peff
+		Reviewed-by: Z
+		Signed-off-by: Z
+		Bug #42
+	EOF
+	git interpret-trailers --where after --trailer "ack: Peff" --no-where \
+		--trailer "bug: 42" complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'using "where = after"' '
 	test_config trailer.ack.key "Acked-by= " &&
 	test_config trailer.ack.where "after" &&
@@ -1176,6 +1223,56 @@ test_expect_success 'overriding configuration with "--if-exists replace"' '
 	test_cmp expected actual
 '
 
+# "trailer.ifexists" is set to "doNothing", so using "--no-if-exists" defaults
+# to this "doNothing" behavior. So the "Fixes: 53" trailer does not get added.
+test_expect_success 'using "--if-exists replace" with "--no-if-exists" defaults to configuration' '
+	test_config trailer.ifexists "doNothing" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Fixes: Z
+		Acked-by: Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+	EOF
+	git interpret-trailers --if-exists replace --no-if-exists --trailer "Fixes: 53" \
+		<complex_message >actual &&
+	test_cmp expected actual
+'
+
+# No "ifexists" configuration is set, so using "--no-if-exists" makes it default
+# to addIfDifferentNeighbor. Because we do have a different neighbor "Fixes: 53"
+# (because it got added by overriding with "--if-exists replace" earlier in the
+# arguments list), we add "Signed-off-by: addme".
+test_expect_success 'using "--no-if-exists" defaults to hardcoded default if nothing configured' '
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Acked-by: Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+		Fixes: 53
+		Signed-off-by: addme
+	EOF
+	git interpret-trailers --if-exists replace --trailer "Fixes: 53" --no-if-exists \
+		--trailer "Signed-off-by: addme" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+# The second "Fixes: 53" trailer is discarded, because the "--no-if-exists" here
+# makes us default to addIfDifferentNeighbor, and we already added the "Fixes:
+# 53" trailer earlier in the argument list.
+test_expect_success 'using "--no-if-exists" defaults to hardcoded default if nothing configured (no addition)' '
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+		Acked-by: Z
+		Reviewed-by: Z
+		Signed-off-by: Z
+		Fixes: 53
+	EOF
+	git interpret-trailers --if-exists replace --trailer "Fixes: 53" --no-if-exists \
+		--trailer "Fixes: 53" <complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'using "ifExists = replace"' '
 	test_config trailer.fix.key "Fixes: " &&
 	test_config trailer.fix.ifExists "replace" &&
@@ -1425,6 +1522,39 @@ test_expect_success 'using "ifMissing = doNothing"' '
 	test_cmp expected actual
 '
 
+# Ignore the "IgnoredTrailer" because of "--if-missing doNothing", but also
+# ignore the "StillIgnoredTrailer" because we set "trailer.ifMissing" to
+# "doNothing" in configuration.
+test_expect_success 'using "--no-if-missing" defaults to configuration' '
+	test_config trailer.ifMissing "doNothing" &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+			Fixes: Z
+			Acked-by: Z
+			Reviewed-by: Z
+			Signed-off-by: Z
+	EOF
+	git interpret-trailers --if-missing doNothing --trailer "IgnoredTrailer: ignoreme" --no-if-missing \
+			--trailer "StillIgnoredTrailer: ignoreme" <complex_message >actual &&
+	test_cmp expected actual
+'
+
+# Add the "AddedTrailer" because the "--no-if-missing" clears the "--if-missing
+# doNothing" from earlier in the argument list.
+test_expect_success 'using "--no-if-missing" defaults to hardcoded default if nothing configured' '
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+			Fixes: Z
+			Acked-by: Z
+			Reviewed-by: Z
+			Signed-off-by: Z
+			AddedTrailer: addme
+	EOF
+	git interpret-trailers --if-missing doNothing --trailer "IgnoredTrailer: ignoreme" --no-if-missing \
+			--trailer "AddedTrailer: addme" <complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'default "where" is now "after"' '
 	git config trailer.where "after" &&
 	test_config trailer.ack.ifExists "add" &&
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v3 04/13] trailer doc: narrow down scope of --where and related flags
  2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
                       ` (2 preceding siblings ...)
  2023-09-07 22:19     ` [PATCH v3 03/13] trailer: add tests to check defaulting behavior with --no-* flags Linus Arver via GitGitGadget
@ 2023-09-07 22:20     ` Linus Arver via GitGitGadget
  2023-09-07 22:20     ` [PATCH v3 05/13] trailer: trailer location is a place, not an action Linus Arver via GitGitGadget
                       ` (8 subsequent siblings)
  12 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

The wording "all configuration variables" is misleading (the same could
be said to the descriptions of the "--[no-]if-exists" and the
"--[no-]if-missing" options).  Specifying --where=value overrides only
the trailer.where variable and applicable trailer.<token>.where
variables, and --no-where stops the overriding of these variables.
Ditto for the other two with their relevant configuration variables.

Reported-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 91a4dbc9a72..72f5bdb652f 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -112,7 +112,8 @@ OPTIONS
 --where <placement>::
 --no-where::
 	Specify where all new trailers will be added.  A setting
-	provided with '--where' overrides all configuration variables
+	provided with '--where' overrides the `trailer.where` and any
+	applicable `trailer.<token>.where` configuration variables
 	and applies to all '--trailer' options until the next occurrence of
 	'--where' or '--no-where'. Upon encountering '--no-where', clear the
 	effect of any previous use of '--where', such that the relevant configuration
@@ -123,7 +124,8 @@ OPTIONS
 --no-if-exists::
 	Specify what action will be performed when there is already at
 	least one trailer with the same <token> in the input.  A setting
-	provided with '--if-exists' overrides all configuration variables
+	provided with '--if-exists' overrides the `trailer.ifExists` and any
+	applicable `trailer.<token>.ifExists` configuration variables
 	and applies to all '--trailer' options until the next occurrence of
 	'--if-exists' or '--no-if-exists'. Upon encountering '--no-if-exists, clear the
 	effect of any previous use of '--if-exists, such that the relevant configuration
@@ -134,7 +136,8 @@ OPTIONS
 --no-if-missing::
 	Specify what action will be performed when there is no other
 	trailer with the same <token> in the input.  A setting
-	provided with '--if-missing' overrides all configuration variables
+	provided with '--if-missing' overrides the `trailer.ifMissing` and any
+	applicable `trailer.<token>.ifMissing` configuration variables
 	and applies to all '--trailer' options until the next occurrence of
 	'--if-missing' or '--no-if-missing'. Upon encountering '--no-if-missing,
 	clear the effect of any previous use of '--if-missing, such that the relevant
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v3 05/13] trailer: trailer location is a place, not an action
  2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
                       ` (3 preceding siblings ...)
  2023-09-07 22:20     ` [PATCH v3 04/13] trailer doc: narrow down scope of --where and related flags Linus Arver via GitGitGadget
@ 2023-09-07 22:20     ` Linus Arver via GitGitGadget
  2023-09-19 22:13       ` Jonathan Tan
  2023-09-07 22:20     ` [PATCH v3 06/13] trailer --no-divider help: describe usual "---" meaning Linus Arver via GitGitGadget
                       ` (7 subsequent siblings)
  12 siblings, 1 reply; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

Fix the help text to say "placement" instead of "action" because the
values are placements, not actions.

While we're at it, tweak the documentation to say "placements" instead
of "values", similar to how the existing language for "--if-exists" uses
the word "action" to describe both the syntax (with the phrase
"--if-exists <action>") and the possible values (with the phrase
"possible actions").

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 2 +-
 builtin/interpret-trailers.c             | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 72f5bdb652f..b5284c3d33f 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -117,7 +117,7 @@ OPTIONS
 	and applies to all '--trailer' options until the next occurrence of
 	'--where' or '--no-where'. Upon encountering '--no-where', clear the
 	effect of any previous use of '--where', such that the relevant configuration
-	variables are no longer overridden. Possible values are `after`,
+	variables are no longer overridden. Possible placements are `after`,
 	`before`, `end` or `start`.
 
 --if-exists <action>::
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index c5e83452654..cf4f703c4e2 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -97,7 +97,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
 		OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")),
 
-		OPT_CALLBACK(0, "where", NULL, N_("action"),
+		OPT_CALLBACK(0, "where", NULL, N_("placement"),
 			     N_("where to place the new trailer"), option_parse_where),
 		OPT_CALLBACK(0, "if-exists", NULL, N_("action"),
 			     N_("action if trailer already exists"), option_parse_if_exists),
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v3 06/13] trailer --no-divider help: describe usual "---" meaning
  2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
                       ` (4 preceding siblings ...)
  2023-09-07 22:20     ` [PATCH v3 05/13] trailer: trailer location is a place, not an action Linus Arver via GitGitGadget
@ 2023-09-07 22:20     ` Linus Arver via GitGitGadget
  2023-09-08 21:53       ` Junio C Hamano
  2023-09-07 22:20     ` [PATCH v3 07/13] trailer --parse help: expose aliased options Linus Arver via GitGitGadget
                       ` (6 subsequent siblings)
  12 siblings, 1 reply; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

It's unclear what treating something "specially" means.

Signed-off-by: Linus Arver <linusa@google.com>
---
 builtin/interpret-trailers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index cf4f703c4e2..a7623dbfb2e 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -109,7 +109,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "unfold", &opts.unfold, N_("join whitespace-continued values")),
 		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")),
+		OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
 		OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
 				N_("trailer(s) to add"), option_parse_trailer),
 		OPT_END()
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v3 07/13] trailer --parse help: expose aliased options
  2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
                       ` (5 preceding siblings ...)
  2023-09-07 22:20     ` [PATCH v3 06/13] trailer --no-divider help: describe usual "---" meaning Linus Arver via GitGitGadget
@ 2023-09-07 22:20     ` Linus Arver via GitGitGadget
  2023-09-19 22:16       ` Jonathan Tan
  2023-09-07 22:20     ` [PATCH v3 08/13] trailer --only-input: prefer "configuration variables" over "rules" Linus Arver via GitGitGadget
                       ` (5 subsequent siblings)
  12 siblings, 1 reply; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

The existing description "set parsing options" is vague, because
arguably _all_ of the options for interpret-trailers have to do with
parsing to some degree.

Explain what this flag does to match what is in the docs, namely how
it is an alias for "--only-trailers --only-input --unfold".

Signed-off-by: Linus Arver <linusa@google.com>
---
 builtin/interpret-trailers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index a7623dbfb2e..5f3e1a38eee 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -107,7 +107,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_CALLBACK_F(0, "parse", &opts, NULL, N_("set parsing options"),
+		OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("alias for --only-trailers --only-input --unfold"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
 		OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
 		OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v3 08/13] trailer --only-input: prefer "configuration variables" over "rules"
  2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
                       ` (6 preceding siblings ...)
  2023-09-07 22:20     ` [PATCH v3 07/13] trailer --parse help: expose aliased options Linus Arver via GitGitGadget
@ 2023-09-07 22:20     ` Linus Arver via GitGitGadget
  2023-09-07 22:20     ` [PATCH v3 09/13] trailer --parse docs: add explanation for its usefulness Linus Arver via GitGitGadget
                       ` (4 subsequent siblings)
  12 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

Use the phrase "configuration variables" instead of "rules" because

(1) we already say "configuration variables" in multiple
    places in the docs (where the word "rules" is only used for describing
    "--only-input" behavior and for an unrelated case of mentioning how
    the trailers do not follow "rules for RFC 822 headers"), and

(2) this phrase is more specific than just "rules".

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 4 ++--
 builtin/interpret-trailers.c             | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index b5284c3d33f..0eea937c30e 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -149,8 +149,8 @@ OPTIONS
 
 --only-input::
 	Output only trailers that exist in the input; do not add any
-	from the command-line or by following configured `trailer.*`
-	rules.
+	from the command-line or by applying `trailer.*` configuration
+	variables.
 
 --unfold::
 	Remove any whitespace-continuation in trailers, so that each
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 5f3e1a38eee..f70c5df8d4b 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -105,7 +105,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 			     N_("action if trailer is missing"), option_parse_if_missing),
 
 		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, "only-input", &opts.only_input, N_("do not apply trailer.* configuration variables")),
 		OPT_BOOL(0, "unfold", &opts.unfold, N_("join whitespace-continued values")),
 		OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("alias for --only-trailers --only-input --unfold"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v3 09/13] trailer --parse docs: add explanation for its usefulness
  2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
                       ` (7 preceding siblings ...)
  2023-09-07 22:20     ` [PATCH v3 08/13] trailer --only-input: prefer "configuration variables" over "rules" Linus Arver via GitGitGadget
@ 2023-09-07 22:20     ` Linus Arver via GitGitGadget
  2023-09-08 21:57       ` Junio C Hamano
  2023-09-07 22:20     ` [PATCH v3 10/13] trailer --unfold help: prefer "reformat" over "join" Linus Arver via GitGitGadget
                       ` (3 subsequent siblings)
  12 siblings, 1 reply; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

For users who are skimming the docs to go straight to the individual
breakdown of each flag, it may not be clear why --parse is a convenience
alias (without them also looking at the other options that --parse turns
on). To save them the trouble of looking at the other options (and
computing what that would mean), describe a summary of the overall
effect.

Similarly update the area when we first mention --parse near the top of
the doc.

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 0eea937c30e..2d49445b1c3 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -31,7 +31,9 @@ the last two lines starting with "Signed-off-by" are trailers.
 
 This command reads commit messages from either the
 <file> arguments or the standard input if no <file> is specified.
-If `--parse` is specified, the output consists of the parsed trailers.
+If `--parse` is specified, the output consists of the parsed trailers
+coming from the input, without influencing them with any command line
+options or configuration variables.
 Otherwise, this command applies the arguments passed using the
 `--trailer` option, if any, to each input file. The result is emitted on the
 standard output.
@@ -158,7 +160,10 @@ OPTIONS
 
 --parse::
 	A convenience alias for `--only-trailers --only-input
-	--unfold`.
+	--unfold`. This makes it easier to only see the trailers coming from the
+	input without influencing them with any command line options or
+	configuration variables, while also making the output machine-friendly with
+	--unfold.
 
 --no-divider::
 	Do not treat `---` as the end of the commit message. Use this
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v3 10/13] trailer --unfold help: prefer "reformat" over "join"
  2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
                       ` (8 preceding siblings ...)
  2023-09-07 22:20     ` [PATCH v3 09/13] trailer --parse docs: add explanation for its usefulness Linus Arver via GitGitGadget
@ 2023-09-07 22:20     ` Linus Arver via GitGitGadget
  2023-09-07 22:20     ` [PATCH v3 11/13] trailer doc: emphasize the effect of configuration variables Linus Arver via GitGitGadget
                       ` (2 subsequent siblings)
  12 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

The phrase "join whitespace-continued values" requires some additional
context. For example, "whitespace" means newlines (not just space
characters), and "join" means to join only the multiple lines together
for a single trailer (and not that we are joining multiple trailers
together). That is, "join" means to convert

    token: This is a very long value, with spaces and
      newlines in it.

to

    token: This is a very long value, with spaces and newlines in it.

and does not mean to convert

    token: value1
    token: value2

to

    token: value1 value2.

Update the help text to resolve the above ambiguity. While we're add it,
update the docs to use similar language as the change in the help text.

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 4 ++--
 builtin/interpret-trailers.c             | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 2d49445b1c3..62ba2b1232e 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -155,8 +155,8 @@ OPTIONS
 	variables.
 
 --unfold::
-	Remove any whitespace-continuation in trailers, so that each
-	trailer appears on a line by itself with its full content.
+	If a trailer has a value that runs over multiple lines (aka "folded"),
+	reformat the value into a single line.
 
 --parse::
 	A convenience alias for `--only-trailers --only-input
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index f70c5df8d4b..832f86a770a 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -106,7 +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 trailer.* configuration variables")),
-		OPT_BOOL(0, "unfold", &opts.unfold, N_("join whitespace-continued values")),
+		OPT_BOOL(0, "unfold", &opts.unfold, N_("reformat multiline trailer values as single-line values")),
 		OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("alias for --only-trailers --only-input --unfold"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
 		OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v3 11/13] trailer doc: emphasize the effect of configuration variables
  2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
                       ` (9 preceding siblings ...)
  2023-09-07 22:20     ` [PATCH v3 10/13] trailer --unfold help: prefer "reformat" over "join" Linus Arver via GitGitGadget
@ 2023-09-07 22:20     ` Linus Arver via GitGitGadget
  2023-09-07 22:20     ` [PATCH v3 12/13] trailer doc: separator within key suppresses default separator Linus Arver via GitGitGadget
  2023-09-07 22:20     ` [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both Linus Arver via GitGitGadget
  12 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

The sentence does not mention the effect of configuration variables at
all, when they are actively used by default (unless --parse is
specified) to potentially add new trailers, without the user having to
always supply --trailer manually.

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 62ba2b1232e..a288ff111cb 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -34,9 +34,12 @@ This command reads commit messages from either the
 If `--parse` is specified, the output consists of the parsed trailers
 coming from the input, without influencing them with any command line
 options or configuration variables.
-Otherwise, this command applies the arguments passed using the
-`--trailer` option, if any, to each input file. The result is emitted on the
-standard output.
+
+Otherwise, this command applies `trailer.*` configuration variables
+(which could potentially add new trailers, as well as reposition them),
+as well as any command line arguments that can override configuration
+variables (such as `--trailer=...` which could also add new trailers),
+to each input file. The result is emitted on the standard output.
 
 This command can also operate on the output of linkgit:git-format-patch[1],
 which is more elaborate than a plain commit message. Namely, such output
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v3 12/13] trailer doc: separator within key suppresses default separator
  2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
                       ` (10 preceding siblings ...)
  2023-09-07 22:20     ` [PATCH v3 11/13] trailer doc: emphasize the effect of configuration variables Linus Arver via GitGitGadget
@ 2023-09-07 22:20     ` Linus Arver via GitGitGadget
  2023-09-07 22:20     ` [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both Linus Arver via GitGitGadget
  12 siblings, 0 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index a288ff111cb..25433e1a1ff 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -255,8 +255,8 @@ trailer.<token>.key::
 	but this can be changed using the `trailer.separators` config
 	variable.
 +
-If there is a separator, then the key will be used instead of both the
-<token> and the default separator when adding the trailer.
+If there is a separator in the key, then it overrides the default
+separator when adding the trailer.
 
 trailer.<token>.where::
 	This option takes the same values as the 'trailer.where'
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 52+ messages in thread
* [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both
  2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
                       ` (11 preceding siblings ...)
  2023-09-07 22:20     ` [PATCH v3 12/13] trailer doc: separator within key suppresses default separator Linus Arver via GitGitGadget
@ 2023-09-07 22:20     ` Linus Arver via GitGitGadget
  2023-09-19 22:59       ` Jonathan Tan
  2023-11-10  6:33       ` Teng Long
  12 siblings, 2 replies; 52+ messages in thread
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
  To: git; +Cc: Christian Couder, Linus Arver, Linus Arver

From: Linus Arver <linusa@google.com>

The `--trailer` option takes a "<token>=<value>" argument, for example

    --trailer "Acked-by=Bob"

And in this exampple it is understood that "Acked-by" is the <token>.
However, the user can use a shorter "ack" string by defining
configuration like

    git config trailer.ack.key "Acked-by"

However, in the docs we define the above configuration as

    trailer.<token>.key

so the <token> can mean either the longer "Acked-by" or the shorter
"ack".

Separate the two meanings of <token> into <key> and <keyAlias>, and
update the configuration syntax to say "trailer.<keyAlias>.key".

Signed-off-by: Linus Arver <linusa@google.com>
---
 Documentation/git-interpret-trailers.txt | 136 +++++++++++++----------
 builtin/interpret-trailers.c             |   2 +-
 2 files changed, 77 insertions(+), 61 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 25433e1a1ff..418265f044d 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -9,7 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git interpret-trailers' [--in-place] [--trim-empty]
-			[(--trailer <token>[(=|:)<value>])...]
+			[(--trailer (<key>|<keyAlias>)[(=|:)<value>])...]
 			[--parse] [<file>...]
 
 DESCRIPTION
@@ -53,22 +53,32 @@ are applied to each input and the way any existing trailer in
 the input is changed. They also make it possible to
 automatically add some trailers.
 
-By default, a '<token>=<value>' or '<token>:<value>' argument given
+By default, a '<key>=<value>' or '<key>:<value>' argument given
 using `--trailer` will be appended after the existing trailers only if
-the last trailer has a different (<token>, <value>) pair (or if there
-is no existing trailer). The <token> and <value> parts will be trimmed
+the last trailer has a different (<key>, <value>) pair (or if there
+is no existing trailer). The <key> and <value> parts will be trimmed
 to remove starting and trailing whitespace, and the resulting trimmed
-<token> and <value> will appear in the output like this:
+<key> and <value> will appear in the output like this:
 
 ------------------------------------------------
-token: value
+key: value
 ------------------------------------------------
 
-This means that the trimmed <token> and <value> will be separated by
-`': '` (one colon followed by one space). For convenience, the <token> can be a
-shortened string key (e.g., "sign") instead of the full string which should
-appear before the separator on the output (e.g., "Signed-off-by"). This can be
-configured using the 'trailer.<token>.key' configuration variable.
+This means that the trimmed <key> and <value> will be separated by
+`': '` (one colon followed by one space).
+
+For convenience, a <keyAlias> can be configured to make using `--trailer`
+shorter to type on the command line. This can be configured using the
+'trailer.<keyAlias>.key' configuration variable. The <keyAlias> must be a prefix
+of the full <key> string, although case sensitivity does not matter. For
+example, if you have
+
+------------------------------------------------
+trailer.sign.key "Signed-off-by: "
+------------------------------------------------
+
+in your configuration, you only need to specify `--trailer="sign: foo"`
+on the command line instead of `--trailer="Signed-off-by: foo"`.
 
 By default the new trailer will appear at the end of all the existing
 trailers. If there is no existing trailer, the new trailer will appear
@@ -85,14 +95,14 @@ non-whitespace lines before a line that starts with '---' (followed by a
 space or the end of the line).
 
 When reading trailers, there can be no whitespace before or inside the
-<token>, but any number of regular space and tab characters are allowed
-between the <token> and the separator. There can be whitespaces before,
+<key>, but any number of regular space and tab characters are allowed
+between the <key> and the separator. There can be whitespaces before,
 inside or after the <value>. The <value> may be split over multiple lines
 with each subsequent line starting with at least one whitespace, like
 the "folding" in RFC 822. Example:
 
 ------------------------------------------------
-token: This is a very long value, with spaces and
+key: This is a very long value, with spaces and
   newlines in it.
 ------------------------------------------------
 
@@ -109,8 +119,8 @@ OPTIONS
 	the whole trailer will be removed from the output.
 	This applies to existing trailers as well as new trailers.
 
---trailer <token>[(=|:)<value>]::
-	Specify a (<token>, <value>) pair that should be applied as a
+--trailer <key>[(=|:)<value>]::
+	Specify a (<key>, <value>) pair that should be applied as a
 	trailer to the inputs. See the description of this
 	command.
 
@@ -118,7 +128,7 @@ OPTIONS
 --no-where::
 	Specify where all new trailers will be added.  A setting
 	provided with '--where' overrides the `trailer.where` and any
-	applicable `trailer.<token>.where` configuration variables
+	applicable `trailer.<keyAlias>.where` configuration variables
 	and applies to all '--trailer' options until the next occurrence of
 	'--where' or '--no-where'. Upon encountering '--no-where', clear the
 	effect of any previous use of '--where', such that the relevant configuration
@@ -128,9 +138,9 @@ OPTIONS
 --if-exists <action>::
 --no-if-exists::
 	Specify what action will be performed when there is already at
-	least one trailer with the same <token> in the input.  A setting
+	least one trailer with the same <key> in the input.  A setting
 	provided with '--if-exists' overrides the `trailer.ifExists` and any
-	applicable `trailer.<token>.ifExists` configuration variables
+	applicable `trailer.<keyAlias>.ifExists` configuration variables
 	and applies to all '--trailer' options until the next occurrence of
 	'--if-exists' or '--no-if-exists'. Upon encountering '--no-if-exists, clear the
 	effect of any previous use of '--if-exists, such that the relevant configuration
@@ -140,9 +150,9 @@ OPTIONS
 --if-missing <action>::
 --no-if-missing::
 	Specify what action will be performed when there is no other
-	trailer with the same <token> in the input.  A setting
+	trailer with the same <key> in the input.  A setting
 	provided with '--if-missing' overrides the `trailer.ifMissing` and any
-	applicable `trailer.<token>.ifMissing` configuration variables
+	applicable `trailer.<keyAlias>.ifMissing` configuration variables
 	and applies to all '--trailer' options until the next occurrence of
 	'--if-missing' or '--no-if-missing'. Upon encountering '--no-if-missing,
 	clear the effect of any previous use of '--if-missing, such that the relevant
@@ -187,11 +197,11 @@ used when another separator is not specified in the config for this
 trailer.
 +
 For example, if the value for this option is "%=$", then only lines
-using the format '<token><sep><value>' with <sep> containing '%', '='
+using the format '<key><sep><value>' with <sep> containing '%', '='
 or '$' and then spaces will be considered trailers. And '%' will be
 the default separator used, so by default trailers will appear like:
-'<token>% <value>' (one percent sign and one space will appear between
-the token and the value).
+'<key>% <value>' (one percent sign and one space will appear between
+the key and the value).
 
 trailer.where::
 	This option tells where a new trailer will be added.
@@ -205,41 +215,41 @@ If it is `start`, then each new trailer will appear at the start,
 instead of the end, of the existing trailers.
 +
 If it is `after`, then each new trailer will appear just after the
-last trailer with the same <token>.
+last trailer with the same <key>.
 +
 If it is `before`, then each new trailer will appear just before the
-first trailer with the same <token>.
+first trailer with the same <key>.
 
 trailer.ifexists::
 	This option makes it possible to choose what action will be
 	performed when there is already at least one trailer with the
-	same <token> in the input.
+	same <key> in the input.
 +
 The valid values for this option are: `addIfDifferentNeighbor` (this
 is the default), `addIfDifferent`, `add`, `replace` or `doNothing`.
 +
 With `addIfDifferentNeighbor`, a new trailer will be added only if no
-trailer with the same (<token>, <value>) pair is above or below the line
+trailer with the same (<key>, <value>) pair is above or below the line
 where the new trailer will be added.
 +
 With `addIfDifferent`, a new trailer will be added only if no trailer
-with the same (<token>, <value>) pair is already in the input.
+with the same (<key>, <value>) pair is already in the input.
 +
 With `add`, a new trailer will be added, even if some trailers with
-the same (<token>, <value>) pair are already in the input.
+the same (<key>, <value>) pair are already in the input.
 +
-With `replace`, an existing trailer with the same <token> will be
+With `replace`, an existing trailer with the same <key> will be
 deleted and the new trailer will be added. The deleted trailer will be
-the closest one (with the same <token>) to the place where the new one
+the closest one (with the same <key>) to the place where the new one
 will be added.
 +
 With `doNothing`, nothing will be done; that is no new trailer will be
-added if there is already one with the same <token> in the input.
+added if there is already one with the same <key> in the input.
 
 trailer.ifmissing::
 	This option makes it possible to choose what action will be
 	performed when there is not yet any trailer with the same
-	<token> in the input.
+	<key> in the input.
 +
 The valid values for this option are: `add` (this is the default) and
 `doNothing`.
@@ -248,34 +258,40 @@ With `add`, a new trailer will be added.
 +
 With `doNothing`, nothing will be done.
 
-trailer.<token>.key::
-	This `key` will be used instead of <token> in the trailer. At
-	the end of this key, a separator can appear and then some
-	space characters. By default the only valid separator is ':',
-	but this can be changed using the `trailer.separators` config
-	variable.
+trailer.<keyAlias>.key::
+	Defines a <keyAlias> for the <key>. The <keyAlias> must be a
+	prefix (case does not matter) of the <key>. For example, in `git
+	config trailer.ack.key "Acked-by"` the "Acked-by" is the <key> and
+	the "ack" is the <keyAlias>. This configuration allows the shorter
+	`--trailer "ack:..."` invocation on the command line using the "ack"
+	<keyAlias> instead of the longer `--trailer "Acked-by:..."`.
++
+At the end of the <key>, a separator can appear and then some
+space characters. By default the only valid separator is ':',
+but this can be changed using the `trailer.separators` config
+variable.
 +
 If there is a separator in the key, then it overrides the default
 separator when adding the trailer.
 
-trailer.<token>.where::
+trailer.<keyAlias>.where::
 	This option takes the same values as the 'trailer.where'
 	configuration variable and it overrides what is specified by
-	that option for trailers with the specified <token>.
+	that option for trailers with the specified <keyAlias>.
 
-trailer.<token>.ifexists::
+trailer.<keyAlias>.ifexists::
 	This option takes the same values as the 'trailer.ifexists'
 	configuration variable and it overrides what is specified by
-	that option for trailers with the specified <token>.
+	that option for trailers with the specified <keyAlias>.
 
-trailer.<token>.ifmissing::
+trailer.<keyAlias>.ifmissing::
 	This option takes the same values as the 'trailer.ifmissing'
 	configuration variable and it overrides what is specified by
-	that option for trailers with the specified <token>.
+	that option for trailers with the specified <keyAlias>.
 
-trailer.<token>.command::
-	Deprecated in favor of 'trailer.<token>.cmd'.
-	This option behaves in the same way as 'trailer.<token>.cmd', except
+trailer.<keyAlias>.command::
+	Deprecated in favor of 'trailer.<keyAlias>.cmd'.
+	This option behaves in the same way as 'trailer.<keyAlias>.cmd', except
 	that it doesn't pass anything as argument to the specified command.
 	Instead the first occurrence of substring $ARG is replaced by the
 	<value> that would be passed as argument.
@@ -283,29 +299,29 @@ trailer.<token>.command::
 Note that $ARG in the user's command is
 only replaced once and that the original way of replacing $ARG is not safe.
 +
-When both 'trailer.<token>.cmd' and 'trailer.<token>.command' are given
-for the same <token>, 'trailer.<token>.cmd' is used and
-'trailer.<token>.command' is ignored.
+When both 'trailer.<keyAlias>.cmd' and 'trailer.<keyAlias>.command' are given
+for the same <keyAlias>, 'trailer.<keyAlias>.cmd' is used and
+'trailer.<keyAlias>.command' is ignored.
 
-trailer.<token>.cmd::
+trailer.<keyAlias>.cmd::
 	This option can be used to specify a shell command that will be called
-	once to automatically add a trailer with the specified <token>, and then
-	called each time a '--trailer <token>=<value>' argument is specified to
+	once to automatically add a trailer with the specified <keyAlias>, and then
+	called each time a '--trailer <keyAlias>=<value>' argument is specified to
 	modify the <value> of the trailer that this option would produce.
 +
 When the specified command is first called to add a trailer
-with the specified <token>, the behavior is as if a special
-'--trailer <token>=<value>' argument was added at the beginning
+with the specified <keyAlias>, the behavior is as if a special
+'--trailer <keyAlias>=<value>' argument was added at the beginning
 of the "git interpret-trailers" command, where <value>
 is taken to be the standard output of the command with any
 leading and trailing whitespace trimmed off.
 +
-If some '--trailer <token>=<value>' arguments are also passed
+If some '--trailer <keyAlias>=<value>' arguments are also passed
 on the command line, the command is called again once for each
-of these arguments with the same <token>. And the <value> part
+of these arguments with the same <keyAlias>. And the <value> part
 of these arguments, if any, will be passed to the command as its
 first argument. This way the command can produce a <value> computed
-from the <value> passed in the '--trailer <token>=<value>' argument.
+from the <value> passed in the '--trailer <keyAlias>=<value>' argument.
 
 EXAMPLES
 --------
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 832f86a770a..d2d78fd961f 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -14,7 +14,7 @@
 
 static const char * const git_interpret_trailers_usage[] = {
 	N_("git interpret-trailers [--in-place] [--trim-empty]\n"
-	   "                       [(--trailer <token>[(=|:)<value>])...]\n"
+	   "                       [(--trailer (<key>|<keyAlias>)[(=|:)<value>])...]\n"
 	   "                       [--parse] [<file>...]"),
 	NULL
 };
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 52+ messages in thread
* Re: [PATCH v3 03/13] trailer: add tests to check defaulting behavior with --no-* flags
  2023-09-07 22:19     ` [PATCH v3 03/13] trailer: add tests to check defaulting behavior with --no-* flags Linus Arver via GitGitGadget
@ 2023-09-08 21:52       ` Junio C Hamano
  0 siblings, 0 replies; 52+ messages in thread
From: Junio C Hamano @ 2023-09-08 21:52 UTC (permalink / raw)
  To: Linus Arver via GitGitGadget; +Cc: git, Christian Couder, Linus Arver

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Linus Arver <linusa@google.com>
>
> While the "--no-where" flag is tested, the "--no-if-exists" and
> "--no-if-missing" flags are not, so add tests for them. But also add
> tests for all "--no-*" flags to check their effects, both when (1) there
> are relevant configuration variables set, and (2) they are not set.
>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
>  Documentation/git-interpret-trailers.txt |  14 ++-
>  t/t7513-interpret-trailers.sh            | 130 +++++++++++++++++++++++
>  2 files changed, 140 insertions(+), 4 deletions(-)

The commentary before each test_expect_success makes it quite a
pleasant read.

> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> index ed0fc04bd95..832aff06167 100755
> --- a/t/t7513-interpret-trailers.sh
> +++ b/t/t7513-interpret-trailers.sh
> @@ -812,6 +812,53 @@ test_expect_success 'using "--where after" with "--no-where"' '
>  	test_cmp expected actual
>  '
>  
> +# Check whether using "--no-where" clears out only the "--where after", such
> +# that we still use the configuration in trailer.where (which is different from
> +# the hardcoded default (in WHERE_END) assuming the absence of .gitconfig).
> +# Here, the "start" setting of trailer.where is respected, so the new "Acked-by"
> +# and "Bug" trailers are placed at the beginning, and not at the end which is
> +# the harcoded default.
> +test_expect_success 'using "--where after" with "--no-where" defaults to configuration' '
> +	test_config trailer.ack.key "Acked-by= " &&
> +	test_config trailer.bug.key "Bug #" &&
> +	test_config trailer.separators ":=#" &&
> +	test_config trailer.where "start" &&
> +	cat complex_message_body >expected &&
> +	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
> +		Bug #42
> +		Acked-by= Peff
> +		Fixes: Z
> +		Acked-by= Z
> +		Reviewed-by: Z
> +		Signed-off-by: Z
> +	EOF
> +	git interpret-trailers --where after --no-where --trailer "ack: Peff" \
> +		--trailer "bug: 42" complex_message >actual &&
> +	test_cmp expected actual
> +'
> +
> +# The "--where after" will only get respected for the trailer that came
> +# immediately after it. For the next trailer (Bug #42), we default to using the
> +# hardcoded WHERE_END because we don't have any "trailer.where" or
> +# "trailer.bug.where" configured.
> +test_expect_success 'using "--no-where" defaults to harcoded default if nothing configured' '
> +	test_config trailer.ack.key "Acked-by= " &&
> +	test_config trailer.bug.key "Bug #" &&
> +	test_config trailer.separators ":=#" &&
> +	cat complex_message_body >expected &&
> +	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
> +		Fixes: Z
> +		Acked-by= Z
> +		Acked-by= Peff
> +		Reviewed-by: Z
> +		Signed-off-by: Z
> +		Bug #42
> +	EOF
> +	git interpret-trailers --where after --trailer "ack: Peff" --no-where \
> +		--trailer "bug: 42" complex_message >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'using "where = after"' '
>  	test_config trailer.ack.key "Acked-by= " &&
>  	test_config trailer.ack.where "after" &&
> @@ -1176,6 +1223,56 @@ test_expect_success 'overriding configuration with "--if-exists replace"' '
>  	test_cmp expected actual
>  '
>  
> +# "trailer.ifexists" is set to "doNothing", so using "--no-if-exists" defaults
> +# to this "doNothing" behavior. So the "Fixes: 53" trailer does not get added.
> +test_expect_success 'using "--if-exists replace" with "--no-if-exists" defaults to configuration' '
> +	test_config trailer.ifexists "doNothing" &&
> +	cat complex_message_body >expected &&
> +	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
> +		Fixes: Z
> +		Acked-by: Z
> +		Reviewed-by: Z
> +		Signed-off-by: Z
> +	EOF
> +	git interpret-trailers --if-exists replace --no-if-exists --trailer "Fixes: 53" \
> +		<complex_message >actual &&
> +	test_cmp expected actual
> +'
> +
> +# No "ifexists" configuration is set, so using "--no-if-exists" makes it default
> +# to addIfDifferentNeighbor. Because we do have a different neighbor "Fixes: 53"
> +# (because it got added by overriding with "--if-exists replace" earlier in the
> +# arguments list), we add "Signed-off-by: addme".
> +test_expect_success 'using "--no-if-exists" defaults to hardcoded default if nothing configured' '
> +	cat complex_message_body >expected &&
> +	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
> +		Acked-by: Z
> +		Reviewed-by: Z
> +		Signed-off-by: Z
> +		Fixes: 53
> +		Signed-off-by: addme
> +	EOF
> +	git interpret-trailers --if-exists replace --trailer "Fixes: 53" --no-if-exists \
> +		--trailer "Signed-off-by: addme" <complex_message >actual &&
> +	test_cmp expected actual
> +'
> +
> +# The second "Fixes: 53" trailer is discarded, because the "--no-if-exists" here
> +# makes us default to addIfDifferentNeighbor, and we already added the "Fixes:
> +# 53" trailer earlier in the argument list.
> +test_expect_success 'using "--no-if-exists" defaults to hardcoded default if nothing configured (no addition)' '
> +	cat complex_message_body >expected &&
> +	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
> +		Acked-by: Z
> +		Reviewed-by: Z
> +		Signed-off-by: Z
> +		Fixes: 53
> +	EOF
> +	git interpret-trailers --if-exists replace --trailer "Fixes: 53" --no-if-exists \
> +		--trailer "Fixes: 53" <complex_message >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'using "ifExists = replace"' '
>  	test_config trailer.fix.key "Fixes: " &&
>  	test_config trailer.fix.ifExists "replace" &&
> @@ -1425,6 +1522,39 @@ test_expect_success 'using "ifMissing = doNothing"' '
>  	test_cmp expected actual
>  '
>  
> +# Ignore the "IgnoredTrailer" because of "--if-missing doNothing", but also
> +# ignore the "StillIgnoredTrailer" because we set "trailer.ifMissing" to
> +# "doNothing" in configuration.
> +test_expect_success 'using "--no-if-missing" defaults to configuration' '
> +	test_config trailer.ifMissing "doNothing" &&
> +	cat complex_message_body >expected &&
> +	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
> +			Fixes: Z
> +			Acked-by: Z
> +			Reviewed-by: Z
> +			Signed-off-by: Z
> +	EOF
> +	git interpret-trailers --if-missing doNothing --trailer "IgnoredTrailer: ignoreme" --no-if-missing \
> +			--trailer "StillIgnoredTrailer: ignoreme" <complex_message >actual &&
> +	test_cmp expected actual
> +'
> +
> +# Add the "AddedTrailer" because the "--no-if-missing" clears the "--if-missing
> +# doNothing" from earlier in the argument list.
> +test_expect_success 'using "--no-if-missing" defaults to hardcoded default if nothing configured' '
> +	cat complex_message_body >expected &&
> +	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
> +			Fixes: Z
> +			Acked-by: Z
> +			Reviewed-by: Z
> +			Signed-off-by: Z
> +			AddedTrailer: addme
> +	EOF
> +	git interpret-trailers --if-missing doNothing --trailer "IgnoredTrailer: ignoreme" --no-if-missing \
> +			--trailer "AddedTrailer: addme" <complex_message >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'default "where" is now "after"' '
>  	git config trailer.where "after" &&
>  	test_config trailer.ack.ifExists "add" &&

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v3 06/13] trailer --no-divider help: describe usual "---" meaning
  2023-09-07 22:20     ` [PATCH v3 06/13] trailer --no-divider help: describe usual "---" meaning Linus Arver via GitGitGadget
@ 2023-09-08 21:53       ` Junio C Hamano
  0 siblings, 0 replies; 52+ messages in thread
From: Junio C Hamano @ 2023-09-08 21:53 UTC (permalink / raw)
  To: Linus Arver via GitGitGadget; +Cc: git, Christian Couder, Linus Arver

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Linus Arver <linusa@google.com>
>
> It's unclear what treating something "specially" means.

Good.

>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
>  builtin/interpret-trailers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index cf4f703c4e2..a7623dbfb2e 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -109,7 +109,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL(0, "unfold", &opts.unfold, N_("join whitespace-continued values")),
>  		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")),
> +		OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
>  		OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
>  				N_("trailer(s) to add"), option_parse_trailer),
>  		OPT_END()

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v3 09/13] trailer --parse docs: add explanation for its usefulness
  2023-09-07 22:20     ` [PATCH v3 09/13] trailer --parse docs: add explanation for its usefulness Linus Arver via GitGitGadget
@ 2023-09-08 21:57       ` Junio C Hamano
  0 siblings, 0 replies; 52+ messages in thread
From: Junio C Hamano @ 2023-09-08 21:57 UTC (permalink / raw)
  To: Linus Arver via GitGitGadget; +Cc: git, Christian Couder, Linus Arver

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  --parse::
>  	A convenience alias for `--only-trailers --only-input
> -	--unfold`.
> +	--unfold`. This makes it easier to only see the trailers coming from the
> +	input without influencing them with any command line options or
> +	configuration variables, while also making the output machine-friendly with
> +	--unfold.

Nicely explained.  Without a concise explanation like this, it is
hard to see what the value of the "convenience" alias actually is.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v3 05/13] trailer: trailer location is a place, not an action
  2023-09-07 22:20     ` [PATCH v3 05/13] trailer: trailer location is a place, not an action Linus Arver via GitGitGadget
@ 2023-09-19 22:13       ` Jonathan Tan
  0 siblings, 0 replies; 52+ messages in thread
From: Jonathan Tan @ 2023-09-19 22:13 UTC (permalink / raw)
  To: Linus Arver via GitGitGadget
  Cc: Jonathan Tan, git, Christian Couder, Linus Arver

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
> 
> Fix the help text to say "placement" instead of "action" because the
> values are placements, not actions.
> 
> While we're at it, tweak the documentation to say "placements" instead
> of "values", similar to how the existing language for "--if-exists" uses
> the word "action" to describe both the syntax (with the phrase
> "--if-exists <action>") and the possible values (with the phrase
> "possible actions").
> 
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
>  Documentation/git-interpret-trailers.txt | 2 +-
>  builtin/interpret-trailers.c             | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
> index 72f5bdb652f..b5284c3d33f 100644
> --- a/Documentation/git-interpret-trailers.txt
> +++ b/Documentation/git-interpret-trailers.txt
> @@ -117,7 +117,7 @@ OPTIONS
>  	and applies to all '--trailer' options until the next occurrence of
>  	'--where' or '--no-where'. Upon encountering '--no-where', clear the
>  	effect of any previous use of '--where', such that the relevant configuration
> -	variables are no longer overridden. Possible values are `after`,
> +	variables are no longer overridden. Possible placements are `after`,
>  	`before`, `end` or `start`.
>  
>  --if-exists <action>::

Not shown in the diff as printed in this email, but this option is
indeed documented with "<placement>". Up to and including this patch
makes sense.
 

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v3 07/13] trailer --parse help: expose aliased options
  2023-09-07 22:20     ` [PATCH v3 07/13] trailer --parse help: expose aliased options Linus Arver via GitGitGadget
@ 2023-09-19 22:16       ` Jonathan Tan
  0 siblings, 0 replies; 52+ messages in thread
From: Jonathan Tan @ 2023-09-19 22:16 UTC (permalink / raw)
  To: Linus Arver via GitGitGadget
  Cc: Jonathan Tan, git, Christian Couder, Linus Arver

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index a7623dbfb2e..5f3e1a38eee 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -107,7 +107,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_CALLBACK_F(0, "parse", &opts, NULL, N_("set parsing options"),
> +		OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("alias for --only-trailers --only-input --unfold"),
>  			PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
>  		OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
>  		OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),

Indeed, parse_opt_parse() sets the 3 options described in the commit
message. Looks good.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both
  2023-09-07 22:20     ` [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both Linus Arver via GitGitGadget
@ 2023-09-19 22:59       ` Jonathan Tan
  2023-09-20  6:48         ` Linus Arver
  2023-09-20 15:01         ` Junio C Hamano
  2023-11-10  6:33       ` Teng Long
  1 sibling, 2 replies; 52+ messages in thread
From: Jonathan Tan @ 2023-09-19 22:59 UTC (permalink / raw)
  To: Linus Arver via GitGitGadget
  Cc: Jonathan Tan, git, Christian Couder, Linus Arver

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -248,34 +258,40 @@ With `add`, a new trailer will be added.
>  +
>  With `doNothing`, nothing will be done.
>  
> -trailer.<token>.key::
> -	This `key` will be used instead of <token> in the trailer. At
> -	the end of this key, a separator can appear and then some
> -	space characters. By default the only valid separator is ':',
> -	but this can be changed using the `trailer.separators` config
> -	variable.
> +trailer.<keyAlias>.key::
> +	Defines a <keyAlias> for the <key>. The <keyAlias> must be a
> +	prefix (case does not matter) of the <key>. For example, in `git
> +	config trailer.ack.key "Acked-by"` the "Acked-by" is the <key> and
> +	the "ack" is the <keyAlias>. This configuration allows the shorter
> +	`--trailer "ack:..."` invocation on the command line using the "ack"
> +	<keyAlias> instead of the longer `--trailer "Acked-by:..."`.
> ++
> +At the end of the <key>, a separator can appear and then some
> +space characters. By default the only valid separator is ':',
> +but this can be changed using the `trailer.separators` config
> +variable.

I think all the other patches will be a great help to the user, but I'm
on the fence about this one. Someone who knows these trailer components
by their old names might be confused upon seeing tne new ones, so I'm
inclined to minimize such changes. I do think that the new names make
more sense, though.

The documentation doesn't seem to say what happens when trailer.ack.cmd
and trailer.Acked-by.cmd (replace "cmd" with whatever) are defined, but
that was true previously too (and knowing this does not really enable
the user to be able to do something they previously couldn't, so this
is fine).

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both
  2023-09-19 22:59       ` Jonathan Tan
@ 2023-09-20  6:48         ` Linus Arver
  2023-09-20 15:01         ` Junio C Hamano
  1 sibling, 0 replies; 52+ messages in thread
From: Linus Arver @ 2023-09-20  6:48 UTC (permalink / raw)
  To: Jonathan Tan, Linus Arver via GitGitGadget
  Cc: Jonathan Tan, git, Christian Couder

Jonathan Tan <jonathantanmy@google.com> writes:

> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> @@ -248,34 +258,40 @@ With `add`, a new trailer will be added.
>>  +
>>  With `doNothing`, nothing will be done.
>>  
>> -trailer.<token>.key::
>> -	This `key` will be used instead of <token> in the trailer. At
>> -	the end of this key, a separator can appear and then some
>> -	space characters. By default the only valid separator is ':',
>> -	but this can be changed using the `trailer.separators` config
>> -	variable.
>> +trailer.<keyAlias>.key::
>> +	Defines a <keyAlias> for the <key>. The <keyAlias> must be a
>> +	prefix (case does not matter) of the <key>. For example, in `git
>> +	config trailer.ack.key "Acked-by"` the "Acked-by" is the <key> and
>> +	the "ack" is the <keyAlias>. This configuration allows the shorter
>> +	`--trailer "ack:..."` invocation on the command line using the "ack"
>> +	<keyAlias> instead of the longer `--trailer "Acked-by:..."`.
>> ++
>> +At the end of the <key>, a separator can appear and then some
>> +space characters. By default the only valid separator is ':',
>> +but this can be changed using the `trailer.separators` config
>> +variable.
>
> I think all the other patches will be a great help to the user, but I'm
> on the fence about this one.

Ack. I'm OK with dropping this patch (I assume I don't need to reroll
this series for that?).

> Someone who knows these trailer components
> by their old names might be confused upon seeing tne new ones, so I'm
> inclined to minimize such changes.

True, I had some inclination to do the same kind of change but for the
trailer.c code also (that code uses the "token" language also,
furthering the ambiguity, sadly) but wanted to just do the user-facing
part first because I thought the users shouldn't need to know about the
internals anyway.

If I did revise this patch to include the same changes in trailer.c and
not just the docs, would you be more willing to support the (new) patch?
I'm mainly curious about what will make you more comfortable to accept
this change.

> I do think that the new names make
> more sense, though.

Thanks!

> The documentation doesn't seem to say what happens when trailer.ack.cmd
> and trailer.Acked-by.cmd (replace "cmd" with whatever) are defined, but
> that was true previously too (and knowing this does not really enable
> the user to be able to do something they previously couldn't, so this
> is fine).

Ah interesting point. I'll try to remember this when I revisit this
patch again in the (near?) future.

^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both
  2023-09-19 22:59       ` Jonathan Tan
  2023-09-20  6:48         ` Linus Arver
@ 2023-09-20 15:01         ` Junio C Hamano
  2023-09-22 18:26           ` Linus Arver
  1 sibling, 1 reply; 52+ messages in thread
From: Junio C Hamano @ 2023-09-20 15:01 UTC (permalink / raw)
  To: Jonathan Tan
  Cc: Linus Arver via GitGitGadget, git, Christian Couder, Linus Arver

Jonathan Tan <jonathantanmy@google.com> writes:

> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> @@ -248,34 +258,40 @@ With `add`, a new trailer will be added.
>>  +
>>  With `doNothing`, nothing will be done.
>>  
>> -trailer.<token>.key::
>> -	This `key` will be used instead of <token> in the trailer. At
>> -	the end of this key, a separator can appear and then some
>> -	space characters. By default the only valid separator is ':',
>> -	but this can be changed using the `trailer.separators` config
>> -	variable.
>> +trailer.<keyAlias>.key::
>> +	Defines a <keyAlias> for the <key>. The <keyAlias> must be a
>> +	prefix (case does not matter) of the <key>. For example, in `git
>> +	config trailer.ack.key "Acked-by"` the "Acked-by" is the <key> and
>> +	the "ack" is the <keyAlias>. This configuration allows the shorter
>> +	`--trailer "ack:..."` invocation on the command line using the "ack"
>> +	<keyAlias> instead of the longer `--trailer "Acked-by:..."`.
>> ++
>> +At the end of the <key>, a separator can appear and then some
>> +space characters. By default the only valid separator is ':',
>> +but this can be changed using the `trailer.separators` config
>> +variable.
>
> I think all the other patches will be a great help to the user, but I'm
> on the fence about this one. Someone who knows these trailer components
> by their old names might be confused upon seeing tne new ones, so I'm
> inclined to minimize such changes. I do think that the new names make
> more sense, though.

As long as the new names describe the world order better than the
old description, I do not mind rephrasing the documentation, and you
seem to find the more descriptive <keyAlias> easier to understand
compared to the non-descriptive <token>.  Adding a concrete example
(ack vs acked-by) is also a good change.


^ permalink raw reply	[flat|nested] 52+ messages in thread
* Re: [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both
  2023-09-20 15:01         ` Junio C Hamano
@ 2023-09-22 18:26           ` Linus Arver
  0 siblings, 0 replies; 52+ messages in thread
From: Linus Arver @ 2023-09-22 18:26 UTC (permalink / raw)
  To: Junio C Hamano, Jonathan Tan
  Cc: Linus Arver via GitGitGadget, git, Christian Couder

Junio C Hamano <gitster@pobox.com> writes:

> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>> @@ -248,34 +258,40 @@ With `add`, a new trailer will be added.
>>>  +
>>>  With `doNothing`, nothing will be done.
>>>  
>>> -trailer.<token>.key::
>>> -	This `key` will be used instead of <token> in the trailer. At
>>> -	the end of this key, a separator can appear and then some
>>> -	space characters. By default the only valid separator is ':',
>>> -	but this can be changed using the `trailer.separators` config
>>> -	variable.
>>> +trailer.<keyAlias>.key::
>>> +	Defines a <keyAlias> for the <key>. The <keyAlias> must be a
>>> +	prefix (case does not matter) of the <key>. For example, in `git
>>> +	config trailer.ack.key "Acked-by"` the "Acked-by" is the <key> and
>>> +	the "ack" is the <keyAlias>. This configuration allows the shorter
>>> +	`--trailer "ack:..."` invocation on the command line using the "ack"
>>> +	<keyAlias> instead of the longer `--trailer "Acked-by:..."`.
>>> ++
>>> +At the end of the <key>, a separator can appear and then some
>>> +space characters. By default the only valid separator is ':',
>>> +but this can be changed using the `trailer.separators` config
>>> +variable.
>>
>> I think all the other patches will be a great help to the user, but I'm
>> on the fence about this one. Someone who knows these trailer components
>> by their old names might be confused upon seeing tne new ones, so I'm
>> inclined to minimize such changes. I do think that the new names make
>> more sense, though.
>
> As long as the new names describe the world order better than the
> old description, I do not mind rephrasing the documentation, and you
> seem to find the more descriptive <keyAlias> easier to understand
> compared to the non-descriptive <token>.  Adding a concrete example
> (ack vs acked-by) is also a good change.

SGTM (i.e., I am OK to keep this patch as is). So I will not re-roll.

In the future I will be cleaning up more of the trailer system to make
use of the disinction between the key vs keyAlias where appropriate.

Thanks!

^ permalink raw reply	[flat|nested] 52+ messages in thread
* [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both
  2023-09-07 22:20     ` [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both Linus Arver via GitGitGadget
  2023-09-19 22:59       ` Jonathan Tan
@ 2023-11-10  6:33       ` Teng Long
  1 sibling, 0 replies; 52+ messages in thread
From: Teng Long @ 2023-11-10  6:33 UTC (permalink / raw)
  To: gitgitgadget; +Cc: chriscool, git, linusa

Linus Arver <linusa@google.com> writes:

> diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
> index 25433e1a1ff..418265f044d 100644
> --- a/Documentation/git-interpret-trailers.txt
> +++ b/Documentation/git-interpret-trailers.txt
> @@ -9,7 +9,7 @@ SYNOPSIS
>  --------
>  [verse]
>  'git interpret-trailers' [--in-place] [--trim-empty]
> -			[(--trailer <token>[(=|:)<value>])...]
> +			[(--trailer (<key>|<keyAlias>)[(=|:)<value>])...]

Maybe use 'key-alias' instead of 'keyAlias'? After checking the naming in some
other documents, it seems that they are basically in the former format, not
camel case format.

Thanks.

^ permalink raw reply	[flat|nested] 52+ messages in thread
end of thread, other threads:[~2023-11-10  6:33 UTC | newest]

Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-05  4:45 [PATCH 0/5] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
2023-08-05  4:45 ` [PATCH 1/5] trailer tests: make test cases self-contained Linus Arver via GitGitGadget
2023-08-07  5:50   ` Linus Arver
2023-08-05  4:45 ` [PATCH 2/5] trailer test description: this tests --where=after, not --where=before Linus Arver via GitGitGadget
2023-08-05  4:45 ` [PATCH 3/5] trailer: add tests to check defaulting behavior with --no-* flags Linus Arver via GitGitGadget
2023-08-07  1:13   ` Junio C Hamano
2023-08-07  5:28     ` Linus Arver
2023-08-07  5:37       ` Linus Arver
2023-08-07  6:35       ` Linus Arver
2023-08-07 15:53         ` Junio C Hamano
2023-08-05  4:45 ` [PATCH 4/5] trailer: trailer location is a place, not an action Linus Arver via GitGitGadget
2023-08-05  4:45 ` [PATCH 5/5] trailer --no-divider help: describe usual "---" meaning Linus Arver via GitGitGadget
2023-08-10 21:17 ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Linus Arver via GitGitGadget
2023-08-10 21:17   ` [PATCH v2 01/13] trailer tests: make test cases self-contained Linus Arver via GitGitGadget
2023-08-10 21:17   ` [PATCH v2 02/13] trailer test description: this tests --where=after, not --where=before Linus Arver via GitGitGadget
2023-08-10 21:17   ` [PATCH v2 03/13] trailer: add tests to check defaulting behavior with --no-* flags Linus Arver via GitGitGadget
2023-08-10 21:17   ` [PATCH v2 04/13] trailer doc: narrow down scope of --where and related flags Linus Arver via GitGitGadget
2023-08-10 21:17   ` [PATCH v2 05/13] trailer: trailer location is a place, not an action Linus Arver via GitGitGadget
2023-08-10 21:17   ` [PATCH v2 06/13] trailer --no-divider help: describe usual "---" meaning Linus Arver via GitGitGadget
2023-08-10 21:17   ` [PATCH v2 07/13] trailer --parse help: expose aliased options Linus Arver via GitGitGadget
2023-08-10 21:17   ` [PATCH v2 08/13] trailer --only-input: prefer "configuration variables" over "rules" Linus Arver via GitGitGadget
2023-08-10 21:17   ` [PATCH v2 09/13] trailer --parse docs: add explanation for its usefulness Linus Arver via GitGitGadget
2023-08-10 21:18   ` [PATCH v2 10/13] trailer --unfold help: prefer "reformat" over "join" Linus Arver via GitGitGadget
2023-08-10 21:18   ` [PATCH v2 11/13] trailer doc: emphasize the effect of configuration variables Linus Arver via GitGitGadget
2023-08-10 21:18   ` [PATCH v2 12/13] trailer doc: separator within key suppresses default separator Linus Arver via GitGitGadget
2023-08-10 21:18   ` [PATCH v2 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both Linus Arver via GitGitGadget
2023-08-11  1:41   ` [PATCH v2 00/13] Fixes to trailer test script, help text, and documentation Junio C Hamano
2023-08-11 17:38     ` Linus Arver
2023-09-07 22:19   ` [PATCH v3 " Linus Arver via GitGitGadget
2023-09-07 22:19     ` [PATCH v3 01/13] trailer tests: make test cases self-contained Linus Arver via GitGitGadget
2023-09-07 22:19     ` [PATCH v3 02/13] trailer test description: this tests --where=after, not --where=before Linus Arver via GitGitGadget
2023-09-07 22:19     ` [PATCH v3 03/13] trailer: add tests to check defaulting behavior with --no-* flags Linus Arver via GitGitGadget
2023-09-08 21:52       ` Junio C Hamano
2023-09-07 22:20     ` [PATCH v3 04/13] trailer doc: narrow down scope of --where and related flags Linus Arver via GitGitGadget
2023-09-07 22:20     ` [PATCH v3 05/13] trailer: trailer location is a place, not an action Linus Arver via GitGitGadget
2023-09-19 22:13       ` Jonathan Tan
2023-09-07 22:20     ` [PATCH v3 06/13] trailer --no-divider help: describe usual "---" meaning Linus Arver via GitGitGadget
2023-09-08 21:53       ` Junio C Hamano
2023-09-07 22:20     ` [PATCH v3 07/13] trailer --parse help: expose aliased options Linus Arver via GitGitGadget
2023-09-19 22:16       ` Jonathan Tan
2023-09-07 22:20     ` [PATCH v3 08/13] trailer --only-input: prefer "configuration variables" over "rules" Linus Arver via GitGitGadget
2023-09-07 22:20     ` [PATCH v3 09/13] trailer --parse docs: add explanation for its usefulness Linus Arver via GitGitGadget
2023-09-08 21:57       ` Junio C Hamano
2023-09-07 22:20     ` [PATCH v3 10/13] trailer --unfold help: prefer "reformat" over "join" Linus Arver via GitGitGadget
2023-09-07 22:20     ` [PATCH v3 11/13] trailer doc: emphasize the effect of configuration variables Linus Arver via GitGitGadget
2023-09-07 22:20     ` [PATCH v3 12/13] trailer doc: separator within key suppresses default separator Linus Arver via GitGitGadget
2023-09-07 22:20     ` [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both Linus Arver via GitGitGadget
2023-09-19 22:59       ` Jonathan Tan
2023-09-20  6:48         ` Linus Arver
2023-09-20 15:01         ` Junio C Hamano
2023-09-22 18:26           ` Linus Arver
2023-11-10  6:33       ` Teng Long

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).