Re: [PATCH 1/3] fsck: verify commit graph when implicitly enabled
From: Taylor Blau <hidden>
Date: 2021-09-13 19:29:30
On Mon, Sep 13, 2021 at 11:12:19AM -0700, Glen Choo wrote:
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, even though commit graph is enabled by default.
Small nit; "the_repository->settings()" should be spelled as "the_repository->settings", since "settings" is not a function. It may be worth noting that this was totally fine before core.commitGraph's default changed to true. That happened in 31b1de6a09 (commit-graph: turn on commit-graph by default, 2019-08-13), which is the first time this sort of regression would have appeared.
quoted hunk ↗ jump to hunk
if (connectivity_only) { for_each_loose_object(mark_loose_for_connectivity, NULL, 0);@@ -908,7 +909,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix) check_connectivity(); - if (!git_config_get_bool("core.commitgraph", &i) && i) { + if (the_repository->settings.core_commit_graph) { struct child_process commit_graph_verify = CHILD_PROCESS_INIT; const char *verify_argv[] = { "commit-graph", "verify", NULL, NULL, NULL };
Here's the main part of your change, which is obviously correct (I'm glossing over the earlier part where you call prepare_repo_settings(); also required and obviously correct).
+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 && + git -c core.commitGraph=false fsck
Nit; I recommend replacing the `-c` style configuration with `test_config`, which modifies `$GIT_DIR/config` but only for the duration of the sub-shell.
+' + +test_expect_success 'git fsck (checks commit-graph when config unset)' ' + test_when_finished "git config core.commitGraph true" &&
... which would allow you to get rid of something like this (since you can avoid modifying the state visible outside of this test).
+ cd "$TRASH_DIRECTORY/full" && + git fsck && + corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \ + "incorrect checksum" && + git config --unset core.commitGraph &&
But I'm not aware of a way to temporarily unset a configuration variable
for the duration of a test, so here I would probably write:
test_must_fail git -c core.commitGraph= fsck
which Git interprets as "pretend this variable is unset in-core".
Thanks,
Taylor