[PATCH v3 0/4] clean up some code around editors
- From
D. Ben Knoble <ben.knoble+github@gmail.com>
- Date
- Aug 11, 2025, 22:16 UTC
- Message-ID
- <20250811221706.67168-1-ben.knoble+github@gmail.com>
- In-Reply-To
- <20250810160323.49372-1-ben.knoble+github@gmail.com>
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}/4Thanks 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/ Published-as: https://github.com/benknoble/tree/editor-cleanup
D. Ben Knoble (4):
t7005: use modern test style
t7005: stop abusing --exec-path
t7005: sanitize test environment for subsequent tests
editor: use standard strvec API to receive environment for external
editorsbuiltin/commit.c | 2 +- editor.c | 10 ++-- editor.h | 7 ++- t/t7005-editor.sh | 147 +++++++++++++++++++--------------------------- 4 files changed, 69 insertions(+), 97 deletions(-)
Diff-intervalle contre v2 :
1: e7629202a1 ! 1: 8ad2904a18 t7005: use modern test style
@@ Metadata
## Commit message ##
t7005: use modern test style
- Tests in t7005 mask Git error codes and do not use our nice helpers for
- comparing results. Improve that, and drop a few old-style blank lines
- while at it.
+ Tests in t7005 mask Git error codes and do not use our nice test
+ helpers. Improve that, move some code into the setup test, and drop a
+ few old-style blank lines while at it.
+ Best-viewed-with: --ignore-all-space
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
## t/t7005-editor.sh ##
@@ t/t7005-editor.sh
-
'
- if ! expr "$vi" : '[a-z]*$' >/dev/null
-@@
- fi
-
- test_expect_success setup '
+-if ! expr "$vi" : '[a-z]*$' >/dev/null
+-then
+- vi=
+-fi
-
+-for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
+-do
+- cat >e-$i.sh <<-EOF
+- #!$SHELL_PATH
+- echo "Edited by $i" >"\$1"
+- EOF
+- chmod +x e-$i.sh
+-done
+-
+-if ! test -z "$vi"
+-then
+- mv e-$vi.sh $vi
+-fi
+-
+ test_expect_success setup '
++ if ! expr "$vi" : "[a-z]*$" >/dev/null
++ then
++ vi=
++ fi &&
++
++ for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
++ do
++ write_script e-$i.sh <<-EOF || return 1
++ echo "Edited by $i" >"\$1"
++ EOF
++ done &&
++
++ if ! test -z "$vi"
++ then
++ mv e-$vi.sh $vi
++ fi &&
+
msg="Hand-edited" &&
test_commit "$msg" &&
- echo "$msg" >expect &&
+- echo "$msg" >expect &&
- git show -s --format=%s > actual &&
-+ git show -s --format=%s >actual &&
- test_cmp expect actual
+- test_cmp expect actual
-
++ test_commit_message HEAD -m "$msg"
'
TERM=dumb
export TERM
test_expect_success 'dumb should error out when falling back on vi' '
-
- if git commit --amend
- then
- echo "Oops?"
-@@
+- if git commit --amend
+- then
+- echo "Oops?"
+- false
+- else
+- : happy
+- fi
++ test_must_fail git commit --amend
'
test_expect_success 'dumb should prefer EDITOR to VISUAL' '
@@ t/t7005-editor.sh
git commit --amend &&
- test "$(git show -s --format=%s)" = "Edited by EDITOR"
-
-+ echo "Edited by EDITOR" >expect &&
-+ git show -s --format=%s >actual &&
-+ test_cmp expect actual
++ test_commit_message HEAD -m "Edited by EDITOR"
'
TERM=vt100
@@ t/t7005-editor.sh
git --exec-path=. commit --amend &&
- git show -s --pretty=oneline |
- sed -e "s/^[0-9a-f]* //" >actual &&
-+ git show -s --pretty=oneline >show &&
-+ <show sed -e "s/^[0-9a-f]* //" >actual &&
- test_cmp expect actual
+- test_cmp expect actual
++ test_commit_message HEAD expect
'
done
+
@@
esac
test_expect_success "Using $i (override)" '
git --exec-path=. commit --amend &&
- git show -s --pretty=oneline |
- sed -e "s/^[0-9a-f]* //" >actual &&
-+ git show -s --pretty=oneline >show &&
-+ <show sed -e "s/^[0-9a-f]* //" >actual &&
- test_cmp expect actual
+- test_cmp expect actual
++ test_commit_message HEAD expect
'
done
+
@@
echo "echo space >\"\$1\"" >"e space.sh" &&
chmod a+x "e space.sh" &&
GIT_EDITOR="./e\ space.sh" git commit --amend &&
- test space = "$(git show -s --pretty=format:%s)"
-
-+ echo space >expect &&
-+ git show -s --pretty=tformat:%s >actual &&
-+ test_cmp expect actual
++ test_commit_message HEAD -m space
'
unset GIT_EDITOR
@@ t/t7005-editor.sh
git commit --amend &&
- test space = "$(git show -s --pretty=format:%s)"
-
-+ echo space >expect &&
-+ git show -s --pretty=tformat:%s >actual &&
-+ test_cmp expect actual
++ test_commit_message HEAD -m space
'
test_done
-: ---------- > 2: 9451e4f0f6 t7005: stop abusing --exec-path
2: 0467e33ed0 ! 3: 61cb116780 t7005: sanitize test environment for subsequent tests
@@ Commit message
that affect future tests, but those modifications are visible to future
tests and create a footgun for them.
- Use test_config, subshells, and test helpers to automatically undo
- environment and config modifications once finished.
+ Use test_config, subshells, single-command environment overrides, and
+ test helpers to automatically undo environment and config modifications
+ once finished.
Best-viewed-with: --ignore-all-space
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
## t/t7005-editor.sh ##
@@
- test_cmp expect actual
+ test_commit_message HEAD -m "$msg"
'
-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_must_fail git commit --amend
++ 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 &&
-- echo "Edited by EDITOR" >expect &&
-- git show -s --format=%s >actual &&
-+ (
-+ TERM=dumb &&
-+ export TERM &&
-+ EDITOR=./e-EDITOR.sh &&
-+ VISUAL=./e-VISUAL.sh &&
-+ export EDITOR VISUAL &&
++ TERM=dumb EDITOR=./e-EDITOR.sh VISUAL=./e-VISUAL.sh \
+ git commit --amend &&
-+ echo "Edited by EDITOR" >expect &&
-+ git show -s --format=%s >actual
-+ ) &&
- test_cmp expect actual
+ test_commit_message HEAD -m "Edited by EDITOR"
'
-TERM=vt100
-export TERM
--for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
--do
+ 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
@@ t/t7005-editor.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 &&
+ test_expect_success "Using $i" '
+- PATH="$PWD:$PATH" git commit --amend &&
+- test_commit_message HEAD expect
++ if test "$i" = core_editor
++ then
++ test_config core.editor ./e-core_editor.sh
++ fi &&
++ (
+ 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
-+ )
-+'
++ PATH="$PWD:$PATH" TERM=vt100 git commit --amend
++ ) &&
++ test_commit_message HEAD -m "Edited by $i"
+ '
+ done
-unset EDITOR VISUAL GIT_EDITOR
-git config --unset-all core.editor
@@ t/t7005-editor.sh
- ;;
- 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
+- PATH="$PWD:$PATH" git commit --amend &&
+- test_commit_message HEAD expect
- '
-done
+test_expect_success 'Using editors with overrides' '
@@ t/t7005-editor.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
++ 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" &&
@@
- test_cmp expect actual
+ test_commit_message HEAD -m space
'
-unset GIT_EDITOR
@@ t/t7005-editor.sh
- 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 &&
+ test_commit_message HEAD -m space
+ '
3: 4a9bb52470 = 4: ea269f2442 editor: use standard strvec API to receive environment for external editorsbase-commit: 112648dd6bdd8e4f485cd0ae11636807959d48be
-- 2.48.1