Re: [PATCH v2 1/3] fsck: verify commit graph when implicitly enabled
From: Eric Sunshine <hidden>
Date: 2021-09-29 06:09:28
On 9/17/21 6:54 PM, Glen Choo wrote:
quoted hunk ↗ jump to hunk
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). Replace git_config_get_bool("core.commitgraph") in fsck with the equivalent call to the_repository->settings.core_commit_graph. [...] Signed-off-by: Glen Choo <redacted> ---diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh@@ -674,7 +674,7 @@ test_expect_success 'detect incorrect chunk count' ' -test_expect_success 'git fsck (checks commit-graph)' ' +test_expect_success 'git fsck (checks commit-graph when config set to true)' ' cd "$TRASH_DIRECTORY/full" && git fsck && corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \@@ -683,6 +683,28 @@ test_expect_success 'git fsck (checks commit-graph)' ' test_must_fail git fsck '
Because I took the time to scan backward through this test script, I understand that `core.commitGraph` is already `true` by the time this test is reached, thus the new test title is accurate (for now). However, it would be a bit easier to reason about this and make it more robust by having the test itself guarantee that it is set to `true` by invoking `git config core.commitGraph true`.
+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 +'
test_config() unsets the config variable when the test completes, so I'm wondering if its use is in fact desirable here. I ask because, from a quick scan through the file, it appears that many tests in this script assume that `core.commitGraph` is `true`, so it's not clear that unsetting it at the end of this test is a good idea in general.
+test_expect_success 'git fsck (checks commit-graph when config unset)' ' + test_when_finished "git config core.commitGraph true" && + + cd "$TRASH_DIRECTORY/full" &&
I find it somewhat confusing to reason about the behavior of test_when_finished() when it is invoked like this before the `cd`. It's true that the end-of-test `git config core.commitGraph true` will be invoked within `full` as desired (except in the very unusual case of the `cd` failing), so it's probably correct, but it requires extra thought to come to that conclusion. Switching the order of these two lines might lower the cognitive load a bit.
+ 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 +'
Taking Taylor's comment[1] on v1 patch [2/3] into account, I wonder if
it would be simpler for these all to be combined into a single test.
Something like (untested):
cd "$TRASH_DIRECTORY/full" &&
test_when_finished "git config core.commitGraph true" &&
git config core.commitGraph true &&
git fsck &&
corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \
"incorrect checksum" &&
cp commit-graph-pre-write-test $objdir/info/commit-graph &&
test_must_fail git fsck &&
git config core.commitGraph false &&
git fsck &&
git config --unset-all core.commitGraph &&
test_must_fail git fsck
I didn't bother using test_unconfig() since, in this case, we _know_
that the config variable is set, thus don't have to worry about `git
config --unset-all` failing.
A downside of combining the tests like this is that it makes it a bit
more cumbersome to diagnose a failure (because there is more code and
more cases to wade through).
Anyhow, I don't have a strong feeling one way or the other about
combining the test or leaving them separate.
[1]: https://lore.kernel.org/git/YT+n81yGPYEiMXwQ@nand.local/ (local)