Skip to contentSkip to conversation

checkout -p: handle new files correctly

PATCH In review Johannes Schindelin via GitGitGadget 4 participants 5 messages May 27, 2020 – May 28, 2020

lore
Share & tools
lore

Johannes Schindelin via GitGitGadget

author
#0 lore raw

PATCH 1/1 checkout -p: handle new files correctly

From: Johannes Schindelin <[email protected]>

The original patch selection code was written for git add -p, and the fundamental unit on which it works is a hunk.

We hacked around that to handle deletions back in 24ab81ae4d (add-interactive: handle deletion of empty files, 2009-10-27). But git add -p would never see a new file, since we only consider the set of tracked files in the index.

However, since the same machinery was used for git checkout -p & friends, we can see new files.

Handle this case specifically, adding a new prompt for it that is modeled after the deleted file case.

This also fixes the problem where added _empty_ files could not be staged via git checkout -p.

Reported-by Merlin Büge <[email protected]>
Helped-by Jeff King <[email protected]>
Signed-off-by Johannes Schindelin <[email protected]>

---
Allow empty, new files to be staged via git checkout -p

For ease of backporting, this patch is based on js/add-p-leftover-bits.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-646%2Fdscho%2Fcheckout-p-empty-file-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-646/dscho/checkout-p-empty-file-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/646

 add-patch.c                | 30 +++++++++++++++++++++++-------
 git-add--interactive.perl  | 21 +++++++++++++++++++--
 t/t3701-add-interactive.sh | 19 +++++++++++++++++++
 3 files changed, 61 insertions(+), 9 deletions(-)
add-patch.c +23 -7
index d8dafa8168d..eaace1b824f 100644
@@ -9,7 +9,7 @@
 #include "compat/terminal.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 */
 };
 

@@ -32,6 +32,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 "

@@ -53,6 +54,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 "

@@ -76,6 +78,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 "

@@ -98,6 +101,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 "

@@ -120,6 +124,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 "

@@ -142,6 +147,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 "

@@ -163,6 +169,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 "

@@ -185,6 +192,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 "

@@ -206,6 +214,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 "

@@ -247,7 +256,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;
 

@@ -441,7 +450,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;

@@ -460,11 +469,12 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
 		} else if (p == plain->buf)
 			BUG("diff starts with unexpected line:\n"
 			    "%.*s\n", (int)(eol - p), p);
-		else if (file_diff->deleted)
+		else if (file_diff->deleted || file_diff->added)
 			; /* 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

@@ -484,6 +494,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;
 

@@ -536,8 +548,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);
 

@@ -1397,6 +1411,8 @@ static int patch_update_file(struct add_p_state *s,
 
 		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
git-add--interactive.perl +19 -2
index 10fd30ae16a..f36c0078ac9 100755
@@ -754,16 +754,18 @@ sub parse_diff_header {
 	my $head = { TEXT => [], DISPLAY => [], TYPE => 'header' };
 	my $mode = { TEXT => [], DISPLAY => [], TYPE => 'mode' };
 	my $deletion = { TEXT => [], DISPLAY => [], TYPE => 'deletion' };
+	my $addition = { TEXT => [], DISPLAY => [], TYPE => 'addition' };
 
 	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] =~ /^new file/ ? $addition :
 		   $head;
 		push @{$dest->{TEXT}}, $src->{TEXT}->[$i];
 		push @{$dest->{DISPLAY}}, $src->{DISPLAY}->[$i];
 	}
-	return ($head, $mode, $deletion);
+	return ($head, $mode, $deletion, $addition);
 }
 
 sub hunk_splittable {

@@ -1427,46 +1429,55 @@ sub display_hunks {
 	stage => {
 		mode => N__("Stage mode change [y,n,q,a,d%s,?]? "),
 		deletion => N__("Stage deletion [y,n,q,a,d%s,?]? "),
+		addition => N__("Stage addition [y,n,q,a,d%s,?]? "),
 		hunk => N__("Stage this hunk [y,n,q,a,d%s,?]? "),
 	},
 	stash => {
 		mode => N__("Stash mode change [y,n,q,a,d%s,?]? "),
 		deletion => N__("Stash deletion [y,n,q,a,d%s,?]? "),
+		addition => N__("Stash addition [y,n,q,a,d%s,?]? "),
 		hunk => N__("Stash this hunk [y,n,q,a,d%s,?]? "),
 	},
 	reset_head => {
 		mode => N__("Unstage mode change [y,n,q,a,d%s,?]? "),
 		deletion => N__("Unstage deletion [y,n,q,a,d%s,?]? "),
+		addition => N__("Unstage addition [y,n,q,a,d%s,?]? "),
 		hunk => N__("Unstage this hunk [y,n,q,a,d%s,?]? "),
 	},
 	reset_nothead => {
 		mode => N__("Apply mode change to index [y,n,q,a,d%s,?]? "),
 		deletion => N__("Apply deletion to index [y,n,q,a,d%s,?]? "),
+		addition => N__("Apply addition to index [y,n,q,a,d%s,?]? "),
 		hunk => N__("Apply this hunk to index [y,n,q,a,d%s,?]? "),
 	},
 	checkout_index => {
 		mode => N__("Discard mode change from worktree [y,n,q,a,d%s,?]? "),
 		deletion => N__("Discard deletion from worktree [y,n,q,a,d%s,?]? "),
+		addition => N__("Discard addition from worktree [y,n,q,a,d%s,?]? "),
 		hunk => N__("Discard this hunk from worktree [y,n,q,a,d%s,?]? "),
 	},
 	checkout_head => {
 		mode => N__("Discard mode change from index and worktree [y,n,q,a,d%s,?]? "),
 		deletion => N__("Discard deletion from index and worktree [y,n,q,a,d%s,?]? "),
+		addition => N__("Discard addition from index and worktree [y,n,q,a,d%s,?]? "),
 		hunk => N__("Discard this hunk from index and worktree [y,n,q,a,d%s,?]? "),
 	},
 	checkout_nothead => {
 		mode => N__("Apply mode change to index and worktree [y,n,q,a,d%s,?]? "),
 		deletion => N__("Apply deletion to index and worktree [y,n,q,a,d%s,?]? "),
+		addition => N__("Apply addition to index and worktree [y,n,q,a,d%s,?]? "),
 		hunk => N__("Apply this hunk to index and worktree [y,n,q,a,d%s,?]? "),
 	},
 	worktree_head => {
 		mode => N__("Discard mode change from worktree [y,n,q,a,d%s,?]? "),
 		deletion => N__("Discard deletion from worktree [y,n,q,a,d%s,?]? "),
+		addition => N__("Discard addition from worktree [y,n,q,a,d%s,?]? "),
 		hunk => N__("Discard this hunk from worktree [y,n,q,a,d%s,?]? "),
 	},
 	worktree_nothead => {
 		mode => N__("Apply mode change to worktree [y,n,q,a,d%s,?]? "),
 		deletion => N__("Apply deletion to worktree [y,n,q,a,d%s,?]? "),
+		addition => N__("Apply addition to worktree [y,n,q,a,d%s,?]? "),
 		hunk => N__("Apply this hunk to worktree [y,n,q,a,d%s,?]? "),
 	},
 );

@@ -1476,7 +1487,7 @@ sub patch_update_file {
 	my ($ix, $num);
 	my $path = shift;
 	my ($head, @hunk) = parse_diff($path);
-	($head, my $mode, my $deletion) = parse_diff_header($head);
+	($head, my $mode, my $deletion, my $addition) = parse_diff_header($head);
 	for (@{$head->{DISPLAY}}) {
 		print;
 	}

@@ -1490,6 +1501,12 @@ sub patch_update_file {
 			push @{$deletion->{DISPLAY}}, @{$hunk->{DISPLAY}};
 		}
 		@hunk = ($deletion);
+	} elsif (@{$addition->{TEXT}}) {
+		foreach my $hunk (@hunk) {
+			push @{$addition->{TEXT}}, @{$hunk->{TEXT}};
+			push @{$addition->{DISPLAY}}, @{$hunk->{DISPLAY}};
+		}
+		@hunk = ($addition);
 	}
 
 	$num = scalar @hunk;
t/t3701-add-interactive.sh +19 -0
index ac43f835a5b..7bc0c3fe6ef 100755
@@ -403,6 +403,25 @@ test_expect_success 'deleting an empty file' '
 	diff_cmp expected diff
 '
 
+test_expect_success 'adding an empty file' '
+	git init added &&
+	(
+		cd added &&
+		test_commit initial &&
+		>empty &&
+		git add empty &&
+		test_tick &&
+		git commit -m empty &&
+		git tag added-file &&
+		git reset --hard HEAD^ &&
+		test_path_is_missing empty &&
+
+		echo y | git checkout -p added-file -- >actual &&
+		test_path_is_file empty &&
+		test_i18ngrep "Apply addition to index and worktree" actual
+	)
+'
+
 test_expect_success 'split hunk setup' '
 	git reset --hard &&
 	test_write_lines 10 20 30 40 50 60 >test &&

base-commit: b2627cc3d4be2f8086711097f99d79a32c6a703a

Signature · 1 line
gitgitgadget
#1 lore raw

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 2 lines · writes:

This also fixes the problem where added _empty_ files could not be staged via git checkout -p.

Nice. Will queue.

Quoted 1 line

For ease of backporting, this patch is based on js/add-p-leftover-bits.

Yup, I agree that it would be a sensible fork point.

#2 lore raw
In reply to Johannes Schindelin via GitGitGadget
Quoted 5 lines · Johannes Schindelin via GitGitGadget

However, since the same machinery was used for git checkout -p & friends, we can see new files.

Handle this case specifically, adding a new prompt for it that is modeled after the deleted file case.

Thanks! I was planning to dig further into this topic today, and here it is all wrapped up with a bow. :)

Quoted 2 lines
 add-patch.c                | 30 +++++++++++++++++++++++-------
 git-add--interactive.perl  | 21 +++++++++++++++++++--

Ooh, you even fixed the perl version, too. I was just going to leave it in the dust and add a test that set GIT_TEST_ADD_I_USE_BUILTIN.

Both versions look good, and are similar to what I expected from looking at it last night.

Quoted 7 lines

The original patch selection code was written for git add -p, and the fundamental unit on which it works is a hunk.

We hacked around that to handle deletions back in 24ab81ae4d (add-interactive: handle deletion of empty files, 2009-10-27). But git add -p would never see a new file, since we only consider the set of tracked files in the index.

I lied a little with "would never see a new file". There _is_ a related case with "add -p" that might be worth thinking about: intent-to-add files.

  $ git init
  $ >empty
  $ echo content >not-empty
  $ git add -N .
  $ git add -p
  diff --git a/not-empty b/not-empty
  index e69de29..d95f3ad 100644
  --- a/not-empty
  +++ b/not-empty
  @@ -0,0 +1 @@
  +content
  (1/1) Stage this hunk [y,n,q,a,d,e,?]? n

[no mention of empty file!]

I think the culprit here is diff-files, though, which doesn't show a patch for intent-to-add:

  $ git diff-files
  :100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M	empty
  :100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M	not-empty
  $ git diff-files -p
  diff --git a/not-empty b/not-empty
  index e69de29..d95f3ad 100644
  --- a/not-empty
  +++ b/not-empty
  @@ -0,0 +1 @@
  +content

I don't think this really intersects with the patch here at all, because diff-files is not producing "new file" lines for these entries (even for the non-empty one).

The solution _might_ be to convince diff-files to treat i-t-a entries as creations. And then with your patch here, we'd naturally do the right thing. So I don't think this needs to hold up your patch in any way, nor do we necessarily need to deal with i-t-a now. I was mostly curious how they worked, since we don't support added files. The answer is just that they don't always. ;)

-Peff

#3 lore raw

Hi Peff,

Quoted 10 lines · Jeff King
Quoted 5 lines · Johannes Schindelin via GitGitGadget

However, since the same machinery was used for git checkout -p & friends, we can see new files.

Handle this case specifically, adding a new prompt for it that is modeled after the deleted file case.

Thanks! I was planning to dig further into this topic today, and here it is all wrapped up with a bow. :)

:-)

Quoted 6 lines
Quoted 3 lines
 add-patch.c                | 30 +++++++++++++++++++++++-------
 git-add--interactive.perl  | 21 +++++++++++++++++++--

Ooh, you even fixed the perl version, too. I was just going to leave it in the dust and add a test that set GIT_TEST_ADD_I_USE_BUILTIN.

As long as there is an escape hatch, I try to keep it working.

Quoted 2 lines

Both versions look good, and are similar to what I expected from looking at it last night.

Thank you!

Quoted 11 lines
Quoted 7 lines

The original patch selection code was written for git add -p, and the fundamental unit on which it works is a hunk.

We hacked around that to handle deletions back in 24ab81ae4d (add-interactive: handle deletion of empty files, 2009-10-27). But git add -p would never see a new file, since we only consider the set of tracked files in the index.

I lied a little with "would never see a new file". There _is_ a related case with "add -p" that might be worth thinking about: intent-to-add files.

Indeed. Maybe I can leave that as #leftoverbits?

Ciao,
Dscho

Quoted 44 lines
  $ git init
  $ >empty
  $ echo content >not-empty
  $ git add -N .
  $ git add -p
  diff --git a/not-empty b/not-empty
  index e69de29..d95f3ad 100644
  --- a/not-empty
  +++ b/not-empty
  @@ -0,0 +1 @@
  +content
  (1/1) Stage this hunk [y,n,q,a,d,e,?]? n

[no mention of empty file!]

I think the culprit here is diff-files, though, which doesn't show a patch for intent-to-add:

  $ git diff-files
  :100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M	empty
  :100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M	not-empty
  $ git diff-files -p
  diff --git a/not-empty b/not-empty
  index e69de29..d95f3ad 100644
  --- a/not-empty
  +++ b/not-empty
  @@ -0,0 +1 @@
  +content

I don't think this really intersects with the patch here at all, because diff-files is not producing "new file" lines for these entries (even for the non-empty one).

The solution _might_ be to convince diff-files to treat i-t-a entries as creations. And then with your patch here, we'd naturally do the right thing. So I don't think this needs to hold up your patch in any way, nor do we necessarily need to deal with i-t-a now. I was mostly curious how they worked, since we don't support added files. The answer is just that they don't always. ;)

-Peff

#4 lore raw

Hi,

Quoted 7 lines · Johannes Schindelin
Quoted 3 lines · Jeff King

I lied a little with "would never see a new file". There _is_ a related case with "add -p" that might be worth thinking about:
intent-to-add files.

Indeed. Maybe I can leave that as #leftoverbits?

Added as https://github.com/gitgitgadget/git/issues/647

Ciao,
Dscho