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

[phillip.wood123@gmail.com]

Hi Rubén

On 16/09/2024 23:09, Rubén Justo wrote:
> On Mon, Sep 16, 2024 at 02:33:54PM +0100, Phillip Wood wrote:
> 
> I can imagine that we could give the flawed and annotated patch back to
> the user, if they want to fix it and try again.

Exactly

> At any rate, I'm thinking about small fixes and/or avoiding to use a
> backup (":w! /tmp/patch" + ":r /tmp/patch") if I have doubts about
> making a mistake after spending some time thinking about a hunk, so as
> not to lose some work.

The problem is there is no good solution at the moment. Either we throw 
away the user's work if the edited patch does not apply or we keep the 
broken patch and say "this is broken, please figure out what's wrong 
with it and fix it". As I explained previously fixing a broken patch is 
not necessarily straight forward especially for new users. A few times 
when editing patches that are going to be applied in reverse (from "git 
checkout HEAD -- <path>") I've found it impossible to figure out why a 
particular edit was being rejected. In that case starting again with the 
original patch is my only hope. If you want to be able to re-edit a 
broken hunk perhaps we should add an option for that when we ask the 
user if they want to try again.

>>> diff --git a/add-patch.c b/add-patch.c
>>> index 557903310d..125e79a5ae 100644
>>> --- a/add-patch.c
>>> +++ b/add-patch.c
>>> @@ -1146,6 +1147,10 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
>>>    				      "addp-hunk-edit.diff", NULL) < 0)
>>>    		return -1;
>>> +	/* Drop possible previous edits */
>>> +	strbuf_setlen(&s->plain, plain_len);
>>> +	strbuf_setlen(&s->colored, colored_len);
>>> +
>>
>> At this point hunk->end points past s->plain.len. If the user has deleted
>> all the lines then we return with hunk->end in this invalid state. I think
>> that turns out not to matter as we end up restoring hunk->end from the
>> backup we make at the beginning of edit_hunk_loop() but it is not straight
>> forward to reason about.
> 
> I'm not sure I understand your comment.  We are adjusting "hunk" right
> after that, no?

Sorry I should have said hunk->colored_end and s->colored.len. If we 
return early then we don't call recolor_hunk().

>>> +	echo been-here > "$1"
>>> +	EOF
>>> +	test_set_editor "$(pwd)/fake_editor.sh"
>>> +'
>>
>> I don't think we need to write the fake editor in a separate test. Also it
>> would be better to call test_set_editor in a subshell so that it does not
>> affect later tests.
> 
> Yes, t3701 deserves an update.  I tried to respect its current style.
> I didn't want to start a mix.

I see are four instances of "test_set_editor" in this file, two of which 
setup the editor within the test that uses them and are called from a 
subshell. We should do the same here rather than creating more work for 
whoever decides to clean up this file in the future.

Best Wishes

Phillip