Re: [RFC PATCH 1/1] config: surface editor failure in exit code

Karthik Nayak <[email protected]> writes:

>> +test_expect_success 'git config --edit successful exit' '
>> +	test_when_finished "rm -rf repo" &&
>> +	git init repo &&
>> +	GIT_EDITOR=true &&
>> +	export GIT_EDITOR &&
>> +	git -C repo config -e &&
>> +	unset GIT_EDITOR
>> +'
>
> Nit: couldn't this be simply `test_env GIT_EDITOR=true git -C repo
> config -e` and avoid the set, export and unset?

No, it should just be a single liner:

	GIT_EDITOR=true git -C repo config -e

I would recommend against use of test_env in most cases, because it
introduces a subshell without making it obvious.

>> +test_expect_success 'git config --edit failure exit' '
>> +	test_when_finished "rm -rf repo" &&
>> +	git init repo &&
>> +	GIT_EDITOR=false &&
>> +	export GIT_EDITOR &&
>> +	test_must_fail git -C repo config -e &&
>> +	unset GIT_EDITOR
>> +'
>
> Same here..

Even when you truly a need subshell, it is better to spell the
subshell invocation out explicitly, i.e.,

    ...
    git init repo &&
    (
	GIT_EDITOR=false &&
	export GIT_EDITOR &&
	test_must_fail git -C repo config -e
    )

rather than using test_env.

But in a case like this where you do not even need a subshell to
help you shield your actions from later steps, you can just use
"env", like everybody else:

	test_must_fail env GIT_EDITOR=false git -C repo config -e

There are many uses of this pattern.

Thanks.