Re: [PATCH 1/5] t3416: set $EDITOR in subshell

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 16, 2022, 13:53 UTC
Message-ID: <eab55cef-62bd-af2b-c295-1e76beca032f@gmail.com>
In-Reply-To: <xmqqo7wl30tt.fsf@gitster.g>

Hi Junio

On 15/08/2022 17:53, Junio C Hamano wrote:

Show 12 quoted lines

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> As $EDITOR is exported setting it in one test affects all subsequent
>> tests. Avoid this by always setting it in a subshell and remove a
>> couple of unnecessary call to set_fake_editor.
> 
> Unnecessary because it reuses the one that was established in the
> previous test [1]?  Or unnecessary because we know "rebase -i" would
> fail even before it gets to the point of asking an editor to tweak
> the todo sequence [2]?  Or something else?

I meant unnecessary as the editor does not change the todo list, but [2] also applies.

> If [1], it makes us wonder what happens when an earlier test gets
> skipped.  If [2], it makes us wonder what happens when "rebase -i"
> fails to fail as expected (does the test correctly diagnose it as a
> new breakage in "rebase -i"?).

I think those tests could be tightened up, I'll add a new patch that renames them to describe what they are testing (that we fail if there is more than one merge base) and greps for the expected error message.

Best Wishes

Phillip

Show 21 quoted lines

>> @@ -102,7 +106,6 @@ test_expect_success 'rebase -i --onto main...side' '
>>   	git checkout side &&
>>   	git reset --hard K &&
>>   
>> -	set_fake_editor &&
>>   	test_must_fail git rebase -i --onto main...side J
>>   '
> 
> This is one of the "removing" instances.
> 
>> @@ -187,8 +194,12 @@ test_expect_success 'rebase -i --keep-base main from side' '
>>   	git checkout side &&
>>   	git reset --hard K &&
>>   
>> -	set_fake_editor &&
>>   	test_must_fail git rebase -i --keep-base main
>>   '
> 
> And this is the other one.
> 
> Thanks.