Firmin Martin [off-list ref] writes:
"whenever possible" in the subject is not wrong per-se but because
we do not want to accept a patch that uses it where it is impossible
to be used anyway, it does not mean all that much.
Use of test_config to replace #1 below is a correctness improvement,
and use of test_config to replace #2 below is a readability thing.
t: use more test_config for correctness and readability
perhaps?
Replace patterns of the form
1.
git config <config-option> <value> &&
...
git config --unset <config-option> &&
Adding "because this form fails to unset the config if an earlier
step fails." would be helpful to the readers to clarify that this is
a correctness "fix".
2.
test_when_finished "git config --unset <config-option>" &&
git config <config-option> <value> &&
...
to the concise
test_config <config-option> <value>
Nice.
In t5526, two tests have been further simplified as the output file is
written before "git config --global --unset".
quoted hunk
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index ed11569d8d..ff18263171 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
...
@@ -429,11 +429,7 @@ test_expect_success "'fetch.recurseSu
(
cd downstream &&
git config fetch.recurseSubmodules on-demand &&
- git fetch >../actual.out 2>../actual.err
- ) &&
- git config --global --unset fetch.recurseSubmodules &&
- (
- cd downstream &&
+ git fetch >../actual.out 2>../actual.err &&
git config --unset fetch.recurseSubmodules
) &&
test_must_be_empty actual.out &&
The original seems to be making sure that the config set in
"downstream" repository is used successfully before it is unset from
the global configuration and then the same fetch in the downstream
will work even when the configuration is removed from the global
configuration. The squashing of the two tests into one would change
what is being tested, wouldn't it?
Surely, the actual.out and actual.err in the first invocation *was*
overwritten without being looked at, but the exit status of that
fetch (i.e. fetch before global config gets unset) was still checked
in the original. So I'd call this a "simplification"
(
cd downstream &&
git config fetch.recurseSubmodules on-demand &&
- git fetch >../actual.out 2>../actual.err
+ git fetch
) &&
git config --global --unset fetch.recurseSubmodules &&
I.e. these redirected output files were not used and the next fetch
will overwrite them.
The patch I am responding to claims that
- "make sure it succeeds with global set, and then without global
set, make sure it still works" is not worth testing.
- it is OK to replace it with a single "it succeeds without
unsetting the global config".
It is not clear if either is true. Same for the other hunk that
squashes two tests into one for this path.
Hi Junio,
Junio C Hamano [off-list ref] writes:
Use of test_config to replace #1 below is a correctness improvement,
and use of test_config to replace #2 below is a readability thing.
t: use more test_config for correctness and readability
perhaps?
OK.
Adding "because this form fails to unset the config if an earlier
step fails." would be helpful to the readers to clarify that this is
a correctness "fix".
Will do.
To be honest, I'm completely lost in the remaining of your reply (in
fact, I suspect that you misread the diff, cf. the end of the email).
I copy here the full test with line number to make sure that we are in
the same page.
415 test_expect_success "'fetch.recurseSubmodules=on-demand' overrides global config" '
416 (
417 cd downstream &&
418 git fetch --recurse-submodules
419 ) &&
420 add_upstream_commit &&
421 git config --global fetch.recurseSubmodules false &&
422 head1=$(git rev-parse --short HEAD) &&
423 git add submodule &&
424 git commit -m "new submodule" &&
425 head2=$(git rev-parse --short HEAD) &&
426 echo "From $pwd/." > expect.err.2 &&
427 echo " $head1..$head2 super -> origin/super" >>expect.err.2 &&
428 head -3 expect.err >> expect.err.2 &&
429 (
430 cd downstream &&
431 git config fetch.recurseSubmodules on-demand &&
432 git fetch >../actual.out 2>../actual.err
433 ) &&
434 git config --global --unset fetch.recurseSubmodules &&
435 (
436 cd downstream &&
437 git config --unset fetch.recurseSubmodules
438 ) &&
439 test_must_be_empty actual.out &&
440 test_cmp expect.err.2 actual.err
The original seems to be making sure that the config set in
"downstream" repository is used successfully before it is unset from
the global configuration
OK.
and then the same fetch in the downstream will work even when the
What do you mean by "the same fetch"? the one of line 432 or any
instance of "git -C downstream fetch" in later tests?
configuration is removed from the global configuration.
The squashing of the two tests into one
Same question as above, I don't understand "the second test" you are
referring to (specifically, which lines?).
would change what is being tested, wouldn't it?
I agree that the semantic isn't the same (as test_config_global will
unset global config after unsetting the "downstream" config), but since
there is *not* another git-fetch after the global unset and before the
"downstream" unset I thought that it is fine.
Surely, the actual.out and actual.err in the first invocation *was*
overwritten without being looked at, but the exit status of that
Aren't they considered in the end of the test ? (lines 439-440).
fetch (i.e. fetch before global config gets unset) was still checked
in the original. So I'd call this a "simplification"
quoted
(
cd downstream &&
git config fetch.recurseSubmodules on-demand &&
- git fetch >../actual.out 2>../actual.err
+ git fetch
) &&
git config --global --unset fetch.recurseSubmodules &&
I.e. these redirected output files were not used and the next fetch
will overwrite them.
Same question as above.
The patch I am responding to claims that
- "make sure it succeeds with global set, and then without global
set, make sure it still works" is not worth testing.
In this test, there is no "make sure it still works without global set"
(i.e. I didn't see any git-fetch invocation after the global
configuration unset). Are you referring later tests?
- it is OK to replace it with a single "it succeeds without
unsetting the global config".
At this point, I have the feeling that you have misread the diff
(do correct me If I'm wrong) and saw ...
(
cd downstream &&
git config fetch.recurseSubmodules on-demand &&
git fetch >../actual.out 2>../actual.err
) &&
git config --global --unset fetch.recurseSubmodules &&
(
cd downstream &&
git fetch >../actual.out 2>../actual.err
git config --unset fetch.recurseSubmodules
) &&
(note the second extra git-fetch)
... instead of
429 (
430 cd downstream &&
431 git config fetch.recurseSubmodules on-demand &&
432 git fetch >../actual.out 2>../actual.err
433 ) &&
434 git config --global --unset fetch.recurseSubmodules &&
435 (
436 cd downstream &&
437 git config --unset fetch.recurseSubmodules
438 ) &&
In this case, what you said (e.g. simplification etc.) makes perfect sense.
Thanks,
Firmin