Question about behaviour of git-checkout --patch option

Question about behaviour of git-checkout --patch option

[Merlin Büge]

Hi all!


I have a question about the behaviour of the git-checkout --patch/-p
option.

I have a small local git repository containing empty and non-empty
files. I wanted to replicate it in another directory and did the
following:

	cd <target_dir>
	git init
	git remote add origin <origin_dir>
	git fetch
	git branch --track master origin/master
	git checkout master .

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?


Thanks!

-- 
Merlin Büge

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

[Johannes Schindelin]

[-- 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

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

[Johannes Schindelin]

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

Hi Merlin,

On Wed, 27 May 2020, Merlin Büge wrote:

> On Wed, 27 May 2020 04:00:09 -0400
> Jeff King <peff@peff.net> wrote:
>
> > Trying your case with:
> >
> >   git -c add.interactive.usebuiltin=true checkout -p master .
> >
> > shows that it does not list the added file at all. I suspect the form of
> > the fix will be similar, but it may have to plumb through the file
> > addition from the diff layer, as well.
>
> Thanks a lot for your explanation! Good to know what's going on there.
>
> Out of curiosity: How does the git community keep track of open bugs?
> On https://git-scm.com/ I read that bug reports should be just send to
> the mailing list. But if they are not fixed within a few days, wouldn't
> they just get lost...?

There is not really any centralized way to keep track of open bugs.

And yes, sometimes they get lost after a few days.

In your case, however, there is now
https://lore.kernel.org/git/pull.646.git.1590613746507.gitgitgadget@gmail.com/T/#u
which is better than just a ticket: it is already a patch.

Ciao,
Johannes

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

[Jeff King]

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.

-Peff

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

[Jeff King]

On Wed, May 27, 2020 at 03:56:48AM -0400, Jeff King wrote:

> 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).

Trying your case with:

  git -c add.interactive.usebuiltin=true checkout -p master .

shows that it does not list the added file at all. I suspect the form of
the fix will be similar, but it may have to plumb through the file
addition from the diff layer, as well.

-Peff

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

[Merlin Büge]

On Wed, 27 May 2020 04:00:09 -0400
Jeff King <peff@peff.net> wrote:

> Trying your case with:
> 
>   git -c add.interactive.usebuiltin=true checkout -p master .
> 
> shows that it does not list the added file at all. I suspect the form of
> the fix will be similar, but it may have to plumb through the file
> addition from the diff layer, as well.

Thanks a lot for your explanation! Good to know what's going on there.

Out of curiosity: How does the git community keep track of open bugs?
On https://git-scm.com/ I read that bug reports should be just send to
the mailing list. But if they are not fixed within a few days, wouldn't
they just get lost...?

Thanks.

-- 
Merlin Büge

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

[Merlin Büge]

On Wed, 27 May 2020 06:48:28 +0200 (CEST), Johannes Schindelin wrote:

> There is not really any centralized way to keep track of open bugs.
> 
> And yes, sometimes they get lost after a few days.
> 
> In your case, however, there is now
> https://lore.kernel.org/git/pull.646.git.1590613746507.gitgitgadget@gmail.com/T/#u
> which is better than just a ticket: it is already a patch.

Wow, that was fast!

Thanks a lot!

-- 
Merlin Büge