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

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

On Mon, Sep 13, 2021 at 7:19 PM Glen Choo [off-list ref] wrote:

quoted

On Mon, Sep 13, 2021 at 03:29:25PM -0400, Taylor Blau wrote:

quoted

quoted

+        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