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

On Thu, Sep 30, 2021 at 2:35 PM Glen Choo [off-list ref] wrote:

quoted

quoted

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.

This is a good point. I made the original change in response to Taylor's
comment, but I think we both didn't consider that this script assumes
`core.commitGraph` is `true`. The rest of the tests pass, but only
because the default value is true and I'd rather not have tests rely on
a happy accident.

I said I would incorporate these suggestions, but I didn't propose what
changes I would actually make.

Given that each test depends on a global config value in the setup
phase, it might be simplest to read if we try to avoid anything that
touches that value. The easiest way to achieve this is to use git -c
core.commitGraph {true,false} for the {true,false} cases. Since there is
no -c equivalent for the unset case, I'll continue to use
test_unconfig() + test_when_finished() to temporarily unset the value.

That was my thought, as well. (I wasn't quite sure why Taylor
recommended test_config() over `-c` which you used originally. It may
just be his personal preference. Perhaps he can chime in?)