On Wed, May 21, 2025 at 3:56 AM Patrick Steinhardt <ps@xxxxxx> wrote: > > 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 <ben.knoble+github@xxxxxxxxx> > > --- > > 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 && Thanks for the demo; I considered pushing the test inside the loop, but I assumed (perhaps wrongly?) that the `export` needed to be _outside_ the test script in order to correctly test the sequence of overrides. Thoughts? > > > > @@ -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
