Hi Junio, Junio C Hamano <gitster@xxxxxxxxx> 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" > >> ( >> 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
