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

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

The "edit" option allows the user to directly modify the hunk to be
applied.

If the modified hunk returned by the user is not an applicable patch,
they will be given the opportunity to try again.

For this new attempt we give them the original hunk;  they have to
repeat the modification from scratch.

Instead, let's give them the modified patch back, so they can identify
and fix the problem.

If they really want to start over with a fresh patch they still can
say "no" to cancel the "edit" and start anew [*].

    * In the old script-based version of "add -p", this "no" meant
      discarding the hunk and moving on to the next one.

      This changed, probably unintentionally, during its conversion to
      C in bcdd297b78 (built-in add -p: implement hunk editing,
      2019-12-13).

      It now makes more sense not to move to the next hunk when the
      user requests to discard their edits.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

This iteration addresses Phillip's comments:

 - Ensure that `edit_hunk_manually()` exits with sane values in
   `hunk`.

 - Merge the tests into a single one and use a subshell to prevent
   leaking `EDITOR`.

Thanks.

 add-patch.c                | 31 +++++++++++++++++++------------
 t/t3701-add-interactive.sh | 13 +++++++++++++
 2 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 557903310d..75b5129281 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1111,7 +1111,8 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk)
 	hunk->colored_end = s->colored.len;
 }
 
-static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
+static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk,
+			      size_t plain_len, size_t colored_len)
 {
 	size_t i;
 
@@ -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);
+
 	/* strip out commented lines */
 	hunk->start = s->plain.len;
 	for (i = 0; i < s->buf.len; ) {
@@ -1157,12 +1162,13 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
 	}
 
 	hunk->end = s->plain.len;
+
+	recolor_hunk(s, hunk);
+
 	if (hunk->end == hunk->start)
 		/* The user aborted editing by deleting everything */
 		return 0;
 
-	recolor_hunk(s, hunk);
-
 	/*
 	 * If the hunk header is intact, parse it, otherwise simply use the
 	 * hunk header prior to editing (which will adjust `hunk->start` to
@@ -1257,15 +1263,14 @@ static int edit_hunk_loop(struct add_p_state *s,
 	backup = *hunk;
 
 	for (;;) {
-		int res = edit_hunk_manually(s, hunk);
+		int res = edit_hunk_manually(s, hunk, plain_len, colored_len);
 		if (res == 0) {
 			/* abandoned */
-			*hunk = backup;
-			return -1;
+			break;
 		}
 
 		if (res > 0) {
-			hunk->delta +=
+			hunk->delta = backup.delta +
 				recount_edited_hunk(s, hunk,
 						    backup.header.old_count,
 						    backup.header.new_count);
@@ -1273,10 +1278,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;
 
 		/*
 		 * TRANSLATORS: do not translate [y/n]
@@ -1289,8 +1290,14 @@ static int edit_hunk_loop(struct add_p_state *s,
 					"Edit again (saying \"no\" discards!) "
 					"[y/n]? "));
 		if (res < 1)
-			return -1;
+			break;
 	}
+
+	/* Drop a possible edit */
+	strbuf_setlen(&s->plain, plain_len);
+	strbuf_setlen(&s->colored, colored_len);
+	*hunk = backup;
+	return -1;
 }
 
 static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff,
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 718438ffc7..f3206a317b 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -165,6 +165,19 @@ test_expect_success 'dummy edit works' '
 	diff_cmp expected diff
 '
 
+test_expect_success 'editing again works' '
+	git reset &&
+	write_script "fake_editor.sh" <<-\EOF &&
+	grep been-here "$1" >output
+	echo been-here >"$1"
+	EOF
+	(
+		test_set_editor "$(pwd)/fake_editor.sh" &&
+		test_write_lines e y | GIT_TRACE=1 git add -p
+	) &&
+	test_grep been-here output
+'
+
 test_expect_success 'setup patch' '
 	cat >patch <<-\EOF
 	@@ -1,1 +1,4 @@

Range-diff:
1:  bcf32d0979 ! 1:  2b55a759d5 add-patch: edit the hunk again
    @@ add-patch.c: static int edit_hunk_manually(struct add_p_state *s, struct hunk *h
      	/* strip out commented lines */
      	hunk->start = s->plain.len;
      	for (i = 0; i < s->buf.len; ) {
    +@@ add-patch.c: static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
    + 	}
    + 
    + 	hunk->end = s->plain.len;
    ++
    ++	recolor_hunk(s, hunk);
    ++
    + 	if (hunk->end == hunk->start)
    + 		/* The user aborted editing by deleting everything */
    + 		return 0;
    + 
    +-	recolor_hunk(s, hunk);
    +-
    + 	/*
    + 	 * If the hunk header is intact, parse it, otherwise simply use the
    + 	 * hunk header prior to editing (which will adjust `hunk->start` to
     @@ add-patch.c: static int edit_hunk_loop(struct add_p_state *s,
      	backup = *hunk;
      
    @@ t/t3701-add-interactive.sh: 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
    -+	echo been-here > "$1"
    -+	EOF
    -+	test_set_editor "$(pwd)/fake_editor.sh"
    -+'
    -+
     +test_expect_success 'editing again works' '
     +	git reset &&
    -+	test_write_lines e y | GIT_TRACE=1 git add -p &&
    -+	grep found output
    ++	write_script "fake_editor.sh" <<-\EOF &&
    ++	grep been-here "$1" >output
    ++	echo been-here >"$1"
    ++	EOF
    ++	(
    ++		test_set_editor "$(pwd)/fake_editor.sh" &&
    ++		test_write_lines e y | GIT_TRACE=1 git add -p
    ++	) &&
    ++	test_grep been-here output
     +'
     +
      test_expect_success 'setup patch' '
-- 
2.46.1.507.gdd29a28bc2