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

Thanks for the thorough review! I really appreciate it :)

On Mon, Sep 13, 2021 at 03:29:25PM -0400, Taylor Blau wrote:

Small nit; "the_repository->settings()" should be spelled as
"the_repository->settings", since "settings" is not a function.

Oh that's a good catch. Thanks!

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.

Sounds good, I'll note that down.

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.

Sounds good. This works great :)

quoted

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

From my testing, I suspect that git does not pretend the variable is
unset, but rather, it pretends that the variable is set to the empty
string. It seems that git behaves as if the variable is set to "false".
This is called out in Documentation/git.txt:

  Including the equals but with an empty value (like `git -c
  foo.bar= ...`) sets `foo.bar` to the empty string which `git config
  --type=bool` will convert to `false`.

If the variable really is set to false, how might we proceed here? Shall
we stick with test_when_finished?