Hi Junio
It seems to be that the config handling here is another example an
interdependence between tests that makes life harder for contributors
and reviewers.
On 13/10/2020 23:06, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
+test_expect_success 'rebase -r, GPG and merge strategies' '
+ git reset --hard merged &&
+ git rebase -fr --gpg-sign -s resolve --root &&
+ git verify-commit HEAD
+'
Unfortunately I've just noticed that the test above runs
git config commit.gpgsign true
So this test would pass anyway if merge picks up that config
setting. The previous test needs to be changed to
test_config commit.gpgsign true
so that the config setting is cleared when that test finishes.
Thanks for a review, but I do not think that is a right way to go.
test_config has an inherent assumption that not having the config at
all is somehow the "natural" state, and if that holds true, that
would be OK. But what is "natural" is subjective X-<.
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 in later tests if they don't want them it makes it very hard
for contributors and reviewers to check that new tests are sound as they
have to analyze each existing test in the script. In this example I
believe the new test was contributed by dscho who is an experienced
contributor with an interest in the test suite. However the test did not
clear the relevant config variable - if an experienced contributor did
not realize that the variable needed to 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.
The way each test is run by calling test_rebase_gpg_sign repeatedly
uses a different and more robust approach to ensure that previous
test does not affect the current one. Each invocation of test
explicitly sets the configuration to the state the test wants to,
cancelling what the previous test did.
It is only robust if contributors and reviewers realize that is what is
expected. Reviewers that only read the patch without loading up the test
file in their editor have no indication that the test should be clearing
the config variable.
Best Wishes
Phillip
To blend in better with existing tests and match their robustness
expectations, the right fix is for this new test to explicitly use
"git config --set" or "git config --unset" to make the variable into
the desired state, regardless of what the previous tests did.
If the test quoted at the beginning of this message wants to make
sure that --gpg-sign from the command line takes effect without
commit.gpgsign set, it should unset the variable explicitly.
Another combination worth testing is to ensure that --gpg-sign takes
effect when commit.gpgsign is explicitly set to false (not "left
unset").
Thanks.
