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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 30, 2025, 16:35 UTC
Message-ID: <xmqqjz4t2nn9.fsf@gitster.g>
In-Reply-To: <e5c40d37750fe7effa95e8bf2cc24945150364d7.1751128487.git.gitgitgadget@gmail.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...

Show 8 quoted lines

>  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 ...

Show 6 quoted lines

>  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.

Show 6 quoted lines

>  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.