> 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
