From: Ben Knoble <ben.knoble@gmail.com>
Date: Fri, 13 Feb 2026 13:42:35 GMT
Subject: Re: [PATCH 2/4] builtin/history: check for merges before asking for user input
Message-ID: <3C51B8C4-E594-44ED-97F0-0EB7C36A3BCB@gmail.com>
In-Reply-To: <aY670nh0ElOZU46j@pks.im>


> Le 13 févr. 2026 à 00:51, Patrick Steinhardt <ps@pks.im> a écrit :
> 
> On Thu, Feb 12, 2026 at 05:20:08PM -0500, D. Ben Knoble wrote:
>>> On Thu, Feb 12, 2026 at 7:45 AM Patrick Steinhardt <ps@pks.im> wrote:
>>> diff --git a/t/t3451-history-reword.sh b/t/t3451-history-reword.sh
>>> index 6775ed62f9..12a9a7d051 100755
>>> --- a/t/t3451-history-reword.sh
>>> +++ b/t/t3451-history-reword.sh
>>> @@ -203,7 +203,7 @@ test_expect_success 'can reword a merge commit' '
>>> 
>>>                # It is not possible to replay merge commits embedded in the
>>>                # history (yet).
>>> -               test_must_fail git history reword HEAD~ 2>err &&
>>> +               test_must_fail git -c core.editor=false history reword HEAD~ 2>err &&
>>>                test_grep "replaying merge commits is not supported yet" err &&
>>> 
>>>                # But it is possible to reword a merge commit directly.
>> 
>> Hm, I don't quite see how "false" tests that we don't invoke the
>> editor at all, though I'm not sure if that behavior should be encoded
>> in the test or not. (Looks like Junio noticed the same in 1/4; I'm
>> getting better at reading my mail before sending, sometimes… :)
>> 
>> Otherwise looks good!
> 
> As replied in the other thread, this would abort with "Aborting commit
> as launching the editor failed." if Git tried to execute the editor.
> 
> I'm not quite sure whether I get the other remark about "should be
> encoded in the test or not". Do you mean to say we should use a separate
> test?
> 
> Patrick

No, just musing on whether “did not launch editor” is an important behavior to avoid regressing. I think yes, and I have no real objection to it :)