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

["Rubén Justo" <rjusto@gmail.com>]

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.