Re: [PATCH v3 2/4] test: use "test_config"

[Junio C Hamano <gitster@pobox.com>]

"Leon Michalak via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Leon Michalak <leonmichalak6@gmail.com>
>
> Use the modern "test_config" test utility instead of manual"git config"
> as the former provides clean up on test completion.

Here "completion" is not "all the tests in the script are done", but
"each of the test_expect_{success,failure} piece that uses test_config".

Drop "modern".  It was invented 14 years ago (the same can be said
for test_grep which was called test_i18ngrep and had an extra
purpose, which was invented in the same year).

This conversion, unlike "test_grep" needs to be done a bit
carefully.  The fact the configuration is removed after the test
piece "test_config" was used means any tests after the test that was
originally using "git config" needs to be inspected to make sure
it was *not* relying on the value that was left by the previous
test piece.

For example...

>  test_expect_success 'diff.context honored by "log"' '
>  	git log -1 -p >output &&
>  	test_grep ! firstline output &&
> -	git config diff.context 8 &&
> +	test_config diff.context 8 &&
>  	git log -1 -p >output &&
>  	test_grep "^ firstline" output
>  '

... the test piece after this one may have assumed (wrongly!  The
assumption does not hold if this test failed before reaching "git
config") that diff.context is still set to 8 but that is no longer
the case.  But that one is OK because ...

>  test_expect_success 'The -U option overrides diff.context' '
> -	git config diff.context 8 &&
> +	test_config diff.context 8 &&
>  	git log -U4 -1 >output &&
>  	test_grep ! "^ firstline" output
>  '

... it was setting it for itself.  The same can be said with other
tests (not quoted in this reply).  They all set things up for
themselves, which is a good hygiene.

That means the last one in the patch needs to be inspected
carefully.  We do not know from the post-context of the patch what
the next test used to expect.

>  test_expect_success '-U0 is valid, so is diff.context=0' '
> -	git config diff.context 0 &&
> +	test_config diff.context 0 &&
>  	git diff >output &&
>  	test_grep "^-ADDED" output &&
>  	test_grep "^+MODIFIED" output

The one that comes after this one is about giving an explicit
command line option -U<n>.  It should not be affected by a
diff.context conffiguration variable that is lower-precedence
so we should be OK.

Thanks.