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

[Glen Choo <chooglen@google.com>]

Eric Sunshine <sunshine@sunshineco.com> 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.