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

From

Rubén Justo <rjusto@gmail.com>

Date

Sep 18, 2024, 17:51 UTC

Message-ID

<4dd5a2c7-26a8-470f-b651-e1fe2d1dbcec@gmail.com>

In-Reply-To

<21ddf64f-10c2-4087-a778-0bd2e82aef42@gmail.com>

    * 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