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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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