Hi everyone! This patch was created in response to something we observed in
Google, where fsck failed to detect that the commit graph was invalid. We
initially assumed that fsck never checked the commit graph, but it turns out
that it does so only when core.commitgraph is set, even though we set defaults
for "whether to use the commit graph" in the repository settings.
Instead of using the config, let's use repository settings where
available. Replace core.commitGraph and core.multiPackIndex with their
equivalent repository settings in fsck and gc.
I don't anticipate any more business logic changes and the previous comments
were focused on testing style, so hopefully this should be good to merge.
Changes since v2:
* Various small test fixes (thanks Eric!). Most notably, I've used -c instead of
test_config because test_config can affect the values in subsequent tests.
* Rewording fix in patch 3 commit message
* Refactor tests in patch 3 so that we use a single helper function instead of
copy-pasted code
Changes since v1:
* clean up typo in patch 1 commit message
* document the commits that patches 1 and 2 address
* use test helpers in patch 1
* rewrite patch 2's tests so that it's easier to tell that each test
does something different
* reword patch 3 commit message to explain the bug
* add tests to patch 3
Glen Choo (3):
fsck: verify commit graph when implicitly enabled
fsck: verify multi-pack-index when implictly enabled
gc: perform incremental repack when implictly enabled
builtin/fsck.c | 5 +++--
builtin/gc.c | 5 ++---
t/t5318-commit-graph.sh | 23 ++++++++++++++++++++++-
t/t5319-multi-pack-index.sh | 5 ++++-
t/t7900-maintenance.sh | 28 ++++++++++++++++++++++++----
5 files changed, 55 insertions(+), 11 deletions(-)
Range-diff against v2:
1: 97ab2bb529 ! 1: 2f9ff949e6 fsck: verify commit graph when implicitly enabled
@@ t/t5318-commit-graph.sh: test_expect_success 'detect incorrect chunk count' '
cd "$TRASH_DIRECTORY/full" &&
git fsck &&
corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
-@@ t/t5318-commit-graph.sh: test_expect_success 'git fsck (checks commit-graph)' '
- test_must_fail git fsck
- '
-
+ "incorrect checksum" &&
+ cp commit-graph-pre-write-test $objdir/info/commit-graph &&
++ test_must_fail git -c core.commitGraph=true fsck
++'
++
+test_expect_success 'git fsck (ignores commit-graph when config set to false)' '
+ cd "$TRASH_DIRECTORY/full" &&
+ git fsck &&
+ corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
+ "incorrect checksum" &&
+ cp commit-graph-pre-write-test $objdir/info/commit-graph &&
-+ test_config core.commitGraph false &&
-+ git fsck
++ git -c core.commitGraph=false fsck
+'
+
+test_expect_success 'git fsck (checks commit-graph when config unset)' '
++ cd "$TRASH_DIRECTORY/full" &&
+ test_when_finished "git config core.commitGraph true" &&
+
-+ cd "$TRASH_DIRECTORY/full" &&
+ git fsck &&
+ corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
+ "incorrect checksum" &&
+ test_unconfig core.commitGraph &&
+ cp commit-graph-pre-write-test $objdir/info/commit-graph &&
-+ test_must_fail git fsck
-+'
-+
- test_expect_success 'setup non-the_repository tests' '
- rm -rf repo &&
- git init repo &&
+ test_must_fail git fsck
+ '
+
2: ace92453ca ! 2: b13ca2a695 fsck: verify multi-pack-index when implictly enabled
@@ t/t5319-multi-pack-index.sh: test_expect_success 'verify incorrect offset' '
corrupt_midx_and_verify $MIDX_BYTE_OFFSET "\377" $objdir \
"incorrect object offset" \
- "git -c core.multipackindex=true fsck"
-+ "git -c core.multipackindex=true fsck" &&
-+ test_must_fail git fsck &&
-+ git -c core.multipackindex=false fsck
++ "git -c core.multiPackIndex=true fsck" &&
++ test_unconfig core.multiPackIndex &&
++ test_must_fail git fsck &&
++ git -c core.multiPackIndex=false fsck
'
test_expect_success 'corrupt MIDX is not reused' '
3: d6959d61c1 < -: ---------- gc: perform incremental repack when implictly enabled
-: ---------- > 3: 76943aff80 gc: perform incremental repack when implictly enabled
--
2.33.0.800.g4c38ced690-goog
