From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sun, 10 Aug 2025 19:44:26 GMT
Subject: Re: [PATCH 2/3] t7005: sanitize test environment for subsequent tests
Message-ID: <144b6ee4-d4b4-4843-841c-93a109e71aa9@gmail.com>
In-Reply-To: <20250810160323.49372-3-ben.knoble+github@gmail.com>

Hi Ben

On 10/08/2025 17:03, D. Ben Knoble wrote:
>   
> -TERM=dumb
> -export TERM
>   test_expect_success 'dumb should error out when falling back on vi' '
> -	if git commit --amend
> -	then
> -		echo "Oops?"
> -		false
> -	else
> -		: happy
> -	fi
> +	(
> +		TERM=dumb &&
> +		export TERM &&
> +		if git commit --amend
> +		then
> +			echo "Oops?"
> +			false
> +		else
> +			: happy
> +		fi
> +	)
>   '
>   
>   test_expect_success 'dumb should prefer EDITOR to VISUAL' '
> -	EDITOR=./e-EDITOR.sh &&
> -	VISUAL=./e-VISUAL.sh &&
> -	export EDITOR VISUAL &&
> -	git commit --amend &&
> -	echo "Edited by EDITOR" >expect &&
> -	git show -s --format=%s >actual &&
> +	(
> +		TERM=dumb &&
> +		export TERM &&

We may as well export this with EDITOR and VISUAL

> +		EDITOR=./e-EDITOR.sh &&
> +		VISUAL=./e-VISUAL.sh &&
> +		export EDITOR VISUAL &&
> +		git commit --amend &&
> +		echo "Edited by EDITOR" >expect &&
> +		git show -s --format=%s >actual
> +	) &&
>   	test_cmp expect actual
>   '
>   
> -TERM=vt100
> -export TERM
> -for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
> -do
> -	echo "Edited by $i" >expect
> -	unset EDITOR VISUAL GIT_EDITOR
> -	git config --unset-all core.editor
> -	case "$i" in
> -	core_editor)
> -		git config core.editor ./e-core_editor.sh
> -		;;
> -	[A-Z]*)
> -		eval "$i=./e-$i.sh"
> -		export $i
> -		;;
> -	esac
> -	test_expect_success "Using $i" '
> -		git --exec-path=. commit --amend &&
> -		git show -s --pretty=oneline >show &&
> -		<show sed -e "s/^[0-9a-f]* //" >actual &&
> -		test_cmp expect actual
> -	'
> -done
> +test_expect_success 'Using individual editors' '
> +	test_when_finished "test_unconfig --unset-all core.editor" &&
> +	(
> +		TERM=vt100 &&
> +		export TERM &&
> +		for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
> +		do
> +			sane_unset EDITOR VISUAL GIT_EDITOR &&
> +			test_might_fail git config --unset-all core.editor &&
> +			echo "Edited by $i" >expect &&
> +			case "$i" in
> +			core_editor)
> +				git config core.editor ./e-core_editor.sh
> +				;;
> +			[A-Z]*)
> +				eval "$i=./e-$i.sh" &&
> +				export $i
> +				;;
> +			esac &&
> +			git --exec-path=. commit --amend &&

It would be nice to stop abusing --exec-path here and in the next test 
by adding the current directory to $PATH with

	PATH="$(pwd):$PATH" git commit --amend

> +			git show -s --pretty=oneline >show &&
> +			<show sed -e "s/^[0-9a-f]* //" >actual &&
> +			test_cmp expect actual

We need to add "|| return 1" to the last line here and in the test below 
to reliably error out when test_cmp fails. I'd have thought that our 
test linting should hove picked this up but maybe it is confused by the 
subshell.

Thanks

Phillip
> +		done
> +	)
> +'
>   
> -unset EDITOR VISUAL GIT_EDITOR
> -git config --unset-all core.editor
> -for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
> -do
> -	echo "Edited by $i" >expect
> -	case "$i" in
> -	core_editor)
> -		git config core.editor ./e-core_editor.sh
> -		;;
> -	[A-Z]*)
> -		eval "$i=./e-$i.sh"
> -		export $i
> -		;;
> -	esac
> -	test_expect_success "Using $i (override)" '
> -		git --exec-path=. commit --amend &&
> -		git show -s --pretty=oneline >show &&
> -		<show sed -e "s/^[0-9a-f]* //" >actual &&
> -		test_cmp expect actual
> -	'
> -done
> +test_expect_success 'Using editors with overrides' '
> +	(
> +		TERM=vt100 &&
> +		export TERM &&
> +		for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
> +		do
> +			echo "Edited by $i" >expect &&
> +			case "$i" in
> +			core_editor)
> +				git config core.editor ./e-core_editor.sh
> +				;;
> +			[A-Z]*)
> +				eval "$i=./e-$i.sh" &&
> +				export $i
> +				;;
> +			esac &&
> +			git --exec-path=. commit --amend &&
> +			git show -s --pretty=oneline >show &&
> +			<show sed -e "s/^[0-9a-f]* //" >actual &&
> +			test_cmp expect actual
> +		done
> +	)
> +'
>   
>   test_expect_success 'editor with a space' '
>   	echo "echo space >\"\$1\"" >"e space.sh" &&
> @@ -115,9 +126,8 @@
>   	test_cmp expect actual
>   '
>   
> -unset GIT_EDITOR
>   test_expect_success 'core.editor with a space' '
> -	git config core.editor \"./e\ space.sh\" &&
> +	test_config core.editor \"./e\ space.sh\" &&
>   	git commit --amend &&
>   	echo space >expect &&
>   	git show -s --pretty=tformat:%s >actual &&