On Thu, Sep 30, 2021 at 2:35 PM Glen Choo <chooglen@xxxxxxxxxx> wrote:
> >> 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?)
