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. This re-roll is primarily motivated by the CI failures noted by Junio in [1]. The underlying cause is that GIT_TEST_COMMIT_GRAPH=1 (enabled in the linux-gcc job) causes commands like "git commit" to write commit-graphs, but certain tests like t/t0410-partial-clone.sh and t/t3800-mktag.sh intentionally create corruptions that cause commit-graphs to become out-of-sync/invalid. Patch 1 fixes a bug where fsck did not check commit-graphs by default, which means that these tests will now fail because they have invalid commit-graphs. The easiest solution I found is to disable this confusing and noisy behavior with GIT_TEST_COMMIT_GRAPH=0. And since I am re-rolling, I incorporated Ævar's feedback regarding the commit messages (thanks!). I considered combining patches 1 and 2, but patch 1 has a grown a little to fix the CI issues, so I've decided to keep patches 1 and 2 separate. This series has also seen a healthy amount of interest in test style and coverage. We haven't converged on a path for the future, but I'd like to think that the tests here are still a step in (approximately) the right direction. Hopefully this version LGT us while we figure out what to do with tests :) [1] https://lore.kernel.org/git/xmqqfstafyox.fsf@gitster.g/ Changes since v3 * Disable GIT_TEST_COMMIT_GRAPH in tests that intentionally corrupt things in a way that is incompatible with commit-graphs. * Make patch 1 and 2's commit messages more concise (thanks Ævar!). 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/t0410-partial-clone.sh | 6 +++++- t/t3800-mktag.sh | 5 +++++ t/t5318-commit-graph.sh | 23 ++++++++++++++++++++++- t/t5319-multi-pack-index.sh | 5 ++++- t/t7900-maintenance.sh | 28 ++++++++++++++++++++++++---- 7 files changed, 65 insertions(+), 12 deletions(-) Range-diff against v3: 1: 2f9ff949e6 ! 1: aac1253e7b fsck: verify commit graph when implicitly enabled @@ Metadata ## Commit message ## fsck: verify commit graph when implicitly enabled - the_repository->settings is the preferred way to get certain - settings (such as "core.commitGraph") because it gets default values - from prepare_repo_settings(). However, cmd_fsck() reads the config - directly via git_config_get_bool(), which bypasses these default values. - This causes fsck to ignore the commit graph if "core.commitgraph" is not - explicitly set in the config. This worked fine until commit-graph was - enabled by default in 31b1de6a09 (commit-graph: turn on commit-graph by - default, 2019-08-13). + Change fsck to check the "core_commit_graph" variable set in + "repo-settings.c" instead of reading the "core.commitGraph" variable. + This fixes a bug where we wouldn't verify the commit-graph if the + config key was missing. This bug was introduced in + 31b1de6a09 (commit-graph: turn on commit-graph by default, 2019-08-13), + where core.commitGraph was turned on by default. - Replace git_config_get_bool("core.commitgraph") in fsck with the - equivalent call to the_repository->settings.core_commit_graph. - - The expected behavior is that fsck respects the config value when it is - set, but uses the default value when it is unset. For example, for - core.commitGraph, there are three cases: - - - Config value is set to true -> enabled - - Config value is set to false -> disabled - - Config value is unset -> enabled - - As such, tests cover all three cases. + Add tests to "t5318-commit-graph.sh" to verify that fsck checks the + commit-graph as expected for the 3 values of core.commitGraph. Also, + disable GIT_TEST_COMMIT_GRAPH for tests that use fsck in ways that + assume that commit-graph checking is disabled (t/t3800-mktag.sh, + t/t0410-partial-clone.sh). + Helped-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> Signed-off-by: Glen Choo <chooglen@xxxxxxxxxx> ## builtin/fsck.c ## @@ builtin/fsck.c: int cmd_fsck(int argc, const char **argv, const char *prefix) const char *verify_argv[] = { "commit-graph", "verify", NULL, NULL, NULL }; + ## t/t0410-partial-clone.sh ## +@@ t/t0410-partial-clone.sh: test_description='partial clone' + + # missing promisor objects cause repacks which write bitmaps to fail + GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0 ++# When enabled, some commands will write commit-graphs. This causes fsck ++# to fail when delete_object() is called because fsck will attempt to ++# verify the out-of-sync commit graph. ++GIT_TEST_COMMIT_GRAPH=0 + + delete_object () { + rm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|') +@@ t/t0410-partial-clone.sh: test_expect_success 'rev-list stops traversal at missing and promised commit' ' + + git -C repo config core.repositoryformatversion 1 && + git -C repo config extensions.partialclone "arbitrary string" && +- GIT_TEST_COMMIT_GRAPH=0 git -C repo -c core.commitGraph=false rev-list --exclude-promisor-objects --objects bar >out && ++ git -C repo rev-list --exclude-promisor-objects --objects bar >out && + grep $(git -C repo rev-parse bar) out && + ! grep $FOO out + ' + + ## t/t3800-mktag.sh ## +@@ t/t3800-mktag.sh: test_description='git mktag: tag object verify test' + + . ./test-lib.sh + ++# When enabled, some commands will automatically write commit-graphs. ++# This will cause the mktag tests to fail because fsck will attempt to ++# verify the out-of-sync commit graph. ++GIT_TEST_COMMIT_GRAPH=0 ++ + ########################################################### + # check the tag.sig file, expecting verify_tag() to fail, + # and checking that the error message matches the pattern + ## t/t5318-commit-graph.sh ## @@ t/t5318-commit-graph.sh: test_expect_success 'detect incorrect chunk count' ' $GRAPH_CHUNK_LOOKUP_OFFSET 2: b13ca2a695 ! 2: ed64983430 fsck: verify multi-pack-index when implictly enabled @@ Metadata ## Commit message ## fsck: verify multi-pack-index when implictly enabled - Like the previous commit, the_repository->settings.core_multi_pack_index - is preferable to reading "core.multiPackIndex" from the config because - prepare_repo_settings() sets a default value for - the_repository->settings. This worked fine until core.multiPackIndex was - enabled by default in 18e449f86b (midx: enable core.multiPackIndex by - default, 2020-09-25). - - Replace git_config_get_bool("core.multiPackIndex") in fsck with the - equivalent call to the_repository->settings.multi_pack_index. + Like the previous commit, change fsck to check the + "core_multi_pack_index" variable set in "repo-settings.c" instead of + reading the "core.multiPackIndex" config variable. This fixes a bug + where we wouldn't verify midx if the config key was missing. This bug + was introduced in 18e449f86b (midx: enable core.multiPackIndex by + default, 2020-09-25) where core.multiPackIndex was turned on by default. + Helped-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> Signed-off-by: Glen Choo <chooglen@xxxxxxxxxx> ## builtin/fsck.c ## 3: 76943aff80 = 3: 821b492d8b gc: perform incremental repack when implictly enabled -- 2.33.0.882.g93a45727a2-goog
