git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH] add-patch: edit the hunk again

From
Rubén Justo <rjusto@gmail.com>
Date
Sep 15, 2024, 11:38 UTC
Message-ID
<21ddf64f-10c2-4087-a778-0bd2e82aef42@gmail.com>

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.

Instead, let's give them the faulty 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 current hunk and moving on to the next one.
      This changed, presumably unintentionally, during the conversion
      to C in bcdd297b78 (built-in add -p: implement hunk editing,
      2019-12-13).
      Now makes perfect 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>
---

The message "saying 'no' discards!" comes from ac083c47ea (git-add--interactive: manual hunk editing mode, 2008-07-03).

I think it was referring to discarding user modifications, not the current hunk; which is what we were doing then (and now regaining).

However, we stopped behaving that way in 2b8ea7f3c7 (add -p: calculate offset delta for edited patches, 2018-03-05), perhaps for some reason I'm missing.

Therefore, this patch also modifies what we did, possibly unintentionally, in 2b8ea7f3c7.

Thanks.
 add-patch.c                | 26 ++++++++++++++++----------
 t/t3701-add-interactive.sh | 14 ++++++++++++++
 2 files changed, 30 insertions(+), 10 deletions(-)
diff --git a/add-patch.c b/add-patch.c
index 557903310d..125e79a5ae 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; ) {
@@ -1257,15 +1262,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 +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;
 
 		/*
 		 * TRANSLATORS: do not translate [y/n]
@@ -1289,8 +1289,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..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
+	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
+'
+
 test_expect_success 'setup patch' '
 	cat >patch <<-\EOF
 	@@ -1,1 +1,4 @@
-- 
2.46.1.507.gbcf32d0979
Next: Phillip Wood
Message 1 of 17 in “add-patch: edit the hunk again”
  1. add-patch: edit the hunk againRubén Justo, Sep 15, 2024
  2. Phillip WoodSep 16, 2024
  3. Junio C HamanoSep 16, 2024
  4. Rubén JustoSep 16, 2024
  5. phillip.wood123@gmail.comSep 18, 2024
  6. Rubén JustoSep 18, 2024
  7. add-patch: edit the hunk againRubén Justo, Sep 18, 2024
  8. phillip.wood123@gmail.comSep 23, 2024
  9. Junio C HamanoSep 23, 2024
  10. Rubén JustoSep 24, 2024
  11. Phillip WoodOct 1, 2024
  12. Rubén JustoOct 2, 2024
  13. add-patch: edit the hunk againRubén Justo, Sep 28, 2024
  14. Phillip WoodOct 1, 2024
  15. Junio C HamanoOct 1, 2024
  16. Rubén JustoOct 2, 2024
  17. Rubén JustoOct 2, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.