* [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 03/13] trailer: add tests to check defaulting behavior with --no-* flags
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>
While the "--no-where" flag is tested, the "--no-if-exists" and
"--no-if-missing" flags are not, so add tests for them. But also add
tests for all "--no-*" flags to check their effects, both when (1) there
are relevant configuration variables set, and (2) they are not set.
Signed-off-by: Linus Arver <linusa@google.com>
---
Documentation/git-interpret-trailers.txt | 14 ++-
t/t7513-interpret-trailers.sh | 130 +++++++++++++++++++++++
2 files changed, 140 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 55d89614661..91a4dbc9a72 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -114,8 +114,10 @@ OPTIONS
Specify where all new trailers will be added. A setting
provided with '--where' overrides all configuration variables
and applies to all '--trailer' options until the next occurrence of
- '--where' or '--no-where'. Possible values are `after`, `before`,
- `end` or `start`.
+ '--where' or '--no-where'. Upon encountering '--no-where', clear the
+ effect of any previous use of '--where', such that the relevant configuration
+ variables are no longer overridden. Possible values are `after`,
+ `before`, `end` or `start`.
--if-exists <action>::
--no-if-exists::
@@ -123,7 +125,9 @@ OPTIONS
least one trailer with the same <token> in the input. A setting
provided with '--if-exists' overrides all configuration variables
and applies to all '--trailer' options until the next occurrence of
- '--if-exists' or '--no-if-exists'. Possible actions are `addIfDifferent`,
+ '--if-exists' or '--no-if-exists'. Upon encountering '--no-if-exists, clear the
+ effect of any previous use of '--if-exists, such that the relevant configuration
+ variables are no longer overridden. Possible actions are `addIfDifferent`,
`addIfDifferentNeighbor`, `add`, `replace` and `doNothing`.
--if-missing <action>::
@@ -132,7 +136,9 @@ OPTIONS
trailer with the same <token> in the input. A setting
provided with '--if-missing' overrides all configuration variables
and applies to all '--trailer' options until the next occurrence of
- '--if-missing' or '--no-if-missing'. Possible actions are `doNothing`
+ '--if-missing' or '--no-if-missing'. Upon encountering '--no-if-missing,
+ clear the effect of any previous use of '--if-missing, such that the relevant
+ configuration variables are no longer overridden. Possible actions are `doNothing`
or `add`.
--only-trailers::
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index ed0fc04bd95..832aff06167 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -812,6 +812,53 @@ test_expect_success 'using "--where after" with "--no-where"' '
test_cmp expected actual
'
+# Check whether using "--no-where" clears out only the "--where after", such
+# that we still use the configuration in trailer.where (which is different from
+# the hardcoded default (in WHERE_END) assuming the absence of .gitconfig).
+# Here, the "start" setting of trailer.where is respected, so the new "Acked-by"
+# and "Bug" trailers are placed at the beginning, and not at the end which is
+# the harcoded default.
+test_expect_success 'using "--where after" with "--no-where" defaults to configuration' '
+ test_config trailer.ack.key "Acked-by= " &&
+ test_config trailer.bug.key "Bug #" &&
+ test_config trailer.separators ":=#" &&
+ test_config trailer.where "start" &&
+ cat complex_message_body >expected &&
+ sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+ Bug #42
+ Acked-by= Peff
+ Fixes: Z
+ Acked-by= Z
+ Reviewed-by: Z
+ Signed-off-by: Z
+ EOF
+ git interpret-trailers --where after --no-where --trailer "ack: Peff" \
+ --trailer "bug: 42" complex_message >actual &&
+ test_cmp expected actual
+'
+
+# The "--where after" will only get respected for the trailer that came
+# immediately after it. For the next trailer (Bug #42), we default to using the
+# hardcoded WHERE_END because we don't have any "trailer.where" or
+# "trailer.bug.where" configured.
+test_expect_success 'using "--no-where" defaults to harcoded default if nothing configured' '
+ test_config trailer.ack.key "Acked-by= " &&
+ test_config trailer.bug.key "Bug #" &&
+ test_config trailer.separators ":=#" &&
+ cat complex_message_body >expected &&
+ sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+ Fixes: Z
+ Acked-by= Z
+ Acked-by= Peff
+ Reviewed-by: Z
+ Signed-off-by: Z
+ Bug #42
+ EOF
+ git interpret-trailers --where after --trailer "ack: Peff" --no-where \
+ --trailer "bug: 42" complex_message >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'using "where = after"' '
test_config trailer.ack.key "Acked-by= " &&
test_config trailer.ack.where "after" &&
@@ -1176,6 +1223,56 @@ test_expect_success 'overriding configuration with "--if-exists replace"' '
test_cmp expected actual
'
+# "trailer.ifexists" is set to "doNothing", so using "--no-if-exists" defaults
+# to this "doNothing" behavior. So the "Fixes: 53" trailer does not get added.
+test_expect_success 'using "--if-exists replace" with "--no-if-exists" defaults to configuration' '
+ test_config trailer.ifexists "doNothing" &&
+ cat complex_message_body >expected &&
+ sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+ Fixes: Z
+ Acked-by: Z
+ Reviewed-by: Z
+ Signed-off-by: Z
+ EOF
+ git interpret-trailers --if-exists replace --no-if-exists --trailer "Fixes: 53" \
+ <complex_message >actual &&
+ test_cmp expected actual
+'
+
+# No "ifexists" configuration is set, so using "--no-if-exists" makes it default
+# to addIfDifferentNeighbor. Because we do have a different neighbor "Fixes: 53"
+# (because it got added by overriding with "--if-exists replace" earlier in the
+# arguments list), we add "Signed-off-by: addme".
+test_expect_success 'using "--no-if-exists" defaults to hardcoded default if nothing configured' '
+ cat complex_message_body >expected &&
+ sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+ Acked-by: Z
+ Reviewed-by: Z
+ Signed-off-by: Z
+ Fixes: 53
+ Signed-off-by: addme
+ EOF
+ git interpret-trailers --if-exists replace --trailer "Fixes: 53" --no-if-exists \
+ --trailer "Signed-off-by: addme" <complex_message >actual &&
+ test_cmp expected actual
+'
+
+# The second "Fixes: 53" trailer is discarded, because the "--no-if-exists" here
+# makes us default to addIfDifferentNeighbor, and we already added the "Fixes:
+# 53" trailer earlier in the argument list.
+test_expect_success 'using "--no-if-exists" defaults to hardcoded default if nothing configured (no addition)' '
+ cat complex_message_body >expected &&
+ sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+ Acked-by: Z
+ Reviewed-by: Z
+ Signed-off-by: Z
+ Fixes: 53
+ EOF
+ git interpret-trailers --if-exists replace --trailer "Fixes: 53" --no-if-exists \
+ --trailer "Fixes: 53" <complex_message >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'using "ifExists = replace"' '
test_config trailer.fix.key "Fixes: " &&
test_config trailer.fix.ifExists "replace" &&
@@ -1425,6 +1522,39 @@ test_expect_success 'using "ifMissing = doNothing"' '
test_cmp expected actual
'
+# Ignore the "IgnoredTrailer" because of "--if-missing doNothing", but also
+# ignore the "StillIgnoredTrailer" because we set "trailer.ifMissing" to
+# "doNothing" in configuration.
+test_expect_success 'using "--no-if-missing" defaults to configuration' '
+ test_config trailer.ifMissing "doNothing" &&
+ cat complex_message_body >expected &&
+ sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+ Fixes: Z
+ Acked-by: Z
+ Reviewed-by: Z
+ Signed-off-by: Z
+ EOF
+ git interpret-trailers --if-missing doNothing --trailer "IgnoredTrailer: ignoreme" --no-if-missing \
+ --trailer "StillIgnoredTrailer: ignoreme" <complex_message >actual &&
+ test_cmp expected actual
+'
+
+# Add the "AddedTrailer" because the "--no-if-missing" clears the "--if-missing
+# doNothing" from earlier in the argument list.
+test_expect_success 'using "--no-if-missing" defaults to hardcoded default if nothing configured' '
+ cat complex_message_body >expected &&
+ sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
+ Fixes: Z
+ Acked-by: Z
+ Reviewed-by: Z
+ Signed-off-by: Z
+ AddedTrailer: addme
+ EOF
+ git interpret-trailers --if-missing doNothing --trailer "IgnoredTrailer: ignoreme" --no-if-missing \
+ --trailer "AddedTrailer: addme" <complex_message >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'default "where" is now "after"' '
git config trailer.where "after" &&
test_config trailer.ack.ifExists "add" &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 02/13] trailer test description: this tests --where=after, not --where=before
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>
Signed-off-by: Linus Arver <linusa@google.com>
---
t/t7513-interpret-trailers.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 5b31896070a..ed0fc04bd95 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -792,7 +792,7 @@ test_expect_success 'overriding configuration with "--where after"' '
test_cmp expected actual
'
-test_expect_success 'using "where = before" with "--no-where"' '
+test_expect_success 'using "--where after" with "--no-where"' '
test_config trailer.ack.key "Acked-by= " &&
test_config trailer.ack.where "before" &&
test_config trailer.bug.key "Bug #" &&
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 00/13] Fixes to trailer test script, help text, and documentation
From: Linus Arver via GitGitGadget @ 2023-09-07 22:19 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Linus Arver
In-Reply-To: <pull.1564.v2.git.1691702283.gitgitgadget@gmail.com>
This series contains various fixes to the trailer code. They pertain to
fixes to the test script, the command line help text for the
interpret-trailers builtin, and the documentation.
Patch 1 is the most important as it does cleanups in the tests where we used
'git config' in a test case without cleaning up that state for the next
test. This makes the tests self-contained, making it easier to add new tests
anywhere along the script, without worrying about previously-set implicit
state. These test cleanups exposed lots of cases where the test cases are
mutating more configuration state than is necessary to test the specific
behavior in the test; however such extraneous configurations were not
cleaned up to make these patches easier to review (again, we are not
changing any behavior and we are also not changing what the test cases
themselves purport to do).
Note that Patch 1 was originally a 22-commit series, but was squashed
together to make it easier to see the final diff for each test case. You can
see the 22-commit breakdown at
https://github.com/listx/git/tree/backup-trailer-22-commit-breakdown
Patch 3 adds some tests to check the behavior of '--no-if-exists' and
'--no-if-missing', which weren't previously tested. It also adds
similarly-themed test cases for '--no-where' which only had 1 test case for
it.
The other patches aren't as important, but are included here because I think
they are too small to include in a separate series.
Updates in v3
=============
* Fix t0450 failure due to mismatch between the updated documentation which
uses " or " and the help text of the interpret-trailers command.
Updates in v2
=============
* Many additional patches to fix the help text and docs. No changes to any
of the patches touching the actual tests (that is, Patch 1 and 3 have
stayed the same, other than a rewording of the commit message for Patch
1).
* Of these new patches, I think the last one (about <keyAlias>) is the most
important as it resolves a longtime ambiguity about what a <token> can
be.
Linus Arver (13):
trailer tests: make test cases self-contained
trailer test description: this tests --where=after, not --where=before
trailer: add tests to check defaulting behavior with --no-* flags
trailer doc: narrow down scope of --where and related flags
trailer: trailer location is a place, not an action
trailer --no-divider help: describe usual "---" meaning
trailer --parse help: expose aliased options
trailer --only-input: prefer "configuration variables" over "rules"
trailer --parse docs: add explanation for its usefulness
trailer --unfold help: prefer "reformat" over "join"
trailer doc: emphasize the effect of configuration variables
trailer doc: separator within key suppresses default separator
trailer doc: <token> is a <key> or <keyAlias>, not both
Documentation/git-interpret-trailers.txt | 183 ++++----
builtin/interpret-trailers.c | 12 +-
t/t7513-interpret-trailers.sh | 506 +++++++++++++++++++----
3 files changed, 545 insertions(+), 156 deletions(-)
base-commit: 1b0a5129563ebe720330fdc8f5c6843d27641137
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1564%2Flistx%2Ftrailer-fixes-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1564/listx/trailer-fixes-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1564
Range-diff vs v2:
1: 1623dd000dd = 1: 1623dd000dd trailer tests: make test cases self-contained
2: f680e76de84 = 2: f680e76de84 trailer test description: this tests --where=after, not --where=before
3: 4b5c458ef43 = 3: 4b5c458ef43 trailer: add tests to check defaulting behavior with --no-* flags
4: 0df12c5c2dd = 4: 0df12c5c2dd trailer doc: narrow down scope of --where and related flags
5: 040766861e2 = 5: 040766861e2 trailer: trailer location is a place, not an action
6: 3e58b6f5ea2 = 6: 3e58b6f5ea2 trailer --no-divider help: describe usual "---" meaning
7: d1780a0127a = 7: d1780a0127a trailer --parse help: expose aliased options
8: 5cfff52da8f = 8: 5cfff52da8f trailer --only-input: prefer "configuration variables" over "rules"
9: ef6b77016cd = 9: ef6b77016cd trailer --parse docs: add explanation for its usefulness
10: a08d78618ba = 10: a08d78618ba trailer --unfold help: prefer "reformat" over "join"
11: 4db823ac354 = 11: 4db823ac354 trailer doc: emphasize the effect of configuration variables
12: 66087eaf5bd = 12: 66087eaf5bd trailer doc: separator within key suppresses default separator
13: 7b66cf29d29 ! 13: 0b9525db5a0 trailer doc: <token> is a <key> or <keyAlias>, not both
@@ Documentation/git-interpret-trailers.txt: trailer.<token>.command::
EXAMPLES
--------
+
+ ## builtin/interpret-trailers.c ##
+@@
+
+ static const char * const git_interpret_trailers_usage[] = {
+ N_("git interpret-trailers [--in-place] [--trim-empty]\n"
+- " [(--trailer <token>[(=|:)<value>])...]\n"
++ " [(--trailer (<key>|<keyAlias>)[(=|:)<value>])...]\n"
+ " [--parse] [<file>...]"),
+ NULL
+ };
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH 2/8] builtin/repack.c: extract marking packs for deletion
From: Taylor Blau @ 2023-09-07 22:10 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <20230907075931.GB1260718@coredump.intra.peff.net>
On Thu, Sep 07, 2023 at 03:59:31AM -0400, Jeff King wrote:
> There are really two "factor outs" here: we pull the code from
> cmd_repack() into a helper, and then the helper is also just a thin
> wrapper around its "_1" variant. That latter part isn't needed yet, but
> I can guess from your description that we'll eventually have the main
> function dispatch to the "_1" helper for lists.
Yeah... the "_1" variant looks ugly in isolation in this patch, but I
think makes things cleaner in subsequent patches.
Thanks,
Taylor
^ permalink raw reply
* [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size`
From: Taylor Blau @ 2023-09-07 21:52 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Jonathan Tan
In-Reply-To: <cover.1694123506.git.me@ttaylorr.com>
Cruft packs are an alternative mechanism for storing a collection of
unreachable objects whose mtimes are recent enough to avoid being
pruned out of the repository.
When cruft packs were first introduced back in b757353676
(builtin/pack-objects.c: --cruft without expiration, 2022-05-20) and
a7d493833f (builtin/pack-objects.c: --cruft with expiration,
2022-05-20), the recommended workflow consisted of:
- Repacking periodically, either by packing anything loose in the
repository (via `git repack -d`) or producing a geometric sequence
of packs (via `git repack --geometric=<d> -d`).
- Every so often, splitting the repository into two packs, one cruft
to store the unreachable objects, and another non-cruft pack to
store the reachable objects.
Repositories may (out of band with the above) choose periodically to
prune out some unreachable objects which have aged out of the grace
period by generating a pack with `--cruft-expiration=<approxidate>`.
This allowed repositories to maintain relatively few packs on average,
and quarantine unreachable objects together in a cruft pack, avoiding
the pitfalls of holding unreachable objects as loose while they age out
(for more, see some of the details in 3d89a8c118
(Documentation/technical: add cruft-packs.txt, 2022-05-20)).
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.
Since there is at most one cruft pack in the above scheme, each time we
update the cruft pack it must be rewritten from scratch. Because much of
the pack is reused, this is a relatively inexpensive operation from a
CPU-perspective, but is very costly in terms of I/O since we end up
rewriting basically the same pack (plus any new unreachable objects that
have entered the repository since the last time a cruft pack was
generated).
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.
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.
When pruning unreachable objects, we bypass the new paths which combine
small cruft packs together, and instead start from scratch, passing in
the appropriate `--max-pack-size` down to `pack-objects`, putting it in
charge of keeping the resulting set of cruft packs sized correctly.
This may seem like further I/O churn, but in practice it isn't so bad.
We could prune old cruft packs for whom all or most objects are removed,
and then generate a new cruft pack with just the remaining set of
objects. But this additional complexity buys us relatively little,
because most objects end up being pruned anyway, so the I/O churn is
well contained.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Documentation/config/gc.txt | 6 +
Documentation/git-gc.txt | 7 +
Documentation/git-repack.txt | 9 ++
builtin/gc.c | 8 ++
builtin/repack.c | 133 +++++++++++++++++--
t/t6500-gc.sh | 27 ++++
t/t7704-repack-cruft.sh | 245 +++++++++++++++++++++++++++++++++++
7 files changed, 423 insertions(+), 12 deletions(-)
diff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt
index ca47eb2008..795fcfbb69 100644
--- a/Documentation/config/gc.txt
+++ b/Documentation/config/gc.txt
@@ -86,6 +86,12 @@ gc.cruftPacks::
linkgit:git-repack[1]) instead of as loose objects. The default
is `true`.
+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].
+
gc.pruneExpire::
When 'git gc' is run, it will call 'prune --expire 2.weeks.ago'
(and 'repack --cruft --cruft-expiration 2.weeks.ago' if using
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.
+
--prune=<date>::
Prune loose objects older than date (default is 2 weeks ago,
overridable by the config variable `gc.pruneExpire`).
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`.
+
--expire-to=<dir>::
Write a cruft pack containing pruned objects (if any) to the
directory `<dir>`. This option is useful for keeping a copy of
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;
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);
} else {
strvec_push(&repack, "-A");
if (prune_expire)
@@ -575,6 +580,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
N_("prune unreferenced objects"),
PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire },
OPT_BOOL(0, "cruft", &cruft_packs, N_("pack unreferenced objects separately")),
+ OPT_STRING(0, "cruft-max-size", &cruft_max_size,
+ N_("bytes"),
+ N_("with --cruft, limit the size of new cruft packs")),
OPT_BOOL(0, "aggressive", &aggressive, N_("be more thorough (increased runtime)")),
OPT_BOOL_F(0, "auto", &auto_gc, N_("enable auto-gc mode"),
PARSE_OPT_NOCOMPLETE),
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))
+
static int pack_everything;
static int delta_base_offset = 1;
static int pack_kept_objects = -1;
@@ -113,6 +116,11 @@ static int has_existing_non_kept_packs(const struct existing_packs *existing)
return existing->non_kept_packs.nr || existing->cruft_packs.nr;
}
+static int pack_is_retained(struct string_list_item *item)
+{
+ return (uintptr_t)item->util & RETAIN_PACK;
+}
+
static void mark_packs_for_deletion_1(struct string_list *names,
struct string_list *list)
{
@@ -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;
+ }
}
}
+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")",
+ (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;
+
+ 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));
+ }
+ }
+
+ 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);
+ }
for_each_string_list_item(item, &existing->kept_packs)
fprintf(in, "%s.pack\n", item->string);
fclose(in);
@@ -912,6 +1018,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
PACK_CRUFT),
OPT_STRING(0, "cruft-expiration", &cruft_expiration, N_("approxidate"),
N_("with --cruft, expire objects older than this")),
+ OPT_STRING(0, "cruft-max-size", &cruft_po_args.max_pack_size,
+ N_("bytes"),
+ N_("with --cruft, limit the size of new cruft packs")),
OPT_BOOL('d', NULL, &delete_redundant,
N_("remove redundant packs, and run git-prune-packed")),
OPT_BOOL('f', NULL, &po_args.no_reuse_delta,
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 69509d0c11..5e737c47be 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -303,6 +303,33 @@ test_expect_success 'gc.bigPackThreshold ignores cruft packs' '
)
'
+cruft_max_size_opts="git repack -d -l --cruft --cruft-expiration=2.weeks.ago"
+
+test_expect_success 'setup for --cruft-max-size tests' '
+ git init cruft--max-size &&
+ (
+ cd cruft--max-size &&
+ prepare_cruft_history
+ )
+'
+
+test_expect_success '--cruft-max-size sets appropriate repack options' '
+ GIT_TRACE2_EVENT=$(pwd)/trace2.txt git -C cruft--max-size \
+ gc --cruft --cruft-max-size=1M &&
+ test_subcommand $cruft_max_size_opts --cruft-max-size=1M <trace2.txt
+'
+
+test_expect_success 'gc.cruftMaxSize sets appropriate repack options' '
+ GIT_TRACE2_EVENT=$(pwd)/trace2.txt \
+ git -C cruft--max-size -c gc.cruftMaxSize=2M gc --cruft &&
+ test_subcommand $cruft_max_size_opts --cruft-max-size=2M <trace2.txt &&
+
+ GIT_TRACE2_EVENT=$(pwd)/trace2.txt \
+ git -C cruft--max-size -c gc.cruftMaxSize=2M gc --cruft \
+ --cruft-max-size=3M &&
+ test_subcommand $cruft_max_size_opts --cruft-max-size=3M <trace2.txt
+'
+
run_and_wait_for_auto_gc () {
# We read stdout from gc for the side effect of waiting until the
# background gc process exits, closing its fd 9. Furthermore, the
diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh
index d91fcf1af1..e0de09b77b 100755
--- a/t/t7704-repack-cruft.sh
+++ b/t/t7704-repack-cruft.sh
@@ -5,6 +5,7 @@ test_description='git repack works correctly'
. ./test-lib.sh
objdir=.git/objects
+packdir=$objdir/pack
test_expect_success '--expire-to stores pruned objects (now)' '
git init expire-to-now &&
@@ -127,4 +128,248 @@ test_expect_success '--expire-to stores pruned objects (5.minutes.ago)' '
)
'
+generate_random_blob() {
+ test-tool genrandom "$@" >blob &&
+ git hash-object -w -t blob blob &&
+ rm blob
+}
+
+pack_random_blob () {
+ generate_random_blob "$@" &&
+ git repack -d -q >/dev/null
+}
+
+generate_cruft_pack () {
+ pack_random_blob "$@" >/dev/null &&
+
+ ls $packdir/pack-*.pack | xargs -n 1 basename >in &&
+ pack="$(git pack-objects --cruft $packdir/pack <in)" &&
+ git prune-packed &&
+
+ echo "$packdir/pack-$pack.mtimes"
+}
+
+test_expect_success '--cruft-max-size creates new packs when above threshold' '
+ git init cruft-max-size-large &&
+ (
+ cd cruft-max-size-large &&
+ test_commit base &&
+
+ foo="$(pack_random_blob foo $((1*1024*1024)))" &&
+ git repack --cruft -d &&
+ cruft_foo="$(ls $packdir/pack-*.mtimes)" &&
+
+ bar="$(pack_random_blob bar $((1*1024*1024)))" &&
+ git repack --cruft -d --cruft-max-size=1M &&
+ cruft_bar="$(ls $packdir/pack-*.mtimes | grep -v $cruft_foo)" &&
+
+ test-tool pack-mtimes $(basename "$cruft_foo") >foo.objects &&
+ test-tool pack-mtimes $(basename "$cruft_bar") >bar.objects &&
+
+ grep "^$foo" foo.objects &&
+ test_line_count = 1 foo.objects &&
+ grep "^$bar" bar.objects &&
+ test_line_count = 1 bar.objects
+ )
+'
+
+test_expect_success '--cruft-max-size combines existing packs when below threshold' '
+ git init cruft-max-size-small &&
+ (
+ cd cruft-max-size-small &&
+ test_commit base &&
+
+ foo="$(pack_random_blob foo $((1*1024*1024)))" &&
+ git repack --cruft -d &&
+
+ bar="$(pack_random_blob bar $((1*1024*1024)))" &&
+ git repack --cruft -d --cruft-max-size=10M &&
+
+ cruft=$(ls $packdir/pack-*.mtimes) &&
+ test-tool pack-mtimes $(basename "$cruft") >cruft.objects &&
+
+ grep "^$foo" cruft.objects &&
+ grep "^$bar" cruft.objects &&
+ test_line_count = 2 cruft.objects
+ )
+'
+
+test_expect_success '--cruft-max-size combines smaller packs first' '
+ git init cruft-max-size-consume-small &&
+ (
+ cd cruft-max-size-consume-small &&
+
+ test_commit base &&
+ git repack -ad &&
+
+ cruft_foo="$(generate_cruft_pack foo 524288)" && # 0.5 MiB
+ cruft_bar="$(generate_cruft_pack bar 524288)" && # 0.5 MiB
+ cruft_baz="$(generate_cruft_pack baz 1048576)" && # 1.0 MiB
+ cruft_quux="$(generate_cruft_pack quux 1572864)" && # 1.5 MiB
+
+ test-tool pack-mtimes "$(basename $cruft_foo)" >expect.raw &&
+ test-tool pack-mtimes "$(basename $cruft_bar)" >>expect.raw &&
+ sort expect.raw >expect.objects &&
+
+ # repacking with `--cruft-max-size=2M` should combine
+ # both 0.5 MiB packs together, instead of, say, one of
+ # the 0.5 MiB packs with the 1.0 MiB pack
+ ls $packdir/pack-*.mtimes | sort >cruft.before &&
+ git repack -d --cruft --cruft-max-size=2M &&
+ ls $packdir/pack-*.mtimes | sort >cruft.after &&
+
+ comm -13 cruft.before cruft.after >cruft.new &&
+ comm -23 cruft.before cruft.after >cruft.removed &&
+
+ test_line_count = 1 cruft.new &&
+ test_line_count = 2 cruft.removed &&
+
+ # the two smaller packs should be rolled up first
+ printf "%s\n" $cruft_foo $cruft_bar | sort >expect.removed &&
+ test_cmp expect.removed cruft.removed &&
+
+ # ...and contain the set of objects rolled up
+ test-tool pack-mtimes "$(basename $(cat cruft.new))" >actual.raw &&
+ sort actual.raw >actual.objects &&
+
+ test_cmp expect.objects actual.objects
+ )
+'
+
+test_expect_success 'setup --cruft-max-size with freshened objects' '
+ git init cruft-max-size-freshen &&
+ (
+ cd cruft-max-size-freshen &&
+
+ test_commit base &&
+ git repack -ad &&
+
+ foo="$(generate_random_blob foo 64)" &&
+ test-tool chmtime --get -10000 \
+ "$objdir/$(test_oid_to_path "$foo")" >foo.mtime &&
+
+ git repack --cruft -d &&
+
+ cruft="$(ls $packdir/pack-*.mtimes)" &&
+ test-tool pack-mtimes "$(basename $cruft)" >actual &&
+ echo "$foo $(cat foo.mtime)" >expect &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success '--cruft-max-size with freshened objects (loose)' '
+ (
+ cd cruft-max-size-freshen &&
+
+ # regenerate the object, setting its mtime to be more recent
+ foo="$(generate_random_blob foo 64)" &&
+ test-tool chmtime --get -100 \
+ "$objdir/$(test_oid_to_path "$foo")" >foo.mtime &&
+
+ git repack --cruft -d &&
+
+ cruft="$(ls $packdir/pack-*.mtimes)" &&
+ test-tool pack-mtimes "$(basename $cruft)" >actual &&
+ echo "$foo $(cat foo.mtime)" >expect &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success '--cruft-max-size with freshened objects (packed)' '
+ (
+ cd cruft-max-size-freshen &&
+
+ # regenerate the object and store it in a packfile,
+ # setting its mtime to be more recent
+ #
+ # store it alongside another cruft object so that we
+ # do not create an identical copy of the existing
+ # cruft pack (which contains $foo).
+ foo="$(generate_random_blob foo 64)" &&
+ bar="$(generate_random_blob bar 64)" &&
+ foo_pack="$(printf "%s\n" $foo $bar | git pack-objects $packdir/pack)" &&
+ git prune-packed &&
+
+ test-tool chmtime --get -10 \
+ "$packdir/pack-$foo_pack.pack" >foo.mtime &&
+
+ git repack --cruft -d &&
+
+ cruft="$(ls $packdir/pack-*.mtimes)" &&
+ test-tool pack-mtimes "$(basename $cruft)" >actual &&
+ echo "$foo $(cat foo.mtime)" >expect.raw &&
+ echo "$bar $(cat foo.mtime)" >>expect.raw &&
+ sort expect.raw >expect &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success '--cruft-max-size with pruning' '
+ git init cruft-max-size-prune &&
+ (
+ cd cruft-max-size-prune &&
+
+ test_commit base &&
+ foo="$(generate_random_blob foo $((1024*1024)))" &&
+ bar="$(generate_random_blob bar $((1024*1024)))" &&
+ baz="$(generate_random_blob baz $((1024*1024)))" &&
+
+ test-tool chmtime -10000 "$objdir/$(test_oid_to_path "$foo")" &&
+
+ git repack -d --cruft --cruft-max-size=1M &&
+
+ # backdate the mtimes of all cruft packs to validate
+ # that they were rewritten as a result of pruning
+ ls $packdir/pack-*.mtimes | sort >cruft.before &&
+ for cruft in $(cat cruft.before)
+ do
+ mtime="$(test-tool chmtime --get -10000 "$cruft")" &&
+ echo $cruft $mtime >>mtimes || return 1
+ done &&
+
+ # repack (and prune) with a --cruft-max-size to ensure
+ # that we appropriately split the resulting set of packs
+ git repack -d --cruft --cruft-max-size=1M \
+ --cruft-expiration=10.seconds.ago &&
+ ls $packdir/pack-*.mtimes | sort >cruft.after &&
+
+ for cruft in $(cat cruft.after)
+ do
+ old_mtime="$(grep $cruft mtimes | cut -d" " -f2)" &&
+ new_mtime="$(test-tool chmtime --get $cruft)" &&
+ test $old_mtime -lt $new_mtime || return 1
+ done &&
+
+ test_line_count = 3 cruft.before &&
+ test_line_count = 2 cruft.after &&
+ test_must_fail git cat-file -e $foo &&
+ git cat-file -e $bar &&
+ git cat-file -e $baz
+ )
+'
+
+test_expect_success '--cruft-max-size ignores non-local packs' '
+ repo="cruft-max-size-non-local" &&
+ git init $repo &&
+ (
+ cd $repo &&
+ test_commit base &&
+ generate_random_blob foo 64 &&
+ git repack --cruft -d
+ ) &&
+
+ git clone --reference=$repo $repo $repo-alt &&
+ (
+ cd $repo-alt &&
+
+ test_commit other &&
+ generate_random_blob bar 64 &&
+
+ # ensure that we do not attempt to pick up packs from
+ # the non-alternated repository, which would result in a
+ # crash
+ git repack --cruft --cruft-max-size=1M -d
+ )
+'
+
test_done
--
2.42.0.138.g7e4e42e1aa
^ permalink raw reply related
* [PATCH 1/2] t7700: split cruft-related tests to t7704
From: Taylor Blau @ 2023-09-07 21:52 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Jonathan Tan
In-Reply-To: <cover.1694123506.git.me@ttaylorr.com>
---
t/t7700-repack.sh | 121 -------------------------------------
t/t7704-repack-cruft.sh | 130 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 130 insertions(+), 121 deletions(-)
create mode 100755 t/t7704-repack-cruft.sh
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 27b66807cd..55ee3eb8ae 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -633,125 +633,4 @@ test_expect_success '-n overrides repack.updateServerInfo=true' '
test_server_info_missing
'
-test_expect_success '--expire-to stores pruned objects (now)' '
- git init expire-to-now &&
- (
- cd expire-to-now &&
-
- git branch -M main &&
-
- test_commit base &&
-
- git checkout -b cruft &&
- test_commit --no-tag cruft &&
-
- git rev-list --objects --no-object-names main..cruft >moved.raw &&
- sort moved.raw >moved.want &&
-
- git rev-list --all --objects --no-object-names >expect.raw &&
- sort expect.raw >expect &&
-
- git checkout main &&
- git branch -D cruft &&
- git reflog expire --all --expire=all &&
-
- git init --bare expired.git &&
- git repack -d \
- --cruft --cruft-expiration="now" \
- --expire-to="expired.git/objects/pack/pack" &&
-
- expired="$(ls expired.git/objects/pack/pack-*.idx)" &&
- test_path_is_file "${expired%.idx}.mtimes" &&
-
- # Since the `--cruft-expiration` is "now", the effective
- # behavior is to move _all_ unreachable objects out to
- # the location in `--expire-to`.
- git show-index <$expired >expired.raw &&
- cut -d" " -f2 expired.raw | sort >expired.objects &&
- git rev-list --all --objects --no-object-names \
- >remaining.objects &&
-
- # ...in other words, the combined contents of this
- # repository and expired.git should be the same as the
- # set of objects we started with.
- cat expired.objects remaining.objects | sort >actual &&
- test_cmp expect actual &&
-
- # The "moved" objects (i.e., those in expired.git)
- # should be the same as the cruft objects which were
- # expired in the previous step.
- test_cmp moved.want expired.objects
- )
-'
-
-test_expect_success '--expire-to stores pruned objects (5.minutes.ago)' '
- git init expire-to-5.minutes.ago &&
- (
- cd expire-to-5.minutes.ago &&
-
- git branch -M main &&
-
- test_commit base &&
-
- # Create two classes of unreachable objects, one which
- # is older than 5 minutes (stale), and another which is
- # newer (recent).
- for kind in stale recent
- do
- git checkout -b $kind main &&
- test_commit --no-tag $kind || return 1
- done &&
-
- git rev-list --objects --no-object-names main..stale >in &&
- stale="$(git pack-objects $objdir/pack/pack <in)" &&
- mtime="$(test-tool chmtime --get =-600 $objdir/pack/pack-$stale.pack)" &&
-
- # expect holds the set of objects we expect to find in
- # this repository after repacking
- git rev-list --objects --no-object-names recent >expect.raw &&
- sort expect.raw >expect &&
-
- # moved.want holds the set of objects we expect to find
- # in expired.git
- git rev-list --objects --no-object-names main..stale >out &&
- sort out >moved.want &&
-
- git checkout main &&
- git branch -D stale recent &&
- git reflog expire --all --expire=all &&
- git prune-packed &&
-
- git init --bare expired.git &&
- git repack -d \
- --cruft --cruft-expiration=5.minutes.ago \
- --expire-to="expired.git/objects/pack/pack" &&
-
- # Some of the remaining objects in this repository are
- # unreachable, so use `cat-file --batch-all-objects`
- # instead of `rev-list` to get their names
- git cat-file --batch-all-objects --batch-check="%(objectname)" \
- >remaining.objects &&
- sort remaining.objects >actual &&
- test_cmp expect actual &&
-
- (
- cd expired.git &&
-
- expired="$(ls objects/pack/pack-*.mtimes)" &&
- test-tool pack-mtimes $(basename $expired) >out &&
- cut -d" " -f1 out | sort >../moved.got &&
-
- # Ensure that there are as many objects with the
- # expected mtime as were moved to expired.git.
- #
- # In other words, ensure that the recorded
- # mtimes of any moved objects was written
- # correctly.
- grep " $mtime$" out >matching &&
- test_line_count = $(wc -l <../moved.want) matching
- ) &&
- test_cmp moved.want moved.got
- )
-'
-
test_done
diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh
new file mode 100755
index 0000000000..d91fcf1af1
--- /dev/null
+++ b/t/t7704-repack-cruft.sh
@@ -0,0 +1,130 @@
+#!/bin/sh
+
+test_description='git repack works correctly'
+
+. ./test-lib.sh
+
+objdir=.git/objects
+
+test_expect_success '--expire-to stores pruned objects (now)' '
+ git init expire-to-now &&
+ (
+ cd expire-to-now &&
+
+ git branch -M main &&
+
+ test_commit base &&
+
+ git checkout -b cruft &&
+ test_commit --no-tag cruft &&
+
+ git rev-list --objects --no-object-names main..cruft >moved.raw &&
+ sort moved.raw >moved.want &&
+
+ git rev-list --all --objects --no-object-names >expect.raw &&
+ sort expect.raw >expect &&
+
+ git checkout main &&
+ git branch -D cruft &&
+ git reflog expire --all --expire=all &&
+
+ git init --bare expired.git &&
+ git repack -d \
+ --cruft --cruft-expiration="now" \
+ --expire-to="expired.git/objects/pack/pack" &&
+
+ expired="$(ls expired.git/objects/pack/pack-*.idx)" &&
+ test_path_is_file "${expired%.idx}.mtimes" &&
+
+ # Since the `--cruft-expiration` is "now", the effective
+ # behavior is to move _all_ unreachable objects out to
+ # the location in `--expire-to`.
+ git show-index <$expired >expired.raw &&
+ cut -d" " -f2 expired.raw | sort >expired.objects &&
+ git rev-list --all --objects --no-object-names \
+ >remaining.objects &&
+
+ # ...in other words, the combined contents of this
+ # repository and expired.git should be the same as the
+ # set of objects we started with.
+ cat expired.objects remaining.objects | sort >actual &&
+ test_cmp expect actual &&
+
+ # The "moved" objects (i.e., those in expired.git)
+ # should be the same as the cruft objects which were
+ # expired in the previous step.
+ test_cmp moved.want expired.objects
+ )
+'
+
+test_expect_success '--expire-to stores pruned objects (5.minutes.ago)' '
+ git init expire-to-5.minutes.ago &&
+ (
+ cd expire-to-5.minutes.ago &&
+
+ git branch -M main &&
+
+ test_commit base &&
+
+ # Create two classes of unreachable objects, one which
+ # is older than 5 minutes (stale), and another which is
+ # newer (recent).
+ for kind in stale recent
+ do
+ git checkout -b $kind main &&
+ test_commit --no-tag $kind || return 1
+ done &&
+
+ git rev-list --objects --no-object-names main..stale >in &&
+ stale="$(git pack-objects $objdir/pack/pack <in)" &&
+ mtime="$(test-tool chmtime --get =-600 $objdir/pack/pack-$stale.pack)" &&
+
+ # expect holds the set of objects we expect to find in
+ # this repository after repacking
+ git rev-list --objects --no-object-names recent >expect.raw &&
+ sort expect.raw >expect &&
+
+ # moved.want holds the set of objects we expect to find
+ # in expired.git
+ git rev-list --objects --no-object-names main..stale >out &&
+ sort out >moved.want &&
+
+ git checkout main &&
+ git branch -D stale recent &&
+ git reflog expire --all --expire=all &&
+ git prune-packed &&
+
+ git init --bare expired.git &&
+ git repack -d \
+ --cruft --cruft-expiration=5.minutes.ago \
+ --expire-to="expired.git/objects/pack/pack" &&
+
+ # Some of the remaining objects in this repository are
+ # unreachable, so use `cat-file --batch-all-objects`
+ # instead of `rev-list` to get their names
+ git cat-file --batch-all-objects --batch-check="%(objectname)" \
+ >remaining.objects &&
+ sort remaining.objects >actual &&
+ test_cmp expect actual &&
+
+ (
+ cd expired.git &&
+
+ expired="$(ls objects/pack/pack-*.mtimes)" &&
+ test-tool pack-mtimes $(basename $expired) >out &&
+ cut -d" " -f1 out | sort >../moved.got &&
+
+ # Ensure that there are as many objects with the
+ # expected mtime as were moved to expired.git.
+ #
+ # In other words, ensure that the recorded
+ # mtimes of any moved objects was written
+ # correctly.
+ grep " $mtime$" out >matching &&
+ test_line_count = $(wc -l <../moved.want) matching
+ ) &&
+ test_cmp moved.want moved.got
+ )
+'
+
+test_done
--
2.42.0.138.g7e4e42e1aa
^ permalink raw reply related
* [PATCH 0/2] repack: implement `--cruft-max-size`
From: Taylor Blau @ 2023-09-07 21:51 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Jonathan Tan
(These patches should be applied on top of a merge with
tb/repack-existing-packs-cleanup, and tb/multi-cruft-pack).
This series attempts to give users some more robust tools for managing
repositories with a large number of unreachable objects by storing them
in separate cruft packs, via a new option `--cruft-max-size`, like so:
$ git.compile repack -d --cruft --max-pack-size=10M
[...]
Enumerating cruft objects: 617483, done.
Counting objects: 100% (83791/83791), done.
Delta compression using up to 20 threads
Compressing objects: 100% (59696/59696), done.
Writing objects: 100% (83791/83791), done.
Total 83791 (delta 19251), reused 82502 (delta 19148), pack-reused 0
$ ls -la .git/objects/pack/pack-*.mtimes
-r--r--r-- 1 ttaylorr ttaylorr 179144 Sep 7 17:46 .git/objects/pack/pack-1a95260d26f2897abfd2d54f1d58f535acb81d23.mtimes
-r--r--r-- 1 ttaylorr ttaylorr 452 Sep 7 17:46 .git/objects/pack/pack-5fde8701ae0f2e5553f1fa33de05faf12f94c07f.mtimes
-r--r--r-- 1 ttaylorr ttaylorr 155720 Sep 7 17:46 .git/objects/pack/pack-91f9e66921e0ebe1b5e35d34842551468cecdc28.mtimes
-r--r--r-- 1 ttaylorr ttaylorr 56 Sep 7 17:46 .git/objects/pack/pack-95fe626743207b177b45f32b60fdc313e525ea60.mtimes
The details are explained in the second patch, but the gist is that we
will combine cruft packs up until they reach a certain threshold (as
specified by `--cruft-max-size`) and then begin a new "generation" of
cruft packs. That younger generation will grow up until it reaches the
configured threshold, at which point it will become "frozen" and then
any new unreachable objects will be written into a new generation of
cruft packs.
The goal of this series is to reduce I/O churn in repositories that
either (a) have a large number of unreachable objects, (b) rarely prune
them, or (c) both.
Instead of having to rewrite a cruft pack containing every unreachable
object in the repository, we only have to rewrite a cruft pack up until
it reaches the given threshold, at which point it is effectively kept
(i.e., it behaves as if the cruft pack had a ".keep" file tied to it,
provided that the threshold is held constant).
Thanks in advance for your review!
Taylor Blau (2):
t7700: split cruft-related tests to t7704
builtin/repack.c: implement support for `--cruft-max-size`
Documentation/config/gc.txt | 6 +
Documentation/git-gc.txt | 7 +
Documentation/git-repack.txt | 9 +
builtin/gc.c | 8 +
builtin/repack.c | 133 +++++++++++--
t/t6500-gc.sh | 27 +++
t/t7700-repack.sh | 121 -----------
t/t7704-repack-cruft.sh | 375 +++++++++++++++++++++++++++++++++++
8 files changed, 553 insertions(+), 133 deletions(-)
create mode 100755 t/t7704-repack-cruft.sh
--
2.42.0.138.g7e4e42e1aa
^ permalink raw reply
* Re: [PATCH v3 2/2] doc: revert: add discussion
From: Linus Arver @ 2023-09-07 21:29 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git, Junio C Hamano
In-Reply-To: <ZNcyhUL89WVXOv3F@ugly>
First, I apologize for the long delay in my response. I only work on Git
20% of the time, and that 20% can become 0% due to factors outside my
control.
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> On Fri, Aug 11, 2023 at 04:00:53PM -0700, Linus Arver wrote:
>>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>
>>> On Thu, Aug 10, 2023 at 02:50:59PM -0700, Linus Arver wrote:
>>>>Nit: the "doc: revert: add discussion" subject line should probably be more
>>>>like "revert doc: suggest adding the 'why' behind reverts".
>>>>
>>> this is counter to the prevalent "big endian" prefix style, and is in
>>> this case really easy to misread.
>>
>>I also learned recently that there should just be one colon ":" in the
>>subject, which is why I suggested "revert doc" as the prefix instead of
>>"doc: revert: ...".
>>
> in what context was this preference expressed?
IIRC, it was from a conversation off-list with the folks at Google's
Git-core team.
> because here, it's rather counter-productive: most commands are verbs
> for obvious reasons, so using that style sets the reader up for
> misparsing the subject on first try.
I think the convention for commit titles is
<prefix>: <action>
so the phrase "revert doc: add discussion", where the <prefix> is
"revert doc" does not parse any worse than "doc: revert: add
discussion". That is, the <prefix> is never confused with the <action>
(they are separated by the colon).
> this could be avoided by quoting
> the command, but that looks noisy in the subject.
> so rather, i'd follow another precedent, 'git-revert.txt: ', which is
> unambiguous.
SGTM.
>>> i also intentionally kept the subject generic, because the content
>>> covers two matters (the reasoning and the subjects, which is also the
>>> reason why this is a separate patch to start with).
>>
>>I think the phrase "add discussion" in "doc: revert: add discussion"
>>doesn't add much value, because your patch's diff is very easy to read
>>(in that it adds a new DISCUSSION section). I just wanted to replace it
>>with something more useful that gives more information than
>
>>just repeat
>>(somewhat redundantly) what is obvious by looking at the patch.
>>
> but ... that's exactly what a subject is supposed to do!
I think the rule of thumb is to explain the goodness of what a commit
brings, rather than focus on what is literally happening. This is
because the former is more valuable. So instead of
"git-revert.txt: add discussion"
you could say
"git-revert.txt: advise against default commit message"
and now you don't have to look at the patch to see (roughly) what kind
of discussion was added.
>>>>Please consider rewording such
>>>> subject lines to reflect the reason why the original commit is being
>>>> reapplied again.
>>>>
>>> the reasoning most likely wouldn't fit into the subject.
>>
>>Hence the language "to _reflect_ the reason", because the "reason"
>>should belong in the commit message body text.
>>
> i don't think that's how most people would actually read this.
> and i still don't see how that instruction could be meaningfully
> followed.
OK, you may be right.
>>> also, the original request to explain the reasoning applies
>>> transitively, so i don't think it's really necessary to point it out
>>> explicitly.
>>
>>It may be that a user will think only giving the revert reason in the
>>body text is enough, while leaving the subject line as is. I wanted to
>>break this line of thinking by providing additional instructions.
>>
> yes, that's the whole intention of this patch. but i don't see how
> making it more convoluted than my proposal helps in any way.
Well, even if a review makes something more convoluted, it may generate
discussion and drive consensus on the better way(s) of doing something.
I see value in that course of events.
Of course you are free to reject review comments that you truly believe
are inferior to the approach you've already taken.
But overall, when I see a reviewer's comment on this mailing list, I
assume they are trying to make my patch better. Similarly when I
reviewed your patch my intent was to provide actionable feedback to try
to make it better. I'm sorry if I did not come across that way.
>>This is definitely better. But others in this thread have already
>>commented that my version looks good (after seeing your version also,
>>presumably).
>>
> well, i'm also an "others" when it comes to your proposal, and i find it
> confusing.
I think you did the right thing by responding to my comments, and
pointing to things you found confusing.
^ permalink raw reply
* Re: [PATCH] rebase -i: ignore signals when forking subprocesses
From: Junio C Hamano @ 2023-09-07 21:16 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget
Cc: git, Johannes Schindelin, Jeff King, Phillip Wood
In-Reply-To: <pull.1581.git.1694080982621.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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.
The above made me wonder if we can guarantee that the intention of
the end-user is always to kill only the external program and not
"git". But with or without this change, "git" will stop making
progress after such a signal (in other words, it is not like killing
"exec sleep 20" will make "git rebase -i -x 'sleep 20'" to move to
the next step without complaining), so "ignore signals" is not all
that harmful as the phrasing makes it sound like. With the patch,
we just handle signals that will kill the external programs, and
their consequences, a bit more gracefully.
But that makes me wonder what happens if the external program has
good reasons to ignore the signal (that is, the program does not die
when signaled, without misbehaving). If "git" dies in such a case,
would it help the overall end-user experience, or would it even
hurt? If the latter, then "git" that ignores these interactive
interrupts would be improvement in both cases (i.e. external
programs that dies with signals, and those that ignores them). If
the former, however, "git" that ignores the signals would be a
regression.
Other than that, the change is well reasoned, I would think.
Thanks.
^ permalink raw reply
* Re: [PATCH] rebase -i: ignore signals when forking subprocesses
From: Jeff King @ 2023-09-07 21:06 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget; +Cc: git, Johannes Schindelin, Phillip Wood
In-Reply-To: <pull.1581.git.1694080982621.gitgitgadget@gmail.com>
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).
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!).
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. 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");
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.
-Peff
^ permalink raw reply
* Re: [PATCH v3 11/15] replay: use standard revision ranges
From: Dragan Simic @ 2023-09-07 21:02 UTC (permalink / raw)
To: Christian Couder
Cc: Toon Claes, git, Junio C Hamano, Patrick Steinhardt,
Johannes Schindelin, Elijah Newren, John Cai, Derrick Stolee,
Phillip Wood, Felipe Contreras, Calvin Wan, Christian Couder
In-Reply-To: <CAP8UFD1_67rNke1y-GV7qX46GXQkNJzw7fXfK=kkUjc-jb5S_g@mail.gmail.com>
On 2023-09-07 10:32, Christian Couder wrote:
> On Thu, Jun 22, 2023 at 12:03 PM Toon Claes <toon@iotcl.com> wrote:
>>
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>> > +DESCRIPTION
>> > +-----------
>> > +
>> > +Takes a range of commits, and replays them onto a new location. Does
>> > +not touch the working tree or index, and does not update any
>> > +references. However, the output of this command is meant to be used
>>
>> Small suggestion here:
>>
>> Takes a range of commits, and replays them onto a new location. Does
>> neither touch the working tree nor index, and does not update any
>> references.
>
> I am not a native speaker, so I am not sure what's best here. I find
> your suggestion a bit less clear though, so until a native speaker
> agrees with it or maybe finds something even better, I prefer to leave
> it as-is.
I'm also not a native English speaker, but I spent about 2.5 years
contributing a whole lot to English Wikipedia, so I'd dare to say I've
honed my English skills rather well. Thus, here's my take on this:
Takes a range of commits and replays them onto a new location.
Leaves the working tree and the index untouched, and updates no
references. The output of this command is to be used...
This is written in a concise yet slightly imperative way, which should
be suitable for the purpose. I hope you agree.
^ permalink raw reply
* Re: [PATCH v4 02/15] replay: introduce new builtin
From: Johannes Schindelin @ 2023-09-07 10:23 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Patrick Steinhardt, Elijah Newren, John Cai,
Derrick Stolee, Phillip Wood, Calvin Wan, Toon Claes,
Christian Couder
In-Reply-To: <20230907092521.733746-3-christian.couder@gmail.com>
Hi Christian & Elijah,
On Thu, 7 Sep 2023, Christian Couder wrote:
> diff --git a/command-list.txt b/command-list.txt
> index 54b2a50f5f..d74836ab21 100644
> --- a/command-list.txt
> +++ b/command-list.txt
> @@ -160,6 +160,7 @@ git-reflog ancillarymanipulators complete
> git-remote ancillarymanipulators complete
> git-repack ancillarymanipulators complete
> git-replace ancillarymanipulators complete
> +git-replay mainporcelain history
I recall this having come up before, but in light of the still active
discussion revolving around command-line parameter validation, I would
strongly advise removing `git replay` from the main page and instead go
for plumbingmanipulators.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth
From: Jeff King @ 2023-09-07 20:40 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <724641d2-1cba-3768-6008-01e8a1cdca4e@web.de>
On Thu, Sep 07, 2023 at 10:20:53PM +0200, René Scharfe wrote:
> Am 05.09.23 um 09:21 schrieb Jeff King:
> > On Sat, Sep 02, 2023 at 08:54:54PM +0200, René Scharfe wrote:
> >
> > In general, I wonder how many of the results from:
> >
> > git grep '{ OPTION'
> >
> > could be converted to use the macros and end up more readable. There are
> > a number of OPTARG ones, which I guess can't use macros. Looks like
> > there are a handful of others (mostly for OPT_HIDDEN).
>
> Indeed, and I have a semantic patch for that, but mostly because the
> macros allow injecting a type check.
>
> OPTARG would need a new macro to allow specifying the default value. Or
> is there a variadic macro trick that we could use?
Hmm, I had just assumed OPTARG was a lost cause (or we would need an
"OPTARG" variant of each macro; yuck).
I suspect variadic macros could be made to work, but you'd lose some
compile-time safety. If I say:
OPT_BOOL('x', NULL, &v, NULL, "turn on x")
now, the compiler will complain about the number of arguments. In a
variadic world, it silently ignores the final one. I feel like I've made
this kind of error before (e.g., when switching to/from _F variants, or
between types).
You'd want some semantic check between what's in flags (i.e., is the
OPTARG flag set), but I think that's beyond what the compiler itself can
do (you could probably write a coccinelle rule for it, though).
I think it also squats on the variadic concept for the macro, so that no
other features can use it. I.e., if you accidentally give _two_ extra
arguments, I don't know that we can parse them out individually.
So yeah, I think you'd really want a separate macro. The combinations
start to add up (or multiply up, if you prefer ;) ). They _could_ be
generated mechanically, I think, as they can all be implemented in terms
of a master macro that knows about all features:
#define OPT_BOOL_F(s, l, v, h, f) OPT_BOOL_ALL(s, l, v, h, f, 0)
#define OPT_BOOL(s, l, v, h, f) OPT_BOOL_F(s, l, v, h, 0)
#define OPT_BOOL_OPTARG_F(s, l, v, h, arg) OPT_BOOL_ALL(s, l, v, h, f | PARSE_OPT_OPTARG, arg)
#define OPT_BOOL_OPTARG(s, l, v, h, arg) OPT_BOOL_OPTARG_F(s, l, v, h, 0, arg)
But I'm not sure if cpp is up to that, or if I'd want to see what the
resulting code looks like.
-Peff
^ permalink raw reply
* Re: [PATCH] grep: reject --no-or
From: Junio C Hamano @ 2023-09-07 20:34 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <6aeb0ebe-0fea-ccd3-089a-ee0b5b5baf10@web.de>
René Scharfe <l.s.r@web.de> writes:
> Since 3e230fa1b2 (grep: use parseopt, 2009-05-07) git grep has been
> accepting the option --no-or. It does the same as --or: nothing.
> That's confusing and unintended. Forbid negating --or.
OK. It is just a no-op Boolean but that is not a good reason for us
to be loose. Will queue. Thanks.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> builtin/grep.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 50e712a184..2a261074f1 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -990,7 +990,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> OPT_CALLBACK_F(0, "and", &opt, NULL,
> N_("combine patterns specified with -e"),
> PARSE_OPT_NOARG | PARSE_OPT_NONEG, and_callback),
> - OPT_BOOL(0, "or", &dummy, ""),
> + OPT_BOOL_F(0, "or", &dummy, "", PARSE_OPT_NONEG),
> OPT_CALLBACK_F(0, "not", &opt, NULL, "",
> PARSE_OPT_NOARG | PARSE_OPT_NONEG, not_callback),
> OPT_CALLBACK_F('(', NULL, &opt, NULL, "",
> --
> 2.42.0
^ permalink raw reply
* Re: [PATCH v4 0/7] rebase -i: impove handling of failed commands
From: Junio C Hamano @ 2023-09-07 20:33 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Phillip Wood via GitGitGadget, git, Stefan Haller, Phillip Wood,
Eric Sunshine, Glen Choo, Phillip Wood
In-Reply-To: <6b927687-cf6e-d73e-78fb-bd4f46736928@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> ...
>> 6: 2ed7cbe5fff = 6: 0ca5fccca17 rebase --continue: refuse to commit after failed command
>> 7: bbe0afde512 = 7: 8d5f6d51e19 rebase -i: fix adding failed command to the todo list
>
> Thank you for indulging me. This iteration looks good to me!
Thanks, all. Let's merge it down to 'next'.
^ permalink raw reply
* Re: [PATCH v4 0/7] rebase -i: impove handling of failed commands
From: Johannes Schindelin @ 2023-09-07 9:56 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget
Cc: git, Junio C Hamano, Stefan Haller, Phillip Wood, Eric Sunshine,
Glen Choo, Phillip Wood
In-Reply-To: <pull.1492.v4.git.1694013771.gitgitgadget@gmail.com>
Hi Phillip,
On Wed, 6 Sep 2023, Phillip Wood via GitGitGadget wrote:
> Range-diff vs v3:
>
> 1: 1ab1ad2ef07 ! 1: ae4f873b3d0 rebase -i: move unlink() calls
> @@ Metadata
> ## Commit message ##
> rebase -i: move unlink() calls
>
> - At the start of each iteration the loop that picks commits removes
> - state files from the previous pick. However some of these are only
> - written if there are conflicts and so we break out of the loop after
> - writing them. Therefore they only need to be removed when the rebase
> - continues, not in each iteration.
> + At the start of each iteration the loop that picks commits removes the
> + state files from the previous pick. However some of these files are only
> + written if there are conflicts in which case we exit the loop before the
> + end of the loop body. Therefore they only need to be removed when the
> + rebase continues, not at the start of each iteration.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 2: e2a758eb4a5 ! 2: f540ed1d607 rebase -i: remove patch file after conflict resolution
> @@ Commit message
> now used in two different places rebase_path_patch() is added and used
> to obtain the path for the patch.
>
> + To construct the path write_patch() previously used get_dir() which
> + returns different paths depending on whether we're rebasing or
> + cherry-picking/reverting. As this function is only called when
> + rebasing it is safe to use a hard coded string for the directory
> + instead. An assertion is added to make sure we don't starting calling
> + this function when cherry-picking in the future.
> +
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> ## sequencer.c ##
> @@ sequencer.c: static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
> * For the post-rewrite hook, we make a list of rewritten commits and
> * their new sha1s. The rewritten-pending list keeps the sha1s of
> @@ sequencer.c: static int make_patch(struct repository *r,
> + char hex[GIT_MAX_HEXSZ + 1];
> + int res = 0;
> +
> ++ if (!is_rebase_i(opts))
> ++ BUG("make_patch should only be called when rebasing");
> ++
> + oid_to_hex_r(hex, &commit->object.oid);
> + if (write_message(hex, strlen(hex), rebase_path_stopped_sha(), 1) < 0)
> return -1;
> res |= write_rebase_head(&commit->object.oid);
>
> 3: 8f6c0e40567 ! 3: 818bdaf772d sequencer: use rebase_path_message()
> @@ Commit message
> made function to get the path name instead. This was the last
> remaining use of the strbuf so remove it as well.
>
> + As with the previous patch we now use a hard coded string rather than
> + git_dir() when constructing the path. This is safe for the same
> + reason (make_patch() is only called when rebasing) and is protected by
> + the assertion added in the previous patch.
> +
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> ## sequencer.c ##
> 4: a1fad70f4b9 = 4: bd67765a864 sequencer: factor out part of pick_commits()
> 5: df401945866 ! 5: f6f330f7063 rebase: fix rewritten list for failed pick
> @@ Commit message
> disabled the user will see the messages from the merge machinery
> detailing the problem.
>
> - To simplify writing REBASE_HEAD in this case pick_one_commit() is
> - modified to avoid duplicating the code that adds the failed command
> - back into the todo list.
> + The code to add a failed command back into the todo list is duplicated
> + between pick_one_commit() and the loop in pick_commits(). Both sites
> + print advice about the command being rescheduled, decrement the current
> + item and save the todo list. To avoid duplicating this code
> + pick_one_commit() is modified to set a flag to indicate that the command
> + should be rescheduled in the main loop. This simplifies things as only
> + the remaining copy of the code needs to be modified to set REBASE_HEAD
> + rather than calling error_with_patch().
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 6: 2ed7cbe5fff = 6: 0ca5fccca17 rebase --continue: refuse to commit after failed command
> 7: bbe0afde512 = 7: 8d5f6d51e19 rebase -i: fix adding failed command to the todo list
Thank you for indulging me. This iteration looks good to me!
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v2 10/10] parse-options: mark unused parameters in noop callback
From: René Scharfe @ 2023-09-07 20:20 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20230905070948.GD199565@coredump.intra.peff.net>
Am 05.09.23 um 09:09 schrieb Jeff King:
>
> Your patch looks correct to me. I probably wouldn't have bothered to
> write it, but now you did. :) My inclination would be to go with my
> dumb-simple one for this series,
Fair enough.
> and it looks like you may have
> collected a few slight-more-risky-but-maybe-worthwhile parseopt cleanups
> on top that could make their own series.
Well, my aim is more to simplify and improve type safety, but I get
carried away a bit at times.
René
^ permalink raw reply
* [PATCH] grep: reject --no-or
From: René Scharfe @ 2023-09-07 20:20 UTC (permalink / raw)
To: Git List
Since 3e230fa1b2 (grep: use parseopt, 2009-05-07) git grep has been
accepting the option --no-or. It does the same as --or: nothing.
That's confusing and unintended. Forbid negating --or.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
builtin/grep.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 50e712a184..2a261074f1 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -990,7 +990,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
OPT_CALLBACK_F(0, "and", &opt, NULL,
N_("combine patterns specified with -e"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, and_callback),
- OPT_BOOL(0, "or", &dummy, ""),
+ OPT_BOOL_F(0, "or", &dummy, "", PARSE_OPT_NONEG),
OPT_CALLBACK_F(0, "not", &opt, NULL, "",
PARSE_OPT_NOARG | PARSE_OPT_NONEG, not_callback),
OPT_CALLBACK_F('(', NULL, &opt, NULL, "",
--
2.42.0
^ permalink raw reply related
* Re: [PATCH] name-rev: use OPT_HIDDEN_BOOL for --peel-tag
From: René Scharfe @ 2023-09-07 20:20 UTC (permalink / raw)
To: Jeff King; +Cc: Git List
In-Reply-To: <20230905071719.GF199565@coredump.intra.peff.net>
Am 05.09.23 um 09:17 schrieb Jeff King:
> On Sat, Sep 02, 2023 at 08:38:34PM +0200, René Scharfe wrote:
>
> OPT_HIDDEN_BOOL() itself is a little funny to me. I guess back then we
> did not have the "_F" variants, but really it is just:
>
> OPT_BOOL_F(0, "peel-tag", &peel_tag,
> N_("dereference tags in the input (internal use)")),
> PARSE_OPT_HIDDEN);
>
> which would remove one more special case (after all, being hidden is
> orthogonal to the type).
Good point!
> But there are enough of them that maybe having
> a special name for this is worth it. I dunno.
A special case just for a flag feels wasteful. _F is more general at a
reasonable price of some verbosity:
OPT_HIDDEN_BOOL(s, l, v, h),
OPT_BOOL_F(s, l, v, h, PARSE_OPT_HIDDEN),
Hindsight..
> But we could probably simplify the definition, at least. :)
Sure, that would be some kind of consolation price.
René
^ permalink raw reply
* Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth
From: René Scharfe @ 2023-09-07 20:20 UTC (permalink / raw)
To: Jeff King; +Cc: Git List
In-Reply-To: <20230905072122.GG199565@coredump.intra.peff.net>
Am 05.09.23 um 09:21 schrieb Jeff King:
> On Sat, Sep 02, 2023 at 08:54:54PM +0200, René Scharfe wrote:
>
> In general, I wonder how many of the results from:
>
> git grep '{ OPTION'
>
> could be converted to use the macros and end up more readable. There are
> a number of OPTARG ones, which I guess can't use macros. Looks like
> there are a handful of others (mostly for OPT_HIDDEN).
Indeed, and I have a semantic patch for that, but mostly because the
macros allow injecting a type check.
OPTARG would need a new macro to allow specifying the default value. Or
is there a variadic macro trick that we could use?
René
^ permalink raw reply
* Re: [PATCH] completion: commit: complete configured trailer tokens
From: Junio C Hamano @ 2023-09-07 20:20 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget; +Cc: git, ZheNing Hu, Philippe Blain
In-Reply-To: <pull.1583.git.1694108551683.gitgitgadget@gmail.com>
"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> Since 2daae3d1d1 (commit: add --trailer option, 2021-03-23), 'git
> commit' can add trailers to commit messages. To make that feature more
> pleasant to use at the command line, update the Bash completion code to
> offer configured trailer tokens.
>
> Add a __git_trailer_tokens function to list the configured trailers
> tokens, and use it in _git_commit to suggest the configured tokens,
> suffixing the completion words with ':' so that the user only has to add
> the trailer value.
Nice attention to the details.
I do not use custom trailers myself, but I can see how this will be
useful. The choice of the source of the information (i.e. the
configuration variables trailer.*.key) sounds sensible, too.
Will queue. Thanks.
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> completion: commit: complete configured trailer tokens
>
> Since 2daae3d1d1 (commit: add --trailer option, 2021-03-23), 'git
> commit' can add trailers to commit messages. To make that feature more
> pleasant to use at the command line, update the Bash completion code to
> offer configured trailer tokens.
>
> Add a __git_trailer_tokens function to list the configured trailers
> tokens, and use it in _git_commit to suggest the configured tokens,
> suffixing the completion words with ':' so that the user only has to add
> the trailer value.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1583%2Fphil-blain%2Fcompletion-commit-trailers-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1583/phil-blain/completion-commit-trailers-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1583
>
> contrib/completion/git-completion.bash | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 133ec92bfae..b5eb75aadc5 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1677,6 +1677,11 @@ _git_clone ()
>
> __git_untracked_file_modes="all no normal"
>
> +__git_trailer_tokens ()
> +{
> + git config --name-only --get-regexp trailer.\*.key | awk -F. '{print $2}'
> +}
> +
> _git_commit ()
> {
> case "$prev" in
> @@ -1701,6 +1706,10 @@ _git_commit ()
> __gitcomp "$__git_untracked_file_modes" "" "${cur##--untracked-files=}"
> return
> ;;
> + --trailer=*)
> + __gitcomp_nl "$(__git_trailer_tokens)" "" "${cur##--trailer=}" ":"
> + return
> + ;;
> --*)
> __gitcomp_builtin commit
> return
>
> base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
^ permalink raw reply
* Re: Is "bare"ness in the context of multiple worktrees weird? Bitmap error in git gc.
From: Kristoffer Haugsbakk @ 2023-09-07 20:11 UTC (permalink / raw)
To: Tao Klerks
Cc: Sergey Organov, Eric Sunshine, Johannes Schindelin, Taylor Blau,
Patrick Steinhardt, git, Junio C Hamano
In-Reply-To: <CAPMMpojTLswqubRk0Ly3RQqkrnpx_9Hiu_TRK1=ASPbPNz4ApQ@mail.gmail.com>
Hi again Tao
On Thu, Sep 7, 2023, at 06:53, Tao Klerks wrote:
> I've definitely changed my mind about "inline", I agree "main" is
> better. I'm not convinced it's the best word we could come up with,
> but if it's well-established, I'm happy with it.
Note that the conversation was forked:
1. New nomenclature to describe things more precisely
2. How good those words in themselves are at describing these things
I liked “inline” since it helped to clarify the case of a bare repository
with linked worktrees. Sure, emphasizing that it has no “inline worktree”
might be redundant (it can be inferred, perhaps), it's nice to be able to
emphasize things for pedagogical purposes.
(I'm less sure if “attached” is needed.)
This is what we can say for certain about a repository:
• There definitely is a repository somewhere (maybe not in whatever
worktree you are in right now though)
• It has zero or more worktrees
So “main worktree” is optional. And that optionality makes things awkward
since it also used to describe where the repository lives—even when the
repository has no *inline worktree* (it is bare).
The bottom line is that it's nice if one can avoid having to get into
situations like this made-up conversation:
A: — I'm in the deployment worktree now. Where's the main
worktree in our workflow? [I don't know how to use `git worktree`]
B: — That's `repository.git`.
A: — Okay nice. Is that worktree used for the mainline development?
B: — No, it has no worktree. It's bare.
A: — What? But didn't you say that it was the main worktree?
B: — Yes, in the sense that it's where the repository is. But it has no
worktree itself.
>> We can read that (1) a non-bare repository itself is considered
>> its "main worktree", (2) a bare repository, by inference, has no
>> main worktree (otherwise we wouldn't have said "if it's not"), and
>> (3) both bare and non-bare repositories can have linked worktrees
>> (again, otherwise we wouldn't have brought up a bare repository in
>> the description).
>>
>> Perhaps we should borrow it to update the glossary, like so?
>>
>
> Looks good to me, but that leaves me with a different nitpick: we say
> 'One "worktree" consists of a "working tree" and repository metadata,
> most of which are shared among other worktrees of a single repository,
> and some of which are maintained separately per worktree'
>
> This claims that the *shared metadata* (presumably the refs, the
> branch reflogs, the objects, the config, etc) are *part of the
> worktree* (a worktree "consists of" them and other things). That seems
> like a very strange way to conceive of things, to me.
>
> I would find it reasonable to state that the main worktree is part of
> the repo - certainly that's now most everyday users would think of it,
> if they were made to think of the worktree concept at all - but not
> that the shared repo metadata is part of the worktree, and especially
> not that the shared repo metadata is part of the attached worktrees.
Without getting into the subtle distinctions between is-a and has-a: I
think it could make sense to think of this in terms of which one needs the
other one. A worktree needs a repository, so one could say that a worktree
“consists of” that. A repository on the other hand doesn't need to have
any worktrees.
(But the vice-versa also makes sense.)
> I imagine that this weird phrasing intends to allude to the fact that
> a worktree is "broken" without the repository metadata folder that
> contains both its worktree-specific metadata and the shared metadata
> that it depends just as much on... but can we come up with better
> "relationship words here?
I don't see why one needs to phrase or define things in terms of what
would make it corrupted or not-that-thing any more. Like, a “working tree”
without a Git repository is just a directory tree—it's got nothing to do
with Git whatsoever.
> Sorry to continue nitpicking - I would love to see a clear
> nomenclature and description of these parts and their relationships
> for people (with less git experience) to "get it" more easily.
As a Git user I think this is a very productive topic.
Cheers
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH 0/8] repack: refactor pack snapshot-ing logic
From: Jeff King @ 2023-09-07 9:00 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <cover.1693946195.git.me@ttaylorr.com>
On Tue, Sep 05, 2023 at 04:36:37PM -0400, Taylor Blau wrote:
> This series refactors some of the 'git repack' internals to keep track
> of existing packs in a set of string lists stored in a single structure,
> instead of passing around multiple string lists to different functions
> throughout the various paths.
>
> The result is that the interface around pack deletion, marking packs as
> redundant, and handling the set of pre-existing packs (both kept and
> non-kept) is significantly cleaner without introducing any functional
> changes.
>
> I didn't mean to produce so much churn when I started writing these
> patches, which began as a simple effort to rename a couple of variables
> for more consistency.
These all look pretty reasonable to me. I wasn't quite sure about patch
7 and left some comments there, but all of the options are kind of ugly.
Since it's limited to that one file, it may not be worth spending too
much time or brain-power on trying to polish it further.
-Peff
^ permalink raw reply
* Re: [PATCH 7/8] builtin/repack.c: drop `DELETE_PACK` macro
From: Jeff King @ 2023-09-07 8:58 UTC (permalink / raw)
To: Taylor Blau; +Cc: Junio C Hamano, git, Patrick Steinhardt
In-Reply-To: <ZPi1c98o2fKB/U+e@nand.local>
On Wed, Sep 06, 2023 at 01:22:59PM -0400, Taylor Blau wrote:
> --- 8< ---
> Subject: [PATCH] builtin/repack.c: treat string_list_item util as booleans
>
> The `->util` field corresponding to each string_list_item used to track
> the existence of some pack at the beginning of a repack operation was
> originally intended to be used as a bitfield.
>
> This bitfield tracked:
>
> - (1 << 0): whether or not the pack should be deleted
> - (1 << 1): whether or not the pack is cruft
>
> The previous commit removed the use of the second bit, meaning that we
> can treat this field as a boolean instead of a bitset.
I do think the boolean is syntactically a little nicer than the bit-set,
just because of the casting we have to with the void pointer). After
reading the earlier parts, I was hoping the culmination of this series
would be dropping the use of util entirely (presumably in favor of
separate lists). But maybe that would be too disruptive; I didn't try
it.
> -#define DELETE_PACK 1
> +#define DELETE_PACK ((void*)(uintptr_t)1)
> [...]
> @@ -130,7 +134,7 @@ static void mark_packs_for_deletion_1(struct string_list *names,
> * (if `-d` was given).
> */
> if (!string_list_has_string(names, sha1))
> - item->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);
> + item->util = DELETE_PACK;
> }
> }
I do like the use of the macro here to make the meaning of the boolean
more plain, since the name "util" is totally meaningless (but we are
stuck with it).
But on the other side, things get more mysterious:
> @@ -158,7 +162,7 @@ static void remove_redundant_packs_1(struct string_list *packs)
> {
> struct string_list_item *item;
> for_each_string_list_item(item, packs) {
> - if (!((uintptr_t)item->util & DELETE_PACK))
> + if (!item->util)
> continue;
This is syntactically much nicer, but the meaning of "util" as a boolean
for "we should delete this" is lost.
So I dunno. The end result of this patch is more readable syntactically,
but arguably less so semantically. Unless we have a good reason to ditch
the bit-set entirely, I wonder if we could have the best of both with
some macro helpers. Or even a set of matching helper functions like:
void pack_mark_for_deletion(struct string_list_item *item)
{
(uintptr_t)item->util = 1;
}
int pack_should_deleted(const struct string_list_item *item)
{
return item->util;
}
which could be a bool or a bit-set; the callers no longer need to care
and get to use human-readable names.
I dunno. It is a file-local convention and there aren't that many spots,
so maybe it is not worth worrying too much about. I'm pretty sure that I
got confused by the use of "util" here when looking at the code before,
as it did not have the DELETE_PACK name until your 72263ffc32
(builtin/repack.c: use named flags for existing_packs, 2022-05-20). But
maybe the comment that you added is sufficient.
If we had generic pointer-bitset macros, then perhaps other string-list
users could benefit, too. I thought maybe fast-export could use this for
its mark_to_ptr() stuff, but that is storing a whole 32-bit value, not a
bitset. So maybe this is just a weird localized thing.
A more radical idea is that we don't care very much about the data
structure as long as it is ordered. So we could just do:
struct existing_pack {
struct list_head list;
int to_delete;
char name[FLEX_ARRAY];
};
and ditch string-list entirely. That lets us use "to_delete" in a
natural way. Though I suspect it makes all the _other_ code unreadable,
as we have to allocate them, deal with list_entry(), and so on.
So I guess I'm fine with any path (including the one in this patch).
-Peff
^ 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