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

On Mon, Sep 13, 2021 at 9:09 PM Taylor Blau [off-list ref] wrote:

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

quoted

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

quoted

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.

There may be a misunderstanding. I wasn't saying that test_unconfig()
alone would work. My "That's probably reasonable" referred to Glen's
proposal of combining `git config --unset` with test_when_finished()
to restore the variable. In addition to that, I suggested
test_unconfig() as being a more robust choice in that recipe.

quoted

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

Yep.