{"thread":{"id":"62113","subject":"[PATCH] add-patch: edit the hunk again","startedAt":"2024-09-15T11:38:08Z","lastAt":"2024-10-02T17:34:05Z","messageCount":17,"participants":["Rubén Justo","Phillip Wood","Junio C Hamano","phillip.wood123@gmail.com"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"502805","messageId":"21ddf64f-10c2-4087-a778-0bd2e82aef42@gmail.com","threadId":"62113","inReplyTo":null,"subject":"[PATCH] add-patch: edit the hunk again","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-09-15T11:38:05Z","receivedAt":"2024-09-15T11:38:08Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"The \"edit\" option allows the user to directly modify the hunk to be\napplied.\n\nIf the modified hunk returned is not an applicable patch, we give the\nopportunity to try again.\n\nFor this new attempt we provide, again, the original hunk;  the user\nhas to repeat the modification from scratch.\n\nInstead, let's give them the faulty modified patch back, so they can\nidentify and fix the problem.\n\nIf they really want to start over with a fresh patch they still can\nsay 'no' to cancel the \"edit\" and start anew [*].\n\n    * In the old script-based version of \"add -p\", this \"no\" meant\n      discarding the current hunk and moving on to the next one.\n\n      This changed, presumably unintentionally, during the conversion\n      to C in bcdd297b78 (built-in add -p: implement hunk editing,\n      2019-12-13).\n\n      Now makes perfect sense not to move to the next hunk when the\n      user requests to discard their edits.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n\nThe message \"saying 'no' discards!\" comes from ac083c47ea\n(git-add--interactive: manual hunk editing mode, 2008-07-03).\n\nI think it was referring to discarding user modifications, not the\ncurrent hunk; which is what we were doing then (and now regaining).\n\nHowever, we stopped behaving that way in 2b8ea7f3c7 (add -p:\ncalculate offset delta for edited patches, 2018-03-05), perhaps for\nsome reason I'm missing.\n\nTherefore, this patch also modifies what we did, possibly\nunintentionally, in 2b8ea7f3c7.\n\nThanks.\n\n\n add-patch.c                | 26 ++++++++++++++++----------\n t/t3701-add-interactive.sh | 14 ++++++++++++++\n 2 files changed, 30 insertions(+), 10 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 557903310d..125e79a5ae 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1111,7 +1111,8 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)\n \thunk->colored_end = s->colored.len;\n }\n \n-static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n+static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk,\n+\t\t\t      size_t plain_len, size_t colored_len)\n {\n \tsize_t i;\n \n@@ -1146,6 +1147,10 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n \t\t\t\t      \"addp-hunk-edit.diff\", NULL) < 0)\n \t\treturn -1;\n \n+\t/* Drop possible previous edits */\n+\tstrbuf_setlen(&s->plain, plain_len);\n+\tstrbuf_setlen(&s->colored, colored_len);\n+\n \t/* strip out commented lines */\n \thunk->start = s->plain.len;\n \tfor (i = 0; i < s->buf.len; ) {\n@@ -1257,15 +1262,14 @@ static int edit_hunk_loop(struct add_p_state *s,\n \tbackup = *hunk;\n \n \tfor (;;) {\n-\t\tint res = edit_hunk_manually(s, hunk);\n+\t\tint res = edit_hunk_manually(s, hunk, plain_len, colored_len);\n \t\tif (res == 0) {\n \t\t\t/* abandoned */\n-\t\t\t*hunk = backup;\n-\t\t\treturn -1;\n+\t\t\tbreak;\n \t\t}\n \n \t\tif (res > 0) {\n-\t\t\thunk->delta +=\n+\t\t\thunk->delta = backup.delta +\n \t\t\t\trecount_edited_hunk(s, hunk,\n \t\t\t\t\t\t    backup.header.old_count,\n \t\t\t\t\t\t    backup.header.new_count);\n@@ -1273,10 +1277,6 @@ static int edit_hunk_loop(struct add_p_state *s,\n \t\t\t\treturn 0;\n \t\t}\n \n-\t\t/* Drop edits (they were appended to s->plain) */\n-\t\tstrbuf_setlen(&s->plain, plain_len);\n-\t\tstrbuf_setlen(&s->colored, colored_len);\n-\t\t*hunk = backup;\n \n \t\t/*\n \t\t * TRANSLATORS: do not translate [y/n]\n@@ -1289,8 +1289,14 @@ static int edit_hunk_loop(struct add_p_state *s,\n \t\t\t\t\t\"Edit again (saying \\\"no\\\" discards!) \"\n \t\t\t\t\t\"[y/n]? \"));\n \t\tif (res < 1)\n-\t\t\treturn -1;\n+\t\t\tbreak;\n \t}\n+\n+\t/* Drop a possible edit */\n+\tstrbuf_setlen(&s->plain, plain_len);\n+\tstrbuf_setlen(&s->colored, colored_len);\n+\t*hunk = backup;\n+\treturn -1;\n }\n \n static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff,\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 718438ffc7..6af5636221 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -165,6 +165,20 @@ test_expect_success 'dummy edit works' '\n \tdiff_cmp expected diff\n '\n \n+test_expect_success 'setup re-edit editor' '\n+\twrite_script \"fake_editor.sh\" <<-\\EOF &&\n+\tgrep been-here \"$1\" && echo found >output\n+\techo been-here > \"$1\"\n+\tEOF\n+\ttest_set_editor \"$(pwd)/fake_editor.sh\"\n+'\n+\n+test_expect_success 'editing again works' '\n+\tgit reset &&\n+\ttest_write_lines e y | GIT_TRACE=1 git add -p &&\n+\tgrep found output\n+'\n+\n test_expect_success 'setup patch' '\n \tcat >patch <<-\\EOF\n \t@@ -1,1 +1,4 @@\n-- \n2.46.1.507.gbcf32d0979\n"},{"id":"502892","messageId":"cba63486-2186-4e8e-aad4-ed7f54606ec7@gmail.com","threadId":"62113","inReplyTo":"21ddf64f-10c2-4087-a778-0bd2e82aef42@gmail.com","subject":"Re: [PATCH] add-patch: edit the hunk again","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-09-16T13:33:54Z","receivedAt":"2024-09-16T13:33:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 15/09/2024 12:38, Rubén Justo wrote:\n> The \"edit\" option allows the user to directly modify the hunk to be\n> applied.\n> \n> If the modified hunk returned is not an applicable patch, we give the\n> opportunity to try again.\n> \n> For this new attempt we provide, again, the original hunk;  the user\n> has to repeat the modification from scratch.\n\nAs you say below it looks like we started doing this by accident with \n2b8ea7f3c7 (add -p: calculate offset delta for edited patches, \n2018-03-05). I think that although the change was accidental it was \nactually a move in the right direction for several reasons.\n\n  - The error message from \"git apply\" makes it is virtually impossible\n    to tell what is wrong with the edited patch. The line numbers in the\n    error message refer to the complete patch but the user is editing a\n    single hunk so the user has no idea which line of the hunk the error\n    message applies to.\n\n  - If the user uses a terminal based editor then they cannot see the\n    error messages while they're re-editing the hunk.\n\n  - If the user has deleted a pre-image line then they need to somehow\n    magic it back before the hunk will apply.\n\n> Instead, let's give them the faulty modified patch back, so they can\n> identify and fix the problem.\n\nThe problem is how do they identify the problem? I have some unfinished \npatches [1] that annotate the edited patch with comments explaining \nwhat's wrong. Because we know what the unedited patch looked like and \nthat the pre-image lines should be unchanged it is possible to provide \nmuch better error messages than we get from trying to apply the whole \npatch with \"git apply\". It also makes it possible to restore deleted \npre-image lines.\n\n[1] https://github.com/phillipwood/git/tree/wip/add-p-editing-improvements\n     Note that the later patches do not even compile at the moment. I've\n     been meaning to split out the first eight patches and clean them up\n     as they're mostly functional and just need the commit messages\n     cleaning up.\n\n> diff --git a/add-patch.c b/add-patch.c\n> index 557903310d..125e79a5ae 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1146,6 +1147,10 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n>   \t\t\t\t      \"addp-hunk-edit.diff\", NULL) < 0)\n>   \t\treturn -1;\n>   \n> +\t/* Drop possible previous edits */\n> +\tstrbuf_setlen(&s->plain, plain_len);\n> +\tstrbuf_setlen(&s->colored, colored_len);\n> +\n\nAt this point hunk->end points past s->plain.len. If the user has \ndeleted all the lines then we return with hunk->end in this invalid \nstate. I think that turns out not to matter as we end up restoring \nhunk->end from the backup we make at the beginning of edit_hunk_loop() \nbut it is not straight forward to reason about.\n\n> @@ -1273,10 +1277,6 @@ static int edit_hunk_loop(struct add_p_state *s,\n>   \t\t\t\treturn 0;\n>   \t\t}\n>   \n> -\t\t/* Drop edits (they were appended to s->plain) */\n> -\t\tstrbuf_setlen(&s->plain, plain_len);\n> -\t\tstrbuf_setlen(&s->colored, colored_len);\n> -\t\t*hunk = backup;\n\nIn the old version we always restore the hunk from the backup when we \ntrim the edited patch which maintains the invariant \"hunk->end <= \ns->plain->end\"\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 718438ffc7..6af5636221 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -165,6 +165,20 @@ test_expect_success 'dummy edit works' '\n>   \tdiff_cmp expected diff\n>   '\n>   \n> +test_expect_success 'setup re-edit editor' '\n> +\twrite_script \"fake_editor.sh\" <<-\\EOF &&\n> +\tgrep been-here \"$1\" && echo found >output\n\n'grep been-here \"$1\" >output' should be sufficient I think\n\n> +\techo been-here > \"$1\"\n> +\tEOF\n> +\ttest_set_editor \"$(pwd)/fake_editor.sh\"\n> +'\n\nI don't think we need to write the fake editor in a separate test. Also \nit would be better to call test_set_editor in a subshell so that it does \nnot affect later tests.\n\n> +test_expect_success 'editing again works' '\n> +\tgit reset &&\n> +\ttest_write_lines e y | GIT_TRACE=1 git add -p &&\n\nIt would be nice to add \"n q\" to the input to make it complete.\n\n> +\tgrep found output\n\nUsing test_grep makes it easier to debug test failures.\n\n\nBest Wishes\n\nPhillip\n"},{"id":"502897","messageId":"xmqq7cbbqz6a.fsf@gitster.g","threadId":"62113","inReplyTo":"cba63486-2186-4e8e-aad4-ed7f54606ec7@gmail.com","subject":"Re: [PATCH] add-patch: edit the hunk again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-16T17:35:41Z","receivedAt":"2024-09-16T17:38:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> Because we know what the unedited patch\n> looked like and that the pre-image lines should be unchanged it is\n> possible to provide much better error messages than we get from trying\n> to apply the whole patch with \"git apply\". It also makes it possible\n> to restore deleted pre-image lines.\n> ...\n> [1] https://github.com/phillipwood/git/tree/wip/add-p-editing-improvements\n>     Note that the later patches do not even compile at the moment. I've\n>     been meaning to split out the first eight patches and clean them up\n>     as they're mostly functional and just need the commit messages\n>     cleaning up.\n\nI'd love this.  With both patches before and after the edit session,\nwe should be able to give more accurate line counts than having \"git\napply\" look at only the post-edit shape of the hunk, which wouldn't\nsee where any brokenness of the hunk comes from even if it wanted\nto.  We should probably be able to stop relying on \"--recount\" once\nwe do this right, which would be very nice outcome, too.\n\nThanks.\n\n\n"},{"id":"502911","messageId":"be0149e3-148b-4e25-9e44-f3f9a3303fcd@gmail.com","threadId":"62113","inReplyTo":"cba63486-2186-4e8e-aad4-ed7f54606ec7@gmail.com","subject":"Re: [PATCH] add-patch: edit the hunk again","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-09-16T22:09:42Z","receivedAt":"2024-09-16T22:09:45Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Sep 16, 2024 at 02:33:54PM +0100, Phillip Wood wrote:\n\n> > The \"edit\" option allows the user to directly modify the hunk to be\n> > applied.\n> > \n> > If the modified hunk returned is not an applicable patch, we give the\n> > opportunity to try again.\n> > \n> > For this new attempt we provide, again, the original hunk;  the user\n> > has to repeat the modification from scratch.\n> \n> As you say below it looks like we started doing this by accident with\n> 2b8ea7f3c7 (add -p: calculate offset delta for edited patches, 2018-03-05).\n> I think that although the change was accidental it was actually a move in\n> the right direction for several reasons.\n> \n>  - The error message from \"git apply\" makes it is virtually impossible\n>    to tell what is wrong with the edited patch. The line numbers in the\n>    error message refer to the complete patch but the user is editing a\n>    single hunk so the user has no idea which line of the hunk the error\n>    message applies to.\n> \n>  - If the user uses a terminal based editor then they cannot see the\n>    error messages while they're re-editing the hunk.\n> \n>  - If the user has deleted a pre-image line then they need to somehow\n>    magic it back before the hunk will apply.\n> \n> > Instead, let's give them the faulty modified patch back, so they can\n> > identify and fix the problem.\n> \n> The problem is how do they identify the problem? I have some unfinished\n> patches [1] that annotate the edited patch with comments explaining what's\n> wrong. Because we know what the unedited patch looked like and that the\n> pre-image lines should be unchanged it is possible to provide much better\n> error messages than we get from trying to apply the whole patch with \"git\n> apply\". It also makes it possible to restore deleted pre-image lines.\n> \n> [1] https://github.com/phillipwood/git/tree/wip/add-p-editing-improvements\n>     Note that the later patches do not even compile at the moment. I've\n>     been meaning to split out the first eight patches and clean them up\n>     as they're mostly functional and just need the commit messages\n>     cleaning up.\n\nI can imagine that we could give the flawed and annotated patch back to\nthe user, if they want to fix it and try again.  Am I misunderstanding\nyour envision?\n\nAt any rate, I'm thinking about small fixes and/or avoiding to use a\nbackup (\":w! /tmp/patch\" + \":r /tmp/patch\") if I have doubts about\nmaking a mistake after spending some time thinking about a hunk, so as\nnot to lose some work.\n\n> \n> > diff --git a/add-patch.c b/add-patch.c\n> > index 557903310d..125e79a5ae 100644\n> > --- a/add-patch.c\n> > +++ b/add-patch.c\n> > @@ -1146,6 +1147,10 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n> >   \t\t\t\t      \"addp-hunk-edit.diff\", NULL) < 0)\n> >   \t\treturn -1;\n> > +\t/* Drop possible previous edits */\n> > +\tstrbuf_setlen(&s->plain, plain_len);\n> > +\tstrbuf_setlen(&s->colored, colored_len);\n> > +\n> \n> At this point hunk->end points past s->plain.len. If the user has deleted\n> all the lines then we return with hunk->end in this invalid state. I think\n> that turns out not to matter as we end up restoring hunk->end from the\n> backup we make at the beginning of edit_hunk_loop() but it is not straight\n> forward to reason about.\n\nI'm not sure I understand your comment.  We are adjusting \"hunk\" right\nafter that, no?\n\n> \n> > @@ -1273,10 +1277,6 @@ static int edit_hunk_loop(struct add_p_state *s,\n> >   \t\t\t\treturn 0;\n> >   \t\t}\n> > -\t\t/* Drop edits (they were appended to s->plain) */\n> > -\t\tstrbuf_setlen(&s->plain, plain_len);\n> > -\t\tstrbuf_setlen(&s->colored, colored_len);\n> > -\t\t*hunk = backup;\n> \n> In the old version we always restore the hunk from the backup when we trim\n> the edited patch which maintains the invariant \"hunk->end <= s->plain->end\"\n\nSame here.  Are we losing that invariant?\n\n> \n> > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> > index 718438ffc7..6af5636221 100755\n> > --- a/t/t3701-add-interactive.sh\n> > +++ b/t/t3701-add-interactive.sh\n> > @@ -165,6 +165,20 @@ test_expect_success 'dummy edit works' '\n> >   \tdiff_cmp expected diff\n> >   '\n> > +test_expect_success 'setup re-edit editor' '\n> > +\twrite_script \"fake_editor.sh\" <<-\\EOF &&\n> > +\tgrep been-here \"$1\" && echo found >output\n> \n> 'grep been-here \"$1\" >output' should be sufficient I think\n\nAs I was writing the test, it was clearer to me using \"&& echo found\"\nhere and \"grep found\" below.\n\n> \n> > +\techo been-here > \"$1\"\n> > +\tEOF\n> > +\ttest_set_editor \"$(pwd)/fake_editor.sh\"\n> > +'\n> \n> I don't think we need to write the fake editor in a separate test. Also it\n> would be better to call test_set_editor in a subshell so that it does not\n> affect later tests.\n\nYes, t3701 deserves an update.  I tried to respect its current style.\nI didn't want to start a mix.\n\n> \n> > +test_expect_success 'editing again works' '\n> > +\tgit reset &&\n> > +\ttest_write_lines e y | GIT_TRACE=1 git add -p &&\n> \n> It would be nice to add \"n q\" to the input to make it complete.\n\nI have no objection to that.\n\n> \n> > +\tgrep found output\n> \n> Using test_grep makes it easier to debug test failures.\n> \n> \n> Best Wishes\n> \n> Phillip\n\nThanks for your review.\n"},{"id":"502996","messageId":"d20d030b-7d3e-49c6-a988-ab7fe480dd47@gmail.com","threadId":"62113","inReplyTo":"be0149e3-148b-4e25-9e44-f3f9a3303fcd@gmail.com","subject":"Re: [PATCH] add-patch: edit the hunk again","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-09-18T10:06:34Z","receivedAt":"2024-09-18T10:06:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 16/09/2024 23:09, Rubén Justo wrote:\n> On Mon, Sep 16, 2024 at 02:33:54PM +0100, Phillip Wood wrote:\n> \n> I can imagine that we could give the flawed and annotated patch back to\n> the user, if they want to fix it and try again.\n\nExactly\n\n> At any rate, I'm thinking about small fixes and/or avoiding to use a\n> backup (\":w! /tmp/patch\" + \":r /tmp/patch\") if I have doubts about\n> making a mistake after spending some time thinking about a hunk, so as\n> not to lose some work.\n\nThe problem is there is no good solution at the moment. Either we throw \naway the user's work if the edited patch does not apply or we keep the \nbroken patch and say \"this is broken, please figure out what's wrong \nwith it and fix it\". As I explained previously fixing a broken patch is \nnot necessarily straight forward especially for new users. A few times \nwhen editing patches that are going to be applied in reverse (from \"git \ncheckout HEAD -- <path>\") I've found it impossible to figure out why a \nparticular edit was being rejected. In that case starting again with the \noriginal patch is my only hope. If you want to be able to re-edit a \nbroken hunk perhaps we should add an option for that when we ask the \nuser if they want to try again.\n\n>>> diff --git a/add-patch.c b/add-patch.c\n>>> index 557903310d..125e79a5ae 100644\n>>> --- a/add-patch.c\n>>> +++ b/add-patch.c\n>>> @@ -1146,6 +1147,10 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n>>>    \t\t\t\t      \"addp-hunk-edit.diff\", NULL) < 0)\n>>>    \t\treturn -1;\n>>> +\t/* Drop possible previous edits */\n>>> +\tstrbuf_setlen(&s->plain, plain_len);\n>>> +\tstrbuf_setlen(&s->colored, colored_len);\n>>> +\n>>\n>> At this point hunk->end points past s->plain.len. If the user has deleted\n>> all the lines then we return with hunk->end in this invalid state. I think\n>> that turns out not to matter as we end up restoring hunk->end from the\n>> backup we make at the beginning of edit_hunk_loop() but it is not straight\n>> forward to reason about.\n> \n> I'm not sure I understand your comment.  We are adjusting \"hunk\" right\n> after that, no?\n\nSorry I should have said hunk->colored_end and s->colored.len. If we \nreturn early then we don't call recolor_hunk().\n\n>>> +\techo been-here > \"$1\"\n>>> +\tEOF\n>>> +\ttest_set_editor \"$(pwd)/fake_editor.sh\"\n>>> +'\n>>\n>> I don't think we need to write the fake editor in a separate test. Also it\n>> would be better to call test_set_editor in a subshell so that it does not\n>> affect later tests.\n> \n> Yes, t3701 deserves an update.  I tried to respect its current style.\n> I didn't want to start a mix.\n\nI see are four instances of \"test_set_editor\" in this file, two of which \nsetup the editor within the test that uses them and are called from a \nsubshell. We should do the same here rather than creating more work for \nwhoever decides to clean up this file in the future.\n\nBest Wishes\n\nPhillip\n"},{"id":"503010","messageId":"08b29649-eeeb-49e4-82ac-2a3473dd2ad5@gmail.com","threadId":"62113","inReplyTo":"d20d030b-7d3e-49c6-a988-ab7fe480dd47@gmail.com","subject":"Re: [PATCH] add-patch: edit the hunk again","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-09-18T17:46:26Z","receivedAt":"2024-09-18T17:46:30Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Wed, Sep 18, 2024 at 11:06:34AM +0100, phillip.wood123@gmail.com wrote:\n> Hi Rubén\n> \n> On 16/09/2024 23:09, Rubén Justo wrote:\n> > On Mon, Sep 16, 2024 at 02:33:54PM +0100, Phillip Wood wrote:\n> > \n> > I can imagine that we could give the flawed and annotated patch back to\n> > the user, if they want to fix it and try again.\n> \n> Exactly\n\nSo we agree on where we're going ...\n\n> \n> > At any rate, I'm thinking about small fixes and/or avoiding to use a\n> > backup (\":w! /tmp/patch\" + \":r /tmp/patch\") if I have doubts about\n> > making a mistake after spending some time thinking about a hunk, so as\n> > not to lose some work.\n> \n> The problem is there is no good solution at the moment.\n\nalthough not in the length of the stride :)\n\nMaybe in the future we can provide better error descriptions, or even\nadd annotations to the faulty patch explaining the faults.\n\nBut we're not there yet, and honestly, it's not my intention to work\non that.\n\n> Either we keep the broken\n> patch and say \"this is broken, please figure out what's wrong with it and\n> fix it\"\n\nYes, we should keep the users's work if they say \"yes\" to:\n\n    Your edited hunk does not apply. Edit again (saying \"no\" discards!) [y/n]?\n\n> or throw away\n> the user's work if the edited patch does not apply.\n\nOnly if the user says \"no\" (as we say in the message).\n\nAfter that \"no\", the user has another opportunity to decide about the\nhunk:\n\n    Your edited hunk does not apply. Edit again (saying \"no\" discards!) [y/n]? no\n\n    (n/m) Stage this hunk [y,n,q,a,d,e,p,?]\n\nAnd then, \"edit\" will allow them to start over and edit the original\nhunk, again.\n\n> As I explained previously fixing a broken patch is not necessarily\n> straight forward especially for new users.\n\nVery true.  But I don't think that should be a reason to stop the user\nfrom trying.\n\n> A few times when editing patches\n> that are going to be applied in reverse (from \"git checkout HEAD -- <path>\")\n> I've found it impossible to figure out why a particular edit was being\n> rejected. In that case starting again with the original patch is my only\n> hope.\n\nMy experience is usually small last-minute adjustments that aren't\nworth canceling the interactive session for, and I don't want to have\nto remember to make them later.\n\nA small error in a large hunk has been frustrating several times\nbecause I have to go back and review the whole thing.\n\n> If you want to be able to re-edit a broken hunk perhaps we should add\n> an option for that when we ask the user if they want to try again.\n\nAs we commented in a previous message, this is what we are regaining\nwith this patch.  The option was introduced in ac083c47ea\n(git-add--interactive: manual hunk editing mode, 2008-07-03) and lost\nin 2b8ea7f3c7 (add -p: calculate offset delta for edited patches,\n2018-03-05).\n\nThanks.\n"},{"id":"503011","messageId":"4dd5a2c7-26a8-470f-b651-e1fe2d1dbcec@gmail.com","threadId":"62113","inReplyTo":"21ddf64f-10c2-4087-a778-0bd2e82aef42@gmail.com","subject":"[PATCH v2] add-patch: edit the hunk again","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-09-18T17:51:41Z","receivedAt":"2024-09-18T17:51:44Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"The \"edit\" option allows the user to directly modify the hunk to be\napplied.\n\nIf the modified hunk returned by the user is not an applicable patch,\nthey will be given the opportunity to try again.\n\nFor this new attempt we give them the original hunk;  they have to\nrepeat the modification from scratch.\n\nInstead, let's give them the modified patch back, so they can identify\nand fix the problem.\n\nIf they really want to start over with a fresh patch they still can\nsay \"no\" to cancel the \"edit\" and start anew [*].\n\n    * In the old script-based version of \"add -p\", this \"no\" meant\n      discarding the hunk and moving on to the next one.\n\n      This changed, probably unintentionally, during its conversion to\n      C in bcdd297b78 (built-in add -p: implement hunk editing,\n      2019-12-13).\n\n      It now makes more sense not to move to the next hunk when the\n      user requests to discard their edits.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n\nThis iteration addresses Phillip's comments:\n\n - Ensure that `edit_hunk_manually()` exits with sane values in\n   `hunk`.\n\n - Merge the tests into a single one and use a subshell to prevent\n   leaking `EDITOR`.\n\nThanks.\n\n add-patch.c                | 31 +++++++++++++++++++------------\n t/t3701-add-interactive.sh | 13 +++++++++++++\n 2 files changed, 32 insertions(+), 12 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 557903310d..75b5129281 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1111,7 +1111,8 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)\n \thunk->colored_end = s->colored.len;\n }\n \n-static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n+static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk,\n+\t\t\t      size_t plain_len, size_t colored_len)\n {\n \tsize_t i;\n \n@@ -1146,6 +1147,10 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n \t\t\t\t      \"addp-hunk-edit.diff\", NULL) < 0)\n \t\treturn -1;\n \n+\t/* Drop possible previous edits */\n+\tstrbuf_setlen(&s->plain, plain_len);\n+\tstrbuf_setlen(&s->colored, colored_len);\n+\n \t/* strip out commented lines */\n \thunk->start = s->plain.len;\n \tfor (i = 0; i < s->buf.len; ) {\n@@ -1157,12 +1162,13 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n \t}\n \n \thunk->end = s->plain.len;\n+\n+\trecolor_hunk(s, hunk);\n+\n \tif (hunk->end == hunk->start)\n \t\t/* The user aborted editing by deleting everything */\n \t\treturn 0;\n \n-\trecolor_hunk(s, hunk);\n-\n \t/*\n \t * If the hunk header is intact, parse it, otherwise simply use the\n \t * hunk header prior to editing (which will adjust `hunk->start` to\n@@ -1257,15 +1263,14 @@ static int edit_hunk_loop(struct add_p_state *s,\n \tbackup = *hunk;\n \n \tfor (;;) {\n-\t\tint res = edit_hunk_manually(s, hunk);\n+\t\tint res = edit_hunk_manually(s, hunk, plain_len, colored_len);\n \t\tif (res == 0) {\n \t\t\t/* abandoned */\n-\t\t\t*hunk = backup;\n-\t\t\treturn -1;\n+\t\t\tbreak;\n \t\t}\n \n \t\tif (res > 0) {\n-\t\t\thunk->delta +=\n+\t\t\thunk->delta = backup.delta +\n \t\t\t\trecount_edited_hunk(s, hunk,\n \t\t\t\t\t\t    backup.header.old_count,\n \t\t\t\t\t\t    backup.header.new_count);\n@@ -1273,10 +1278,6 @@ static int edit_hunk_loop(struct add_p_state *s,\n \t\t\t\treturn 0;\n \t\t}\n \n-\t\t/* Drop edits (they were appended to s->plain) */\n-\t\tstrbuf_setlen(&s->plain, plain_len);\n-\t\tstrbuf_setlen(&s->colored, colored_len);\n-\t\t*hunk = backup;\n \n \t\t/*\n \t\t * TRANSLATORS: do not translate [y/n]\n@@ -1289,8 +1290,14 @@ static int edit_hunk_loop(struct add_p_state *s,\n \t\t\t\t\t\"Edit again (saying \\\"no\\\" discards!) \"\n \t\t\t\t\t\"[y/n]? \"));\n \t\tif (res < 1)\n-\t\t\treturn -1;\n+\t\t\tbreak;\n \t}\n+\n+\t/* Drop a possible edit */\n+\tstrbuf_setlen(&s->plain, plain_len);\n+\tstrbuf_setlen(&s->colored, colored_len);\n+\t*hunk = backup;\n+\treturn -1;\n }\n \n static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff,\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 718438ffc7..f3206a317b 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -165,6 +165,19 @@ test_expect_success 'dummy edit works' '\n \tdiff_cmp expected diff\n '\n \n+test_expect_success 'editing again works' '\n+\tgit reset &&\n+\twrite_script \"fake_editor.sh\" <<-\\EOF &&\n+\tgrep been-here \"$1\" >output\n+\techo been-here >\"$1\"\n+\tEOF\n+\t(\n+\t\ttest_set_editor \"$(pwd)/fake_editor.sh\" &&\n+\t\ttest_write_lines e y | GIT_TRACE=1 git add -p\n+\t) &&\n+\ttest_grep been-here output\n+'\n+\n test_expect_success 'setup patch' '\n \tcat >patch <<-\\EOF\n \t@@ -1,1 +1,4 @@\n\nRange-diff:\n1:  bcf32d0979 ! 1:  2b55a759d5 add-patch: edit the hunk again\n    @@ add-patch.c: static int edit_hunk_manually(struct add_p_state *s, struct hunk *h\n      \t/* strip out commented lines */\n      \thunk->start = s->plain.len;\n      \tfor (i = 0; i < s->buf.len; ) {\n    +@@ add-patch.c: static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n    + \t}\n    + \n    + \thunk->end = s->plain.len;\n    ++\n    ++\trecolor_hunk(s, hunk);\n    ++\n    + \tif (hunk->end == hunk->start)\n    + \t\t/* The user aborted editing by deleting everything */\n    + \t\treturn 0;\n    + \n    +-\trecolor_hunk(s, hunk);\n    +-\n    + \t/*\n    + \t * If the hunk header is intact, parse it, otherwise simply use the\n    + \t * hunk header prior to editing (which will adjust `hunk->start` to\n     @@ add-patch.c: static int edit_hunk_loop(struct add_p_state *s,\n      \tbackup = *hunk;\n      \n    @@ t/t3701-add-interactive.sh: test_expect_success 'dummy edit works' '\n      \tdiff_cmp expected diff\n      '\n      \n    -+test_expect_success 'setup re-edit editor' '\n    -+\twrite_script \"fake_editor.sh\" <<-\\EOF &&\n    -+\tgrep been-here \"$1\" && echo found >output\n    -+\techo been-here > \"$1\"\n    -+\tEOF\n    -+\ttest_set_editor \"$(pwd)/fake_editor.sh\"\n    -+'\n    -+\n     +test_expect_success 'editing again works' '\n     +\tgit reset &&\n    -+\ttest_write_lines e y | GIT_TRACE=1 git add -p &&\n    -+\tgrep found output\n    ++\twrite_script \"fake_editor.sh\" <<-\\EOF &&\n    ++\tgrep been-here \"$1\" >output\n    ++\techo been-here >\"$1\"\n    ++\tEOF\n    ++\t(\n    ++\t\ttest_set_editor \"$(pwd)/fake_editor.sh\" &&\n    ++\t\ttest_write_lines e y | GIT_TRACE=1 git add -p\n    ++\t) &&\n    ++\ttest_grep been-here output\n     +'\n     +\n      test_expect_success 'setup patch' '\n-- \n2.46.1.507.gdd29a28bc2\n"},{"id":"503243","messageId":"2ad1f7b1-714c-4d6e-89a6-fd65271222b9@gmail.com","threadId":"62113","inReplyTo":"4dd5a2c7-26a8-470f-b651-e1fe2d1dbcec@gmail.com","subject":"Re: [PATCH v2] add-patch: edit the hunk again","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-09-23T09:07:08Z","receivedAt":"2024-09-23T09:07:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nThanks for the re-roll. I'm still not convinced that changing this \nwithout keeping an easy way to get the current behavior is a good idea.\n\nOn 18/09/2024 18:51, Rubén Justo wrote:\n> The \"edit\" option allows the user to directly modify the hunk to be\n> applied.\n> \n> If the modified hunk returned by the user is not an applicable patch,\n> they will be given the opportunity to try again.\n> \n> For this new attempt we give them the original hunk;  they have to\n> repeat the modification from scratch.\n> \n> Instead, let's give them the modified patch back, so they can identify\n> and fix the problem.\n\nIt's still not clear how an inexperienced user is meant to do that.\n\n> If they really want to start over with a fresh patch they still can\n> say \"no\" to cancel the \"edit\" and start anew [*].\n\nThis is not very obvious to the user, it would be much better to give \nthem the choice when we prompt them about editing the hunk again. We've \nbeen giving the user the original hunk for the last six and a half years \nso I think it's a bit late to unilaterally change that now.\n\n> diff --git a/add-patch.c b/add-patch.c\n> index 557903310d..75b5129281 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1111,7 +1111,8 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)\n>   \thunk->colored_end = s->colored.len;\n>   }\n>   \n> -static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n> +static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk,\n\nI would add\n\t\t\t\tconst struct hunk *backup,\n\nhere\n\n> +\t\t\t      size_t plain_len, size_t colored_len)\n>   {\n>   \tsize_t i;\n>   \n> @@ -1146,6 +1147,10 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n>   \t\t\t\t      \"addp-hunk-edit.diff\", NULL) < 0)\n>   \t\treturn -1;\n>   \n> +\t/* Drop possible previous edits */\n> +\tstrbuf_setlen(&s->plain, plain_len);\n> +\tstrbuf_setlen(&s->colored, colored_len);\n\nthen we can restore the back up here with\n\n\t*hunk = *backup;\n\nThat would make it clear that we're resetting the hunk and would \ncontinue to work if we change struct hunk in the future.\n\n>   \t/* strip out commented lines */\n>   \thunk->start = s->plain.len;\n>   \tfor (i = 0; i < s->buf.len; ) {\n> @@ -1157,12 +1162,13 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n>   \t}\n>   \n>   \thunk->end = s->plain.len;\n> +\n> +\trecolor_hunk(s, hunk);\n> +\n\nThis means we're now forking an external process when there is no hunk \nto color. It would be better to avoid that by leaving this code where it \nwas and restoring the backup hunk above.\n\n>   \tif (hunk->end == hunk->start)\n>   \t\t/* The user aborted editing by deleting everything */\n>   \t\treturn 0;\n>   \n> -\trecolor_hunk(s, hunk);\n> -\n >\n>   \t\t/*\n>   \t\t * TRANSLATORS: do not translate [y/n]\n> @@ -1289,8 +1290,14 @@ static int edit_hunk_loop(struct add_p_state *s,\n>   \t\t\t\t\t\"Edit again (saying \\\"no\\\" discards!) \"\n>   \t\t\t\t\t\"[y/n]? \"));\n\nI think we should make this a three-way choice so the user can choose to \nkeep their changes or start from a valid hunk.\n\n>   \t\tif (res < 1)\n> -\t\t\treturn -1;\n> +\t\t\tbreak;\n>   \t}\n> +\n> +\t/* Drop a possible edit */\n> +\tstrbuf_setlen(&s->plain, plain_len);\n> +\tstrbuf_setlen(&s->colored, colored_len);\n> +\t*hunk = backup;\n> +\treturn -1;\n>   }\n>   \n>   static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff,\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 718438ffc7..f3206a317b 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -165,6 +165,19 @@ test_expect_success 'dummy edit works' '\n>   \tdiff_cmp expected diff\n>   '\n>   \n> +test_expect_success 'editing again works' '\n> +\tgit reset &&\n> +\twrite_script \"fake_editor.sh\" <<-\\EOF &&\n> +\tgrep been-here \"$1\" >output\n> +\techo been-here >\"$1\"\n> +\tEOF\n> +\t(\n> +\t\ttest_set_editor \"$(pwd)/fake_editor.sh\" &&\n> +\t\ttest_write_lines e y | GIT_TRACE=1 git add -p\n\nThis is still missing \"n q\". Apart from that the test is looking good.\n\nBest Wishes\n\nPhillip\n\n> +\t) &&\n> +\ttest_grep been-here output\n> +'\n> +\n>   test_expect_success 'setup patch' '\n>   \tcat >patch <<-\\EOF\n>   \t@@ -1,1 +1,4 @@\n> \n> Range-diff:\n> 1:  bcf32d0979 ! 1:  2b55a759d5 add-patch: edit the hunk again\n>      @@ add-patch.c: static int edit_hunk_manually(struct add_p_state *s, struct hunk *h\n>        \t/* strip out commented lines */\n>        \thunk->start = s->plain.len;\n>        \tfor (i = 0; i < s->buf.len; ) {\n>      +@@ add-patch.c: static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n>      + \t}\n>      +\n>      + \thunk->end = s->plain.len;\n>      ++\n>      ++\trecolor_hunk(s, hunk);\n>      ++\n>      + \tif (hunk->end == hunk->start)\n>      + \t\t/* The user aborted editing by deleting everything */\n>      + \t\treturn 0;\n>      +\n>      +-\trecolor_hunk(s, hunk);\n>      +-\n>      + \t/*\n>      + \t * If the hunk header is intact, parse it, otherwise simply use the\n>      + \t * hunk header prior to editing (which will adjust `hunk->start` to\n>       @@ add-patch.c: static int edit_hunk_loop(struct add_p_state *s,\n>        \tbackup = *hunk;\n>        \n>      @@ t/t3701-add-interactive.sh: test_expect_success 'dummy edit works' '\n>        \tdiff_cmp expected diff\n>        '\n>        \n>      -+test_expect_success 'setup re-edit editor' '\n>      -+\twrite_script \"fake_editor.sh\" <<-\\EOF &&\n>      -+\tgrep been-here \"$1\" && echo found >output\n>      -+\techo been-here > \"$1\"\n>      -+\tEOF\n>      -+\ttest_set_editor \"$(pwd)/fake_editor.sh\"\n>      -+'\n>      -+\n>       +test_expect_success 'editing again works' '\n>       +\tgit reset &&\n>      -+\ttest_write_lines e y | GIT_TRACE=1 git add -p &&\n>      -+\tgrep found output\n>      ++\twrite_script \"fake_editor.sh\" <<-\\EOF &&\n>      ++\tgrep been-here \"$1\" >output\n>      ++\techo been-here >\"$1\"\n>      ++\tEOF\n>      ++\t(\n>      ++\t\ttest_set_editor \"$(pwd)/fake_editor.sh\" &&\n>      ++\t\ttest_write_lines e y | GIT_TRACE=1 git add -p\n>      ++\t) &&\n>      ++\ttest_grep been-here output\n>       +'\n>       +\n>        test_expect_success 'setup patch' '\n"},{"id":"503259","messageId":"xmqqbk0e5pff.fsf@gitster.g","threadId":"62113","inReplyTo":"2ad1f7b1-714c-4d6e-89a6-fd65271222b9@gmail.com","subject":"Re: [PATCH v2] add-patch: edit the hunk again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-23T16:02:12Z","receivedAt":"2024-09-23T16:02:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"phillip.wood123@gmail.com writes:\n\n> Thanks for the re-roll. I'm still not convinced that changing this\n> without keeping an easy way to get the current behavior is a good\n> idea.\n>\n> This is not very obvious to the user, it would be much better to give\n> them the choice when we prompt them about editing the hunk\n> again. We've been giving the user the original hunk for the last six\n> and a half years so I think it's a bit late to unilaterally change\n> that now.\n\nI almost never use the (e)dit in \"add -p\", but after trying it and\ndeliberately screwing up the edit, I tend to agree with you.  It is\nvery easy to lose what the original change was, what you wanted it\nto say after the edit in the end state, and how the patch for the\ncurrent state should look like, and being able to easily start over\n(and more importantly, knowing that I'd get the version that has\nnone of my screw-ups) was the only thing that convinced me that I\nmight in the future try to use the (e)dit mode again when I find an\napplicable situation.\n\nThanks for review.\n"},{"id":"503449","messageId":"6f392446-10b4-4074-a993-97ac444275f8@gmail.com","threadId":"62113","inReplyTo":"2ad1f7b1-714c-4d6e-89a6-fd65271222b9@gmail.com","subject":"Re: [PATCH v2] add-patch: edit the hunk again","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-09-24T22:54:22Z","receivedAt":"2024-09-24T22:54:26Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Sep 23, 2024 at 10:07:08AM +0100, phillip.wood123@gmail.com wrote:\n\n> Thanks for the re-roll. I'm still not convinced that changing this without\n> keeping an easy way to get the current behavior is a good idea.\n\nThank you for maintaining an open attitude and helping make the change\nreasonable.\n\n> \n> On 18/09/2024 18:51, Rubén Justo wrote:\n> > The \"edit\" option allows the user to directly modify the hunk to be\n> > applied.\n> > \n> > If the modified hunk returned by the user is not an applicable patch,\n> > they will be given the opportunity to try again.\n> > \n> > For this new attempt we give them the original hunk;  they have to\n> > repeat the modification from scratch.\n> > \n> > Instead, let's give them the modified patch back, so they can identify\n> > and fix the problem.\n> \n> It's still not clear how an inexperienced user is meant to do that.\n\nIf inexperience refers to the user not being familiar with the patch\nformat, I don't think it's something we should worry about.  It is\nvery likely that few users will attempt to (e)dit, and those who do,\ninexperienced users experimenting, the normal (and sensible) path they\nwill follow at the slightest difficulty will probably be to abandon\npatch editing and try a more accessible option, such as: cancel the\n`add -p` session, edit normally, and restart the session.\n\nIf inexperience refers to users unfamiliar with the (e)dit option but\nknowledgeable about the patch format, I believe they will have enough\nexperience to know that reconstructing a patch is a tricky task.\nAgain, the most likely path they will follow when encountering\ndifficulties will be to restart the edit (Junio's message in the\nthread is an example of this).\n\nAt any rate, although I understand the concern, this series doesn't\naim to improve the mechanisms that help identify the problems in a\nfaulty patch.  The goal of this series is to offer (actually to\nrecover) the possibility for the user to make corrections.\n\nSomething I haven't mentioned before in this thread is that currently,\nif the user makes a mistake while editing a patch, we don't give them\nthe opportunity to review their error.  We leave them in doubt.  This\nhappened recently to me editing a patch with context lines containing\nwhitespace errors and \"whitespace=fix\".  But that's another story that\nI still need to work on [1].\n\nSo, to me, it seems sensible to let the user review the faulty patch,\neven if it's only to discard it.\n\n> \n> > If they really want to start over with a fresh patch they still can\n> > say \"no\" to cancel the \"edit\" and start anew [*].\n> \n> This is not very obvious to the user,\n\nIt has been so for a decade...\n\nKeep in mind that this message will probably only be shown very _very_\nrarely to users who are most likely very familiar with (e)dit.\n\n> it would be much better to give them\n> the choice when we prompt them about editing the hunk again.\n\nThat's an option I explored but abandoned.\n\nI didn't come up with any message that I liked that wasn't literally a\nlong paragraph.\n\nIn the end, I gave up in favor of what I believe is a better option,\nwhich is recovering the original intention of \"no\".\n\nI'm not against the option you propose, I'm just not convinced that\nwhat we already have, since ac083c47ea (git-add--interactive: manual\nhunk editing mode, 2008-07-03), isn't intuitive enough for the\nusers of (e)dit.\n\n> We've been\n> giving the user the original hunk for the last six and a half years so I\n> think it's a bit late to unilaterally change that now.\n\nFor me, this isn't a reason not to make the change.\n\n> \n> > diff --git a/add-patch.c b/add-patch.c\n> > index 557903310d..75b5129281 100644\n> > --- a/add-patch.c\n> > +++ b/add-patch.c\n> > @@ -1111,7 +1111,8 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)\n> >   \thunk->colored_end = s->colored.len;\n> >   }\n> > -static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n> > +static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk,\n> \n> I would add\n> \t\t\t\tconst struct hunk *backup,\n> \n> here\n> \n> > +\t\t\t      size_t plain_len, size_t colored_len)\n> >   {\n> >   \tsize_t i;\n> > @@ -1146,6 +1147,10 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n> >   \t\t\t\t      \"addp-hunk-edit.diff\", NULL) < 0)\n> >   \t\treturn -1;\n> > +\t/* Drop possible previous edits */\n> > +\tstrbuf_setlen(&s->plain, plain_len);\n> > +\tstrbuf_setlen(&s->colored, colored_len);\n> \n> then we can restore the back up here with\n> \n> \t*hunk = *backup;\n> \n> That would make it clear that we're resetting the hunk and would continue to\n> work if we change struct hunk in the future.\n> \n> >   \t/* strip out commented lines */\n> >   \thunk->start = s->plain.len;\n> >   \tfor (i = 0; i < s->buf.len; ) {\n> > @@ -1157,12 +1162,13 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n> >   \t}\n> >   \thunk->end = s->plain.len;\n> > +\n> > +\trecolor_hunk(s, hunk);\n> > +\n> \n> This means we're now forking an external process when there is no hunk to\n> color. It would be better to avoid that by leaving this code where it was\n> and restoring the backup hunk above.\n\nI don't see that external process. ¿?\n\nAfter reviewing the code in the previous iteration, based on your\ncomments, I concluded that `recolor_hunk()` makes more sense before\nthe \"return 0\".  Even if we end up discarding this series.  I think.\n\n> \n> >   \tif (hunk->end == hunk->start)\n> >   \t\t/* The user aborted editing by deleting everything */\n> >   \t\treturn 0;\n> > -\trecolor_hunk(s, hunk);\n> > -\n> >\n> >   \t\t/*\n> >   \t\t * TRANSLATORS: do not translate [y/n]\n> > @@ -1289,8 +1290,14 @@ static int edit_hunk_loop(struct add_p_state *s,\n> >   \t\t\t\t\t\"Edit again (saying \\\"no\\\" discards!) \"\n> >   \t\t\t\t\t\"[y/n]? \"));\n> \n> I think we should make this a three-way choice so the user can choose to\n> keep their changes or start from a valid hunk.\n\nAs I said above, I don't object to this, but I'm not convinced it's\nnecessary.\n\n> \n> >   \t\tif (res < 1)\n> > -\t\t\treturn -1;\n> > +\t\t\tbreak;\n> >   \t}\n> > +\n> > +\t/* Drop a possible edit */\n> > +\tstrbuf_setlen(&s->plain, plain_len);\n> > +\tstrbuf_setlen(&s->colored, colored_len);\n> > +\t*hunk = backup;\n> > +\treturn -1;\n> >   }\n> >   static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff,\n> > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> > index 718438ffc7..f3206a317b 100755\n> > --- a/t/t3701-add-interactive.sh\n> > +++ b/t/t3701-add-interactive.sh\n> > @@ -165,6 +165,19 @@ test_expect_success 'dummy edit works' '\n> >   \tdiff_cmp expected diff\n> >   '\n> > +test_expect_success 'editing again works' '\n> > +\tgit reset &&\n> > +\twrite_script \"fake_editor.sh\" <<-\\EOF &&\n> > +\tgrep been-here \"$1\" >output\n> > +\techo been-here >\"$1\"\n> > +\tEOF\n> > +\t(\n> > +\t\ttest_set_editor \"$(pwd)/fake_editor.sh\" &&\n> > +\t\ttest_write_lines e y | GIT_TRACE=1 git add -p\n> \n> This is still missing \"n q\". Apart from that the test is looking good.\n\nI've been resisting the idea of \"completeness\", because I think \"e y\"\nshould also be fine.  But I'm not going to resist anymore here :-),\nsince I don't think the test has much more value without \"n q\".  So\nI'll add it.\n"},{"id":"503632","messageId":"74289d8b-7211-452a-ac76-f733e89112e6@gmail.com","threadId":"62113","inReplyTo":"4dd5a2c7-26a8-470f-b651-e1fe2d1dbcec@gmail.com","subject":"[PATCH v3] add-patch: edit the hunk again","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-09-28T14:30:14Z","receivedAt":"2024-09-28T14:30:17Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"The \"edit\" option allows the user to directly modify the hunk to be\napplied.\n\nIf the modified hunk returned by the user is not an applicable patch,\nthey will be given the opportunity to try again.\n\nFor this new attempt we give them the original hunk;  they have to\nrepeat the modification from scratch.\n\nInstead, let's give them the modified patch back, so they can identify\nand fix the problem.\n\nIf they really want to start over with a fresh patch they still can\nsay 'no', to cancel the \"edit\" and start anew [*].\n\n    * In the old script-based version of \"add -p\", this \"no\" meant\n      discarding the hunk and moving on to the next one.\n\n      This changed, probably unintentionally, during its conversion to\n      C in bcdd297b78 (built-in add -p: implement hunk editing,\n      2019-12-13).\n\n      It now makes more sense not to move to the next block when the\n      user requests to discard their edits.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n\nIn this iteration, I'm modifying the message 'saying no discards' to\nmake its meaning more explicit, perhaps also gaining some clarity\nalong the way for the user who wants to restart editing from the\noriginal patch.\n\nIn the test, I'm adding \"n q\" to the script as suggested by Phillip.\n\nAnd, just a bit of mental peace by restoring the hunk from the backup\nbefore trimming the two strbuf.\n\nThanks.\n\n add-patch.c                | 33 ++++++++++++++++++++-------------\n t/t3701-add-interactive.sh | 13 +++++++++++++\n 2 files changed, 33 insertions(+), 13 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 557903310d..c847b4a59d 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1111,7 +1111,8 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)\n \thunk->colored_end = s->colored.len;\n }\n \n-static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n+static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk,\n+\t\t\t      size_t plain_len, size_t colored_len)\n {\n \tsize_t i;\n \n@@ -1146,6 +1147,10 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n \t\t\t\t      \"addp-hunk-edit.diff\", NULL) < 0)\n \t\treturn -1; \n \n+\t/* Drop possible previous edits */\n+\tstrbuf_setlen(&s->plain, plain_len);\n+\tstrbuf_setlen(&s->colored, colored_len);\n+\n \t/* strip out commented lines */\n \thunk->start = s->plain.len;\n \tfor (i = 0; i < s->buf.len; ) {\n@@ -1157,12 +1162,13 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n \t}\n \n \thunk->end = s->plain.len;\n+\n+\trecolor_hunk(s, hunk);\n+\n \tif (hunk->end == hunk->start)\n \t\t/* The user aborted editing by deleting everything */\n \t\treturn 0;\n \n-\trecolor_hunk(s, hunk);\n-\n \t/*\n \t * If the hunk header is intact, parse it, otherwise simply use the\n \t * hunk header prior to editing (which will adjust `hunk->start` to\n@@ -1257,15 +1263,14 @@ static int edit_hunk_loop(struct add_p_state *s,\n \tbackup = *hunk;\n \n \tfor (;;) {\n-\t\tint res = edit_hunk_manually(s, hunk);\n+\t\tint res = edit_hunk_manually(s, hunk, plain_len, colored_len);\n \t\tif (res == 0) {\n \t\t\t/* abandoned */\n-\t\t\t*hunk = backup;\n-\t\t\treturn -1;\n+\t\t\tbreak;\n \t\t}\n \n \t\tif (res > 0) {\n-\t\t\thunk->delta +=\n+\t\t\thunk->delta = backup.delta +\n \t\t\t\trecount_edited_hunk(s, hunk,\n \t\t\t\t\t\t    backup.header.old_count,\n \t\t\t\t\t\t    backup.header.new_count);\n@@ -1273,10 +1278,6 @@ static int edit_hunk_loop(struct add_p_state *s,\n \t\t\t\treturn 0;\n \t\t}\n \n-\t\t/* Drop edits (they were appended to s->plain) */\n-\t\tstrbuf_setlen(&s->plain, plain_len);\n-\t\tstrbuf_setlen(&s->colored, colored_len);\n-\t\t*hunk = backup;\n \n \t\t/*\n \t\t * TRANSLATORS: do not translate [y/n]\n@@ -1286,11 +1287,17 @@ static int edit_hunk_loop(struct add_p_state *s,\n \t\t * of the word \"no\" does not start with n.\n \t\t */\n \t\tres = prompt_yesno(s, _(\"Your edited hunk does not apply. \"\n-\t\t\t\t\t\"Edit again (saying \\\"no\\\" discards!) \"\n+\t\t\t\t\t\"Edit again (saying \\\"no\\\" discards your edits!) \"\n \t\t\t\t\t\"[y/n]? \"));\n \t\tif (res < 1)\n-\t\t\treturn -1;\n+\t\t\tbreak;\n \t}\n+\n+\t/* Drop a possible edit */\n+\t*hunk = backup;\n+\tstrbuf_setlen(&s->plain, plain_len);\n+\tstrbuf_setlen(&s->colored, colored_len);\n+\treturn -1;\n }\n \n static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff,\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 718438ffc7..1ceefd96e6 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -165,6 +165,19 @@ test_expect_success 'dummy edit works' '\n \tdiff_cmp expected diff\n '\n \n+test_expect_success 'editing again works' '\n+\tgit reset &&\n+\twrite_script \"fake_editor.sh\" <<-\\EOF &&\n+\tgrep been-here \"$1\" >output\n+\techo been-here >\"$1\"\n+\tEOF\n+\t(\n+\t\ttest_set_editor \"$(pwd)/fake_editor.sh\" &&\n+\t\ttest_write_lines e y n q | GIT_TRACE=1 git add -p\n+\t) &&\n+\ttest_grep been-here output\n+'\n+\n test_expect_success 'setup patch' '\n \tcat >patch <<-\\EOF\n \t@@ -1,1 +1,4 @@\n\nRange-diff against v2:\n1:  2b55a759d5 ! 1:  7e76606751 add-patch: edit the hunk again\n    @@ add-patch.c: static int edit_hunk_loop(struct add_p_state *s,\n      \t\t/*\n      \t\t * TRANSLATORS: do not translate [y/n]\n     @@ add-patch.c: static int edit_hunk_loop(struct add_p_state *s,\n    - \t\t\t\t\t\"Edit again (saying \\\"no\\\" discards!) \"\n    + \t\t * of the word \"no\" does not start with n.\n    + \t\t */\n    + \t\tres = prompt_yesno(s, _(\"Your edited hunk does not apply. \"\n    +-\t\t\t\t\t\"Edit again (saying \\\"no\\\" discards!) \"\n    ++\t\t\t\t\t\"Edit again (saying \\\"no\\\" discards your edits!) \"\n      \t\t\t\t\t\"[y/n]? \"));\n      \t\tif (res < 1)\n     -\t\t\treturn -1;\n    @@ add-patch.c: static int edit_hunk_loop(struct add_p_state *s,\n      \t}\n     +\n     +\t/* Drop a possible edit */\n    ++\t*hunk = backup;\n     +\tstrbuf_setlen(&s->plain, plain_len);\n     +\tstrbuf_setlen(&s->colored, colored_len);\n    -+\t*hunk = backup;\n     +\treturn -1;\n      }\n      \n    @@ t/t3701-add-interactive.sh: test_expect_success 'dummy edit works' '\n     +\tEOF\n     +\t(\n     +\t\ttest_set_editor \"$(pwd)/fake_editor.sh\" &&\n    -+\t\ttest_write_lines e y | GIT_TRACE=1 git add -p\n    ++\t\ttest_write_lines e y n q | GIT_TRACE=1 git add -p\n     +\t) &&\n     +\ttest_grep been-here output\n     +'\n-- \n2.47.0.rc0.1.g1645ecd054\n"},{"id":"503830","messageId":"556ba87e-1eee-438d-848f-bbc5558289fe@gmail.com","threadId":"62113","inReplyTo":"6f392446-10b4-4074-a993-97ac444275f8@gmail.com","subject":"Re: [PATCH v2] add-patch: edit the hunk again","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-10-01T10:02:53Z","receivedAt":"2024-10-01T10:02:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 24/09/2024 23:54, Rubén Justo wrote:\n> On Mon, Sep 23, 2024 at 10:07:08AM +0100, phillip.wood123@gmail.com wrote:\n >\n> So, to me, it seems sensible to let the user review the faulty patch,\n> even if it's only to discard it.\n\nI agree we could add that option but not as the default\n\n>>> If they really want to start over with a fresh patch they still can\n>>> say \"no\" to cancel the \"edit\" and start anew [*].\n>>\n>> This is not very obvious to the user,\n> \n> It has been so for a decade...\n\nThat does not make it obvious though\n\n> Keep in mind that this message will probably only be shown very _very_\n> rarely to users who are most likely very familiar with (e)dit.\n\nI'd argue that users who are not familiar with (e)dit are more likely to \nmake mistakes when editing hunks and are less likely to be able to fix them.\n\n>>> +\n>>> +\trecolor_hunk(s, hunk);\n>>> +\n>>\n>> This means we're now forking an external process when there is no hunk to\n>> color. It would be better to avoid that by leaving this code where it was\n>> and restoring the backup hunk above.\n> \n> I don't see that external process. ¿?\n\nOh sorry, I thought we ran interactive.diffFilter on the edited hunk, \nbut we don't. I'll try and find time to fix that.\n\n>> This is still missing \"n q\". Apart from that the test is looking good.\n> \n> I've been resisting the idea of \"completeness\", because I think \"e y\"\n> should also be fine.  But I'm not going to resist anymore here :-),\n> since I don't think the test has much more value without \"n q\".  So\n> I'll add it.\n\nThe reason I think we should have it is that the tests ought to be \ntesting realistic user input and not rely on getting EOF which is \nunlikely to happen in real life.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"503831","messageId":"9c7af640-ee3a-4a17-84f6-f56fee7efe37@gmail.com","threadId":"62113","inReplyTo":"74289d8b-7211-452a-ac76-f733e89112e6@gmail.com","subject":"Re: [PATCH v3] add-patch: edit the hunk again","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-10-01T10:03:10Z","receivedAt":"2024-10-01T10:03:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 28/09/2024 15:30, Rubén Justo wrote:\n> The \"edit\" option allows the user to directly modify the hunk to be\n> applied.\n> \n> If the modified hunk returned by the user is not an applicable patch,\n> they will be given the opportunity to try again.\n> \n> For this new attempt we give them the original hunk;  they have to\n> repeat the modification from scratch.\n> \n> Instead, let's give them the modified patch back, so they can identify\n> and fix the problem.\n> \n> If they really want to start over with a fresh patch they still can\n> say 'no', to cancel the \"edit\" and start anew [*].\n> \n>      * In the old script-based version of \"add -p\", this \"no\" meant\n>        discarding the hunk and moving on to the next one.\n> \n>        This changed, probably unintentionally, during its conversion to\n>        C in bcdd297b78 (built-in add -p: implement hunk editing,\n>        2019-12-13).\n> \n>        It now makes more sense not to move to the next block when the\n>        user requests to discard their edits.\n> \n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> ---\n> \n> In this iteration, I'm modifying the message 'saying no discards' to\n> make its meaning more explicit, perhaps also gaining some clarity\n> along the way for the user who wants to restart editing from the\n> original patch.\n> \n> In the test, I'm adding \"n q\" to the script as suggested by Phillip.\n> \n> And, just a bit of mental peace by restoring the hunk from the backup\n> before trimming the two strbuf.\n\nI hoped that change would be in edit_hunk_manually() but it isn't.\n\nI'm afraid I still don't think that changing the default is a good idea \nas it is often very difficult to correct a badly edited hunk. Can we \noffer the user a choice of\n\n     (e) edit the original hunk again\n     (f) fix the edited hunk\n     (d) discard the edit\n\nIn [1] you say you discarded that idea because the wording was too \nverbose but something along like the above should be succinct enough.\n\nBest Wishes\n\nPhillip\n\n[1] \nhttps://lore.kernel.org/git/6f392446-10b4-4074-a993-97ac444275f8@gmail.com\n\n> Thanks.\n> \n>   add-patch.c                | 33 ++++++++++++++++++++-------------\n>   t/t3701-add-interactive.sh | 13 +++++++++++++\n>   2 files changed, 33 insertions(+), 13 deletions(-)\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index 557903310d..c847b4a59d 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1111,7 +1111,8 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)\n>   \thunk->colored_end = s->colored.len;\n>   }\n>   \n> -static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n> +static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk,\n> +\t\t\t      size_t plain_len, size_t colored_len)\n>   {\n>   \tsize_t i;\n>   \n> @@ -1146,6 +1147,10 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n>   \t\t\t\t      \"addp-hunk-edit.diff\", NULL) < 0)\n>   \t\treturn -1;\n>   \n> +\t/* Drop possible previous edits */\n> +\tstrbuf_setlen(&s->plain, plain_len);\n> +\tstrbuf_setlen(&s->colored, colored_len);\n> +\n>   \t/* strip out commented lines */\n>   \thunk->start = s->plain.len;\n>   \tfor (i = 0; i < s->buf.len; ) {\n> @@ -1157,12 +1162,13 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n>   \t}\n>   \n>   \thunk->end = s->plain.len;\n> +\n> +\trecolor_hunk(s, hunk);\n> +\n>   \tif (hunk->end == hunk->start)\n>   \t\t/* The user aborted editing by deleting everything */\n>   \t\treturn 0;\n>   \n> -\trecolor_hunk(s, hunk);\n> -\n>   \t/*\n>   \t * If the hunk header is intact, parse it, otherwise simply use the\n>   \t * hunk header prior to editing (which will adjust `hunk->start` to\n> @@ -1257,15 +1263,14 @@ static int edit_hunk_loop(struct add_p_state *s,\n>   \tbackup = *hunk;\n>   \n>   \tfor (;;) {\n> -\t\tint res = edit_hunk_manually(s, hunk);\n> +\t\tint res = edit_hunk_manually(s, hunk, plain_len, colored_len);\n>   \t\tif (res == 0) {\n>   \t\t\t/* abandoned */\n> -\t\t\t*hunk = backup;\n> -\t\t\treturn -1;\n> +\t\t\tbreak;\n>   \t\t}\n>   \n>   \t\tif (res > 0) {\n> -\t\t\thunk->delta +=\n> +\t\t\thunk->delta = backup.delta +\n>   \t\t\t\trecount_edited_hunk(s, hunk,\n>   \t\t\t\t\t\t    backup.header.old_count,\n>   \t\t\t\t\t\t    backup.header.new_count);\n> @@ -1273,10 +1278,6 @@ static int edit_hunk_loop(struct add_p_state *s,\n>   \t\t\t\treturn 0;\n>   \t\t}\n>   \n> -\t\t/* Drop edits (they were appended to s->plain) */\n> -\t\tstrbuf_setlen(&s->plain, plain_len);\n> -\t\tstrbuf_setlen(&s->colored, colored_len);\n> -\t\t*hunk = backup;\n>   \n>   \t\t/*\n>   \t\t * TRANSLATORS: do not translate [y/n]\n> @@ -1286,11 +1287,17 @@ static int edit_hunk_loop(struct add_p_state *s,\n>   \t\t * of the word \"no\" does not start with n.\n>   \t\t */\n>   \t\tres = prompt_yesno(s, _(\"Your edited hunk does not apply. \"\n> -\t\t\t\t\t\"Edit again (saying \\\"no\\\" discards!) \"\n> +\t\t\t\t\t\"Edit again (saying \\\"no\\\" discards your edits!) \"\n>   \t\t\t\t\t\"[y/n]? \"));\n>   \t\tif (res < 1)\n> -\t\t\treturn -1;\n> +\t\t\tbreak;\n>   \t}\n> +\n> +\t/* Drop a possible edit */\n> +\t*hunk = backup;\n> +\tstrbuf_setlen(&s->plain, plain_len);\n> +\tstrbuf_setlen(&s->colored, colored_len);\n> +\treturn -1;\n>   }\n>   \n>   static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff,\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 718438ffc7..1ceefd96e6 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -165,6 +165,19 @@ test_expect_success 'dummy edit works' '\n>   \tdiff_cmp expected diff\n>   '\n>   \n> +test_expect_success 'editing again works' '\n> +\tgit reset &&\n> +\twrite_script \"fake_editor.sh\" <<-\\EOF &&\n> +\tgrep been-here \"$1\" >output\n> +\techo been-here >\"$1\"\n> +\tEOF\n> +\t(\n> +\t\ttest_set_editor \"$(pwd)/fake_editor.sh\" &&\n> +\t\ttest_write_lines e y n q | GIT_TRACE=1 git add -p\n> +\t) &&\n> +\ttest_grep been-here output\n> +'\n> +\n>   test_expect_success 'setup patch' '\n>   \tcat >patch <<-\\EOF\n>   \t@@ -1,1 +1,4 @@\n> \n> Range-diff against v2:\n> 1:  2b55a759d5 ! 1:  7e76606751 add-patch: edit the hunk again\n>      @@ add-patch.c: static int edit_hunk_loop(struct add_p_state *s,\n>        \t\t/*\n>        \t\t * TRANSLATORS: do not translate [y/n]\n>       @@ add-patch.c: static int edit_hunk_loop(struct add_p_state *s,\n>      - \t\t\t\t\t\"Edit again (saying \\\"no\\\" discards!) \"\n>      + \t\t * of the word \"no\" does not start with n.\n>      + \t\t */\n>      + \t\tres = prompt_yesno(s, _(\"Your edited hunk does not apply. \"\n>      +-\t\t\t\t\t\"Edit again (saying \\\"no\\\" discards!) \"\n>      ++\t\t\t\t\t\"Edit again (saying \\\"no\\\" discards your edits!) \"\n>        \t\t\t\t\t\"[y/n]? \"));\n>        \t\tif (res < 1)\n>       -\t\t\treturn -1;\n>      @@ add-patch.c: static int edit_hunk_loop(struct add_p_state *s,\n>        \t}\n>       +\n>       +\t/* Drop a possible edit */\n>      ++\t*hunk = backup;\n>       +\tstrbuf_setlen(&s->plain, plain_len);\n>       +\tstrbuf_setlen(&s->colored, colored_len);\n>      -+\t*hunk = backup;\n>       +\treturn -1;\n>        }\n>        \n>      @@ t/t3701-add-interactive.sh: test_expect_success 'dummy edit works' '\n>       +\tEOF\n>       +\t(\n>       +\t\ttest_set_editor \"$(pwd)/fake_editor.sh\" &&\n>      -+\t\ttest_write_lines e y | GIT_TRACE=1 git add -p\n>      ++\t\ttest_write_lines e y n q | GIT_TRACE=1 git add -p\n>       +\t) &&\n>       +\ttest_grep been-here output\n>       +'\n\n"},{"id":"503853","messageId":"xmqqv7ybyabs.fsf@gitster.g","threadId":"62113","inReplyTo":"9c7af640-ee3a-4a17-84f6-f56fee7efe37@gmail.com","subject":"Re: [PATCH v3] add-patch: edit the hunk again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-01T17:58:15Z","receivedAt":"2024-10-01T17:58:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> I hoped that change would be in edit_hunk_manually() but it isn't.\n>\n> I'm afraid I still don't think that changing the default is a good\n> idea as it is often very difficult to correct a badly edited hunk. Can\n> we offer the user a choice of\n>\n>     (e) edit the original hunk again\n>     (f) fix the edited hunk\n>     (d) discard the edit\n>\n> In [1] you say you discarded that idea because the wording was too\n> verbose but something along like the above should be succinct enough.\n\nI too disagree with the change of the default, but I would not\ncomplain if we offered the feature to re-edit as long as it is\nclearly marked as a new optional choice.\n\nPhillip, thanks for being firm yet still constructive.\n\n"},{"id":"503963","messageId":"b1408033-5366-4e2f-823f-7957a9f30fe9@gmail.com","threadId":"62113","inReplyTo":"556ba87e-1eee-438d-848f-bbc5558289fe@gmail.com","subject":"Re: [PATCH v2] add-patch: edit the hunk again","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-10-02T16:36:09Z","receivedAt":"2024-10-02T16:36:12Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Tue, Oct 01, 2024 at 11:02:53AM +0100, Phillip Wood wrote:\n\n> I'd argue that users who are not familiar with (e)dit are more likely to\n> make mistakes when editing hunks and are less likely to be able to fix them.\n\nThis series is not about making errors more descriptive or making\n(e)dit more (or less) accessible.\n\nEditing the original hunk is already quite challenging and prone to\nerrors.\n\nThis series is about regaining the possibility for the user to see\nand correct their mistakes.\n\n> > > This is still missing \"n q\". Apart from that the test is looking good.\n> > \n> > I've been resisting the idea of \"completeness\", because I think \"e y\"\n> > should also be fine.  But I'm not going to resist anymore here :-),\n> > since I don't think the test has much more value without \"n q\".  So\n> > I'll add it.\n> \n> The reason I think we should have it is that the tests ought to be testing\n> realistic user input and not rely on getting EOF which is unlikely to happen\n> in real life.\n\nSometimes, I use ctrl+d instead of 'q'.  So, some of my interactive\n\"add -p\" sessions can be described better with \"y\" than with \"y q\" or\n\"y n... q\".  And I'm real ;-)\n\nOf course, non-interactively: \"e y\" or \"y\" work as expected as the\ntest in this series and others in t3701 demonstrate.\n\nSince we already have other tests, I didn't mind adding \"n q\" in an\nattempt to move the series forward.\n"},{"id":"503965","messageId":"2ed4f980-a294-4a39-9c74-f63ce9af1f70@gmail.com","threadId":"62113","inReplyTo":"9c7af640-ee3a-4a17-84f6-f56fee7efe37@gmail.com","subject":"Re: [PATCH v3] add-patch: edit the hunk again","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-10-02T17:27:34Z","receivedAt":"2024-10-02T17:27:37Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Tue, Oct 01, 2024 at 11:03:10AM +0100, Phillip Wood wrote:\n\n> I'm afraid I still don't think that changing the default is a good idea as\n> it is often very difficult to correct a badly edited hunk.\n\nThis series isn't about how hard it is to fix a badly edited hunk.\n\n> In [1] you say you discarded that idea because the wording was too verbose\n\nNot really.  I still believe that regaining the original intention of\n\"no\" is a better option than adding new options to the interface.\n\nI am not opposed to that change, I just think it's an unnecessary\ncomplication.\n\nA user who experiences problems with a badly edited hunk, edited by\nthemselves, will probably encounter similar issues as they would when\nediting the original hunk.  I think.\n\nI don't think reconstructing a patch is a realistic (sensible)\nscenario that we should be concerned about.\n\nThe small change in the message, in this iteration, adds a bit of\nclarity for them, I think:\n\n> > @@ -1286,11 +1287,17 @@ static int edit_hunk_loop(struct add_p_state *s,\n> >   \t\t * of the word \"no\" does not start with n.\n> >   \t\t */\n> >   \t\tres = prompt_yesno(s, _(\"Your edited hunk does not apply. \"\n> > -\t\t\t\t\t\"Edit again (saying \\\"no\\\" discards!) \"\n> > +\t\t\t\t\t\"Edit again (saying \\\"no\\\" discards your edits!) \"\n> >   \t\t\t\t\t\"[y/n]? \"));\n\n> \n> [1]\n> https://lore.kernel.org/git/6f392446-10b4-4074-a993-97ac444275f8@gmail.com\n"},{"id":"503966","messageId":"ee33072c-8759-4c42-ad36-3c33ef5d1c57@gmail.com","threadId":"62113","inReplyTo":"xmqqv7ybyabs.fsf@gitster.g","subject":"Re: [PATCH v3] add-patch: edit the hunk again","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-10-02T17:34:02Z","receivedAt":"2024-10-02T17:34:05Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Tue, Oct 01, 2024 at 10:58:15AM -0700, Junio C Hamano wrote:\n\n> I would not\n> complain if we offered the feature to re-edit\n\nI'm using this:\n\n   $ GIT_EDITOR='bash -c \"vim \\\"$1\\\" && cp -f $1 /tmp/backup\"' git add -p\n\nThat way, I have a copy of the edited hunk in case I want to try again\nor review it.  In that case, I can simply try again and reload from\n\"/tmp/backup\".\n\nI'm sure there's a more clever way, but I haven't come up with\nanything more elegant.\n\nMaybe interactive.editor, apply.editor, add.editor?\n"}]}