Subject: Re: [PATCH 1/4] t7005: sanitize test environment for subsequent tests

From: Patrick Steinhardt <[email-masked]>
Date: Wed, 21 May 2025 09:56:19 +0200

On Tue, May 20, 2025 at 03:34:55PM -0400, D. Ben Knoble wrote:
> Some of the editor tests manipulate the environment or config in ways
> that affect future tests (because they test a sequence of overrides),
> but those modifications are visible to future tests and create a footgun
> for them. Use test_config and undo environment modifications once
> finished.
> 
> Signed-off-by: D. Ben Knoble <[email-masked]>
> ---
>  t/t7005-editor.sh | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
> index 5fcf281dfb..06fa1ecd91 100755
> --- a/t/t7005-editor.sh
> +++ b/t/t7005-editor.sh
> @@ -111,6 +111,8 @@
>  	'
>  done
>  
> +unset EDITOR VISUAL GIT_EDITOR
> +git config --unset-all core.editor
>  test_expect_success 'editor with a space' '
>  	echo "echo space >\"\$1\"" >"e space.sh" &&
>  	chmod a+x "e space.sh" &&

Could we maybe adapt the tests that set those envvars to use a
`test_when_finished`? Something like this (untested) patch:

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index 5fcf281dfbf..a14ff4b38c4 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -93,17 +93,19 @@ 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)" '
+		echo "Edited by $i" >expect &&
+		case "$i" in
+		core_editor)
+			test_config core.editor ./e-core_editor.sh
+			;;
+		[A-Z]*)
+			test_when_finished "unset $i" &&
+			eval "$i=./e-$i.sh" &&
+			export $i
+			;;
+		esac &&
+
 		git --exec-path=. commit --amend &&
 		git show -s --pretty=oneline |
 		sed -e "s/^[0-9a-f]* //" >actual &&

> @@ -119,13 +121,10 @@
>  
>  '
>  
> -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 &&
>  	test space = "$(git show -s --pretty=format:%s)"
> -
>  '

This hunk looks good to me.

Patrick