Re: [PATCH v2 1/3] fsck: verify commit graph when implicitly enabled

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 29, 2021, 06:09 UTC
Message-ID: <e1447d0e-091d-c659-b702-c5b33fffdb59@sunshineco.com>
In-Reply-To: <20210917225459.68086-2-chooglen@google.com>

On 9/17/21 6:54 PM, Glen Choo wrote:

Show 24 quoted lines

> 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@google.com>
> ---
> 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`.

Show 9 quoted lines

> +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.

Show 7 quoted lines

> +	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/