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

Eric Sunshine [off-list ref] writes:

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

Reading this and..

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.

I'll make both of these changes. Test readability is important and
people shouldn't be wasting time thinking about test correctness.

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.

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. 

Interesting thought. The marginal benefit here is much lower than in
patch [2/3]. The difference is that corrupt_midx_and_verify() uses an
additional $COMMAND to perform verification, but
corrupt_graph_and_verify() does not. The result is that
corrupt_midx_and_verify() is subtle and confusing when used in separate
tests, but corrupt_graph_and_verify() is not so bad.

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

Yes, and we also lose the test labels that would guide the reader. It
might not be that obvious why the tests for a boolean value has 3
cases {true,false,unset}.

Taking churn into account, I'm inclined to keep the tests separate.