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

From: Glen Choo <chooglen@google.com>
Date: Sep 30, 2021, 18:35 UTC
Message-ID: <kl6lk0ix9aju.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <kl6lmtnu7xle.fsf@chooglen-macbookpro.roam.corp.google.com>

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

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

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.