* [PATCH v3 01/13] trailer tests: make test cases self-contained
From: Linus Arver via GitGitGadget @ 2023-09-07 22:19 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Linus Arver, Linus Arver
In-Reply-To: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
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
* [PATCH v3 04/13] trailer doc: narrow down scope of --where and related flags
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Linus Arver, Linus Arver
In-Reply-To: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
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
* [PATCH v3 06/13] trailer --no-divider help: describe usual "---" meaning
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Linus Arver, Linus Arver
In-Reply-To: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
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
* [PATCH v3 05/13] trailer: trailer location is a place, not an action
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Linus Arver, Linus Arver
In-Reply-To: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
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
* [PATCH v3 08/13] trailer --only-input: prefer "configuration variables" over "rules"
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Linus Arver, Linus Arver
In-Reply-To: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
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
* [PATCH v3 10/13] trailer --unfold help: prefer "reformat" over "join"
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Linus Arver, Linus Arver
In-Reply-To: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
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
* [PATCH v3 09/13] trailer --parse docs: add explanation for its usefulness
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Linus Arver, Linus Arver
In-Reply-To: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
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
* [PATCH v3 07/13] trailer --parse help: expose aliased options
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Linus Arver, Linus Arver
In-Reply-To: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
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
* [PATCH v3 12/13] trailer doc: separator within key suppresses default separator
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Linus Arver, Linus Arver
In-Reply-To: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
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
* [PATCH v3 11/13] trailer doc: emphasize the effect of configuration variables
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Linus Arver, Linus Arver
In-Reply-To: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
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
* [PATCH v3 13/13] trailer doc: <token> is a <key> or <keyAlias>, not both
From: Linus Arver via GitGitGadget @ 2023-09-07 22:20 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Linus Arver, Linus Arver
In-Reply-To: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
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
* Re: [PATCH v2] diff-lib: Fix check_removed when fsmonitor is on
From: Josip Sokcevic @ 2023-09-07 23:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: jonathantanmy, git, git
In-Reply-To: <xmqqa5txluvz.fsf@gitster.g>
On Thu, Sep 7, 2023 at 11:07 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Josip Sokcevic <sokcevic@google.com> writes:
>
> > diff --git a/diff-lib.c b/diff-lib.c
> > index d8aa777a73..664613bb1b 100644
> > --- a/diff-lib.c
> > +++ b/diff-lib.c
> > @@ -39,11 +39,22 @@
> > static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)
> > {
> > assert(is_fsmonitor_refreshed(istate));
>
> Not a problem this patch introduces, but doesn't this call path
>
> diff_cache()
> -> unpack_trees()
> -> oneway_diff()
> -> do_oneway_diff()
> -> show_new_file(), show_modified()
> -> get_stat_data()
> -> check_removed()
>
> violate the assertion? If so, perhaps we should rewrite it into a
> more explicit "if (...) BUG(...)" that is not compiled away.
True, I will update it.
>
> > - if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {
> > - if (!is_missing_file_error(errno))
> > - return -1;
> > - return 1;
> > + if (ce->ce_flags & CE_FSMONITOR_VALID) {
> > + /*
> > + * Both check_removed() and its callers expect lstat() to have
> > + * happened and, in particular, the st_mode field to be set.
> > + * Simulate this with the contents of ce.
> > + */
> > + memset(st, 0, sizeof(*st));
>
> It is true that the original, when CE_FSMONITOR_VALID bit is set,
> bypasses lstat() altogether and leaves the contents of st completely
> uninitialized, but this is still way too insufficient, isn't it?
>
> There are three call sites of the check_removed() function.
>
> * The first one in run_diff_files() only cares about st.st_mode and
> other members of the structure are not looked at. This makes
> readers wonder if the "st" parameter to check_removed() should
> become "mode_t *st_mode" to clarify this point, but the primary
> thing I want to say is that this caller will not mind if we leave
> other members of st bogus (like 0-bit filled) as long as the mode
> is set correctly.
>
> * The second one in run_diff_files() passes the resulting &st to
> match_stat_with_submodule(), which in turn passes it to
> ie_match_stat(), which cares about "struct stat" members that are
> used for quick change detection, like owner, group, mtime.
> Giving it a bogus st will most likely cause it to report a
> change.
>
> * The third one is in get_stat_data(). This also uses the &st to
> call match_stat_with_submodule(), so it is still totally broken
> to give it a bogus st, the same way as the second caller above.
>
> > + st->st_mode = ce->ce_mode;
>
> Does this work correctly when the cache entry points at a gitlink,
> which uses 0160000 that is not a valid st_mode? I think you'd want
> to use a reverse function of create_ce_mode().
I realized this too but after I sent the patch. I don't think we have
a good way to reverse it, so the best we can do is to guess it. But I
don't think we should do that. Instead, should we just zero the struct
and add a TODO? Alternative could be to use a double pointer for stat
and do more checks in call sites, but I'm not familiar with the
codebase to how that branching would need to look like.
Any preference?
--
Josip Sokcevic
^ permalink raw reply
* Re: [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size`
From: Junio C Hamano @ 2023-09-07 23:42 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King, Jonathan Tan
In-Reply-To: <7e4e42e1aacf2111f04a933c0725a8c81769f8d6.1694123506.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> This all works, but can be costly from an I/O-perspective when a
> repository has either (a) many unreachable objects, (b) prunes objects
> relatively infrequently/never, or (c) both.
I can certainly understand (a). If we need to write a lot of
objects into the craft pack, among which many of them are already in
the previous generation of craft pack, we would be copying bits from
the old to the new craft pack, and having to do so for many objects
would involve expensive I/O. But not (b). Whether we prune objects
infrequently or we do so very often, as long as the number and
on-disk size of objects that has to go into craft packs are the
same, wouldn't the cost of I/O pretty much the same? IOW, (b) does
not have much to do with how repacking is costly I/O wise, except
that it is a contributing factor to make (a) worse, which is the
real cause of the I/O cost.
> At the time, we decided against implementing more robust support for
> multiple cruft packs. This patch implements that support which we were
> lacking.
>
> Introduce a new option `--cruft-max-size` which allows repositories to
> accumulate cruft packs up to a given size, after which point a new
> generation of cruft packs can accumulate until it reaches the maximum
> size, and so on. To generate a new cruft pack, the process works like
> so:
>
> - Sort a list of any existing cruft packs in ascending order of pack
> size.
>
> - Starting from the beginning of the list, group cruft packs together
> while the accumulated size is smaller than the maximum specified
> pack size.
>
> - Combine the objects in these cruft packs together into a new cruft
> pack, along with any other unreachable objects which have since
> entered the repository.
The above three steps guarantees that, before the amount of the
accumulated cruft objects reaches the "max-size", we would be moving
bits from old to new cruft packs and incurring the same I/O cost as
in the current system. I suspect (I haven't read what the new code
does) the untold fourth point is what improves the new system over
the current one, which would be "Do not touch the existing cruft
pack(s) that are already large enough, and send enumerated cruft
objects that do not appear in these existing cruft pack(s) to a new
cruft pack. Do not rewrite these existing cruft pack(s), even if
there are some cruft objects that has aged enough, because rewriting
these huge cruft packs only to expunge a few objects from them is
costly in I/O" or something? In any case, I do not think with only
the above three ...
> This limits the I/O churn up to a quadratic function of the value
> specified by the `--cruft-max-size` option, instead of behaving
> quadratically in the number of total unreachable objects.
... I do not quite see how you would limit the I/O churn.
> When pruning unreachable objects, we bypass the new paths which combine
"paths" here refers to...? code paths, I guess?
> +gc.cruftMaxSize::
> + Limit the size of new cruft packs when repacking. When
> + specified in addition to `--cruft-max-size`, the command line
> + option takes priority. See the `--cruft-max-size` option of
> + linkgit:git-repack[1].
Hmph.
I am reasonably sure that I will mix the name up and call it
gc.maxCruftSize in my configuration file and scratch my head
wondering why it is not working.
> diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt
> index 90806fd26a..8a90d684a7 100644
> --- a/Documentation/git-gc.txt
> +++ b/Documentation/git-gc.txt
> @@ -59,6 +59,13 @@ be performed as well.
> cruft pack instead of storing them as loose objects. `--cruft`
> is on by default.
>
> +--cruft-max-size=<n>::
> + When packing unreachable objects into a cruft pack, limit the
> + size of new cruft packs to be at most `<n>`. Overrides any
> + value specified via the `gc.cruftMaxSize` configuration. See
> + the `--cruft-max-size` option of linkgit:git-repack[1] for
> + more.
At least this side giving --max-cruft-size=<n> (which I think is a
lot more natural word order) would cause parse-options to give an
error, so it won't risk mistakes go silently unnoticed.
> diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
> index 4017157949..23fd203d79 100644
> --- a/Documentation/git-repack.txt
> +++ b/Documentation/git-repack.txt
> @@ -74,6 +74,15 @@ to the new separate pack will be written.
> immediately instead of waiting for the next `git gc` invocation.
> Only useful with `--cruft -d`.
>
> +--cruft-max-size=<n>::
> + Repack cruft objects into packs as large as `<n>` before
> + creating new packs. As long as there are enough cruft packs
> + smaller than `<n>`, repacking will cause a new cruft pack to
> + be created containing objects from any combined cruft packs,
> + along with any new unreachable objects. Cruft packs larger
> + than `<n>` will not be modified. Only useful with `--cruft
> + -d`.
Here, the missing fourth point I pointed out above is mentioned,
which is good.
Describe the unit for <n> (I am assuming that is counted in bytes,
honoring the human-friendly suffix, like 100M).
There may be some "interesting" behaviour around the size boundary,
no? If you pack too many objects, your resulting size may slightly
bust <n> and you will get a complaint, but by fixing that "bug", you
will always stop short of filling the whole <n> bytes in the
produced packfiles, and they will not be excempt from rewriting
(becuase they are not "larger than <n>"), which defeats the point of
this patch.
Describe that <n> is a threshold that we stop soon after passing to
explicitly allow us to go beyond it would solve the above problem, I
would presume.
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 1f53b66c7b..b6640abd35 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -52,6 +52,7 @@ static const char * const builtin_gc_usage[] = {
> static int pack_refs = 1;
> static int prune_reflogs = 1;
> static int cruft_packs = 1;
> +static char *cruft_max_size;
I do not think this type is a good idea.
> static int aggressive_depth = 50;
> static int aggressive_window = 250;
> static int gc_auto_threshold = 6700;
> @@ -163,6 +164,7 @@ static void gc_config(void)
> git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit);
> git_config_get_bool("gc.autodetach", &detach_auto);
> git_config_get_bool("gc.cruftpacks", &cruft_packs);
> + git_config_get_string("gc.cruftmaxsize", &cruft_max_size);
> git_config_get_expiry("gc.pruneexpire", &prune_expire);
> git_config_get_expiry("gc.worktreepruneexpire", &prune_worktrees_expire);
> git_config_get_expiry("gc.logexpiry", &gc_log_expire);
> @@ -347,6 +349,9 @@ static void add_repack_all_option(struct string_list *keep_pack)
> strvec_push(&repack, "--cruft");
> if (prune_expire)
> strvec_pushf(&repack, "--cruft-expiration=%s", prune_expire);
> + if (cruft_max_size)
> + strvec_pushf(&repack, "--cruft-max-size=%s",
> + cruft_max_size);
Even if you are going to pass it intact, as an opaque token, to
another program. Rather, you should parse and make sure it is a
valid non-negative byte count before calling out the other program
(passing the original string down, after parsing it only for error
checking the value, and having "repack" do its own parsing, is
perfectly fine).
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 44cb261371..56e7f5f43d 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -26,6 +26,9 @@
> #define LOOSEN_UNREACHABLE 2
> #define PACK_CRUFT 4
>
> +#define DELETE_PACK ((void*)(uintptr_t)1)
> +#define RETAIN_PACK ((uintptr_t)(1<<1))
Shouldn't these look more similar? That is
((void *)(uintptr_t)(1<<0))
((void *)(uintptr_t)(1<<1))
> + if (pack_is_retained(item)) {
> + item->util = NULL;
> + } else if (!string_list_has_string(names, sha1)) {
Without the pack_marked_for_deletion(item) macro Patrick suggested,
the logic around here looks very uneven.
> + /*
> + * Mark this pack for deletion, which ensures
> + * that this pack won't be included in a MIDX
> + * (if `--write-midx` was given) and that we
> + * will actually delete this pack (if `-d` was
> + * given).
> + */
> + item->util = DELETE_PACK;
> + }
> }
> }
>
> +static void retain_cruft_pack(struct existing_packs *existing,
> + struct packed_git *cruft)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + struct string_list_item *item;
> +
> + strbuf_addstr(&buf, pack_basename(cruft));
> + strbuf_strip_suffix(&buf, ".pack");
> +
> + item = string_list_lookup(&existing->cruft_packs, buf.buf);
> + if (!item)
> + BUG("could not find cruft pack '%s'", pack_basename(cruft));
> +
> + item->util = (void*)RETAIN_PACK;
> + strbuf_release(&buf);
> +}
> +
> static void mark_packs_for_deletion(struct existing_packs *existing,
> struct string_list *names)
>
> @@ -217,6 +247,8 @@ static void collect_pack_filenames(struct existing_packs *existing,
> }
>
> string_list_sort(&existing->kept_packs);
> + string_list_sort(&existing->non_kept_packs);
> + string_list_sort(&existing->cruft_packs);
> strbuf_release(&buf);
> }
>
> @@ -799,6 +831,72 @@ static void remove_redundant_bitmaps(struct string_list *include,
> strbuf_release(&path);
> }
>
> +static int existing_cruft_pack_cmp(const void *va, const void *vb)
> +{
> + struct packed_git *a = *(struct packed_git **)va;
> + struct packed_git *b = *(struct packed_git **)vb;
> +
> + if (a->pack_size < b->pack_size)
> + return -1;
> + if (a->pack_size > b->pack_size)
> + return 1;
> + return 0;
> +}
> +
> +static void collapse_small_cruft_packs(FILE *in, unsigned long max_size,
> + struct existing_packs *existing)
> +{
> + struct packed_git **existing_cruft, *p;
> + struct strbuf buf = STRBUF_INIT;
> + unsigned long total_size = 0;
> + size_t existing_cruft_nr = 0;
> + size_t i;
> +
> + ALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr);
> +
> + for (p = get_all_packs(the_repository); p; p = p->next) {
> + if (!(p->is_cruft && p->pack_local))
> + continue;
> +
> + strbuf_reset(&buf);
> + strbuf_addstr(&buf, pack_basename(p));
> + strbuf_strip_suffix(&buf, ".pack");
> +
> + if (!string_list_has_string(&existing->cruft_packs, buf.buf))
> + continue;
> +
> + if (existing_cruft_nr >= existing->cruft_packs.nr)
> + BUG("too many cruft packs (found %"PRIuMAX", but knew "
> + "of %"PRIuMAX")",
Is that a BUG() that somehow miscounted the packs, or can it be a
runtime error that may happen when a "git push" is pushing new
objects into the repository, creating a new pack we did not know
about? Something like the latter should not be marked a BUG(), but
> + (uintmax_t)existing_cruft_nr + 1,
> + (uintmax_t)existing->cruft_packs.nr);
> + existing_cruft[existing_cruft_nr++] = p;
> + }
> +
> + QSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp);
We use the simplest "from smaller ones to larger ones, combine one
by one together until the result gets large enough", which would not
give us the best packing, but it is OK because it is not our goal to
solve knapsack problem here, I presume?
> + for (i = 0; i < existing_cruft_nr; i++) {
> + off_t proposed;
> +
> + p = existing_cruft[i];
> + proposed = st_add(total_size, p->pack_size);
> +
> + if (proposed <= max_size) {
> + total_size = proposed;
> + fprintf(in, "-%s\n", pack_basename(p));
> + } else {
> + retain_cruft_pack(existing, p);
> + fprintf(in, "%s\n", pack_basename(p));
> + }
> + }
This is exactly what I talked about the possibly funny behaviour
around the boundary earlier, but it may be even worse. This time,
we may decide that a pack with size <n-epsilon> is to be retained,
only because the pack that came next in the existing list happened
to be larger than epsilon, but next time around, it may not be the
case (i.e. our pack may be smaller than epsilon, the next one in the
current round may be larger than epsilon, but before we repack the
next time, a new pack that is slightly smaller than epsilon that is
larger than our pack may have been created---now our pack will be
combined with it), so the algorithm to choose which ones are kept
does not depend on the pack itself alone but also depends on its
surroundings.
^ permalink raw reply
* Re: [PATCH 1/2] t7700: split cruft-related tests to t7704
From: Eric Sunshine @ 2023-09-08 0:01 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Jeff King, Jonathan Tan
In-Reply-To: <de6c2a0d70c3e62bd70fdca8b5c6c2ae6eaa13f5.1694123506.git.me@ttaylorr.com>
On Thu, Sep 7, 2023 at 5:52 PM Taylor Blau <me@ttaylorr.com> wrote:
> t7700: split cruft-related tests to t7704
> ---
Missing sign-off.
^ permalink raw reply
* Please explain avoiding history simplifications when diffing merges
From: Magnus Holmgren @ 2023-09-08 9:09 UTC (permalink / raw)
To: git
Hi (and thanks for a great free tool).
QGit was bitten by
https://github.com/git/git/commit/0dec322d31db3920872f43bdd2a7ddd282a5be67
It looks like passing --simplify-merges to override the default solves the
problem, but I still want to ask here because I'm not sure I fully understand
the reasoning:
> the default history simplification would remove merge commits from
> consideration if the file "path" matched the second parent.
I guess that means that even if there was a difference relative to the first
parent, which you wanted to see, the merge commit would be omitted if there's
no diff relative to the second parent? That's fine, and history simplification
isn't trivial, but now every merge commit ever is included in the commit log
of an individual file, even if that file was only added somewhere in the
middle and never changed since. Is that really desirable? Shouldn't it be
possible to include only merge commits that are not TREESAME to *all* parents?
Thanks,
--
Magnus Holmgren
./¯\_/¯\. Milient
(also holmgren@debian.org)
^ permalink raw reply
* Re: [PATCH] rebase -i: ignore signals when forking subprocesses
From: Phillip Wood @ 2023-09-08 9:59 UTC (permalink / raw)
To: Jeff King, Phillip Wood via GitGitGadget
Cc: git, Johannes Schindelin, Phillip Wood, Junio C Hamano
In-Reply-To: <20230907210638.GB941945@coredump.intra.peff.net>
Hi Peff
Thanks for your thoughts, I hoped I get a interesting response if I cc'd
you and I wasn't disappointed!
On 07/09/2023 22:06, Jeff King wrote:
> On Thu, Sep 07, 2023 at 10:03:02AM +0000, Phillip Wood via GitGitGadget wrote:
>
>> rebase -i: ignore signals when forking subprocesses
>>
>> Having written this I started thinking about what happens when we fork
>> hooks, merge strategies and merge drivers. I now wonder if it would be
>> better to change run_command() instead - are there any cases where we
>> actually want git to be killed when the user interrupts a child process?
>
> I think that would be quite surprising in most cases, as the user may
> not be aware when or if a sub-program is in use.
>
> Imagine that you're running a script which runs git-commit in a loop,
> which in turn runs "gc --auto", which in turn runs a pre-auto-gc hook.
> You want to stop it, so you hit ^C, which sends SIGINT to all of the
> processes.
>
> I think most people would expect the whole process chain to stop
> immediately. But in your proposal, we'd kill the hook only. That kind-of
> propagates up to "gc --auto" (which says "OK, the hook says don't run").
> And then that doesn't really propagate to git-commit, which ignores the
> exit code of gc and continues on, running the post-commit hook and so
> on. And then outer loop of the script continues, invoking the next
> commit, and so on. To actually quit you have to hit ^C several times
> with the right timing (the exact number of which is unknown to you, as
> it depends on the depth of the process chain).
Ah, I hadn't thought about "gc --auto" I was assuming that the calling
code would see the child had been killed and exit but that's not always
the case.
> I think this really comes down to: does the user perceive the child
> process as the current "main" process running in the foreground? For
> most run-command invocations, I would say no. For something like running
> an editor, the answer is more clearly yes.
>
> For something like sequencer "exec" commands, I think it really depends
> on what the command is doing. If it is "git commit --amend", then that
> is going to open an editor and take over the terminal. If it is "make",
> then probably not. But it may be OK to do here, just because we know
> that a signal exit from the child will be immediately read and
> propagated by the sequencer machinery (assuming the child dies; if it
> blocks SIGINT, too, then now you cannot interrupt it at all!).
The child not dying is tricky, if it is in the same process group as git
then even if git dies the I think the shell will wait for the child to
exit before showing the prompt again so it is not clear to me that the
user is disadvantaged by git ignoring SIGINT in that case. Part of the
motivation for this patch is that I'd like to move the sequencer to a
model where it only saves its state to disk when it is stopping for the
user to fix conflicts. To do that safely it cannot die if the user
interprets a child with SIGINT.
> In the classic Unix world, I think the solution here is setsid(),
> process groups, and controlling terminals. One example in your commit
> message is the sequencer kicking off git-commit, which kicks off an
> editor. The user hits ^C then, and the sequencer is killed. But I think
> your patch is papering over the deeper bug. In 913ef36093
> (launch_editor: ignore terminal signals while editor has control,
> 2012-11-30), we did this same "ignore INT" trick.
Yes, that was the inspiration for this patch
> But I think the right
> solution is actually to start the editor in its own process group, and
> let it be the foreground of the terminal. And then a ^C while the editor
> is running would not only not hit git-commit, but it would not hit any
> sequencer or other intermediate processes above it.
>
> I've never done it before, but from my reading we basically want to do
> (in the forked process before we exec):
>
> setsid();
> open("/dev/tty");
Do we want a whole new session? As I understand it to launch a
foreground job shells put the child in its own process group and then
call tcsetpgrp() to change the foreground process group of the
controlling terminal to that of the child. I agree that would be a
better way of doing things on unix.
> But of course none of that probably has any meaning on Windows. I'm not
> sure if there are analogous concepts there we could access with
> alternate code, or if it would need to be a totally different strategy.
Lets see if Johannes has any comments about that.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH] rebase -i: ignore signals when forking subprocesses
From: Phillip Wood @ 2023-09-08 10:02 UTC (permalink / raw)
To: Johannes Schindelin, Phillip Wood via GitGitGadget
Cc: git, Jeff King, Phillip Wood, Junio C Hamano
In-Reply-To: <912205bc-37c5-edbf-2f85-f26991ad65eb@gmx.de>
Hi Johannes
On 07/09/2023 13:57, Johannes Schindelin wrote:
> Hi Phillip,
>
> On Thu, 7 Sep 2023, Phillip Wood via GitGitGadget wrote:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> If the user presses Ctrl-C to interrupt a program run by a rebase "exec"
>> command then SIGINT will also be sent to the git process running the
>> rebase resulting in it being killed. Fortunately the consequences of
>> this are not severe as all the state necessary to continue the rebase is
>> saved to disc but it would be better to avoid killing git and instead
>> report that the command failed. A similar situation occurs when the
>> sequencer runs "git commit" or "git merge". If the user generates SIGINT
>> while editing the commit message then the git processes creating the
>> commit will ignore it but the git process running the rebase will be
>> killed.
>>
>> Fix this by ignoring SIGINT and SIGQUIT when forking "exec" commands,
>> "git commit" and "git merge". This matches what git already does when
>> running the user's editor and matches the behavior of the standard
>> library's system() function.
>
> ACK
Thanks
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>> rebase -i: ignore signals when forking subprocesses
>>
>> Having written this I started thinking about what happens when we fork
>> hooks, merge strategies and merge drivers. I now wonder if it would be
>> better to change run_command() instead - are there any cases where we
>> actually want git to be killed when the user interrupts a child process?
>
> I am not sure that we can rely on arbitrary hooks to do the right thing
> upon Ctrl+C, which is to wrap up and leave. So I _guess_ that we will have
> to leave it an opt-in.
Peff pointed out it doesn't play well with "gc --auto" either. Do you
have any thoughts (particularly about the implications for Windows) on
his suggestion to put the child in it's own session, or putting the
child in its own process group and making that the foreground process
group of the controlling terminal?
> However, we could easily make it an option that `run_command()` handles,
> much like `no_stdin`.
That's an interesting idea.
Best Wishes
Phillip
^ permalink raw reply
* [PATCH] start_command: reset disposition of all signals in child
From: Phillip Wood via GitGitGadget @ 2023-09-08 10:05 UTC (permalink / raw)
To: git; +Cc: Eric Wong, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
In order to avoid invoking a signal handler in the child process between
fork() and exec(), start_command() blocks all signals before calling
fork and then in the child resets the disposition of all signals that
are not ignored to SIG_DFL before unblocking them. The exception for
ignored signals seems to been inspired by ruby's process handling[1]
based on the misconception that execve() will reset them to
SIG_DFL. Unfortunately this is not the case [2] and any signals that are
ignored in the parent will default to being ignored by the program
executed by start_command().
When git ignores SIGPIPE before forking a child process it is to stop
git from being killed if the child exits while git is writing to the
child's stdin. We do not want to ignore SIGPIPE in the child. When git
ignores SIGINT and SIGQUIT before forking a child process it is to stop
git from being killed if the user interrupts the child with Ctrl-C or
Ctrl-\ we do not want the child to ignore those signals [3].
Fortunately the fix is easy - reset the disposition of all signals
regardless of their previous disposition.
[1] https://lore.kernel.org/git/20170413211428.GA5961@whir/
[2] The man page for execve(2) states:
POSIX.1 specifies that the dispositions of any signals that are
ignored or set to the default are left unchanged. POSIX.1
specifies one exception: if SIGCHLD is being ignored, then an
implementation may leave the disposition unchanged or reset it
to the default; Linux does the former.
Page 579 of "The Linux Programming Interface" notes:
SUSv3 recommends that signals should not be blokced or ignored
across an exec() of an arbitrary program. Here, "arbitrary"
means a program that we did not write.
[3] This is really a work-around for not moving the child into its own
process group and changing the foreground process group of the
controlling terminal.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
start_command: reset disposition of all signals in child
As an aside I wonder if we ought to add an option to ignore SIGPIPE when
stdin is redirected and possibly turn it on by default.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1582%2Fphillipwood%2Fstart-command-dont-ignore-signals-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1582/phillipwood/start-command-dont-ignore-signals-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1582
run-command.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/run-command.c b/run-command.c
index a558042c876..765775a1f42 100644
--- a/run-command.c
+++ b/run-command.c
@@ -823,11 +823,8 @@ fail_pipe:
* restore default signal handlers here, in case
* we catch a signal right before execve below
*/
- for (sig = 1; sig < NSIG; sig++) {
- /* ignored signals get reset to SIG_DFL on execve */
- if (signal(sig, SIG_DFL) == SIG_IGN)
- signal(sig, SIG_IGN);
- }
+ for (sig = 1; sig < NSIG; sig++)
+ signal(sig, SIG_DFL);
if (sigprocmask(SIG_SETMASK, &as.old, NULL) != 0)
child_die(CHILD_ERR_SIGPROCMASK);
base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size`
From: Patrick Steinhardt @ 2023-09-08 11:21 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Jeff King, Jonathan Tan
In-Reply-To: <7e4e42e1aacf2111f04a933c0725a8c81769f8d6.1694123506.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 6905 bytes --]
On Thu, Sep 07, 2023 at 05:52:04PM -0400, Taylor Blau wrote:
[snip]
> @@ -125,17 +133,39 @@ static void mark_packs_for_deletion_1(struct string_list *names,
> if (len < hexsz)
> continue;
> sha1 = item->string + len - hexsz;
> - /*
> - * Mark this pack for deletion, which ensures that this
> - * pack won't be included in a MIDX (if `--write-midx`
> - * was given) and that we will actually delete this pack
> - * (if `-d` was given).
> - */
> - if (!string_list_has_string(names, sha1))
> - item->util = (void*)1;
> +
> + if (pack_is_retained(item)) {
> + item->util = NULL;
> + } else if (!string_list_has_string(names, sha1)) {
> + /*
> + * Mark this pack for deletion, which ensures
> + * that this pack won't be included in a MIDX
> + * (if `--write-midx` was given) and that we
> + * will actually delete this pack (if `-d` was
> + * given).
> + */
> + item->util = DELETE_PACK;
> + }
I find the behaviour of this function a tad surprising as it doesn't
only mark a pack for deletion, but it also marks a pack as not being
retained anymore. Shouldn't we rather:
if (pack_is_retained(item)) {
// Theoretically speaking we shouldn't even do this bit here as
// we _un_mark the pack for deletion. But at least we shouldn't
// be removing the `RETAIN_PACK` bit, I'd think.
item->util &= ~DELETE_PACK;
} else if (!string_list_has_string(names, sha1)) {
// And here we shouldn't discard the `RETAIN_PACK` bit either.
item->util |= DELETE_PACK;
}
> }
> }
>
> +static void retain_cruft_pack(struct existing_packs *existing,
> + struct packed_git *cruft)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + struct string_list_item *item;
> +
> + strbuf_addstr(&buf, pack_basename(cruft));
> + strbuf_strip_suffix(&buf, ".pack");
> +
> + item = string_list_lookup(&existing->cruft_packs, buf.buf);
> + if (!item)
> + BUG("could not find cruft pack '%s'", pack_basename(cruft));
> +
> + item->util = (void*)RETAIN_PACK;
> + strbuf_release(&buf);
> +}
> +
Similarly, should we handle potentially pre-existing bits gracefully and
`item->util |= RETAIN_PACK`?
> static void mark_packs_for_deletion(struct existing_packs *existing,
> struct string_list *names)
>
> @@ -217,6 +247,8 @@ static void collect_pack_filenames(struct existing_packs *existing,
> }
>
> string_list_sort(&existing->kept_packs);
> + string_list_sort(&existing->non_kept_packs);
> + string_list_sort(&existing->cruft_packs);
> strbuf_release(&buf);
> }
>
> @@ -799,6 +831,72 @@ static void remove_redundant_bitmaps(struct string_list *include,
> strbuf_release(&path);
> }
>
> +static int existing_cruft_pack_cmp(const void *va, const void *vb)
> +{
> + struct packed_git *a = *(struct packed_git **)va;
> + struct packed_git *b = *(struct packed_git **)vb;
> +
> + if (a->pack_size < b->pack_size)
> + return -1;
> + if (a->pack_size > b->pack_size)
> + return 1;
> + return 0;
> +}
> +
> +static void collapse_small_cruft_packs(FILE *in, unsigned long max_size,
We might want to use `size_t` to denote file sizes instead of `unsigned
long`.
> + struct existing_packs *existing)
> +{
> + struct packed_git **existing_cruft, *p;
> + struct strbuf buf = STRBUF_INIT;
> + unsigned long total_size = 0;
Here, as well.
> + size_t existing_cruft_nr = 0;
> + size_t i;
> +
> + ALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr);
> +
> + for (p = get_all_packs(the_repository); p; p = p->next) {
> + if (!(p->is_cruft && p->pack_local))
> + continue;
> +
> + strbuf_reset(&buf);
> + strbuf_addstr(&buf, pack_basename(p));
> + strbuf_strip_suffix(&buf, ".pack");
> +
> + if (!string_list_has_string(&existing->cruft_packs, buf.buf))
> + continue;
> +
> + if (existing_cruft_nr >= existing->cruft_packs.nr)
> + BUG("too many cruft packs (found %"PRIuMAX", but knew "
> + "of %"PRIuMAX")",
> + (uintmax_t)existing_cruft_nr + 1,
> + (uintmax_t)existing->cruft_packs.nr);
> + existing_cruft[existing_cruft_nr++] = p;
> + }
> +
> + QSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp);
> +
> + for (i = 0; i < existing_cruft_nr; i++) {
> + off_t proposed;
This should also be `size_t` given that `st_add` returns `size_t` and
not `off_t`.
> + p = existing_cruft[i];
> + proposed = st_add(total_size, p->pack_size);
> +
> + if (proposed <= max_size) {
> + total_size = proposed;
> + fprintf(in, "-%s\n", pack_basename(p));
> + } else {
> + retain_cruft_pack(existing, p);
> + fprintf(in, "%s\n", pack_basename(p));
> + }
It's a bit funny that we re-check whether we have exceeded the maximum
size in subsequente iterations once we hit the limit, but it arguably
makes the logic a bit simpler.
> + }
> +
> + for (i = 0; i < existing->non_kept_packs.nr; i++)
> + fprintf(in, "-%s.pack\n",
> + existing->non_kept_packs.items[i].string);
> +
> + strbuf_release(&buf);
> +}
> +
> static int write_cruft_pack(const struct pack_objects_args *args,
> const char *destination,
> const char *pack_prefix,
> @@ -846,10 +944,18 @@ static int write_cruft_pack(const struct pack_objects_args *args,
> in = xfdopen(cmd.in, "w");
> for_each_string_list_item(item, names)
> fprintf(in, "%s-%s.pack\n", pack_prefix, item->string);
> - for_each_string_list_item(item, &existing->non_kept_packs)
> - fprintf(in, "-%s.pack\n", item->string);
> - for_each_string_list_item(item, &existing->cruft_packs)
> - fprintf(in, "-%s.pack\n", item->string);
> + if (args->max_pack_size && !cruft_expiration) {
> + unsigned long max_pack_size;
> + if (!git_parse_ulong(args->max_pack_size, &max_pack_size))
> + return error(_("could not parse --cruft-max-size: '%s'"),
> + args->max_pack_size);
> + collapse_small_cruft_packs(in, max_pack_size, existing);
> + } else {
> + for_each_string_list_item(item, &existing->non_kept_packs)
> + fprintf(in, "-%s.pack\n", item->string);
> + for_each_string_list_item(item, &existing->cruft_packs)
> + fprintf(in, "-%s.pack\n", item->string);
> + }
If I understand correctly, we only collapse small cruft packs in case
we're not expiring any objects at the same time. Is there an inherent
reason why? I would imagine that it can indeed be useful to expire
objects contained in cruft packs and then have git-repack(1) recombine
whatever is left into larger packs.
If the reason is basically "it's complicated" then that is fine with me,
we can still implement the functionality at a later point in time. But
until then I think that we should let callers know that the two options
are incompatible with each other by producing an error when both are
passed.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH] completion(switch/checkout): treat --track and -t the same
From: Johannes Schindelin via GitGitGadget @ 2023-09-08 12:28 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When `git switch --track ` is to be completed, only remote refs are
eligible because that is what the `--track` option targets.
And when the short-hand `-t` is used instead, the same _should_ happen.
Let's make it so.
Note that the bug exists both in the completions of `switch` and
`completion`, even if it manifests in slightly different ways: While
the completion of `git switch -t ` will not even look at remote refs,
the completion of `git checkout -t ` will look at both remote _and_
local refs. Both should look only at remote refs.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Fix the completion of git switch/git checkout: Treat --track and -t the
same
Just something that was nagging me for one year too many.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1584%2Fdscho%2Fcomplete-switch-t-properly-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1584/dscho/complete-switch-t-properly-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1584
contrib/completion/git-completion.bash | 4 ++--
t/t9902-completion.sh | 12 ++++++++++--
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 133ec92bfae..745dc901fbe 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1607,7 +1607,7 @@ _git_checkout ()
if [ -n "$(__git_find_on_cmdline "-b -B -d --detach --orphan")" ]; then
__git_complete_refs --mode="refs"
- elif [ -n "$(__git_find_on_cmdline "--track")" ]; then
+ elif [ -n "$(__git_find_on_cmdline "-t --track")" ]; then
__git_complete_refs --mode="remote-heads"
else
__git_complete_refs $dwim_opt --mode="refs"
@@ -2514,7 +2514,7 @@ _git_switch ()
if [ -n "$(__git_find_on_cmdline "-c -C -d --detach")" ]; then
__git_complete_refs --mode="refs"
- elif [ -n "$(__git_find_on_cmdline "--track")" ]; then
+ elif [ -n "$(__git_find_on_cmdline "-t --track")" ]; then
__git_complete_refs --mode="remote-heads"
else
__git_complete_refs $dwim_opt --mode="heads"
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 8835e16e811..df8bc44c285 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1622,14 +1622,22 @@ test_expect_success 'git checkout - with -d, complete only references' '
'
test_expect_success 'git switch - with --track, complete only remote branches' '
- test_completion "git switch --track " <<-\EOF
+: test_completion "git switch --track " <<-\EOF &&
+ other/branch-in-other Z
+ other/main-in-other Z
+ EOF
+ test_completion "git switch -t " <<-\EOF
other/branch-in-other Z
other/main-in-other Z
EOF
'
test_expect_success 'git checkout - with --track, complete only remote branches' '
- test_completion "git checkout --track " <<-\EOF
+ test_completion "git checkout --track " <<-\EOF &&
+ other/branch-in-other Z
+ other/main-in-other Z
+ EOF
+ test_completion "git checkout -t " <<-\EOF
other/branch-in-other Z
other/main-in-other Z
EOF
base-commit: 94e83dcf5b5faaa22e32729305f8fd7090bfdfed
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] rebase -i: ignore signals when forking subprocesses
From: Phillip Wood @ 2023-09-08 13:11 UTC (permalink / raw)
To: phillip.wood, Jeff King, Phillip Wood via GitGitGadget
Cc: git, Johannes Schindelin, Junio C Hamano
In-Reply-To: <9ba22d4b-3cbe-4d4a-8dba-bc3781e82222@gmail.com>
On 08/09/2023 10:59, Phillip Wood wrote:
>> But I think the right
>> solution is actually to start the editor in its own process group, and
>> let it be the foreground of the terminal. And then a ^C while the editor
>> is running would not only not hit git-commit, but it would not hit any
>> sequencer or other intermediate processes above it.
>>
>> I've never done it before, but from my reading we basically want to do
>> (in the forked process before we exec):
>>
>> setsid();
>> open("/dev/tty");
>
> Do we want a whole new session? As I understand it to launch a
> foreground job shells put the child in its own process group and then
> call tcsetpgrp() to change the foreground process group of the
> controlling terminal to that of the child. I agree that would be a
> better way of doing things on unix.
It is better for handling SIGINT and SIGQUIT when we don't want git to
be killed but in complicates the handling of SIGTSTP and friends. We'd
need to pass WUNTRACED to waitpid() and then do
"raise(WSTOPSIG(wstatus))" to propagate the signal up to the shell. When
resuming we'd need to call tcsetpgrp() again if git is resumed in the
foreground before sending SIGCONT to the child.
>> But of course none of that probably has any meaning on Windows. I'm not
>> sure if there are analogous concepts there we could access with
>> alternate code, or if it would need to be a totally different strategy.
>
> Lets see if Johannes has any comments about that.
I had a quick google and it looks like cygwin somehow manages to
implement tcsetpgrp() but the windows terminal does not have any concept
of a foreground process group
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v2] diff-lib: Fix check_removed when fsmonitor is on
From: Junio C Hamano @ 2023-09-08 15:00 UTC (permalink / raw)
To: Josip Sokcevic; +Cc: jonathantanmy, git, git
In-Reply-To: <CAJiyOigsH=Pac1g_5ztB01uEaCBjefazUpnH5k3ARZ-COm0uLA@mail.gmail.com>
Josip Sokcevic <sokcevic@google.com> writes:
>> > @@ -39,11 +39,22 @@
>> > static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)
>> > {
>> > assert(is_fsmonitor_refreshed(istate));
>>
>> Not a problem this patch introduces, but doesn't this call path
>>
>> diff_cache()
>> -> unpack_trees()
>> -> oneway_diff()
>> -> do_oneway_diff()
>> -> show_new_file(), show_modified()
>> -> get_stat_data()
>> -> check_removed()
>>
>> violate the assertion? If so, perhaps we should rewrite it into a
>> more explicit "if (...) BUG(...)" that is not compiled away.
>
> True, I will update it.
Not as a part of this series, though.
I think this came from the same series that were merged at 858119f6
(Merge branch 'nk/diff-index-fsmonitor', 2021-03-24), so if we just
simply do a moral "revert" of the series, then it should disappear.
More importantly, you'd want to ask help from folks who are more
familiar with fsmonitor, which would take some time, and fixing this
assertion can be backburnered while you are fixing the original
issue that got you started here. I see JeffH is already Cc:ed,
which is good.
>> > - if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {
>> > - if (!is_missing_file_error(errno))
>> > - return -1;
>> > - return 1;
>> > + if (ce->ce_flags & CE_FSMONITOR_VALID) {
>> > + /*
>> > + * Both check_removed() and its callers expect lstat() to have
>> > + * happened and, in particular, the st_mode field to be set.
>> > + * Simulate this with the contents of ce.
>> > + */
>> > + memset(st, 0, sizeof(*st));
>>
>> It is true that the original, when CE_FSMONITOR_VALID bit is set,
>> bypasses lstat() altogether and leaves the contents of st completely
>> uninitialized, but this is still way too insufficient, isn't it?
>>
>> There are three call sites of the check_removed() function.
>>
>> * The first one in run_diff_files() only cares about st.st_mode and
>> other members of the structure are not looked at. This makes
>> readers wonder if the "st" parameter to check_removed() should
>> become "mode_t *st_mode" to clarify this point, but the primary
>> thing I want to say is that this caller will not mind if we leave
>> other members of st bogus (like 0-bit filled) as long as the mode
>> is set correctly.
>>
>> * The second one in run_diff_files() passes the resulting &st to
>> match_stat_with_submodule(), which in turn passes it to
>> ie_match_stat(), which cares about "struct stat" members that are
>> used for quick change detection, like owner, group, mtime.
>> Giving it a bogus st will most likely cause it to report a
>> change.
>>
>> * The third one is in get_stat_data(). This also uses the &st to
>> call match_stat_with_submodule(), so it is still totally broken
>> to give it a bogus st, the same way as the second caller above.
>>
>> > + st->st_mode = ce->ce_mode;
>>
>> Does this work correctly when the cache entry points at a gitlink,
>> which uses 0160000 that is not a valid st_mode? I think you'd want
>> to use a reverse function of create_ce_mode().
>
> I realized this too but after I sent the patch. I don't think we have
> a good way to reverse it, so the best we can do is to guess it. But I
> don't think we should do that. Instead, should we just zero the struct
> and add a TODO? Alternative could be to use a double pointer for stat
> and do more checks in call sites, but I'm not familiar with the
> codebase to how that branching would need to look like.
>
> Any preference?
My preference in the short term actually is to teach this codepath
to ignore CE_FSMONITOR_VALID bit and always run lstat() instead.
That will unbreak this "optimization" that allowed the code to do a
wrong thing quickly.
The solution suitable for a bit longer term becomes rather obvious
when we step back a bit and think about the purpose of this block
inside "if ce_flags says CE_FSMONITOR_VALID is true, do this". The
fsmonitor has already checked and knows that the path has not been
updated, i.e. what we have in the in-core index still matches what
we would get from running lstat(). So now what we need to do is to
pretend as if we called lstat(), recreating the data in "struct
stat" from "struct cache_entry" well enough to fool ie_match_stat()
into saying that the cached stat data in ce still matches what is in
st.
Can we do that? Of course we can. When we added or refreshed the
cache entry, we should have had "struct stat" taken out of lstat()
for the path, and copied some data from the members of "struct stat"
into "struct cache_entry". After all, that is how we can later ask
ie_match_stat() if the cached stat data in the "struct cache_entry"
matches the current "struct stat" data to tell if the path hasn't
been touched since we last refreshed.
We want to have a helper function that we can ask:
With the data in this 'struct cache_entry', please populate this
'struct stat'. Do so well enough that ie_match_stat() says "ok,
the path is unchanged".
You can write such a function by studying two code paths.
* ie_match_stat() and its helpers to find out which members of
"struct stat" matters (i.e. compared against corresponding
members in "struct cache_entry").
* add_to_index() and its helpers, fill_stat_cache_info() in
particular, to see which members of "struct stat" are used to
populate ce->ce_stat_data and how (e.g. some members lose
precision and that is find---the reverse operation will also be
lossy and the resulting "struct stat" you will forge will *not*
exactly match what you would obtain from real lstat(), but it is
good enough to fool ie_match_stat() because you'll be comparing
against an existing ce that was populated in a lossy way).
Then, replace the above partial assignment to st with a call to such
a helper function, which should live in statinfo.c next to
fill_stat_data() and whose function signature should look like
void fill_stat_from_stat_data(struct stat *st, struct stat_data *sd);
You'd probably need to define two macros to update nsec part of c/mtime
members that is conditional near where ST_CTIME_NSEC() macro is
defined to write that reverse function of fill_stat_data(), perhaps
along the lines of ...
#ifdef NO_NSEC
#define FILL_ST_CTIME_NSEC(st, nsec) do { ; /* noop */ } while (0)
#else
#ifdef USE_ST_TIMESPEC
#define FILL_ST_CTIME_NSEC(st, nsec) (st)->st_ctimespec.tv_nsec = (nsec)
#else
#define FILL_ST_CTIME_NSEC(st, nsec) (st)->st_ctim.tv_nsec = (nsec)
#endif
#endif
^ permalink raw reply
* Re: [PATCH] start_command: reset disposition of all signals in child
From: Junio C Hamano @ 2023-09-08 15:42 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget; +Cc: git, Eric Wong, Phillip Wood
In-Reply-To: <pull.1582.git.1694167540231.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> [3] This is really a work-around for not moving the child into its own
> process group and changing the foreground process group of the
> controlling terminal.
I am puzzled, as I somehow thought that "does the user conceive a
subprocess as external and different-from-git entity, or is it
merely an implementation detail? many use of subprocesses in our
codebase, it is the latter." from Peff was a good argument against
such isolation between spawning "git" and spawned subprocesses.
^ permalink raw reply
* Re: [PATCH] completion(switch/checkout): treat --track and -t the same
From: Junio C Hamano @ 2023-09-08 15:51 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <pull.1584.git.1694176123471.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When `git switch --track ` is to be completed, only remote refs are
> eligible because that is what the `--track` option targets.
OK. Presumably that is the same for "checkout --track".
> And when the short-hand `-t` is used instead, the same _should_ happen.
That sounds sensible. The code change for _git_checkout() and
_git_switch() is surprisingly simple ;-) "-t" was not handled any
specially at all and fell through to "refs" complation.
> Let's make it so.
Sounds good.
> Note that the bug exists both in the completions of `switch` and
> `completion`, even if it manifests in slightly different ways: While
> the completion of `git switch -t ` will not even look at remote refs,
> the completion of `git checkout -t ` will look at both remote _and_
> local refs. Both should look only at remote refs.
Correct.
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 133ec92bfae..745dc901fbe 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1607,7 +1607,7 @@ _git_checkout ()
>
> if [ -n "$(__git_find_on_cmdline "-b -B -d --detach --orphan")" ]; then
> __git_complete_refs --mode="refs"
> - elif [ -n "$(__git_find_on_cmdline "--track")" ]; then
> + elif [ -n "$(__git_find_on_cmdline "-t --track")" ]; then
> __git_complete_refs --mode="remote-heads"
> else
> __git_complete_refs $dwim_opt --mode="refs"
> @@ -2514,7 +2514,7 @@ _git_switch ()
>
> if [ -n "$(__git_find_on_cmdline "-c -C -d --detach")" ]; then
> __git_complete_refs --mode="refs"
> - elif [ -n "$(__git_find_on_cmdline "--track")" ]; then
> + elif [ -n "$(__git_find_on_cmdline "-t --track")" ]; then
> __git_complete_refs --mode="remote-heads"
> else
> __git_complete_refs $dwim_opt --mode="heads"
The fallback behaviours are different, which was adequately
described in the proposed log message. As "switch" does not want to
auto-detach upon receiving a ref that is not a local branch, while
"checkout" does, the difference is justifiable.
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 8835e16e811..df8bc44c285 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -1622,14 +1622,22 @@ test_expect_success 'git checkout - with -d, complete only references' '
> '
>
> test_expect_success 'git switch - with --track, complete only remote branches' '
> - test_completion "git switch --track " <<-\EOF
> +: test_completion "git switch --track " <<-\EOF &&
> + other/branch-in-other Z
> + other/main-in-other Z
> + EOF
> + test_completion "git switch -t " <<-\EOF
> other/branch-in-other Z
> other/main-in-other Z
> EOF
> '
So, this demonstrates that '-t' behaves the same way as '--track'.
> test_expect_success 'git checkout - with --track, complete only remote branches' '
> - test_completion "git checkout --track " <<-\EOF
> + test_completion "git checkout --track " <<-\EOF &&
> + other/branch-in-other Z
> + other/main-in-other Z
> + EOF
> + test_completion "git checkout -t " <<-\EOF
> other/branch-in-other Z
> other/main-in-other Z
> EOF
This is, too. If we had a test for "-t" without the code fix, we
would have seen local branches in its output, but now we can see the
candidates are limited to the remote ones.
Good.
Will queue. Thanks.
^ permalink raw reply
* Re: [PATCH] start_command: reset disposition of all signals in child
From: Phillip Wood @ 2023-09-08 15:53 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood via GitGitGadget
Cc: git, Eric Wong, Phillip Wood
In-Reply-To: <xmqqedj8vfht.fsf@gitster.g>
On 08/09/2023 16:42, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> [3] This is really a work-around for not moving the child into its own
>> process group and changing the foreground process group of the
>> controlling terminal.
>
> I am puzzled, as I somehow thought that "does the user conceive a
> subprocess as external and different-from-git entity, or is it
> merely an implementation detail? many use of subprocesses in our
> codebase, it is the latter." from Peff was a good argument against
> such isolation between spawning "git" and spawned subprocesses.
It is and in those cases we do not ignore SIGINT and SIGQUIT in the
parent when we fork the subprocess. What I was trying to say is that in
the few cases where we do ignore SIGINT and SIGQUIT in the parent when
we fork a subprocess we're working round the child being in the same
process group at the parent.
Best Wishes
Phillip
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox