Re: [PATCH 4/5] t7610-mergetool: prefer test_config over git config

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 15, 2014, 21:08 UTC
Message-ID: <xmqqwq81ujlw.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1413362121-17361-4-git-send-email-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---

The reason why this change favours "test_config" over "git config" is not described here, but if we think about that, some of the changes in this may become questionable.

Show 13 quoted lines

> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> index 1a15e06..7eeb207 100755
> --- a/t/t7610-mergetool.sh
> +++ b/t/t7610-mergetool.sh
> @@ -14,7 +14,7 @@ Testing basic merge tool invocation'
>  # running mergetool
>  
>  test_expect_success 'setup' '
> -	git config rerere.enabled true &&
> +	test_config rerere.enabled true &&
>  	echo master >file1 &&
>  	echo master spaced >"spaced name" &&
>  	echo master file11 >file11 &&

This one does not have corresponding "git config" to remove the configuration when the setup is done, so this changes the behaviour. The remainder of the tests will run without rerere.enabled set.

If this change does not make a difference, perhaps we do not even need to set rerere.enabled here in the first place?

But do later tests perform merges that can potentially be affected by the setting of rerere.enabled? If so, this change can break them. If this change does not break existing tests, I would say this is a good change that anticipates that we may add more tests in the future that work in subdir and that makes sure to leave subdir in a predictable state for them.

Show 17 quoted lines

> @@ -112,7 +112,7 @@ test_expect_success 'custom mergetool' '
>  '
>  
>  test_expect_success 'mergetool crlf' '
> -	git config core.autocrlf true &&
> +	test_config core.autocrlf true &&
>  	git checkout -b test2 branch1 &&
>  	test_must_fail git merge master >/dev/null 2>&1 &&
>  	( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
> @@ -129,7 +129,7 @@ test_expect_success 'mergetool crlf' '
>  	git submodule update -N &&
>  	test "$(cat submod/bar)" = "master submodule" &&
>  	git commit -m "branch1 resolved with mergetool - autocrlf" &&
> -	git config core.autocrlf false &&
> +	test_config core.autocrlf false &&
>  	git reset --hard
>  '

This one wants to set core.autocrlf to true while it runs, and then wants to reset to the original. When the test fails in the middle, however, we may abort this test and move on to the next one, while leaving core.autcrlf still set to "true", which may break later tests, hence use of test_config to ensure that the setting is reverted at the end of the test is the right thing to do.

So the hunk #112 is correct, but the hunk #129 is questionable and even misleading.

Show 9 quoted lines

> @@ -176,7 +176,7 @@ test_expect_success 'mergetool skips autoresolved' '
>  test_expect_success 'mergetool merges all from subdir' '
>  	(
>  		cd subdir &&
> -		git config rerere.enabled false &&
> +		test_config rerere.enabled false &&
>  		test_must_fail git merge master &&
>  		( yes "r" | git mergetool ../submod ) &&
>  		( yes "d" "d" | git mergetool --no-prompt ) &&

Same comment as the one for hunk #14 applies here. In principle, this change will make this test more isolated and is a good thing.

Show 9 quoted lines

> @@ -190,7 +190,7 @@ test_expect_success 'mergetool merges all from subdir' '
>  '
>  
>  test_expect_success 'mergetool skips resolved paths when rerere is active' '
> -	git config rerere.enabled true &&
> +	test_config rerere.enabled true &&
>  	rm -rf .git/rr-cache &&
>  	git checkout -b test5 branch1 &&
>  	git submodule update -N &&

Ditto.

Show 9 quoted lines

> @@ -204,7 +204,7 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
>  '
>  
>  test_expect_success 'conflicted stash sets up rerere'  '
> -	git config rerere.enabled true &&
> +	test_config rerere.enabled true &&
>  	git checkout stash1 &&
>  	echo "Conflicting stash content" >file11 &&
>  	git stash &&

Ditto.

Show 9 quoted lines

> @@ -232,7 +232,7 @@ test_expect_success 'conflicted stash sets up rerere'  '
>  
>  test_expect_success 'mergetool takes partial path' '
>  	git reset --hard &&
> -	git config rerere.enabled false &&
> +	test_config rerere.enabled false &&
>  	git checkout -b test12 branch1 &&
>  	git submodule update -N &&
>  	test_must_fail git merge master &&

Ditto.

Show 16 quoted lines

> @@ -505,14 +505,12 @@ test_expect_success 'file with no base' '
>  
>  test_expect_success 'custom commands override built-ins' '
>  	git checkout -b test14 branch1 &&
> -	git config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
> -	git config mergetool.defaults.trustExitCode true &&
> +	test_config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
> +	test_config mergetool.defaults.trustExitCode true &&
>  	test_must_fail git merge master &&
>  	git mergetool --no-prompt --tool defaults -- both &&
>  	echo master both added >expected &&
>  	test_cmp both expected &&
> -	git config --unset mergetool.defaults.cmd &&
> -	git config --unset mergetool.defaults.trustExitCode &&
>  	git reset --hard master >/dev/null 2>&1
>  '

This one is good; with test_config, you do not have to do the unsetting yourself.