Re: [PATCH v2] add-patch: edit the hunk again

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 1, 2024, 10:02 UTC
Message-ID: <556ba87e-1eee-438d-848f-bbc5558289fe@gmail.com>
In-Reply-To: <6f392446-10b4-4074-a993-97ac444275f8@gmail.com>

Hi Rubén

On 24/09/2024 23:54, Rubén Justo wrote:

> On Mon, Sep 23, 2024 at 10:07:08AM +0100, phillip.wood123@gmail.com wrote:

> So, to me, it seems sensible to let the user review the faulty patch,
> even if it's only to discard it.

I agree we could add that option but not as the default

Show 6 quoted lines

>>> If they really want to start over with a fresh patch they still can
>>> say "no" to cancel the "edit" and start anew [*].
>>
>> This is not very obvious to the user,
> 
> It has been so for a decade...

That does not make it obvious though

> Keep in mind that this message will probably only be shown very _very_
> rarely to users who are most likely very familiar with (e)dit.

I'd argue that users who are not familiar with (e)dit are more likely to make mistakes when editing hunks and are less likely to be able to fix them.

Show 9 quoted lines

>>> +
>>> +	recolor_hunk(s, hunk);
>>> +
>>
>> This means we're now forking an external process when there is no hunk to
>> color. It would be better to avoid that by leaving this code where it was
>> and restoring the backup hunk above.
> 
> I don't see that external process. ¿?

Oh sorry, I thought we ran interactive.diffFilter on the edited hunk, but we don't. I'll try and find time to fix that.

Show 6 quoted lines

>> This is still missing "n q". Apart from that the test is looking good.
> 
> I've been resisting the idea of "completeness", because I think "e y"
> should also be fine.  But I'm not going to resist anymore here :-),
> since I don't think the test has much more value without "n q".  So
> I'll add it.

The reason I think we should have it is that the tests ought to be testing realistic user input and not rely on getting EOF which is unlikely to happen in real life.

Best Wishes

Phillip