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

From: Nipunn Koorapati <nipunn1313@gmail.com>
Date: Dec 22, 2020, 02:25 UTC
Message-ID: <CAN8Z4-WpNuqN=HxL4AQU_+zi4hGhkC18d3ZSOJGzbdkMPkYAMQ@mail.gmail.com>
In-Reply-To: <CAPig+cS4F5fhu-ej5ZVpzLR17AUhxzLRKVZxLuKaMCCk937C1A@mail.gmail.com>

Show 15 quoted lines

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

Totally sympathize with wanting to reduce the cognitive load of reading the test suite. As a reader of the test, I found both implementation choices nonobvious - and requiring some diving into test_config and test_unconfig (namely the --unset-all behavior). A comment on the `git config` stating that it's there because `test_config` doesn't yet support `--add` seems equally clarifying as inserting a comment next to the test_unconfig usage. That being said, I suspect anyone in the future poking around with this will have to sourcedive through test_config and test_unconfig to make sense of it.

I'll switch it over to the test_unconfig option on the next reroll if requested.

--Nipunn