Re: [PATCH v2 1/2] sequencer: fix gpg option passed to merge subcommand

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 16, 2020, 20:15 UTC
Message-ID: <xmqq8sc6apwj.fsf@gitster.c.googlers.com>
In-Reply-To: <31ce457b-e71c-0ca0-e5be-a9aebb9cf785@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 6 quoted lines

> What is "natural" is subjective but what is the default config is
> not. If test scripts set random config variables and assumes that they
> will be ...
> ... cleared how are new contributors supposed to figure it out? If each
> test starts with the default config it is much easier to reason about
> it.

If this were a test script with many tests, all of which depend on starting from clean slate wrt the configuration variable space, and if you are adding just one or two tests that wants to run with configuration variable in effect, the story would be quite different. For example, I would think it would make a lot of sense in <20201014232517.3068298-1-emilyshaffer@google.com> to use test_config instead of "git config" as it is clear that leading 100+ lines of tests run with the default configuration, and it is sane to expact that later tests that may be added in the future would be the same.

Look at the test script in question again and notice that it is about seeing behaviour with the single configuration variable set to true or false. Using test_config would still signal the test pieces that use it do depend on the shown setting of the variable, but it does not help at all the test pieces that wants to see what happens when there is no configuration. Explicitly using "git config" and "test_unconfig" actually would _help_ reviewers who only looks between the pre- and post- context lines that are affected by the patch, as they do not have to know or assume that "normal state is nothing configured" (which needs to be verified by seeing all the existing tests that are not shown in the patch use test_config to clear the setting when they are done).

All of the above would be obvious once you think about it, I'd think.

Thanks.