[RFC PATCH 0/1] add -p: support discarding hunks

[RFC PATCH 0/1] add -p: support discarding hunks

[Luiz Campos]

Hi,

This is an RFC for adding a 'discard hunk' action to `git add -p`.

Currently, when using `git add -p`, users can stage or skip hunks,
but cannot discard unwanted changes directly from the working tree.
This often leads to repeatedly skipping the same hunks across
multiple passes.

This patch introduces a new 'x' action to discard the current hunk
by reverse-applying it to the working tree.

This idea was previously discussed on the mailing list:
https://lore.kernel.org/git/X%2FiFCo0bXLR%2BLZXs@coredump.intra.peff.net/t/#m0576e6f3c6375e11cc4693b9dca3c1fc57baadd0

Open questions:
- Should discard happen immediately or be deferred until patch application?
- Are there edge cases involving overlapping hunks or edited hunks?

Feedback is very welcome.

Thanks,
Luiz

Luiz Campos (1):
  [RFC PATCH 0/1] add -p: support discarding hunks with 'x'

 Documentation/git-add.adoc |   7 +-
 add-patch.c                | 137 ++++++++++++++++++++++++++++---------
 t/t3701-add-interactive.sh |  58 ++++++++++------
 3 files changed, 149 insertions(+), 53 deletions(-)

-- 
2.43.0

[RFC PATCH 1/1] add -p: support discarding hunks with 'x'

[Luiz Campos]

When using `git add -p`, users can stage or skip hunks,
but cannot discard unwanted changes from the working tree.

Introduce a new 'x' action to discard the current hunk by
reverse-applying it.

This idea was suggested in a previous mailing list discussion:
https://lore.kernel.org/git/X%2FiFCo0bXLR%2BLZXs@coredump.intra.peff.net/t/#m0576e6f3c6375e11cc4693b9dca3c1fc57baadd0

Feedback is very welcome.

Signed-off-by: Luiz Campos <luizedc1@gmail.com>
---
 Documentation/git-add.adoc |   7 +-
 add-patch.c                | 137 ++++++++++++++++++++++++++++---------
 t/t3701-add-interactive.sh |  58 ++++++++++------
 3 files changed, 149 insertions(+), 53 deletions(-)

diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
index 941135dc63..0ab81e5615 100644
--- a/Documentation/git-add.adoc
+++ b/Documentation/git-add.adoc
@@ -351,12 +351,15 @@ patch::
        K - go to the previous hunk, roll over at the top
        s - split the current hunk into smaller hunks
        e - manually edit the current hunk
+       x - discard this hunk from the worktree
        p - print the current hunk
        P - print the current hunk using the pager
        ? - print help
 +
-After deciding the fate for all hunks, if there is any hunk
-that was chosen, the index is updated with the selected hunks.
+After deciding the fate for all hunks, any hunks marked for
+discard are removed from the working tree (reverted to the index
+version for those lines).  Then, if there is any hunk chosen for
+staging, the index is updated with those hunks.
 +
 You can omit having to type return here, by setting the configuration
 variable `interactive.singleKey` to `true`.
diff --git a/add-patch.c b/add-patch.c
index 4e28e5c187..ea38ab453e 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -259,7 +259,7 @@ struct hunk_header {
 struct hunk {
 	size_t start, end, colored_start, colored_end, splittable_into;
 	ssize_t delta;
-	enum { UNDECIDED_HUNK = 0, SKIP_HUNK, USE_HUNK } use;
+	enum { UNDECIDED_HUNK = 0, SKIP_HUNK, USE_HUNK, DISCARD_HUNK } use;
 	struct hunk_header header;
 };
 
@@ -884,17 +884,35 @@ static void render_diff_header(struct add_p_state *s,
 	}
 }
 
+static bool should_merge_hunk(struct file_diff *file_diff,
+			      size_t hunk_index, int use_all,
+			      int merge_for_discard)
+{
+	if (use_all)
+		return true;
+	return merge_for_discard
+		? file_diff->hunk[hunk_index].use == DISCARD_HUNK
+		: file_diff->hunk[hunk_index].use == USE_HUNK;
+}
+
+enum reassemble_mode {
+	REASSEMBLE_STAGE,
+	REASSEMBLE_DISCARD
+};
+
 /* Coalesce hunks again that were split */
 static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff,
-		       size_t *hunk_index, int use_all, struct hunk *merged)
+		       size_t *hunk_index, int use_all, struct hunk *merged,
+		       int merge_for_discard)
 {
 	size_t i = *hunk_index, delta;
 	struct hunk *hunk = file_diff->hunk + i;
 	/* `header` corresponds to the merged hunk */
 	struct hunk_header *header = &merged->header, *next;
 
-	if (!use_all && hunk->use != USE_HUNK)
+	if (!should_merge_hunk(file_diff, *hunk_index, use_all, merge_for_discard)) {
 		return 0;
+	}
 
 	*merged = *hunk;
 	/* We simply skip the colored part (if any) when merging hunks */
@@ -907,10 +925,10 @@ static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff,
 		/*
 		 * Stop merging hunks when:
 		 *
-		 * - the hunk is not selected for use, or
+		 * - the hunk is not selected for use (or discard, when merging discards), or
 		 * - the hunk does not overlap with the already-merged hunk(s)
 		 */
-		if ((!use_all && hunk->use != USE_HUNK) ||
+		if (!should_merge_hunk(file_diff, i + 1, use_all, merge_for_discard) ||
 		    header->new_offset >= next->new_offset + merged->delta ||
 		    header->new_offset + header->new_count
 		    < next->new_offset + merged->delta)
@@ -1014,11 +1032,13 @@ static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff,
 
 static void reassemble_patch(struct add_p_state *s,
 			     struct file_diff *file_diff, int use_all,
+			     enum reassemble_mode mode,
 			     struct strbuf *out)
 {
 	struct hunk *hunk;
 	size_t save_len = s->plain.len, i;
 	ssize_t delta = 0;
+	int merge_for_discard = (mode == REASSEMBLE_DISCARD);
 
 	render_diff_header(s, file_diff, 0, out);
 
@@ -1026,25 +1046,26 @@ static void reassemble_patch(struct add_p_state *s,
 		struct hunk merged = { 0 };
 
 		hunk = file_diff->hunk + i;
-		if (!use_all && hunk->use != USE_HUNK)
+		if (!should_merge_hunk(file_diff, i, use_all, merge_for_discard)) {
 			delta += hunk->header.old_count
 				- hunk->header.new_count;
-		else {
-			/* merge overlapping hunks into a temporary hunk */
-			if (merge_hunks(s, file_diff, &i, use_all, &merged))
-				hunk = &merged;
+			continue;
+		}
 
-			render_hunk(s, hunk, delta, 0, out);
+		if (merge_hunks(s, file_diff, &i, use_all, &merged,
+				merge_for_discard))
+			hunk = &merged;
 
-			/*
-			 * In case `merge_hunks()` used `plain` as a scratch
-			 * pad (this happens when an edited hunk had to be
-			 * coalesced with another hunk).
-			 */
-			strbuf_setlen(&s->plain, save_len);
+		render_hunk(s, hunk, delta, 0, out);
 
-			delta += hunk->delta;
-		}
+		/*
+		 * In case `merge_hunks()` used `plain` as a scratch
+		 * pad (this happens when an edited hunk had to be
+		 * coalesced with another hunk).
+		 */
+		strbuf_setlen(&s->plain, save_len);
+
+		delta += hunk->delta;
 	}
 }
 
@@ -1348,7 +1369,7 @@ static int run_apply_check(struct add_p_state *s,
 	struct child_process cp = CHILD_PROCESS_INIT;
 
 	strbuf_reset(&s->buf);
-	reassemble_patch(s, file_diff, 1, &s->buf);
+	reassemble_patch(s, file_diff, 1, REASSEMBLE_STAGE, &s->buf);
 
 	setup_child_process(s, &cp,
 			    "apply", "--check", NULL);
@@ -1522,7 +1543,8 @@ static size_t display_hunks(struct add_p_state *s,
 
 		strbuf_reset(&s->buf);
 		strbuf_addf(&s->buf, "%c%2d: ", hunk->use == USE_HUNK ? '+'
-			    : hunk->use == SKIP_HUNK ? '-' : ' ',
+			    : hunk->use == SKIP_HUNK ? '-'
+			    : hunk->use == DISCARD_HUNK ? 'x' : ' ',
 			    (int)start_index);
 		summarize_hunk(s, hunk, &s->buf);
 		fputs(s->buf.buf, stdout);
@@ -1540,6 +1562,7 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
    "/ - search for a hunk matching the given regex\n"
    "s - split the current hunk into smaller hunks\n"
    "e - manually edit the current hunk\n"
+   "x - discard this hunk from the worktree\n"
    "p - print the current hunk\n"
    "P - print the current hunk using the pager\n"
    "> - go to the next file, roll over at the bottom\n"
@@ -1547,21 +1570,57 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
    "? - print help\n"
    "HUNKS SUMMARY - Hunks: %d, USE: %d, SKIP: %d\n");
 
+static int apply_discard_hunks(struct add_p_state *s,
+			       struct file_diff *file_diff)
+{
+	struct child_process check_cp = CHILD_PROCESS_INIT;
+	struct child_process apply_cp = CHILD_PROCESS_INIT;
+
+	strbuf_reset(&s->buf);
+	reassemble_patch(s, file_diff, 0, REASSEMBLE_DISCARD, &s->buf);
+
+	discard_index(s->index);
+
+	setup_child_process(s, &check_cp, "apply", "-R", "--check", NULL);
+	if (pipe_command(&check_cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0)) {
+		error(_("'git apply -R --check' failed"));
+		return -1;
+	}
+
+	setup_child_process(s, &apply_cp, "apply", "-R", NULL);
+	if (pipe_command(&apply_cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0)) {
+		error(_("'git apply -R' failed"));
+		return -1;
+	}
+
+	return 0;
+}
+
 static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	size_t j;
+	int needs_refresh = 0;
+
+	if (s->mode == &patch_mode_add) {
+		for (j = 0; j < file_diff->hunk_nr; j++) {
+			if (file_diff->hunk[j].use == DISCARD_HUNK)
+				break;
+		}
+		if (j < file_diff->hunk_nr && apply_discard_hunks(s, file_diff))
+			return;
+		if (j < file_diff->hunk_nr)
+			needs_refresh = 1;
+	}
 
-	/* Any hunk to be used? */
 	for (j = 0; j < file_diff->hunk_nr; j++)
 		if (file_diff->hunk[j].use == USE_HUNK)
 			break;
 
 	if (j < file_diff->hunk_nr ||
-		(!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
-		/* At least one hunk selected: apply */
+	    (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
 		strbuf_reset(&s->buf);
-		reassemble_patch(s, file_diff, 0, &s->buf);
+		reassemble_patch(s, file_diff, 0, REASSEMBLE_STAGE, &s->buf);
 
 		discard_index(s->index);
 		if (s->mode->apply_for_checkout)
@@ -1574,13 +1633,15 @@ static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
 					NULL, 0, NULL, 0))
 				error(_("'git apply' failed"));
 		}
-		if (read_index_from(s->index, s->index_file, s->r->gitdir) >= 0 &&
-		    s->index == s->r->index) {
-			repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0,
-						     1, NULL, NULL, NULL);
-		}
+		needs_refresh = 1;
 	}
 
+	if (needs_refresh &&
+	    read_index_from(s->index, s->index_file, s->r->gitdir) >= 0 &&
+	    s->index == s->r->index) {
+		repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0,
+					     1, NULL, NULL, NULL);
+	}
 }
 
 static size_t dec_mod(size_t a, size_t m)
@@ -1636,7 +1697,8 @@ static size_t patch_update_file(struct add_p_state *s,
 			ALLOW_SPLIT = 1 << 5,
 			ALLOW_EDIT = 1 << 6,
 			ALLOW_GOTO_PREVIOUS_FILE = 1 << 7,
-			ALLOW_GOTO_NEXT_FILE = 1 << 8
+			ALLOW_GOTO_NEXT_FILE = 1 << 8,
+			ALLOW_DISCARD = 1 << 9
 		} permitted = 0;
 
 		if (hunk_index >= file_diff->hunk_nr)
@@ -1722,6 +1784,10 @@ static size_t patch_update_file(struct add_p_state *s,
 			    !file_diff->deleted) {
 				permitted |= ALLOW_EDIT;
 				strbuf_addstr(&s->buf, ",e");
+				if (s->mode == &patch_mode_add) {
+					permitted |= ALLOW_DISCARD;
+					strbuf_addstr(&s->buf, ",x");
+				}
 			}
 			if (!s->cfg.auto_advance && s->file_diff_nr > 1) {
 				permitted |= ALLOW_GOTO_NEXT_FILE;
@@ -1750,6 +1816,8 @@ static size_t patch_update_file(struct add_p_state *s,
 		if (hunk->use != UNDECIDED_HUNK) {
 			if (hunk->use == USE_HUNK)
 				hunk_use_decision = _(" (was: y)");
+			else if (hunk->use == DISCARD_HUNK)
+				hunk_use_decision = _(" (was: x)");
 			else
 				hunk_use_decision = _(" (was: n)");
 		}
@@ -1780,6 +1848,13 @@ static size_t patch_update_file(struct add_p_state *s,
 		} else if (ch == 'n') {
 			hunk->use = SKIP_HUNK;
 			goto soft_increment;
+		} else if (ch == 'x') {
+			if (!(permitted & ALLOW_DISCARD))
+				err(s, _("Sorry, cannot discard this hunk"));
+			else {
+				hunk->use = DISCARD_HUNK;
+				goto soft_increment;
+			}
 		} else if (ch == 'a') {
 			if (file_diff->hunk_nr) {
 				for (; hunk_index < file_diff->hunk_nr; hunk_index++) {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 6e120a4001..0eb813f74c 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -48,8 +48,8 @@ test_expect_success 'unknown command' '
 	git add -N command &&
 	git diff command >expect &&
 	cat >>expect <<-EOF &&
-	(1/1) Stage addition [y,n,q,a,d,e,p,P,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
-	(1/1) Stage addition [y,n,q,a,d,e,p,P,?]?$SP
+	(1/1) Stage addition [y,n,q,a,d,e,x,p,P,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
+	(1/1) Stage addition [y,n,q,a,d,e,x,p,P,?]?$SP
 	EOF
 	git add -p -- command <command >actual 2>&1 &&
 	test_cmp expect actual
@@ -334,7 +334,7 @@ test_expect_success 'different prompts for mode change/deleted' '
 	cat >expect <<-\EOF &&
 	(1/1) Stage deletion [y,n,q,a,d,p,P,?]?
 	(1/2) Stage mode change [y,n,q,a,d,k,K,j,J,g,/,p,P,?]?
-	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,P,?]?
+	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,x,p,P,?]?
 	EOF
 	test_cmp expect actual.filtered
 '
@@ -447,6 +447,24 @@ test_expect_success 'add first line works' '
 	test_cmp expected-output output
 '
 
+test_expect_success 'add -p discard removes worktree change' '
+	test_when_finished "rm -rf discard-testrepo" &&
+	mkdir discard-testrepo &&
+	(
+		cd discard-testrepo &&
+		git init -b main &&
+		echo clean >discard-me &&
+		git add discard-me &&
+		git commit -m base &&
+		echo extra >>discard-me &&
+		test_write_lines x | git add -p discard-me &&
+		printf "clean\n" >expect &&
+		test_cmp expect discard-me &&
+		git diff --cached >tmp &&
+		test_must_be_empty tmp
+	)
+'
+
 test_expect_success 'setup expected' '
 	cat >expected <<-\EOF
 	diff --git a/non-empty b/non-empty
@@ -521,13 +539,13 @@ test_expect_success 'split hunk setup' '
 test_expect_success 'goto hunk 1 with "g 1"' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
-	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,P,?]? + 1:  -1,2 +1,3          +15
+	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,x,p,P,?]? + 1:  -1,2 +1,3          +15
 	_ 2:  -2,4 +3,8          +21
 	go to which hunk? @@ -1,2 +1,3 @@
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?_
+	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?_
 	EOF
 	test_write_lines s y g 1 | git add -p >actual &&
 	tail -n 7 <actual >actual.trimmed &&
@@ -540,7 +558,7 @@ test_expect_success 'goto hunk 1 with "g1"' '
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?_
+	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?_
 	EOF
 	test_write_lines s y g1 | git add -p >actual &&
 	tail -n 4 <actual >actual.trimmed &&
@@ -550,11 +568,11 @@ test_expect_success 'goto hunk 1 with "g1"' '
 test_expect_success 'navigate to hunk via regex /pattern' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
-	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,P,?]? @@ -1,2 +1,3 @@
+	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,x,p,P,?]? @@ -1,2 +1,3 @@
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?_
+	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?_
 	EOF
 	test_write_lines s y /1,2 | git add -p >actual &&
 	tail -n 5 <actual >actual.trimmed &&
@@ -567,7 +585,7 @@ test_expect_success 'navigate to hunk via regex / pattern' '
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?_
+	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?_
 	EOF
 	test_write_lines s y / 1,2 | git add -p >actual &&
 	tail -n 4 <actual >actual.trimmed &&
@@ -579,11 +597,11 @@ test_expect_success 'print again the hunk' '
 	tr _ " " >expect <<-EOF &&
 	+15
 	 20
-	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? @@ -1,2 +1,3 @@
+	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? @@ -1,2 +1,3 @@
 	 10
 	+15
 	 20
-	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?_
+	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?_
 	EOF
 	test_write_lines s y g 1 p | git add -p >actual &&
 	tail -n 7 <actual >actual.trimmed &&
@@ -595,11 +613,11 @@ test_expect_success TTY 'print again the hunk (PAGER)' '
 	cat >expect <<-EOF &&
 	<GREEN>+<RESET><GREEN>15<RESET>
 	 20<RESET>
-	<BOLD;BLUE>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
 	PAGER  10<RESET>
 	PAGER <GREEN>+<RESET><GREEN>15<RESET>
 	PAGER  20<RESET>
-	<BOLD;BLUE>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? <RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? <RESET>
 	EOF
 	test_write_lines s y g 1 P |
 	(
@@ -796,21 +814,21 @@ test_expect_success 'colors can be overridden' '
 	<BLUE>+<RESET><BLUE>new<RESET>
 	<CYAN> more-context<RESET>
 	<BLUE>+<RESET><BLUE>another-one<RESET>
-	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,P,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
+	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,x,p,P,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
 	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
 	<CYAN> context<RESET>
 	<BOLD>-old<RESET>
 	<BLUE>+<RESET><BLUE>new<RESET>
 	<CYAN> more-context<RESET>
-	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
 	<CYAN> more-context<RESET>
 	<BLUE>+<RESET><BLUE>another-one<RESET>
-	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,P,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
+	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,x,p,P,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
 	<CYAN> context<RESET>
 	<BOLD>-old<RESET>
 	<BLUE>+new<RESET>
 	<CYAN> more-context<RESET>
-	<YELLOW>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? <RESET>
+	<YELLOW>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? <RESET>
 	EOF
 	test_cmp expect actual
 '
@@ -1424,9 +1442,9 @@ test_expect_success 'invalid option s is rejected' '
 	test_write_lines j s q | git add -p >out &&
 	sed -ne "s/ @@.*//" -e "s/ \$//" -e "/^(/p" <out >actual &&
 	cat >expect <<-EOF &&
-	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,s,e,p,P,?]?
-	(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? Sorry, cannot split this hunk
-	(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?
+	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,s,e,x,p,P,?]?
+	(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? Sorry, cannot split this hunk
+	(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?
 	EOF
 	test_cmp expect actual
 '
-- 
2.43.0

Re: [RFC PATCH 1/1] add -p: support discarding hunks with 'x'

[D. Ben Knoble]

On Wed, Mar 25, 2026 at 3:53 AM Luiz Campos <luizedc1@gmail.com> wrote:
>
> When using `git add -p`, users can stage or skip hunks,
> but cannot discard unwanted changes from the working tree.
>
> Introduce a new 'x' action to discard the current hunk by
> reverse-applying it.
>
> This idea was suggested in a previous mailing list discussion:
> https://lore.kernel.org/git/X%2FiFCo0bXLR%2BLZXs@coredump.intra.peff.net/t/#m0576e6f3c6375e11cc4693b9dca3c1fc57baadd0
>
> Feedback is very welcome.
>
> Signed-off-by: Luiz Campos <luizedc1@gmail.com>

One feature the Fugitive Git client for Vim supports is to discard
hunks; when it does so, it also prints a message explaining how to
recover the hunk if you need it.

I think it writes the file as a blob to Git's database before
restoring from the index.

I'm not suggesting Git copy this necessarily, but we might want to
consider how to help folks when they lose a hunk they didn't mean to.
If it's never been in the index, it can be impossible to recover!

PS How different is this from "git restore -p" ?


--
D. Ben Knoble

Re: [RFC PATCH 1/1] add -p: support discarding hunks with 'x'

[Phillip Wood]

Hi Luiz

On 25/03/2026 07:50, Luiz Campos wrote:
> When using `git add -p`, users can stage or skip hunks,
> but cannot discard unwanted changes from the working tree.
> 
> Introduce a new 'x' action to discard the current hunk by
> reverse-applying it.
> 
> This idea was suggested in a previous mailing list discussion:
> https://lore.kernel.org/git/X%2FiFCo0bXLR%2BLZXs@coredump.intra.peff.net/t/#m0576e6f3c6375e11cc4693b9dca3c1fc57baadd0

I tend to agree with peff's comments in that thread that it is rather 
unexpected for "git add" to modify the working copy. I also think that a 
command that lets you stage some changes and discard others could be 
useful as I do both fairly frequently from my editor. Regardless of 
whether we want a new command the implementation will be similar so I've 
left some comments on the code below.

> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
> index 941135dc63..0ab81e5615 100644
> --- a/Documentation/git-add.adoc
> +++ b/Documentation/git-add.adoc
> @@ -351,12 +351,15 @@ patch::
>          K - go to the previous hunk, roll over at the top
>          s - split the current hunk into smaller hunks
>          e - manually edit the current hunk
> +       x - discard this hunk from the worktree
>          p - print the current hunk
>          P - print the current hunk using the pager
>          ? - print help
>   +
> -After deciding the fate for all hunks, if there is any hunk
> -that was chosen, the index is updated with the selected hunks.
> +After deciding the fate for all hunks, any hunks marked for
> +discard are removed from the working tree (reverted to the index
> +version for those lines).  Then, if there is any hunk chosen for
> +staging, the index is updated with those hunks.

Makes sense.

>   +
>   You can omit having to type return here, by setting the configuration
>   variable `interactive.singleKey` to `true`.
> diff --git a/add-patch.c b/add-patch.c
> index 4e28e5c187..ea38ab453e 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -259,7 +259,7 @@ struct hunk_header {
>   struct hunk {
>   	size_t start, end, colored_start, colored_end, splittable_into;
>   	ssize_t delta;
> -	enum { UNDECIDED_HUNK = 0, SKIP_HUNK, USE_HUNK } use;
> +	enum { UNDECIDED_HUNK = 0, SKIP_HUNK, USE_HUNK, DISCARD_HUNK } use;
>   	struct hunk_header header;
>   };
>   
> @@ -884,17 +884,35 @@ static void render_diff_header(struct add_p_state *s,
>   	}
>   }
>   
> +static bool should_merge_hunk(struct file_diff *file_diff,
> +			      size_t hunk_index, int use_all,
> +			      int merge_for_discard)
> +{
> +	if (use_all)
> +		return true;

If we're looking for hunks to discard then we want to return false if 
all the hunks have been selected to be staged.

> +	return merge_for_discard
> +		? file_diff->hunk[hunk_index].use == DISCARD_HUNK
> +		: file_diff->hunk[hunk_index].use == USE_HUNK;

It would be simpler just to take USE_HUNK or DISCARD_HUNK as an argument 
rather than a boolean here.

>   /* Coalesce hunks again that were split */
>   static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff,
> -		       size_t *hunk_index, int use_all, struct hunk *merged)
> +		       size_t *hunk_index, int use_all, struct hunk *merged,
> +		       int merge_for_discard)

Taking the type of hunk we want to retain (USE_HUNK or DISCARD_HUNK) 
would avoid having to convert merge_for_discard back into the hunk type 
in should_merge_hunk().

>   {
>   	size_t i = *hunk_index, delta;
>   	struct hunk *hunk = file_diff->hunk + i;
>   	/* `header` corresponds to the merged hunk */
>   	struct hunk_header *header = &merged->header, *next;
>   
> -	if (!use_all && hunk->use != USE_HUNK)
> +	if (!should_merge_hunk(file_diff, *hunk_index, use_all, merge_for_discard)) {
>   		return 0;
> +	}

There's no need to add braces here

> @@ -1014,11 +1032,13 @@ static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff,
>   
>   static void reassemble_patch(struct add_p_state *s,
>   			     struct file_diff *file_diff, int use_all,
> +			     enum reassemble_mode mode,
>   			     struct strbuf *out)
>   {
>   	struct hunk *hunk;
>   	size_t save_len = s->plain.len, i;
>   	ssize_t delta = 0;
> +	int merge_for_discard = (mode == REASSEMBLE_DISCARD);

It would by simpler just to take USE_HUNK or DISCARD_HUNK as a parameter 
and pass that to reassemble_patch() rather than forcing the caller to 
pass an enum that we then transform to a boolean.

>   
>   	render_diff_header(s, file_diff, 0, out);
>   
> @@ -1026,25 +1046,26 @@ static void reassemble_patch(struct add_p_state *s,
>   		struct hunk merged = { 0 };
>   
>   		hunk = file_diff->hunk + i;
> -		if (!use_all && hunk->use != USE_HUNK)
> +		if (!should_merge_hunk(file_diff, i, use_all, merge_for_discard)) {
>   			delta += hunk->header.old_count
>   				- hunk->header.new_count;
> -		else {
> -			/* merge overlapping hunks into a temporary hunk */
> -			if (merge_hunks(s, file_diff, &i, use_all, &merged))
> -				hunk = &merged;
> +			continue;

I'm not sure this is an improvement - it certainly makes the patch 
harder to read because you end up changing the indentation of otherwise 
unchanged lines that were in the else clause.

> +		}
>   
> -			render_hunk(s, hunk, delta, 0, out);
> +		if (merge_hunks(s, file_diff, &i, use_all, &merged,
> +				merge_for_discard))
> +			hunk = &merged;
>   
> -			/*
> -			 * In case `merge_hunks()` used `plain` as a scratch
> -			 * pad (this happens when an edited hunk had to be
> -			 * coalesced with another hunk).
> -			 */
> -			strbuf_setlen(&s->plain, save_len);
> +		render_hunk(s, hunk, delta, 0, out);

We need to tell render_hunk whether a hunk is being applied in reverse 
or not so that it knows whether to apply delta to the old offset or the 
new offset. Currently it uses s->mode->reverse but that will not be 
correct for the patch that discards changes from the working tree. I'm 
not sure what the best way of doing that is, the simplest approach is to 
invert s->mode->reverse when we want to keep hunks marked DISCARD_HUNK 
and then restore the original value.

> -			delta += hunk->delta;
> -		}
> +		/*
> +		 * In case `merge_hunks()` used `plain` as a scratch
> +		 * pad (this happens when an edited hunk had to be
> +		 * coalesced with another hunk).
> +		 */
> +		strbuf_setlen(&s->plain, save_len);
> +
> +		delta += hunk->delta;

This is unchanged code that re-indented because of the addition of 
"continue" above.

>   	}
>   }

> @@ -1540,6 +1562,7 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
>      "/ - search for a hunk matching the given regex\n"
>      "s - split the current hunk into smaller hunks\n"
>      "e - manually edit the current hunk\n"
> +   "x - discard this hunk from the worktree\n"

It would be nice to avoid showing this for 'checkout -p' etc.

>      "p - print the current hunk\n"
>      "P - print the current hunk using the pager\n"
>      "> - go to the next file, roll over at the bottom\n"
> @@ -1547,21 +1570,57 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
>      "? - print help\n"
>      "HUNKS SUMMARY - Hunks: %d, USE: %d, SKIP: %d\n");
>   
> +static int apply_discard_hunks(struct add_p_state *s,
> +			       struct file_diff *file_diff)
> +{
> +	struct child_process check_cp = CHILD_PROCESS_INIT;
> +	struct child_process apply_cp = CHILD_PROCESS_INIT;
> +
> +	strbuf_reset(&s->buf);
> +	reassemble_patch(s, file_diff, 0, REASSEMBLE_DISCARD, &s->buf);
> +
> +	discard_index(s->index);
> +
> +	setup_child_process(s, &check_cp, "apply", "-R", "--check", NULL);
> +	if (pipe_command(&check_cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0)) {
> +		error(_("'git apply -R --check' failed"));
> +		return -1;
> +	}

Why do we need to run "git apply --check" here?

> +	setup_child_process(s, &apply_cp, "apply", "-R", NULL);
> +	if (pipe_command(&apply_cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0)) {
> +		error(_("'git apply -R' failed"));
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>   static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
>   {
>   	struct child_process cp = CHILD_PROCESS_INIT;
>   	size_t j;
> +	int needs_refresh = 0;
> +
> +	if (s->mode == &patch_mode_add) {
> +		for (j = 0; j < file_diff->hunk_nr; j++) {
> +			if (file_diff->hunk[j].use == DISCARD_HUNK)
> +				break;
> +		}
> +		if (j < file_diff->hunk_nr && apply_discard_hunks(s, file_diff))
> +			return;
> +		if (j < file_diff->hunk_nr)
> +			needs_refresh = 1;
> +	}
>   
> -	/* Any hunk to be used? */

Isn't this comment still relevant?

>   	for (j = 0; j < file_diff->hunk_nr; j++)
>   		if (file_diff->hunk[j].use == USE_HUNK)
>   			break;
>   
>   	if (j < file_diff->hunk_nr ||
> -		(!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
> -		/* At least one hunk selected: apply */
> +	    (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {

What's the point of this change?

>   		strbuf_reset(&s->buf);
> -		reassemble_patch(s, file_diff, 0, &s->buf);
> +		reassemble_patch(s, file_diff, 0, REASSEMBLE_STAGE, &s->buf);
>   
>   		discard_index(s->index);
>   		if (s->mode->apply_for_checkout)
> @@ -1574,13 +1633,15 @@ static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
>   					NULL, 0, NULL, 0))
>   				error(_("'git apply' failed"));
>   		}
> -		if (read_index_from(s->index, s->index_file, s->r->gitdir) >= 0 &&
> -		    s->index == s->r->index) {
> -			repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0,
> -						     1, NULL, NULL, NULL);
> -		}
> +		needs_refresh = 1;
>   	}
>   
> +	if (needs_refresh &&
> +	    read_index_from(s->index, s->index_file, s->r->gitdir) >= 0 &&
> +	    s->index == s->r->index) {
> +		repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0,
> +					     1, NULL, NULL, NULL);

We now wait until we've applied both patches before refreshing the index 
- sounds sensible.

> +	}
>   }


> @@ -1722,6 +1784,10 @@ static size_t patch_update_file(struct add_p_state *s,
>   			    !file_diff->deleted) {
>   				permitted |= ALLOW_EDIT;
>   				strbuf_addstr(&s->buf, ",e");
> +				if (s->mode == &patch_mode_add) {
> +					permitted |= ALLOW_DISCARD;
> +					strbuf_addstr(&s->buf, ",x");
> +				}

So 'x' is only permitted if 'e' is what's the reason for that?
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> [...]
> +test_expect_success 'add -p discard removes worktree change' '
> +	test_when_finished "rm -rf discard-testrepo" &&
> +	mkdir discard-testrepo &&

It's nice to see a test for 'x', but I'm not sure why this test needs to 
be in a separate repository - why can't it use the same repository as 
the other tests?

> +	(
> +		cd discard-testrepo &&
> +		git init -b main &&
> +		echo clean >discard-me &&
> +		git add discard-me &&
> +		git commit -m base &&
> +		echo extra >>discard-me &&
> +		test_write_lines x | git add -p discard-me &&
> +		printf "clean\n" >expect &&
> +		test_cmp expect discard-me &&
> +		git diff --cached >tmp &&
> +		test_must_be_empty tmp

It would be nice to see tests that split a hunk like

-a
+A
  b
-c
+C
  d
-e
+E

and then (1) stage the first and third sub-hunks and discard the second, 
(2) discard the first and third sub-hunks and stage the second. It would 
also be nice to see a test that discards a hunk with pathological 
context lines - see 2bd69b9024c (add -p: fix checkout -p with 
pathological context, 2019-06-12) for an example.

Thanks

Phillip


> +	)
> +'
> +
>   test_expect_success 'setup expected' '
>   	cat >expected <<-\EOF
>   	diff --git a/non-empty b/non-empty
> @@ -521,13 +539,13 @@ test_expect_success 'split hunk setup' '
>   test_expect_success 'goto hunk 1 with "g 1"' '
>   	test_when_finished "git reset" &&
>   	tr _ " " >expect <<-EOF &&
> -	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,P,?]? + 1:  -1,2 +1,3          +15
> +	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,x,p,P,?]? + 1:  -1,2 +1,3          +15
>   	_ 2:  -2,4 +3,8          +21
>   	go to which hunk? @@ -1,2 +1,3 @@
>   	_10
>   	+15
>   	_20
> -	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?_
> +	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?_
>   	EOF
>   	test_write_lines s y g 1 | git add -p >actual &&
>   	tail -n 7 <actual >actual.trimmed &&
> @@ -540,7 +558,7 @@ test_expect_success 'goto hunk 1 with "g1"' '
>   	_10
>   	+15
>   	_20
> -	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?_
> +	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?_
>   	EOF
>   	test_write_lines s y g1 | git add -p >actual &&
>   	tail -n 4 <actual >actual.trimmed &&
> @@ -550,11 +568,11 @@ test_expect_success 'goto hunk 1 with "g1"' '
>   test_expect_success 'navigate to hunk via regex /pattern' '
>   	test_when_finished "git reset" &&
>   	tr _ " " >expect <<-EOF &&
> -	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,P,?]? @@ -1,2 +1,3 @@
> +	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,x,p,P,?]? @@ -1,2 +1,3 @@
>   	_10
>   	+15
>   	_20
> -	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?_
> +	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?_
>   	EOF
>   	test_write_lines s y /1,2 | git add -p >actual &&
>   	tail -n 5 <actual >actual.trimmed &&
> @@ -567,7 +585,7 @@ test_expect_success 'navigate to hunk via regex / pattern' '
>   	_10
>   	+15
>   	_20
> -	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?_
> +	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?_
>   	EOF
>   	test_write_lines s y / 1,2 | git add -p >actual &&
>   	tail -n 4 <actual >actual.trimmed &&
> @@ -579,11 +597,11 @@ test_expect_success 'print again the hunk' '
>   	tr _ " " >expect <<-EOF &&
>   	+15
>   	 20
> -	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? @@ -1,2 +1,3 @@
> +	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? @@ -1,2 +1,3 @@
>   	 10
>   	+15
>   	 20
> -	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?_
> +	(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?_
>   	EOF
>   	test_write_lines s y g 1 p | git add -p >actual &&
>   	tail -n 7 <actual >actual.trimmed &&
> @@ -595,11 +613,11 @@ test_expect_success TTY 'print again the hunk (PAGER)' '
>   	cat >expect <<-EOF &&
>   	<GREEN>+<RESET><GREEN>15<RESET>
>   	 20<RESET>
> -	<BOLD;BLUE>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
> +	<BOLD;BLUE>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
>   	PAGER  10<RESET>
>   	PAGER <GREEN>+<RESET><GREEN>15<RESET>
>   	PAGER  20<RESET>
> -	<BOLD;BLUE>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? <RESET>
> +	<BOLD;BLUE>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? <RESET>
>   	EOF
>   	test_write_lines s y g 1 P |
>   	(
> @@ -796,21 +814,21 @@ test_expect_success 'colors can be overridden' '
>   	<BLUE>+<RESET><BLUE>new<RESET>
>   	<CYAN> more-context<RESET>
>   	<BLUE>+<RESET><BLUE>another-one<RESET>
> -	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,P,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
> +	<YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,x,p,P,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
>   	<MAGENTA>@@ -1,3 +1,3 @@<RESET>
>   	<CYAN> context<RESET>
>   	<BOLD>-old<RESET>
>   	<BLUE>+<RESET><BLUE>new<RESET>
>   	<CYAN> more-context<RESET>
> -	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
> +	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
>   	<CYAN> more-context<RESET>
>   	<BLUE>+<RESET><BLUE>another-one<RESET>
> -	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,P,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
> +	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,x,p,P,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
>   	<CYAN> context<RESET>
>   	<BOLD>-old<RESET>
>   	<BLUE>+new<RESET>
>   	<CYAN> more-context<RESET>
> -	<YELLOW>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? <RESET>
> +	<YELLOW>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? <RESET>
>   	EOF
>   	test_cmp expect actual
>   '
> @@ -1424,9 +1442,9 @@ test_expect_success 'invalid option s is rejected' '
>   	test_write_lines j s q | git add -p >out &&
>   	sed -ne "s/ @@.*//" -e "s/ \$//" -e "/^(/p" <out >actual &&
>   	cat >expect <<-EOF &&
> -	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,s,e,p,P,?]?
> -	(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? Sorry, cannot split this hunk
> -	(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?
> +	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,s,e,x,p,P,?]?
> +	(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? Sorry, cannot split this hunk
> +	(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?
>   	EOF
>   	test_cmp expect actual
>   '

Re: [RFC PATCH 1/1] add -p: support discarding hunks with 'x'

[Johannes Schindelin]

Hi Luiz,

On Wed, 25 Mar 2026, Luiz Campos wrote:

> When using `git add -p`, users can stage or skip hunks,
> but cannot discard unwanted changes from the working tree.
> 
> Introduce a new 'x' action to discard the current hunk by
> reverse-applying it.
> 
> This idea was suggested in a previous mailing list discussion:
> https://lore.kernel.org/git/X%2FiFCo0bXLR%2BLZXs@coredump.intra.peff.net/t/#m0576e6f3c6375e11cc4693b9dca3c1fc57baadd0

Sounds good!

Just two minor comments (not really actionable, I think):

> @@ -1026,25 +1046,26 @@ static void reassemble_patch(struct add_p_state *s,
>  		struct hunk merged = { 0 };
>  
>  		hunk = file_diff->hunk + i;
> -		if (!use_all && hunk->use != USE_HUNK)
> +		if (!should_merge_hunk(file_diff, i, use_all, merge_for_discard)) {
>  			delta += hunk->header.old_count
>  				- hunk->header.new_count;
> -		else {
> -			/* merge overlapping hunks into a temporary hunk */
> -			if (merge_hunks(s, file_diff, &i, use_all, &merged))
> -				hunk = &merged;
> +			continue;
> +		}
>  
> -			render_hunk(s, hunk, delta, 0, out);
> +		if (merge_hunks(s, file_diff, &i, use_all, &merged,
> +				merge_for_discard))
> +			hunk = &merged;
>  
> -			/*
> -			 * In case `merge_hunks()` used `plain` as a scratch
> -			 * pad (this happens when an edited hunk had to be
> -			 * coalesced with another hunk).
> -			 */
> -			strbuf_setlen(&s->plain, save_len);
> +		render_hunk(s, hunk, delta, 0, out);
>  
> -			delta += hunk->delta;
> -		}
> +		/*
> +		 * In case `merge_hunks()` used `plain` as a scratch
> +		 * pad (this happens when an edited hunk had to be
> +		 * coalesced with another hunk).
> +		 */
> +		strbuf_setlen(&s->plain, save_len);
> +
> +		delta += hunk->delta;

This hunk is quite hard to read because of the `if ... else ...` -> `if {
... continue; } ...` change that de-indents a large chunk of code.

After pouring over the diff for a bit, I was able to convince myself that
the diff is correct.

> @@ -1547,21 +1570,57 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
>     "? - print help\n"
>     "HUNKS SUMMARY - Hunks: %d, USE: %d, SKIP: %d\n");
>  
> +static int apply_discard_hunks(struct add_p_state *s,
> +			       struct file_diff *file_diff)
> +{
> +	struct child_process check_cp = CHILD_PROCESS_INIT;
> +	struct child_process apply_cp = CHILD_PROCESS_INIT;
> +
> +	strbuf_reset(&s->buf);
> +	reassemble_patch(s, file_diff, 0, REASSEMBLE_DISCARD, &s->buf);

If you detect an empty patch here and indicate this via an early return
value, then...

> +
> +	discard_index(s->index);
> +
> +	setup_child_process(s, &check_cp, "apply", "-R", "--check", NULL);
> +	if (pipe_command(&check_cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0)) {
> +		error(_("'git apply -R --check' failed"));
> +		return -1;
> +	}
> +
> +	setup_child_process(s, &apply_cp, "apply", "-R", NULL);
> +	if (pipe_command(&apply_cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0)) {
> +		error(_("'git apply -R' failed"));
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	size_t j;
> +	int needs_refresh = 0;
> +
> +	if (s->mode == &patch_mode_add) {
> +		for (j = 0; j < file_diff->hunk_nr; j++) {
> +			if (file_diff->hunk[j].use == DISCARD_HUNK)
> +				break;
> +		}
> +		if (j < file_diff->hunk_nr && apply_discard_hunks(s, file_diff))
> +			return;
> +		if (j < file_diff->hunk_nr)
> +			needs_refresh = 1;
> +	}

... then this loop is no longer necessary.

Other than that, looks good to me!

Ciao,
Johannes

Re: [RFC PATCH 1/1] add -p: support discarding hunks with 'x'

[Luiz Eduardo Campos]

Hi Ben,

Thanks for the feedback!

(Sending this message again because I had not configured my
email client to send it plain text, so I apologize if you received
duplicated content)

> One feature the Fugitive Git client for Vim supports is to discard
> hunks; when it does so, it also prints a message explaining how to
> recover the hunk if you need it.

That’s a great point. I hadn’t considered recoverability in this
initial version. Storing the discarded hunk as a blob (or otherwise
making it recoverable) seems like a useful safety measure.

> If it's never been in the index, it can be impossible to recover!

Agreed — this is probably something that should be addressed before
considering this feature complete.

> PS How different is this from "git restore -p" ?

My understanding is that `git restore -p` already allows discarding
changes interactively, but it requires a separate pass. The goal here
was to allow discarding during `git add -p`, so users can decide what
to do with each hunk in a single pass.

That said, I’m not yet sure whether integrating this into `add -p` is
the best approach, or if this should be handled differently.

Thanks again for the insights!

Luiz


Em qua., 25 de mar. de 2026 às 12:44, D. Ben Knoble
<ben.knoble@gmail.com> escreveu:
>
> On Wed, Mar 25, 2026 at 3:53 AM Luiz Campos <luizedc1@gmail.com> wrote:
> >
> > When using `git add -p`, users can stage or skip hunks,
> > but cannot discard unwanted changes from the working tree.
> >
> > Introduce a new 'x' action to discard the current hunk by
> > reverse-applying it.
> >
> > This idea was suggested in a previous mailing list discussion:
> > https://lore.kernel.org/git/X%2FiFCo0bXLR%2BLZXs@coredump.intra.peff.net/t/#m0576e6f3c6375e11cc4693b9dca3c1fc57baadd0
> >
> > Feedback is very welcome.
> >
> > Signed-off-by: Luiz Campos <luizedc1@gmail.com>
>
> One feature the Fugitive Git client for Vim supports is to discard
> hunks; when it does so, it also prints a message explaining how to
> recover the hunk if you need it.
>
> I think it writes the file as a blob to Git's database before
> restoring from the index.
>
> I'm not suggesting Git copy this necessarily, but we might want to
> consider how to help folks when they lose a hunk they didn't mean to.
> If it's never been in the index, it can be impossible to recover!
>
> PS How different is this from "git restore -p" ?
>
>
> --
> D. Ben Knoble

Re: [RFC PATCH 0/1] add -p: support discarding hunks

[Junio C Hamano]

Luiz Campos <luizedc1@gmail.com> writes:

> Hi,
>
> This is an RFC for adding a 'discard hunk' action to `git add -p`.
>
> Currently, when using `git add -p`, users can stage or skip hunks,
> but cannot discard unwanted changes directly from the working tree.
> This often leads to repeatedly skipping the same hunks across
> multiple passes.
>
> This patch introduces a new 'x' action to discard the current hunk
> by reverse-applying it to the working tree.
>
> This idea was previously discussed on the mailing list:
> https://lore.kernel.org/git/X%2FiFCo0bXLR%2BLZXs@coredump.intra.peff.net/t/#m0576e6f3c6375e11cc4693b9dca3c1fc57baadd0
>
> Open questions:
> - Should discard happen immediately or be deferred until patch application?
> - Are there edge cases involving overlapping hunks or edited hunks?

After reading the discussion (by the way, I do not recall seeing it,
so thank you very much for having a link to it), I agree with what
Peff said back then.  "add -p" that touches the working tree feels
quite weird.

In addition to that, letting it make destructive change makes the
idea even less appetizing.  Once you remove the changes introduced
by the hunk, it is forever gone.  A "discard" in "add -p" would not
solve your problem without adding many unhappy users who lost their
work by mistake.  I do not want to see people trigger "discard" by
mistake in "add -p" session _and_ find that there is no way to undo
that mistaken discard.

"stash -p" followed by "add -p" is probably the best we can do that
is safe.  When the unwanted change is truly unwanted garbage that
you would never ever want to see again, "restore -p" followed by
"add -p" would be an alternative.

One reason why they are not satisfying is because during the later
"add -p" session, we will notice that some unwanted things we failed
to notice and get rid of (either by sending them to stash or restoring
it away) are still there, reminding us that we are imperfect human,
and at that point, it is not easy to switch back to the "stash -p"
or "restore -p" from there.

What you want is probably a _single_ command that lets you inspect
the differences among the HEAD, the index, and the working tree, and
allows you to move things hunk-by-hunk in different directions.

 * You can go through the "git diff --cached" (i.e., changes already in
   the index), and selectively undo/revert the changes to the index,
   similar to "git reset -p".

 * You can go through the "git diff" (i.e., changes between the
   index and the working tree), and selectively apply the changes to
   the index, similar to "git add -p".

 * You can go through the "git diff HEAD" (i.e. changes since your
   last commit), and selectively send the changes to a stash entry,
   similar to "git stash -p".  This is not destructive.

And if the single command lets you switch among working with these
modes, you no longer need to worry about forgetting to send
some changes to stash to concentrate on working on the rest.

In addition, optionally you can also have this in the same command:

 * You can go through the "git diff", and selectively revert the
   changes to the working tree, similar to "git restore -p".

This additional mode *is* destructive, but if you know from the hunk
that you will never need the change in it, it would be a right tool
for it.

Re: [RFC PATCH 1/1] add -p: support discarding hunks with 'x'

[Luiz Eduardo Campos]

Hi Phillip,

Thanks for the thorough review.

> it is rather unexpected for "git add" to modify the working copy

I share that concern. My motivation was allowing stage-and-discard in
a single pass, which I find myself wanting regularly, and discussion in
the linked thread suggested it could live in add -p. That said, I can
see the argument that git add modifying the worktree breaks the mental
model. I'm open to discussing whether a separate command or a flag on
git restore -p would be a better home for this — the implementation
would be largely the same either way.

> It would be simpler just to take USE_HUNK or DISCARD_HUNK as an argument

Agreed. I'll drop both the merge_for_discard boolean and the enum
reassemble_mode, and instead pass the desired hunk type directly
(USE_HUNK or DISCARD_HUNK) through reassemble_patch,
merge_hunks, and should_merge_hunk. That removes the indirection
entirely.

> There's no need to add braces here

Right, the project style omits braces for single-statement blocks.
I'll drop them.

> I'm not sure this is an improvement — it certainly makes the patch harder to
> read because you end up changing the indentation of otherwise unchanged
> lines that were in the else clause.

> This is unchanged code that re-indented because of the addition of
> "continue" above.

Good point. With the simplified should_merge_hunk() taking a hunk
type, I can keep the original if / else structure in
reassemble_patch() and just swap out the condition. That avoids the
continue rewrite and the indentation churn entirely.

> We need to tell render_hunk whether a hunk is being applied in reverse or
> not so that it knows whether to apply delta to the old offset or the new offset.

You're right. I was wrong to think the delta direction would be the same as for
staging. For the discard patch, skipped hunks' changes are still present in
the worktree, so the new_offset (worktree side) is already correct and must not
be adjusted. The adjustment needs to go to old_offset instead — exactly the
is_reverse = 1 behaviour. This is consistent with patch_mode_checkout_index,
which does the same operation (diff-files + apply -R to worktree) and
sets is_reverse = 1.

I'll temporarily flip is_reverse (or pass an explicit flag to render_hunk) when
assembling the discard patch, and add the split-hunk tests below to confirm the
offsets come out correct.

> It would be nice to avoid showing this for 'checkout -p' etc.

The help text display already filters lines from
help_patch_remainder against the options present in s->buf
(the prompt string). Since 'x' is only added to the prompt in
patch_mode_add, the "x - discard..." line will already be suppressed
for checkout -p, reset -p, etc.

I'll add a comment in the next version to make that clearer, and add a
test that verifies checkout -p help output does not mention 'x'.

> Why do we need to run "git apply --check" here?

We don't — the existing staging codepath in apply_patch does not run a
separate --check before applying either; it just runs git apply
directly and checks the exit code. I'll do the same and remove the
redundant --check pass.

I'll remove the redundant check and just run git apply -R directly.

> Isn't this comment still relevant?

Yes, I'll restore it. I removed it by accident when restructuring the
function; it still applies to the USE_HUNK scan that follows.

> What's the point of this change?

There is none — it's an accidental whitespace change on the alignment
of the (!file_diff->hunk_nr continuation line. I'll drop it.

> So 'x' is only permitted if 'e' is — what's the reason for that?

The nesting was unintentional, but the conditions guarding 'e' —
hunk_index + 1 > file_diff->mode_change and
!file_diff->deleted — do also apply to 'x', since reverse-applying
a mode-change or deletion hunk is not meaningful.

However, the ADD_P_DISALLOW_EDIT flag (used by git history split)
should not gate 'x'. I'll give 'x' its own block that checks only
mode_change and deleted, independent of ALLOW_EDIT.

> I'm not sure why this test needs to be in a separate repository

The sub-repo was a workaround: the test creates a commit
(git commit -m base), and that extra commit shifted the history and
caused later tests that depend on specific HEAD state to fail.

Instead, I'll rework the test to use a file already tracked in the
shared repo, dirty it, run git add -p with 'x', and clean up with
test_when_finished. No extra commit needed.

> It would be nice to see tests that split a hunk [...] and pathological context lines

Agreed. I'll add tests that:

(1) split a three-subhunk change (-a/+A, -c/+C, -e/+E), stage the
first and third, and discard the second
(2) the inverse — discard the first and third, stage the second
(3) a pathological-context case along the lines of 2bd69b9024c

Thanks again for the review — very helpful.

Luiz

Em qua., 25 de mar. de 2026 às 13:24, Phillip Wood
<phillip.wood123@gmail.com> escreveu:
>
> Hi Luiz
>
> On 25/03/2026 07:50, Luiz Campos wrote:
> > When using `git add -p`, users can stage or skip hunks,
> > but cannot discard unwanted changes from the working tree.
> >
> > Introduce a new 'x' action to discard the current hunk by
> > reverse-applying it.
> >
> > This idea was suggested in a previous mailing list discussion:
> > https://lore.kernel.org/git/X%2FiFCo0bXLR%2BLZXs@coredump.intra.peff.net/t/#m0576e6f3c6375e11cc4693b9dca3c1fc57baadd0
>
> I tend to agree with peff's comments in that thread that it is rather
> unexpected for "git add" to modify the working copy. I also think that a
> command that lets you stage some changes and discard others could be
> useful as I do both fairly frequently from my editor. Regardless of
> whether we want a new command the implementation will be similar so I've
> left some comments on the code below.
>
> > diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
> > index 941135dc63..0ab81e5615 100644
> > --- a/Documentation/git-add.adoc
> > +++ b/Documentation/git-add.adoc
> > @@ -351,12 +351,15 @@ patch::
> >          K - go to the previous hunk, roll over at the top
> >          s - split the current hunk into smaller hunks
> >          e - manually edit the current hunk
> > +       x - discard this hunk from the worktree
> >          p - print the current hunk
> >          P - print the current hunk using the pager
> >          ? - print help
> >   +
> > -After deciding the fate for all hunks, if there is any hunk
> > -that was chosen, the index is updated with the selected hunks.
> > +After deciding the fate for all hunks, any hunks marked for
> > +discard are removed from the working tree (reverted to the index
> > +version for those lines).  Then, if there is any hunk chosen for
> > +staging, the index is updated with those hunks.
>
> Makes sense.
>
> >   +
> >   You can omit having to type return here, by setting the configuration
> >   variable `interactive.singleKey` to `true`.
> > diff --git a/add-patch.c b/add-patch.c
> > index 4e28e5c187..ea38ab453e 100644
> > --- a/add-patch.c
> > +++ b/add-patch.c
> > @@ -259,7 +259,7 @@ struct hunk_header {
> >   struct hunk {
> >       size_t start, end, colored_start, colored_end, splittable_into;
> >       ssize_t delta;
> > -     enum { UNDECIDED_HUNK = 0, SKIP_HUNK, USE_HUNK } use;
> > +     enum { UNDECIDED_HUNK = 0, SKIP_HUNK, USE_HUNK, DISCARD_HUNK } use;
> >       struct hunk_header header;
> >   };
> >
> > @@ -884,17 +884,35 @@ static void render_diff_header(struct add_p_state *s,
> >       }
> >   }
> >
> > +static bool should_merge_hunk(struct file_diff *file_diff,
> > +                           size_t hunk_index, int use_all,
> > +                           int merge_for_discard)
> > +{
> > +     if (use_all)
> > +             return true;
>
> If we're looking for hunks to discard then we want to return false if
> all the hunks have been selected to be staged.
>
> > +     return merge_for_discard
> > +             ? file_diff->hunk[hunk_index].use == DISCARD_HUNK
> > +             : file_diff->hunk[hunk_index].use == USE_HUNK;
>
> It would be simpler just to take USE_HUNK or DISCARD_HUNK as an argument
> rather than a boolean here.
>
> >   /* Coalesce hunks again that were split */
> >   static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff,
> > -                    size_t *hunk_index, int use_all, struct hunk *merged)
> > +                    size_t *hunk_index, int use_all, struct hunk *merged,
> > +                    int merge_for_discard)
>
> Taking the type of hunk we want to retain (USE_HUNK or DISCARD_HUNK)
> would avoid having to convert merge_for_discard back into the hunk type
> in should_merge_hunk().
>
> >   {
> >       size_t i = *hunk_index, delta;
> >       struct hunk *hunk = file_diff->hunk + i;
> >       /* `header` corresponds to the merged hunk */
> >       struct hunk_header *header = &merged->header, *next;
> >
> > -     if (!use_all && hunk->use != USE_HUNK)
> > +     if (!should_merge_hunk(file_diff, *hunk_index, use_all, merge_for_discard)) {
> >               return 0;
> > +     }
>
> There's no need to add braces here
>
> > @@ -1014,11 +1032,13 @@ static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff,
> >
> >   static void reassemble_patch(struct add_p_state *s,
> >                            struct file_diff *file_diff, int use_all,
> > +                          enum reassemble_mode mode,
> >                            struct strbuf *out)
> >   {
> >       struct hunk *hunk;
> >       size_t save_len = s->plain.len, i;
> >       ssize_t delta = 0;
> > +     int merge_for_discard = (mode == REASSEMBLE_DISCARD);
>
> It would by simpler just to take USE_HUNK or DISCARD_HUNK as a parameter
> and pass that to reassemble_patch() rather than forcing the caller to
> pass an enum that we then transform to a boolean.
>
> >
> >       render_diff_header(s, file_diff, 0, out);
> >
> > @@ -1026,25 +1046,26 @@ static void reassemble_patch(struct add_p_state *s,
> >               struct hunk merged = { 0 };
> >
> >               hunk = file_diff->hunk + i;
> > -             if (!use_all && hunk->use != USE_HUNK)
> > +             if (!should_merge_hunk(file_diff, i, use_all, merge_for_discard)) {
> >                       delta += hunk->header.old_count
> >                               - hunk->header.new_count;
> > -             else {
> > -                     /* merge overlapping hunks into a temporary hunk */
> > -                     if (merge_hunks(s, file_diff, &i, use_all, &merged))
> > -                             hunk = &merged;
> > +                     continue;
>
> I'm not sure this is an improvement - it certainly makes the patch
> harder to read because you end up changing the indentation of otherwise
> unchanged lines that were in the else clause.
>
> > +             }
> >
> > -                     render_hunk(s, hunk, delta, 0, out);
> > +             if (merge_hunks(s, file_diff, &i, use_all, &merged,
> > +                             merge_for_discard))
> > +                     hunk = &merged;
> >
> > -                     /*
> > -                      * In case `merge_hunks()` used `plain` as a scratch
> > -                      * pad (this happens when an edited hunk had to be
> > -                      * coalesced with another hunk).
> > -                      */
> > -                     strbuf_setlen(&s->plain, save_len);
> > +             render_hunk(s, hunk, delta, 0, out);
>
> We need to tell render_hunk whether a hunk is being applied in reverse
> or not so that it knows whether to apply delta to the old offset or the
> new offset. Currently it uses s->mode->reverse but that will not be
> correct for the patch that discards changes from the working tree. I'm
> not sure what the best way of doing that is, the simplest approach is to
> invert s->mode->reverse when we want to keep hunks marked DISCARD_HUNK
> and then restore the original value.
>
> > -                     delta += hunk->delta;
> > -             }
> > +             /*
> > +              * In case `merge_hunks()` used `plain` as a scratch
> > +              * pad (this happens when an edited hunk had to be
> > +              * coalesced with another hunk).
> > +              */
> > +             strbuf_setlen(&s->plain, save_len);
> > +
> > +             delta += hunk->delta;
>
> This is unchanged code that re-indented because of the addition of
> "continue" above.
>
> >       }
> >   }
>
> > @@ -1540,6 +1562,7 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
> >      "/ - search for a hunk matching the given regex\n"
> >      "s - split the current hunk into smaller hunks\n"
> >      "e - manually edit the current hunk\n"
> > +   "x - discard this hunk from the worktree\n"
>
> It would be nice to avoid showing this for 'checkout -p' etc.
>
> >      "p - print the current hunk\n"
> >      "P - print the current hunk using the pager\n"
> >      "> - go to the next file, roll over at the bottom\n"
> > @@ -1547,21 +1570,57 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
> >      "? - print help\n"
> >      "HUNKS SUMMARY - Hunks: %d, USE: %d, SKIP: %d\n");
> >
> > +static int apply_discard_hunks(struct add_p_state *s,
> > +                            struct file_diff *file_diff)
> > +{
> > +     struct child_process check_cp = CHILD_PROCESS_INIT;
> > +     struct child_process apply_cp = CHILD_PROCESS_INIT;
> > +
> > +     strbuf_reset(&s->buf);
> > +     reassemble_patch(s, file_diff, 0, REASSEMBLE_DISCARD, &s->buf);
> > +
> > +     discard_index(s->index);
> > +
> > +     setup_child_process(s, &check_cp, "apply", "-R", "--check", NULL);
> > +     if (pipe_command(&check_cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0)) {
> > +             error(_("'git apply -R --check' failed"));
> > +             return -1;
> > +     }
>
> Why do we need to run "git apply --check" here?
>
> > +     setup_child_process(s, &apply_cp, "apply", "-R", NULL);
> > +     if (pipe_command(&apply_cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0)) {
> > +             error(_("'git apply -R' failed"));
> > +             return -1;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >   static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
> >   {
> >       struct child_process cp = CHILD_PROCESS_INIT;
> >       size_t j;
> > +     int needs_refresh = 0;
> > +
> > +     if (s->mode == &patch_mode_add) {
> > +             for (j = 0; j < file_diff->hunk_nr; j++) {
> > +                     if (file_diff->hunk[j].use == DISCARD_HUNK)
> > +                             break;
> > +             }
> > +             if (j < file_diff->hunk_nr && apply_discard_hunks(s, file_diff))
> > +                     return;
> > +             if (j < file_diff->hunk_nr)
> > +                     needs_refresh = 1;
> > +     }
> >
> > -     /* Any hunk to be used? */
>
> Isn't this comment still relevant?
>
> >       for (j = 0; j < file_diff->hunk_nr; j++)
> >               if (file_diff->hunk[j].use == USE_HUNK)
> >                       break;
> >
> >       if (j < file_diff->hunk_nr ||
> > -             (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
> > -             /* At least one hunk selected: apply */
> > +         (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
>
> What's the point of this change?
>
> >               strbuf_reset(&s->buf);
> > -             reassemble_patch(s, file_diff, 0, &s->buf);
> > +             reassemble_patch(s, file_diff, 0, REASSEMBLE_STAGE, &s->buf);
> >
> >               discard_index(s->index);
> >               if (s->mode->apply_for_checkout)
> > @@ -1574,13 +1633,15 @@ static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
> >                                       NULL, 0, NULL, 0))
> >                               error(_("'git apply' failed"));
> >               }
> > -             if (read_index_from(s->index, s->index_file, s->r->gitdir) >= 0 &&
> > -                 s->index == s->r->index) {
> > -                     repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0,
> > -                                                  1, NULL, NULL, NULL);
> > -             }
> > +             needs_refresh = 1;
> >       }
> >
> > +     if (needs_refresh &&
> > +         read_index_from(s->index, s->index_file, s->r->gitdir) >= 0 &&
> > +         s->index == s->r->index) {
> > +             repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0,
> > +                                          1, NULL, NULL, NULL);
>
> We now wait until we've applied both patches before refreshing the index
> - sounds sensible.
>
> > +     }
> >   }
>
>
> > @@ -1722,6 +1784,10 @@ static size_t patch_update_file(struct add_p_state *s,
> >                           !file_diff->deleted) {
> >                               permitted |= ALLOW_EDIT;
> >                               strbuf_addstr(&s->buf, ",e");
> > +                             if (s->mode == &patch_mode_add) {
> > +                                     permitted |= ALLOW_DISCARD;
> > +                                     strbuf_addstr(&s->buf, ",x");
> > +                             }
>
> So 'x' is only permitted if 'e' is what's the reason for that?
> > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> > [...]
> > +test_expect_success 'add -p discard removes worktree change' '
> > +     test_when_finished "rm -rf discard-testrepo" &&
> > +     mkdir discard-testrepo &&
>
> It's nice to see a test for 'x', but I'm not sure why this test needs to
> be in a separate repository - why can't it use the same repository as
> the other tests?
>
> > +     (
> > +             cd discard-testrepo &&
> > +             git init -b main &&
> > +             echo clean >discard-me &&
> > +             git add discard-me &&
> > +             git commit -m base &&
> > +             echo extra >>discard-me &&
> > +             test_write_lines x | git add -p discard-me &&
> > +             printf "clean\n" >expect &&
> > +             test_cmp expect discard-me &&
> > +             git diff --cached >tmp &&
> > +             test_must_be_empty tmp
>
> It would be nice to see tests that split a hunk like
>
> -a
> +A
>   b
> -c
> +C
>   d
> -e
> +E
>
> and then (1) stage the first and third sub-hunks and discard the second,
> (2) discard the first and third sub-hunks and stage the second. It would
> also be nice to see a test that discards a hunk with pathological
> context lines - see 2bd69b9024c (add -p: fix checkout -p with
> pathological context, 2019-06-12) for an example.
>
> Thanks
>
> Phillip
>
>
> > +     )
> > +'
> > +
> >   test_expect_success 'setup expected' '
> >       cat >expected <<-\EOF
> >       diff --git a/non-empty b/non-empty
> > @@ -521,13 +539,13 @@ test_expect_success 'split hunk setup' '
> >   test_expect_success 'goto hunk 1 with "g 1"' '
> >       test_when_finished "git reset" &&
> >       tr _ " " >expect <<-EOF &&
> > -     (2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,P,?]? + 1:  -1,2 +1,3          +15
> > +     (2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,x,p,P,?]? + 1:  -1,2 +1,3          +15
> >       _ 2:  -2,4 +3,8          +21
> >       go to which hunk? @@ -1,2 +1,3 @@
> >       _10
> >       +15
> >       _20
> > -     (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?_
> > +     (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?_
> >       EOF
> >       test_write_lines s y g 1 | git add -p >actual &&
> >       tail -n 7 <actual >actual.trimmed &&
> > @@ -540,7 +558,7 @@ test_expect_success 'goto hunk 1 with "g1"' '
> >       _10
> >       +15
> >       _20
> > -     (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?_
> > +     (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?_
> >       EOF
> >       test_write_lines s y g1 | git add -p >actual &&
> >       tail -n 4 <actual >actual.trimmed &&
> > @@ -550,11 +568,11 @@ test_expect_success 'goto hunk 1 with "g1"' '
> >   test_expect_success 'navigate to hunk via regex /pattern' '
> >       test_when_finished "git reset" &&
> >       tr _ " " >expect <<-EOF &&
> > -     (2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,P,?]? @@ -1,2 +1,3 @@
> > +     (2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,x,p,P,?]? @@ -1,2 +1,3 @@
> >       _10
> >       +15
> >       _20
> > -     (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?_
> > +     (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?_
> >       EOF
> >       test_write_lines s y /1,2 | git add -p >actual &&
> >       tail -n 5 <actual >actual.trimmed &&
> > @@ -567,7 +585,7 @@ test_expect_success 'navigate to hunk via regex / pattern' '
> >       _10
> >       +15
> >       _20
> > -     (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?_
> > +     (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?_
> >       EOF
> >       test_write_lines s y / 1,2 | git add -p >actual &&
> >       tail -n 4 <actual >actual.trimmed &&
> > @@ -579,11 +597,11 @@ test_expect_success 'print again the hunk' '
> >       tr _ " " >expect <<-EOF &&
> >       +15
> >        20
> > -     (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? @@ -1,2 +1,3 @@
> > +     (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? @@ -1,2 +1,3 @@
> >        10
> >       +15
> >        20
> > -     (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?_
> > +     (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?_
> >       EOF
> >       test_write_lines s y g 1 p | git add -p >actual &&
> >       tail -n 7 <actual >actual.trimmed &&
> > @@ -595,11 +613,11 @@ test_expect_success TTY 'print again the hunk (PAGER)' '
> >       cat >expect <<-EOF &&
> >       <GREEN>+<RESET><GREEN>15<RESET>
> >        20<RESET>
> > -     <BOLD;BLUE>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
> > +     <BOLD;BLUE>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
> >       PAGER  10<RESET>
> >       PAGER <GREEN>+<RESET><GREEN>15<RESET>
> >       PAGER  20<RESET>
> > -     <BOLD;BLUE>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? <RESET>
> > +     <BOLD;BLUE>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? <RESET>
> >       EOF
> >       test_write_lines s y g 1 P |
> >       (
> > @@ -796,21 +814,21 @@ test_expect_success 'colors can be overridden' '
> >       <BLUE>+<RESET><BLUE>new<RESET>
> >       <CYAN> more-context<RESET>
> >       <BLUE>+<RESET><BLUE>another-one<RESET>
> > -     <YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,p,P,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
> > +     <YELLOW>(1/1) Stage this hunk [y,n,q,a,d,s,e,x,p,P,?]? <RESET><BOLD>Split into 2 hunks.<RESET>
> >       <MAGENTA>@@ -1,3 +1,3 @@<RESET>
> >       <CYAN> context<RESET>
> >       <BOLD>-old<RESET>
> >       <BLUE>+<RESET><BLUE>new<RESET>
> >       <CYAN> more-context<RESET>
> > -     <YELLOW>(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
> > +     <YELLOW>(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
> >       <CYAN> more-context<RESET>
> >       <BLUE>+<RESET><BLUE>another-one<RESET>
> > -     <YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,P,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
> > +     <YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,x,p,P,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
> >       <CYAN> context<RESET>
> >       <BOLD>-old<RESET>
> >       <BLUE>+new<RESET>
> >       <CYAN> more-context<RESET>
> > -     <YELLOW>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? <RESET>
> > +     <YELLOW>(1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? <RESET>
> >       EOF
> >       test_cmp expect actual
> >   '
> > @@ -1424,9 +1442,9 @@ test_expect_success 'invalid option s is rejected' '
> >       test_write_lines j s q | git add -p >out &&
> >       sed -ne "s/ @@.*//" -e "s/ \$//" -e "/^(/p" <out >actual &&
> >       cat >expect <<-EOF &&
> > -     (1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,s,e,p,P,?]?
> > -     (2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]? Sorry, cannot split this hunk
> > -     (2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?
> > +     (1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,s,e,x,p,P,?]?
> > +     (2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]? Sorry, cannot split this hunk
> > +     (2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,x,p,P,?]?
> >       EOF
> >       test_cmp expect actual
> >   '
>

Re: [RFC PATCH 1/1] add -p: support discarding hunks with 'x'

[Luiz Eduardo Campos]

Hi Johannes,

Thank you for taking the time to review this!

> This hunk is quite hard to read because of the `if ... else ...` -> `if {
> ... continue; } ...` change that de-indents a large chunk of code.

You are right. Even though you marked it as "not really actionable",
I think it is worth fixing: I can keep the original if/else structure
and just replace the condition with the should_merge_hunk() call.
That avoids the indentation churn and keeps the diff focused on what
actually changes.

> If you detect an empty patch here and indicate this via an early return
> value, then...
> ... then this loop is no longer necessary.

Good idea. I will have apply_discard_hunks() check whether any hunk
is marked DISCARD_HUNK before going through the apply machinery, and
use the return value to distinguish "nothing to do" from "applied" and
"error". With that in place the pre-scan loop in apply_patch() can be
dropped, and needs_refresh can just be set based on whether
apply_discard_hunks() actually applied something.

I will address this in v2; the implementation might still live in add -p,
or I may fold it into a shared path that fits better (suggestions are
welcome!)

Thanks,
Luiz

Em qua., 25 de mar. de 2026 às 13:49, Johannes Schindelin
<Johannes.Schindelin@gmx.de> escreveu:
>
> Hi Luiz,
>
> On Wed, 25 Mar 2026, Luiz Campos wrote:
>
> > When using `git add -p`, users can stage or skip hunks,
> > but cannot discard unwanted changes from the working tree.
> >
> > Introduce a new 'x' action to discard the current hunk by
> > reverse-applying it.
> >
> > This idea was suggested in a previous mailing list discussion:
> > https://lore.kernel.org/git/X%2FiFCo0bXLR%2BLZXs@coredump.intra.peff.net/t/#m0576e6f3c6375e11cc4693b9dca3c1fc57baadd0
>
> Sounds good!
>
> Just two minor comments (not really actionable, I think):
>
> > @@ -1026,25 +1046,26 @@ static void reassemble_patch(struct add_p_state *s,
> >               struct hunk merged = { 0 };
> >
> >               hunk = file_diff->hunk + i;
> > -             if (!use_all && hunk->use != USE_HUNK)
> > +             if (!should_merge_hunk(file_diff, i, use_all, merge_for_discard)) {
> >                       delta += hunk->header.old_count
> >                               - hunk->header.new_count;
> > -             else {
> > -                     /* merge overlapping hunks into a temporary hunk */
> > -                     if (merge_hunks(s, file_diff, &i, use_all, &merged))
> > -                             hunk = &merged;
> > +                     continue;
> > +             }
> >
> > -                     render_hunk(s, hunk, delta, 0, out);
> > +             if (merge_hunks(s, file_diff, &i, use_all, &merged,
> > +                             merge_for_discard))
> > +                     hunk = &merged;
> >
> > -                     /*
> > -                      * In case `merge_hunks()` used `plain` as a scratch
> > -                      * pad (this happens when an edited hunk had to be
> > -                      * coalesced with another hunk).
> > -                      */
> > -                     strbuf_setlen(&s->plain, save_len);
> > +             render_hunk(s, hunk, delta, 0, out);
> >
> > -                     delta += hunk->delta;
> > -             }
> > +             /*
> > +              * In case `merge_hunks()` used `plain` as a scratch
> > +              * pad (this happens when an edited hunk had to be
> > +              * coalesced with another hunk).
> > +              */
> > +             strbuf_setlen(&s->plain, save_len);
> > +
> > +             delta += hunk->delta;
>
> This hunk is quite hard to read because of the `if ... else ...` -> `if {
> ... continue; } ...` change that de-indents a large chunk of code.
>
> After pouring over the diff for a bit, I was able to convince myself that
> the diff is correct.
>
> > @@ -1547,21 +1570,57 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
> >     "? - print help\n"
> >     "HUNKS SUMMARY - Hunks: %d, USE: %d, SKIP: %d\n");
> >
> > +static int apply_discard_hunks(struct add_p_state *s,
> > +                            struct file_diff *file_diff)
> > +{
> > +     struct child_process check_cp = CHILD_PROCESS_INIT;
> > +     struct child_process apply_cp = CHILD_PROCESS_INIT;
> > +
> > +     strbuf_reset(&s->buf);
> > +     reassemble_patch(s, file_diff, 0, REASSEMBLE_DISCARD, &s->buf);
>
> If you detect an empty patch here and indicate this via an early return
> value, then...
>
> > +
> > +     discard_index(s->index);
> > +
> > +     setup_child_process(s, &check_cp, "apply", "-R", "--check", NULL);
> > +     if (pipe_command(&check_cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0)) {
> > +             error(_("'git apply -R --check' failed"));
> > +             return -1;
> > +     }
> > +
> > +     setup_child_process(s, &apply_cp, "apply", "-R", NULL);
> > +     if (pipe_command(&apply_cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0)) {
> > +             error(_("'git apply -R' failed"));
> > +             return -1;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
> >  {
> >       struct child_process cp = CHILD_PROCESS_INIT;
> >       size_t j;
> > +     int needs_refresh = 0;
> > +
> > +     if (s->mode == &patch_mode_add) {
> > +             for (j = 0; j < file_diff->hunk_nr; j++) {
> > +                     if (file_diff->hunk[j].use == DISCARD_HUNK)
> > +                             break;
> > +             }
> > +             if (j < file_diff->hunk_nr && apply_discard_hunks(s, file_diff))
> > +                     return;
> > +             if (j < file_diff->hunk_nr)
> > +                     needs_refresh = 1;
> > +     }
>
> ... then this loop is no longer necessary.
>
> Other than that, looks good to me!
>
> Ciao,
> Johannes

Re: [RFC PATCH 0/1] add -p: support discarding hunks

[Luiz Eduardo Campos]

Hi Junio,

Thanks a lot for the detailed feedback.

> "add -p" that touches the working tree feels quite weird.

I understand the concern, and I agree that having `git add -p`
perform destructive changes on the working tree is a significant
departure from its current mental model.

> people trigger "discard" by mistake ... no way to undo

This is a very good point. I had been thinking of this primarily
as a convenience for workflows where users repeatedly skip hunks,
but you are right that making such an operation easy to trigger
in an interactive session could lead to accidental data loss,
and that would be problematic.

> What you want is probably a single command ...

This is a very interesting direction. My original motivation was
exactly to avoid having to switch between `git add -p`,
`git restore -p`, and `git stash -p` when reviewing changes,
but I had not considered approaching it as a single interface with
modes over the different views (HEAD, index, worktree) and
non-destructive flows like stashing.

That does seem like a more coherent model. In the meantime I will
rely on the workflows you mentioned (`stash -p` or `restore -p`
followed by `git add -p`) rather than pursuing discard within
`add -p` alone.

I will step back from the narrow "discard in add -p" RFC while I
think more about this broader direction. One question I would like
guidance on is whether something like what you describe would be
more appropriate as a new top-level command, or as an extension of
an existing one (and if so, which entry point would be the least
surprising).

If a unified tool ever includes a destructive "revert hunk in the
worktree" mode, I agree it would need to be very clearly separated
and hard to trigger by mistake.

Thanks again for the guidance; it is very helpful.

Luiz

Em qua., 25 de mar. de 2026 às 15:03, Junio C Hamano
<gitster@pobox.com> escreveu:
>
> Luiz Campos <luizedc1@gmail.com> writes:
>
> > Hi,
> >
> > This is an RFC for adding a 'discard hunk' action to `git add -p`.
> >
> > Currently, when using `git add -p`, users can stage or skip hunks,
> > but cannot discard unwanted changes directly from the working tree.
> > This often leads to repeatedly skipping the same hunks across
> > multiple passes.
> >
> > This patch introduces a new 'x' action to discard the current hunk
> > by reverse-applying it to the working tree.
> >
> > This idea was previously discussed on the mailing list:
> > https://lore.kernel.org/git/X%2FiFCo0bXLR%2BLZXs@coredump.intra.peff.net/t/#m0576e6f3c6375e11cc4693b9dca3c1fc57baadd0
> >
> > Open questions:
> > - Should discard happen immediately or be deferred until patch application?
> > - Are there edge cases involving overlapping hunks or edited hunks?
>
> After reading the discussion (by the way, I do not recall seeing it,
> so thank you very much for having a link to it), I agree with what
> Peff said back then.  "add -p" that touches the working tree feels
> quite weird.
>
> In addition to that, letting it make destructive change makes the
> idea even less appetizing.  Once you remove the changes introduced
> by the hunk, it is forever gone.  A "discard" in "add -p" would not
> solve your problem without adding many unhappy users who lost their
> work by mistake.  I do not want to see people trigger "discard" by
> mistake in "add -p" session _and_ find that there is no way to undo
> that mistaken discard.
>
> "stash -p" followed by "add -p" is probably the best we can do that
> is safe.  When the unwanted change is truly unwanted garbage that
> you would never ever want to see again, "restore -p" followed by
> "add -p" would be an alternative.
>
> One reason why they are not satisfying is because during the later
> "add -p" session, we will notice that some unwanted things we failed
> to notice and get rid of (either by sending them to stash or restoring
> it away) are still there, reminding us that we are imperfect human,
> and at that point, it is not easy to switch back to the "stash -p"
> or "restore -p" from there.
>
> What you want is probably a _single_ command that lets you inspect
> the differences among the HEAD, the index, and the working tree, and
> allows you to move things hunk-by-hunk in different directions.
>
>  * You can go through the "git diff --cached" (i.e., changes already in
>    the index), and selectively undo/revert the changes to the index,
>    similar to "git reset -p".
>
>  * You can go through the "git diff" (i.e., changes between the
>    index and the working tree), and selectively apply the changes to
>    the index, similar to "git add -p".
>
>  * You can go through the "git diff HEAD" (i.e. changes since your
>    last commit), and selectively send the changes to a stash entry,
>    similar to "git stash -p".  This is not destructive.
>
> And if the single command lets you switch among working with these
> modes, you no longer need to worry about forgetting to send
> some changes to stash to concentrate on working on the rest.
>
> In addition, optionally you can also have this in the same command:
>
>  * You can go through the "git diff", and selectively revert the
>    changes to the working tree, similar to "git restore -p".
>
> This additional mode *is* destructive, but if you know from the hunk
> that you will never need the change in it, it would be a right tool
> for it.