On Mon, Sep 16, 2024 at 02:33:54PM +0100, Phillip Wood wrote: > > The "edit" option allows the user to directly modify the hunk to be > > applied. > > > > If the modified hunk returned is not an applicable patch, we give the > > opportunity to try again. > > > > For this new attempt we provide, again, the original hunk; the user > > has to repeat the modification from scratch. > > As you say below it looks like we started doing this by accident with > 2b8ea7f3c7 (add -p: calculate offset delta for edited patches, 2018-03-05). > I think that although the change was accidental it was actually a move in > the right direction for several reasons. > > - The error message from "git apply" makes it is virtually impossible > to tell what is wrong with the edited patch. The line numbers in the > error message refer to the complete patch but the user is editing a > single hunk so the user has no idea which line of the hunk the error > message applies to. > > - If the user uses a terminal based editor then they cannot see the > error messages while they're re-editing the hunk. > > - If the user has deleted a pre-image line then they need to somehow > magic it back before the hunk will apply. > > > Instead, let's give them the faulty modified patch back, so they can > > identify and fix the problem. > > The problem is how do they identify the problem? I have some unfinished > patches [1] that annotate the edited patch with comments explaining what's > wrong. Because we know what the unedited patch looked like and that the > pre-image lines should be unchanged it is possible to provide much better > error messages than we get from trying to apply the whole patch with "git > apply". It also makes it possible to restore deleted pre-image lines. > > [1] https://github.com/phillipwood/git/tree/wip/add-p-editing-improvements > Note that the later patches do not even compile at the moment. I've > been meaning to split out the first eight patches and clean them up > as they're mostly functional and just need the commit messages > cleaning up. 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. Am I misunderstanding your envision? 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. > > > 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? > > > @@ -1273,10 +1277,6 @@ static int edit_hunk_loop(struct add_p_state *s, > > return 0; > > } > > - /* Drop edits (they were appended to s->plain) */ > > - strbuf_setlen(&s->plain, plain_len); > > - strbuf_setlen(&s->colored, colored_len); > > - *hunk = backup; > > In the old version we always restore the hunk from the backup when we trim > the edited patch which maintains the invariant "hunk->end <= s->plain->end" Same here. Are we losing that invariant? > > > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > > index 718438ffc7..6af5636221 100755 > > --- a/t/t3701-add-interactive.sh > > +++ b/t/t3701-add-interactive.sh > > @@ -165,6 +165,20 @@ test_expect_success 'dummy edit works' ' > > diff_cmp expected diff > > ' > > +test_expect_success 'setup re-edit editor' ' > > + write_script "fake_editor.sh" <<-\EOF && > > + grep been-here "$1" && echo found >output > > 'grep been-here "$1" >output' should be sufficient I think As I was writing the test, it was clearer to me using "&& echo found" here and "grep found" below. > > > + 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. > > > +test_expect_success 'editing again works' ' > > + git reset && > > + test_write_lines e y | GIT_TRACE=1 git add -p && > > It would be nice to add "n q" to the input to make it complete. I have no objection to that. > > > + grep found output > > Using test_grep makes it easier to debug test failures. > > > Best Wishes > > Phillip Thanks for your review.
