From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 11 Aug 2025 09:59:52 GMT
Subject: Re: [PATCH 2/3] t7005: sanitize test environment for subsequent tests
Message-ID: <7491b9af-adcd-431a-89e7-ff54708beca6@gmail.com>
In-Reply-To: <144b6ee4-d4b4-4843-841c-93a109e71aa9@gmail.com>

On 10/08/2025 20:44, Phillip Wood wrote:
> On 10/08/2025 17:03, D. Ben Knoble wrote:
>>   '
>> -TERM=vt100
>> -export TERM
>> -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
>> -    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" '
>> -        git --exec-path=. commit --amend &&
>> -        git show -s --pretty=oneline >show &&
>> -        <show sed -e "s/^[0-9a-f]* //" >actual &&
>> -        test_cmp expect actual
>> -    '
>> -done

Thinking about it some more, for this test we can put the loop outside 
of the subshell and test_expect_success so that we don't have to worry 
about explicitly clearing the variables set from previous iterations.

for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
do
	test_expect_success "Using $i" '
		if test "$1" = core.editor
		then
			test_config core.editor ./e-core_editor.sh
		fi &&
		(
			case "$i" in
			[A-Z]*)
				eval "$i=./e-$i.sh" &&
				export $i
				;;
			esac &&
			PATH="$(pwd):$PATH" \
			    TERM=vt100 git commit --amend &&
		) &&
		test_commit_message HEAD -m "Edited by $i"
	'
done

Thanks

Phillip

>> +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 &&
>> +            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 &&
> 
> It would be nice to stop abusing --exec-path here and in the next test 
> by adding the current directory to $PATH with
> 
>      PATH="$(pwd):$PATH" git commit --amend
> 
>> +            git show -s --pretty=oneline >show &&
>> +            <show sed -e "s/^[0-9a-f]* //" >actual &&
>> +            test_cmp expect actual
> 
> We need to add "|| return 1" to the last line here and in the test below 
> to reliably error out when test_cmp fails. I'd have thought that our 
> test linting should hove picked this up but maybe it is confused by the 
> subshell.
> 
> Thanks
> 
> Phillip
>> +        done
>> +    )
>> +'
>> -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)" '
>> -        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 editors with overrides' '
>> +    (
>> +        TERM=vt100 &&
>> +        export TERM &&
>> +        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 &&
>> +            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 'editor with a space' '
>>       echo "echo space >\"\$1\"" >"e space.sh" &&
>> @@ -115,9 +126,8 @@
>>       test_cmp expect actual
>>   '
>> -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 &&
>>       echo space >expect &&
>>       git show -s --pretty=tformat:%s >actual &&
>