Re: [PATCH v3 1/3] test-lib-functions: handle --add in test_config

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 22, 2020, 00:13 UTC
Message-ID: <CAPig+cS4F5fhu-ej5ZVpzLR17AUhxzLRKVZxLuKaMCCk937C1A@mail.gmail.com>
In-Reply-To: <CAN8Z4-UG-watOnJMYUe3KU4JHnmJTxvwKSZ3s2DtBg104PACaA@mail.gmail.com>

On Mon, Dec 21, 2020 at 7:00 PM Nipunn Koorapati <nipunn1313@gmail.com> wrote:

Show 9 quoted lines

> I found that since test_unconfig uses --unset-all, I can write a test as such
>
>     test_config -C two remote.one.push +: &&
>     test_must_fail git -C two push one &&
>     git -C two config --add remote.one.push ^refs/heads/master &&
>     git -C two push one
>
> The unconfig of the test_config will --unset-all remote.one.push. I can
> use this technique and add a comment to that extent.

Yes, you could do that, though it is somewhat subtle and increases cognitive load since the reader has to reason about it a bit more -- and perhaps study the internal implementation of test_config() -- to convince himself or herself that the different methods of setting configuration (test_config() vs. `git config`) used in the same test is intentional and works as intended.

The example presented earlier, on the other hand, in which cleanup is explicit via `test_when_finished "test_unconfig ..."` does not suffer from such increased cognitive load since it uses `git config` consistently to set configuration rather than a mix of `git config` and test_config(). This sort of consideration is important not just for reviewers, but for people who need to understand the code down the road. For this reason, I think I favor the version in which the cleanup is explicit. (But that's just my opinion...)