On Mon, Sep 16, 2024 at 02:33:54PM +0100, Phillip Wood wrote:

quoted

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.

quoted

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.

quoted

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?

quoted

@@ -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?

quoted

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.

quoted

+	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.

quoted

+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.

quoted

+	grep found output

Using test_grep makes it easier to debug test failures.


Best Wishes

Phillip

Thanks for your review.