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

From: Glen Choo <chooglen@google.com>
Date: Sep 30, 2021, 18:00 UTC
Message-ID: <kl6lmtnu7xle.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <e1447d0e-091d-c659-b702-c5b33fffdb59@sunshineco.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 6 quoted lines

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

Show 7 quoted lines

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

Show 5 quoted lines

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