Re: git add intent-to-add then git add patch no longer allows edit

"Raymond E. Pasco" [off-list ref] writes:

I fixed half of this in a topic that's on master now (it errors out
entirely if you try to stage it at all in 2.28.0), 

Yup, thanks for that one.

but new file diffs
still aren't splittable into hunks. Phillip Wood (on cc) is looking into
that; the tricky part is that when split into hunks only the first hunk
actually staged can be a "new file" patch.

Out of a change that adds a file with three parts A, B and C (in
this order), you could pick the parts A and C, while leaving the
change to further add B in the middle, and create a patch to add a
file that has A and C, and apply that to the index alone (i.e. "add
-p", pick A and C, and "add" that part by applying that "new file"
diff).  After that, the path is no longer i-t-a but has the real
contents (i.e. part A followed by part C), so further "add -p" would
see the difference between the index and the working tree as a
modification patch.

So as long as you could come up with a good UI to pick parts from a
single hunk "new file" diff, "the second and later application must
be done as modification" should fall out naturally, no?

On 21/08/2020 17:27, Junio C Hamano wrote:

"Raymond E. Pasco" [off-list ref] writes:

quoted

I fixed half of this in a topic that's on master now (it errors out
entirely if you try to stage it at all in 2.28.0),

Yup, thanks for that one.

quoted

but new file diffs
still aren't splittable into hunks. Phillip Wood (on cc) is looking into
that; the tricky part is that when split into hunks only the first hunk
actually staged can be a "new file" patch.

Out of a change that adds a file with three parts A, B and C (in
this order), you could pick the parts A and C, while leaving the
change to further add B in the middle, and create a patch to add a
file that has A and C, and apply that to the index alone (i.e. "add
-p", pick A and C, and "add" that part by applying that "new file"
diff).  After that, the path is no longer i-t-a but has the real
contents (i.e. part A followed by part C), so further "add -p" would
see the difference between the index and the working tree as a
modification patch.

So as long as you could come up with a good UI to pick parts from a
single hunk "new file" diff, "the second and later application must
be done as modification" should fall out naturally, no?

I think I was talking about edit rather than split. I'd forgotten that 
it used to work with i-t-a additions. I just checked seen and it seems 
to be working again since dscho's patch although the user is presented 
with the full diff header rather than just a hunk header in the editor. 
As you say once the user has staged some part of the diff the rest falls 
out naturally.

Best Wishes

Phillip

(I'm about to go off line for a while)

On Sun Aug 23, 2020 at 12:03 PM EDT, Phillip Wood wrote:

I think I was talking about edit rather than split. I'd forgotten that
it used to work with i-t-a additions. I just checked seen and it seems
to be working again since dscho's patch although the user is presented
with the full diff header rather than just a hunk header in the editor.
As you say once the user has staged some part of the diff the rest falls
out naturally.

Which topic is this? I can't find one where it works (it's always
"Sorry, cannot edit this hunk" on seen 2.28.0.508.g7d1bebc7fe).

Yeah, it's split that would be a problem, edit just stages and moves on.
Split would be nice, but I don't actually recall it ever working before
- it doesn't work now on diffs from actual blank files. Getting edit
back (if there's a topic that does this already) makes it work for my
usage.

Hi Raymond

On 24/08/2020 17:23, Raymond E. Pasco wrote:

On Sun Aug 23, 2020 at 12:03 PM EDT, Phillip Wood wrote:

quoted

I think I was talking about edit rather than split. I'd forgotten that
it used to work with i-t-a additions. I just checked seen and it seems
to be working again since dscho's patch although the user is presented
with the full diff header rather than just a hunk header in the editor.
As you say once the user has staged some part of the diff the rest falls
out naturally.

Which topic is this? I can't find one where it works (it's always
"Sorry, cannot edit this hunk" on seen 2.28.0.508.g7d1bebc7fe).

The patch I was referring to is 2c8bd8471a ("checkout -p: handle new 
files correctly", 2020-05-27)

I tested seen at 3981657b13 ("Merge branch 'rp/apply-cached-doc' into 
seen", 2020-08-21). I was using the C version of 'add -p' which is 
opt-in at the moment by setting add.interactive.usebuiltin=true in your 
config (or with git -c). I hope that helps, I'm going off line now for 
10-14 days

Best Wishes

Phillip

Yeah, it's split that would be a problem, edit just stages and moves on.
Split would be nice, but I don't actually recall it ever working before
- it doesn't work now on diffs from actual blank files. Getting edit
back (if there's a topic that does this already) makes it work for my
usage.

On Mon Aug 24, 2020 at 1:28 PM EDT, Phillip Wood wrote:

The patch I was referring to is 2c8bd8471a ("checkout -p: handle new
files correctly", 2020-05-27)

I tested seen at 3981657b13 ("Merge branch 'rp/apply-cached-doc' into
seen", 2020-08-21). I was using the C version of 'add -p' which is
opt-in at the moment by setting add.interactive.usebuiltin=true in your
config (or with git -c). I hope that helps, I'm going off line now for
10-14 days

Indeed, this works and restores my workflow (although it errors out if I
don't manually edit the range information, which isn't necessary with
diffs to existing files). It's a bit unsatisfying as it stands, but
perhaps there are patches I can write.

No need to reply, enjoy your vacation!

On 24/08/2020 22:03, Raymond E. Pasco wrote:

On Mon Aug 24, 2020 at 1:28 PM EDT, Phillip Wood wrote:

quoted

The patch I was referring to is 2c8bd8471a ("checkout -p: handle new
files correctly", 2020-05-27)

I tested seen at 3981657b13 ("Merge branch 'rp/apply-cached-doc' into
seen", 2020-08-21). I was using the C version of 'add -p' which is
opt-in at the moment by setting add.interactive.usebuiltin=true in your
config (or with git -c). I hope that helps, I'm going off line now for
10-14 days

Indeed, this works and restores my workflow (although it errors out if I
don't manually edit the range information, which isn't necessary with
diffs to existing files). It's a bit unsatisfying as it stands, but
perhaps there are patches I can write.

No need to reply, enjoy your vacation!

Thanks, it was really good to get a change of scene. The patch below
fixes the hunk editing for new files in the C version of `add -p` if
anyone wants to try it out. I haven't looked at fixing the perl
version yet - dscho what are your plans for switching over to the C
version?

Best Wishes

Phillip

---- >8 ----
From b0df1953308f8de5224a2d99d435f93cc4093a17 Mon Sep 17 00:00:00 2001
From: Phillip Wood <redacted>
Date: Wed, 2 Sep 2020 15:25:55 +0100
Subject: [PATCH] add -p: fix editing of intent-to-add paths

A popular way of partially staging a new file is to run `git add -N
<path>` and then use the hunk editing of `git add -p` to select the
part of the file that the user wishes to stage. Since
85953a3187 ("diff-files --raw: show correct post-image of
intent-to-add files", 2020-07-01) this has stopped working as
intent-to-add paths are now show as new files rather than changes to
an empty blob and `git apply` refused to apply a creation patch for a
path that was marked as intent-to-add. 7cfde3fa0f ("apply: allow "new
file" patches on i-t-a entries", 2020-08-06) fixed the problem with
apply but it still wasn't possible to edit the added hunk properly.

2c8bd8471a ("checkout -p: handle new files correctly", 2020-05-27)
had previously changed `add -p` to handle new files but it did not
implement patch editing correctly. The perl version simply forbade
editing and the C version opened the editor with the full diff rather
that just the hunk which meant that the user had to edit the hunk
header manually to get it to work.

This patch only fixes the C version to correctly edit new file
patches. To test the C version the tests must be run with
GIT_TEST_ADD_I_USE_BUILTIN=1. It is best viewed with 
--color-moved-ws=allow-indentation-change

Signed-off-by: Phillip Wood <redacted>
Reported-by: Thomas Sullivan <redacted>
---
 add-patch.c                | 83 +++++++++++++++++++++-----------------
 t/t3701-add-interactive.sh | 44 +++++++++++++++++++-
 2 files changed, 89 insertions(+), 38 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index f67b304a55..209a63e4f2 100644
--- a/add-patch.c
+++ b/add-patch.c

@@ -451,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, *added = NULL, *mode_change = NULL;
+		const char *mode_change = NULL;
 
 		if (!eol)
 			eol = pend;

@@ -470,12 +470,7 @@ 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 || 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, "new file", &added)))) {
+		else if (starts_with(p, "@@ ")) {
 			if (marker == '-' || marker == '+')
 				/*
 				 * Should not happen; previous hunk did not end

@@ -493,18 +488,20 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
 			if (colored)
 				hunk->colored_start = colored_p - colored->buf;
 
-			if (deleted)
-				file_diff->deleted = 1;
-			else if (added)
-				file_diff->added = 1;
-			else if (parse_hunk_header(s, hunk) < 0)
+			if (parse_hunk_header(s, hunk) < 0)
 				return -1;
 
 			/*
 			 * Start counting into how many hunks this one can be
 			 * split
 			 */
 			marker = *p;
+		} else if (hunk == &file_diff->head &&
+			   starts_with(p, "new file")) {
+			file_diff->added = 1;
+		} else if (hunk == &file_diff->head &&
+			   starts_with(p, "deleted file")) {
+			file_diff->deleted = 1;
 		} else if (hunk == &file_diff->head &&
 			   skip_prefix(p, "old mode ", &mode_change) &&
 			   is_octal(mode_change, eol - mode_change)) {

@@ -1358,39 +1355,46 @@ static int patch_update_file(struct add_p_state *s,
 	int colored = !!s->colored.len, quit = 0;
 	enum prompt_mode_type prompt_mode_type;
 
-	if (!file_diff->hunk_nr)
+	/* Empty added and deleted files have no hunks */
+	if (!file_diff->hunk_nr && !file_diff->added && !file_diff->deleted)
 		return 0;
 
 	strbuf_reset(&s->buf);
 	render_diff_header(s, file_diff, colored, &s->buf);
 	fputs(s->buf.buf, stdout);
 	for (;;) {
-		if (hunk_index >= file_diff->hunk_nr)
-			hunk_index = 0;
-		hunk = file_diff->hunk + hunk_index;
+		if (file_diff->hunk_nr) {
+			if (hunk_index >= file_diff->hunk_nr)
+				hunk_index = 0;
+			hunk = file_diff->hunk + hunk_index;
 
-		undecided_previous = -1;
-		for (i = hunk_index - 1; i >= 0; i--)
-			if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
-				undecided_previous = i;
-				break;
-			}
+			undecided_previous = -1;
+			for (i = hunk_index - 1; i >= 0; i--)
+				if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
+					undecided_previous = i;
+					break;
+				}
 
-		undecided_next = -1;
-		for (i = hunk_index + 1; i < file_diff->hunk_nr; i++)
-			if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
-				undecided_next = i;
-				break;
-			}
+			undecided_next = -1;
+			for (i = hunk_index + 1; i < file_diff->hunk_nr; i++)
+				if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
+					undecided_next = i;
+					break;
+				}
 
-		/* Everything decided? */
-		if (undecided_previous < 0 && undecided_next < 0 &&
-		    hunk->use != UNDECIDED_HUNK)
-			break;
+			/* Everything decided? */
+			if (undecided_previous < 0 && undecided_next < 0 &&
+			    hunk->use != UNDECIDED_HUNK)
+				break;
 
-		strbuf_reset(&s->buf);
-		render_hunk(s, hunk, 0, colored, &s->buf);
-		fputs(s->buf.buf, stdout);
+			strbuf_reset(&s->buf);
+			render_hunk(s, hunk, 0, colored, &s->buf);
+			fputs(s->buf.buf, stdout);
+		} else {
+			hunk = &file_diff->head;
+			undecided_next = -1;
+			undecided_previous = -1;
+		}
 
 		strbuf_reset(&s->buf);
 		if (undecided_previous >= 0)

@@ -1421,7 +1425,9 @@ static int patch_update_file(struct add_p_state *s,
 		color_fprintf(stdout, s->s.prompt_color,
 			      "(%"PRIuMAX"/%"PRIuMAX") ",
 			      (uintmax_t)hunk_index + 1,
-			      (uintmax_t)file_diff->hunk_nr);
+			      (uintmax_t)(file_diff->hunk_nr
+						? file_diff->hunk_nr
+						: 1));
 		color_fprintf(stdout, s->s.prompt_color,
 			      _(s->mode->prompt_mode[prompt_mode_type]),
 			      s->buf.buf);

@@ -1601,14 +1607,17 @@ static int patch_update_file(struct add_p_state *s,
 						 "%.*s", (int)(eol - p), p);
 			}
 		}
+		if (!file_diff->hunk_nr)
+			break;
 	}
 
 	/* Any hunk to be used? */
 	for (i = 0; i < file_diff->hunk_nr; i++)
 		if (file_diff->hunk[i].use == USE_HUNK)
 			break;
 
-	if (i < file_diff->hunk_nr) {
+	if (i < file_diff->hunk_nr ||
+	    (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
 		/* At least one hunk selected: apply */
 		strbuf_reset(&s->buf);
 		reassemble_patch(s, file_diff, 0, &s->buf);

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index fb73a847cb..49d597979a 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh

@@ -47,7 +47,11 @@ test_expect_success 'setup (initial)' '
 	echo content >file &&
 	git add file &&
 	echo more >>file &&
-	echo lines >>file
+	echo lines >>file &&
+	if test -n "$GIT_TEST_ADD_I_USE_BUILTIN"
+	then
+		test_set_prereq BUILTIN_ADD_I
+	fi
 '
 test_expect_success 'status works (initial)' '
 	git add -i </dev/null >output &&

@@ -814,6 +818,44 @@ test_expect_success 'checkout -p works with pathological context lines' '
 	test_cmp expect a
 '
 
+# This should be called from a subshell as it sets a temporary editor
+setup_new_file() {
+	write_script new-file-editor.sh <<-\EOF &&
+	sed /^#/d "$1" >patch &&
+	sed /^+c/d patch >"$1"
+	EOF
+	test_set_editor "$(pwd)/new-file-editor.sh" &&
+	test_write_lines a b c d e f >new-file &&
+	test_write_lines a b d e f >new-file-expect &&
+	test_write_lines "@@ -0,0 +1,6 @@" +a +b +c +d +e +f >patch-expect
+}
+
+test_expect_success BUILTIN_ADD_I 'add -N followed by add -p patch editing' '
+	git reset --hard &&
+	(
+		setup_new_file &&
+		git add -N new-file &&
+		test_write_lines e n q | git add -p &&
+		git cat-file blob :new-file >actual &&
+		test_cmp new-file-expect actual &&
+		test_cmp patch-expect patch
+	)
+'
+
+test_expect_success BUILTIN_ADD_I 'checkout -p patch editing of added file' '
+	git reset --hard &&
+	(
+		setup_new_file &&
+		git add new-file &&
+		git commit -m "add new file" &&
+		git rm new-file &&
+		git commit -m "remove new file" &&
+		test_write_lines e n q | git checkout -p HEAD^ &&
+		test_cmp new-file-expect new-file &&
+		test_cmp patch-expect patch
+	)
+'
+
 test_expect_success 'show help from add--helper' '
 	git reset --hard &&
 	cat >expect <<-EOF &&

-- 
2.25.1.551.gd3318bf0d3.dirty

Hi Phillip,

On Fri, 4 Sep 2020, Phillip Wood wrote:

[...] I haven't looked at fixing the perl version yet - dscho what are
your plans for switching over to the C version?

Thanks for reminding me, I did not really think about it anymore. The
built-in `git add -i`/`git add -p` has been available since v2.25.0. Since
v2.26.0, we also respect that flag in the `-p` modes of `checkout`,
`stash`, etc

And from the way at least _I_ read the commit log, it seems that the code
has been pretty stable (except for that bug fix where `e` was allowed by
mistake).

The next step will be to invert the default (which is `false` right now)
for `add.interactive.useBuiltin`, I guess.

Ciao,
Dscho

On Fri Sep 4, 2020 at 6:05 AM EDT, Phillip Wood wrote:

Thanks, it was really good to get a change of scene. The patch below
fixes the hunk editing for new files in the C version of `add -p` if
anyone wants to try it out. I haven't looked at fixing the perl
version yet - dscho what are your plans for switching over to the C
version?

No issues with this patch thus far - I'll continue running with it until
it gets picked up. Thanks!