On Mon, Dec 21, 2020 at 7:00 PM Nipunn Koorapati <nipunn1313@xxxxxxxxx> wrote: > 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...)
