Re: [PATCH v3 1/3] t3435: use `test_config` instead of `git config`

Samuel Čavoj [off-list ref] writes:

Replace usages of `git config` with `test_config` in t3435 to prevent
side-effects between tests.

Signed-off-by: Samuel Čavoj <redacted>
---
changed v2 -> v3:
    - added this patch
---
 t/t3435-rebase-gpg-sign.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

As I already said, I think this is not needed.

The way the existing tests protect themselves from what previously
happened is to explicitly set up _their_ expectation in them before
running the part that is affected by the configuration setting,
which is an approach that is easy to read and understand because it
is explicit in what these tests care about.

These are the first three uses of this test helper.

    test_rebase_gpg_sign ! false
    test_rebase_gpg_sign   true
    test_rebase_gpg_sign ! true  --no-gpg-sign

Examine what this patch does to the second test that is run by the
second invocation of test_rebase_gpg_sign, for example.  Before this
patch, "git config" that was done by the first test is left and
commit.gpgsign was set to 'false' when the second test started.
With this patch, it is removed by the clean-up action set by
test_config, so the second test starts without the configuration.

But this patch does *not* affect correctness of the second invocation.
Why?

Because the way these tests protect themselves is NOT based on the
"I muck with configuration, so I'll clean them up for the next
person" attitude which is made easy to do with test_config ANYWAY.
The correctness of the second test still relies on the fact that it
itself tweaks the variable it cares about to the state it wants it
to be.

So in this particular test script, where all test pieces are about
checking their behaviour on different setting of commit.gpgsign, use
of test_config does not buy us anything other than extra clean-up
after each test that is unnecessary.

Where test_config shines is when only a few (or a single) test cares
about different setting of a variable, and all later tests want to
test the behaviour under the default setting.  Among 47 tests, the
second and 42nd tests may want to test with commit.gpgsign set to
true, while the remainder may want to test without the variable at
all.  In such a case, it is easier to declare that the normal state
in that test script is not to have the configuration, and use
test_config in these selected tests to tentatively set it and remove
it after the tests are done.

But that rationale does not apply to this script.  It seems that
each test piece wants to be in control of and be tested under
specific setting of the variable.