Re: [PATCH v4 0/3] clean up some code around editors

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 13, 2025, 10:14 UTC
Message-ID: <2250a8fd-62b8-4218-9858-23032d9c807d@gmail.com>
In-Reply-To: <20250812170256.71751-1-ben.knoble+github@gmail.com>

Hi Ben

On 12/08/2025 18:02, D. Ben Knoble wrote:

> Changes from v3:
> - drop 4/4
> - use test_env (including a case our lint does not catch when the value
>    has spaces)

It's not worth a re-roll but for future reference

	test_env FOO=bar git commit --amend

uses an extra process compared to

	FOO=bar git commit --amend

which slows the test suite down for no real gain. We should only need to use test_env to set environment variables when calling a shell function. In the special case of test_must_fail it supports

	test_must_fail env FOO=bar git commit --amend

which is widely used in our test suite

$ git grep 'test_must_fail env ' origin/master t/t\*.sh | wc -l 152 $ git grep 'test_env .* test_must_fail ' origin/master t/t\*.sh | wc -l 1

Thanks for cleaning up this test file, it's looking much nicer now.

Phillip

Show 126 quoted lines

> Changes from v2:
> - shuffle setup code and use more helpers in 1/4
> - insert 2/4 to stop abusing --exec-path
> - improve environment-cleansing idioms in {2 => 3}/4
> 
> Thanks especially to Phillip's encyclopaedic knowledge of test helpers ;)
> 
> Changes from v1:
> - add a prep patch with style fixes to t7005
> - rework the environment munging to use subshells, per Phillip Wood's
>    suggestion
> 
> This reroll of the previous exec-path series is simplified to contain
> only the first 2 cleanup patches, which were largely acked by the list.
> Drop the controversial and broken PATH munging.
> 
> Also, this version is (still) based on a later master 112648dd6b (Merge
> branch 'master' of https://github.com/j6t/git-gui, 2025-08-04) than the
> original from May.
> 
> These patches clean up some old code in the editor tests and subsystem
> that does not use our modern idioms.
> 
> v1: https://lore.kernel.org/git/20250520193506.95199-1-ben.knoble+github@gmail.com/
> v2: https://lore.kernel.org/git/20250810160323.49372-1-ben.knoble+github@gmail.com/
> v3: https://lore.kernel.org/git/20250811221706.67168-1-ben.knoble+github@gmail.com/
> Published-as: https://github.com/benknoble/tree/editor-cleanup
> 
> D. Ben Knoble (3):
>    t7005: use modern test style
>    t7005: stop abusing --exec-path
>    t7005: sanitize test environment for subsequent tests
> 
>   t/t7005-editor.sh | 149 +++++++++++++++++++---------------------------
>   1 file changed, 60 insertions(+), 89 deletions(-)
> 
> Diff-intervalle contre v3 :
> 1:  8ad2904a18 = 1:  8ad2904a18 t7005: use modern test style
> 2:  9451e4f0f6 ! 2:  44a6fd8eb3 t7005: stop abusing --exec-path
>      @@ t/t7005-editor.sh
>        	esac
>        	test_expect_success "Using $i" '
>       -		git --exec-path=. commit --amend &&
>      -+		PATH="$PWD:$PATH" git commit --amend &&
>      ++		test_env PATH="$PWD:$PATH" git commit --amend &&
>        		test_commit_message HEAD expect
>        	'
>        done
>      @@ t/t7005-editor.sh
>        	esac
>        	test_expect_success "Using $i (override)" '
>       -		git --exec-path=. commit --amend &&
>      -+		PATH="$PWD:$PATH" git commit --amend &&
>      ++		test_env PATH="$PWD:$PATH" git commit --amend &&
>        		test_commit_message HEAD expect
>        	'
>        done
> 3:  61cb116780 ! 3:  135d4368d6 t7005: sanitize test environment for subsequent tests
>      @@ t/t7005-editor.sh
>       -export TERM
>        test_expect_success 'dumb should error out when falling back on vi' '
>       -	test_must_fail git commit --amend
>      -+	TERM=dumb test_must_fail git commit --amend
>      ++	test_env TERM=dumb test_must_fail git commit --amend
>        '
>        
>        test_expect_success 'dumb should prefer EDITOR to VISUAL' '
>      @@ t/t7005-editor.sh
>       -	VISUAL=./e-VISUAL.sh &&
>       -	export EDITOR VISUAL &&
>       -	git commit --amend &&
>      -+	TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \
>      ++	test_env TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \
>       +		git commit --amend &&
>        	test_commit_message HEAD -m "Edited by EDITOR"
>        '
>      @@ t/t7005-editor.sh
>       -		;;
>       -	esac
>        	test_expect_success "Using $i" '
>      --		PATH="$PWD:$PATH" git commit --amend &&
>      +-		test_env PATH="$PWD:$PATH" git commit --amend &&
>       -		test_commit_message HEAD expect
>       +		if test "$i" = core_editor
>       +		then
>      @@ t/t7005-editor.sh
>       +				export $i
>       +				;;
>       +			esac &&
>      -+			PATH="$PWD:$PATH" TERM=vt100 git commit --amend
>      ++			test_env PATH="$PWD:$PATH" TERM=vt100 git commit --amend
>       +		) &&
>       +		test_commit_message HEAD -m "Edited by $i"
>        	'
>      @@ t/t7005-editor.sh
>       -		;;
>       -	esac
>       -	test_expect_success "Using $i (override)" '
>      --		PATH="$PWD:$PATH" git commit --amend &&
>      +-		test_env PATH="$PWD:$PATH" git commit --amend &&
>       -		test_commit_message HEAD expect
>       -	'
>       -done
>      @@ t/t7005-editor.sh
>       +				export $i
>       +				;;
>       +			esac &&
>      -+			PATH="$PWD:$PATH" git commit --amend &&
>      ++			test_env PATH="$PWD:$PATH" git commit --amend &&
>       +			test_commit_message HEAD expect || exit 1
>       +		done
>       +	)
>      @@ t/t7005-editor.sh
>        
>        test_expect_success 'editor with a space' '
>        	echo "echo space >\"\$1\"" >"e space.sh" &&
>      -@@
>      + 	chmod a+x "e space.sh" &&
>      +-	GIT_EDITOR="./e\ space.sh" git commit --amend &&
>      ++	test_env GIT_EDITOR="./e\ space.sh" git commit --amend &&
>        	test_commit_message HEAD -m space
>        '
>        
> 4:  ea269f2442 < -:  ---------- editor: use standard strvec API to receive environment for external editors
> 
> base-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be