Re: [PATCH v2 2/2] t: use test_config whenever possible

[Firmin Martin <firminmartin24@gmail.com>]

Hi Junio,

Junio C Hamano <gitster@pobox.com> 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