On 9/17/21 6:54 PM, 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. 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 <chooglen@xxxxxxxxxx>
---
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/
