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

["D. Ben Knoble" <ben.knoble+github@gmail.com>]

On Wed, May 21, 2025 at 3:56 AM Patrick Steinhardt <ps@pks.im> 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@gmail.com>
> > ---
> >  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