Re: Question about behaviour of git-checkout --patch option

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

[-- Attachment #1: Type: text/plain, Size: 8891 bytes --]

Hi Peff & Merlin,

On Wed, 27 May 2020, Jeff King wrote:

> On Mon, May 25, 2020 at 10:11:00PM +0200, Merlin Büge wrote:
>
> > This works like expected, I end up with a 1:1 copy of the original
> > worktree, including empty files. However, if I include the -p option in
> > the last step:
> >
> > 	git checkout -p master .
> >
> > ... I correctly get asked for any non-empty files/hunks if I want to
> > apply them - but not for empty ones. It would just display e.g.
> >
> > 	diff --git b/emptyfile a/emptyfile
> > 	new file mode 100644
> > 	index 0000000..e69de29
> >
> > and then skip over it, asking for the next non-empty hunk.
> >
> > Why does it skip over empty hunks?
>
> I think this case was just never anticipated, and it's a bug. The
> original patch-selection code was written for "add -p", and the
> fundamental unit it works on is a hunk. We hacked around that to handle
> deletions back in 24ab81ae4d (add-interactive: handle deletion of empty
> files, 2009-10-27). But "add -p" would never see a new file, since we
> only consider the set of tracked files in the index.
>
> However, when it was extended for "checkout -p", etc, we could see new
> files. I guess the right fix is something along these lines, extending
> the deletion concept to new files:
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index c4b75bcb7f..9c8844434e 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -768,7 +768,7 @@ sub parse_diff_header {
>  	for (my $i = 0; $i < @{$src->{TEXT}}; $i++) {
>  		my $dest =
>  		   $src->{TEXT}->[$i] =~ /^(old|new) mode (\d+)$/ ? $mode :
> -		   $src->{TEXT}->[$i] =~ /^deleted file/ ? $deletion :
> +		   $src->{TEXT}->[$i] =~ /^(deleted|new) file/ ? $deletion :
>  		   $head;
>  		push @{$dest->{TEXT}}, $src->{TEXT}->[$i];
>  		push @{$dest->{DISPLAY}}, $src->{DISPLAY}->[$i];
>
> which does do the right thing. But:
>
>   - all of this is in the process of being rewritten in C, so it should
>     probably go into add-interactive.c (it's not yet the default to use
>     the C version, but it probably will be soon).
>
>   - that still says "Apply deletion..." in the UI. We'd need to update
>     the messages to differentiate the two cases.

The C version (with "Apply addition"... in the UI) would look somewhat
like this:

-- snipsnap --
diff --git a/add-patch.c b/add-patch.c
index d8bfe379be4d..c0d8bf2df0c5 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -10,7 +10,7 @@
 #include "prompt.h"

 enum prompt_mode_type {
-	PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_HUNK,
+	PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK,
 	PROMPT_MODE_MAX, /* must be last */
 };

@@ -33,6 +33,7 @@ static struct patch_mode patch_mode_add = {
 	.prompt_mode = {
 		N_("Stage mode change [y,n,q,a,d%s,?]? "),
 		N_("Stage deletion [y,n,q,a,d%s,?]? "),
+		N_("Stage addition [y,n,q,a,d%s,?]? "),
 		N_("Stage this hunk [y,n,q,a,d%s,?]? ")
 	},
 	.edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk "
@@ -54,6 +55,7 @@ static struct patch_mode patch_mode_stash = {
 	.prompt_mode = {
 		N_("Stash mode change [y,n,q,a,d%s,?]? "),
 		N_("Stash deletion [y,n,q,a,d%s,?]? "),
+		N_("Stash addition [y,n,q,a,d%s,?]? "),
 		N_("Stash this hunk [y,n,q,a,d%s,?]? "),
 	},
 	.edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk "
@@ -77,6 +79,7 @@ static struct patch_mode patch_mode_reset_head = {
 	.prompt_mode = {
 		N_("Unstage mode change [y,n,q,a,d%s,?]? "),
 		N_("Unstage deletion [y,n,q,a,d%s,?]? "),
+		N_("Unstage addition [y,n,q,a,d%s,?]? "),
 		N_("Unstage this hunk [y,n,q,a,d%s,?]? "),
 	},
 	.edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk "
@@ -99,6 +102,7 @@ static struct patch_mode patch_mode_reset_nothead = {
 	.prompt_mode = {
 		N_("Apply mode change to index [y,n,q,a,d%s,?]? "),
 		N_("Apply deletion to index [y,n,q,a,d%s,?]? "),
+		N_("Apply addition to index [y,n,q,a,d%s,?]? "),
 		N_("Apply this hunk to index [y,n,q,a,d%s,?]? "),
 	},
 	.edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk "
@@ -121,6 +125,7 @@ static struct patch_mode patch_mode_checkout_index = {
 	.prompt_mode = {
 		N_("Discard mode change from worktree [y,n,q,a,d%s,?]? "),
 		N_("Discard deletion from worktree [y,n,q,a,d%s,?]? "),
+		N_("Discard addition from worktree [y,n,q,a,d%s,?]? "),
 		N_("Discard this hunk from worktree [y,n,q,a,d%s,?]? "),
 	},
 	.edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk "
@@ -143,6 +148,7 @@ static struct patch_mode patch_mode_checkout_head = {
 	.prompt_mode = {
 		N_("Discard mode change from index and worktree [y,n,q,a,d%s,?]? "),
 		N_("Discard deletion from index and worktree [y,n,q,a,d%s,?]? "),
+		N_("Discard addition from index and worktree [y,n,q,a,d%s,?]? "),
 		N_("Discard this hunk from index and worktree [y,n,q,a,d%s,?]? "),
 	},
 	.edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk "
@@ -164,6 +170,7 @@ static struct patch_mode patch_mode_checkout_nothead = {
 	.prompt_mode = {
 		N_("Apply mode change to index and worktree [y,n,q,a,d%s,?]? "),
 		N_("Apply deletion to index and worktree [y,n,q,a,d%s,?]? "),
+		N_("Apply addition to index and worktree [y,n,q,a,d%s,?]? "),
 		N_("Apply this hunk to index and worktree [y,n,q,a,d%s,?]? "),
 	},
 	.edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk "
@@ -186,6 +193,7 @@ static struct patch_mode patch_mode_worktree_head = {
 	.prompt_mode = {
 		N_("Discard mode change from index and worktree [y,n,q,a,d%s,?]? "),
 		N_("Discard deletion from index and worktree [y,n,q,a,d%s,?]? "),
+		N_("Discard addition from index and worktree [y,n,q,a,d%s,?]? "),
 		N_("Discard this hunk from index and worktree [y,n,q,a,d%s,?]? "),
 	},
 	.edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk "
@@ -207,6 +215,7 @@ static struct patch_mode patch_mode_worktree_nothead = {
 	.prompt_mode = {
 		N_("Apply mode change to index and worktree [y,n,q,a,d%s,?]? "),
 		N_("Apply deletion to index and worktree [y,n,q,a,d%s,?]? "),
+		N_("Apply addition to index and worktree [y,n,q,a,d%s,?]? "),
 		N_("Apply this hunk to index and worktree [y,n,q,a,d%s,?]? "),
 	},
 	.edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk "
@@ -248,7 +257,7 @@ struct add_p_state {
 		struct hunk head;
 		struct hunk *hunk;
 		size_t hunk_nr, hunk_alloc;
-		unsigned deleted:1, mode_change:1,binary:1;
+		unsigned deleted:1, added:1, mode_change:1,binary:1;
 	} *file_diff;
 	size_t file_diff_nr;

@@ -442,7 +451,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
 	pend = p + plain->len;
 	while (p != pend) {
 		char *eol = memchr(p, '\n', pend - p);
-		const char *deleted = NULL, *mode_change = NULL;
+		const char *deleted = NULL, *added = NULL, *mode_change = NULL;

 		if (!eol)
 			eol = pend;
@@ -465,7 +474,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
 			; /* keep the rest of the file in a single "hunk" */
 		else if (starts_with(p, "@@ ") ||
 			 (hunk == &file_diff->head &&
-			  skip_prefix(p, "deleted file", &deleted))) {
+			  (skip_prefix(p, "deleted file", &deleted) ||
+			   skip_prefix(p, "new file", &added)))) {
 			if (marker == '-' || marker == '+')
 				/*
 				 * Should not happen; previous hunk did not end
@@ -485,6 +495,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)

 			if (deleted)
 				file_diff->deleted = 1;
+			else if (added)
+				file_diff->added = 1;
 			else if (parse_hunk_header(s, hunk) < 0)
 				return -1;

@@ -537,8 +549,10 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
 			   starts_with(p, "Binary files "))
 			file_diff->binary = 1;

-		if (file_diff->deleted && file_diff->mode_change)
-			BUG("diff contains delete *and* a mode change?!?\n%.*s",
+		if (!!file_diff->deleted + !!file_diff->added +
+		    !!file_diff->mode_change > 1)
+			BUG("diff can only contain delete *or* add *or* a "
+			    "mode change?!?\n%.*s",
 			    (int)(eol - (plain->buf + file_diff->head.start)),
 			    plain->buf + file_diff->head.start);

@@ -1392,11 +1406,13 @@ static int patch_update_file(struct add_p_state *s,
 		if (hunk->splittable_into > 1)
 			strbuf_addstr(&s->buf, ",s");
 		if (hunk_index + 1 > file_diff->mode_change &&
-		    !file_diff->deleted)
+		    !file_diff->deleted && !file_diff->added)
 			strbuf_addstr(&s->buf, ",e");

 		if (file_diff->deleted)
 			prompt_mode_type = PROMPT_DELETION;
+		else if (file_diff->added)
+			prompt_mode_type = PROMPT_ADDITION;
 		else if (file_diff->mode_change && !hunk_index)
 			prompt_mode_type = PROMPT_MODE_CHANGE;
 		else