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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 14, 2021, 01:09 UTC
Message-ID: <YT/2PzbfE2p6TZ9r@nand.local>
In-Reply-To: <CAPig+cS2E6c9YYKTThPSVpyBkouoL+nK2rgpsZxNODPmTr2hzQ@mail.gmail.com>

On Mon, Sep 13, 2021 at 07:32:07PM -0400, Eric Sunshine wrote:

Show 26 quoted lines

> On Mon, Sep 13, 2021 at 7:19 PM Glen Choo <chooglen@google.com> wrote:
> > On Mon, Sep 13, 2021 at 03:29:25PM -0400, Taylor Blau wrote:
> > > > +        git config --unset core.commitGraph &&
> > >
> > > But I'm not aware of a way to temporarily unset a configuration variable
> > > for the duration of a test, so here I would probably write:
> > >
> > >     test_must_fail git -c core.commitGraph= fsck
> > >
> > > which Git interprets as "pretend this variable is unset in-core".
> >
> > From my testing, I suspect that git does not pretend the variable is
> > unset, but rather, it pretends that the variable is set to the empty
> > string. It seems that git behaves as if the variable is set to "false".
> > This is called out in Documentation/git.txt:
> >
> >   Including the equals but with an empty value (like `git -c
> >   foo.bar= ...`) sets `foo.bar` to the empty string which `git config
> >   --type=bool` will convert to `false`.
> >
> > If the variable really is set to false, how might we proceed here? Shall
> > we stick with test_when_finished?
>
> That's probably reasonable, however, for robustness, you should
> probably use test_unconfig() rather than raw `git config --unset` to
> clear the variable.

Hmm. I'm not so sure, do other tests rely on the value of that variable? If so, test_unconfig() won't restore them.

Even if we don't have any such tests now, it seems like we should err on the side of leaving it alone (although I suppose that future tests could set core.commitGraph to whatever value they need as long as they use the test_config function).

> Aside: This certainly makes one wonder if we should have a new
> function in t/test-lib-functions.sh which unsets a variable for the
> duration of a test only. However, that's outside the scope of this
> submission.

:-). I thought the same thing to myself when reviewing earlier today. That's why I recommended using test_when_finished upthread, but either approach is fine (my comments are definitely cosmetic, and don't matter to the substance of this thread, so ultimately I am fine with either).

Thanks, Taylor