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

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

[Thomas Sullivan]

What did you do before the bug happened? (Steps to reproduce your issue)

    rm -rf .git
    cat << EOF > file
    Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do
eiusmod tempor incididunt ut labore et dolore magna aliqua.
    EOF
    git init .
    git add --intent-to-add file
    git add --patch file

What did you expect to happen? (Expected behavior)

To be able to edit the single hunk (`e` option)

What happened instead? (Actual behavior)

The `e` option isn't available (and providing it anyway errors with
`Sorry, cannot edit this hunk`)

Anything else you want to add:

Output of version 2.28.0:

    diff --git a/file b/file
    index 0000000..85e73d4
    --- /dev/null
    +++ b/file
    new file mode 100644
    @@ -0,0 +1 @@
    +Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do
eiusmod tempor incididunt ut labore et dolore magna aliqua.
    (1/1) Stage addition [y,n,q,a,d,?]?

Output of version 2.27.0:

    diff --git a/file b/file
    index e69de29..85e73d4 100644
    --- a/file
    +++ b/file
    @@ -0,0 +1 @@
    +Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do
eiusmod tempor incididunt ut labore et dolore magna aliqua.
    (1/1) Stage this hunk [y,n,q,a,d,e,?]?

Bisecting between the two tags reports this commit as the one
introducing the change:

    [feea6946a5b746ff4ebf8ccdf959e303203a6011] diff-files: treat
"i-t-a" files as "not-in-index"

[System Info]
git version:
git version 2.28.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Darwin 17.7.0 Darwin Kernel Version 17.7.0: Thu Jun 18 21:21:34
PDT 2020; root:xnu-4570.71.82.5~1/RELEASE_X86_64 x86_64
compiler info: clang: 10.0.0 (clang-1000.10.44.4)
libc info: no libc information available
$SHELL (typically, interactive shell): /bin/bash


[Enabled Hooks]

Regards,

Tom Sullivan

Head Developer
Most Significant Bit Software

e:  tom@msbit.com.au
p:  +61 407 890 821

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

[Raymond E. Pasco]

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

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

[Junio C Hamano]

"Raymond E. Pasco" <ray@ameretat.dev> 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?

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

[Phillip Wood]

On 21/08/2020 17:27, Junio C Hamano wrote:
> "Raymond E. Pasco" <ray@ameretat.dev> 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?

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)

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

[Raymond E. Pasco]

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.

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

[Phillip Wood]

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

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

[Raymond E. Pasco]

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!

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

[Johannes Schindelin]

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

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

[Phillip Wood]

On 24/08/2020 22:03, Raymond E. Pasco wrote:
> 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!

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 <phillip.wood@dunelm.org.uk>
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 <phillip.wood@dunelm.org.uk>
Reported-by: Thomas Sullivan <tom@msbit.com.au>
---
 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

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

[Raymond E. Pasco]

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!

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 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

That is 9a5315ed (Merge branch 'js/patch-mode-in-others-in-c', 2020-02-05)

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

As long as it has been widely used, that is.  I do not think we
deeply mind a bug like the `e` one that does not affect the utility
or the correctness of the command that much.  If we do not flip the
"use the built-in variant" for those with feature.experimental we
really should do so to widen the canarying population immediately.

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

[Phillip Wood]

On 06/09/2020 18:10, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>> 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
> 
> That is 9a5315ed (Merge branch 'js/patch-mode-in-others-in-c', 2020-02-05)
> 
>> 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).
> 
> As long as it has been widely used, that is. 

Exactly, I'm not sure it has been that widely used yet. (I'm
interested in this area and only got round to using the C version a
couple of weeks ago so I wonder how many others have)

> I do not think we
> deeply mind a bug like the `e` one that does not affect the utility
> or the correctness of the command that much.

The bug with editing in this thread is also fairly minor I think. I
suspect the main area for serious bugs is hunk splitting and
joining.

>  If we do not flip the
> "use the built-in variant" for those with feature.experimental we
> really should do so to widen the canarying population immediately.

That's a good idea

Best Wishes

Phillip

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>>  If we do not flip the
>> "use the built-in variant" for those with feature.experimental we
>> really should do so to widen the canarying population immediately.
>
> That's a good idea

Like this?  If the more specific one is specifically set, we do not
look at experimental bit, but otherwise we use the built-in version.

 builtin/add.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index b36a99eb7c..26b6ced09e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -192,9 +192,15 @@ int run_add_interactive(const char *revision, const char *patch_mode,
 	int use_builtin_add_i =
 		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
 
-	if (use_builtin_add_i < 0)
-		git_config_get_bool("add.interactive.usebuiltin",
-				    &use_builtin_add_i);
+	if (use_builtin_add_i < 0) {
+		int experimental;
+		if (!git_config_get_bool("add.interactive.usebuiltin",
+					 &use_builtin_add_i))
+			; /* ok */
+		else if (!git_config_get_bool("feature.experimental", &experimental) &&
+			 experimental)
+			use_builtin_add_i = 1;
+	}
 
 	if (use_builtin_add_i == 1) {
 		enum add_p_mode mode;

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

[Johannes Schindelin]

Hi Junio,

On Tue, 8 Sep 2020, Junio C Hamano wrote:

> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> >>  If we do not flip the
> >> "use the built-in variant" for those with feature.experimental we
> >> really should do so to widen the canarying population immediately.
> >
> > That's a good idea
>
> Like this?  If the more specific one is specifically set, we do not
> look at experimental bit, but otherwise we use the built-in version.

Looks fine to me,
Dscho

>
>  builtin/add.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index b36a99eb7c..26b6ced09e 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -192,9 +192,15 @@ int run_add_interactive(const char *revision, const char *patch_mode,
>  	int use_builtin_add_i =
>  		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
>
> -	if (use_builtin_add_i < 0)
> -		git_config_get_bool("add.interactive.usebuiltin",
> -				    &use_builtin_add_i);
> +	if (use_builtin_add_i < 0) {
> +		int experimental;
> +		if (!git_config_get_bool("add.interactive.usebuiltin",
> +					 &use_builtin_add_i))
> +			; /* ok */
> +		else if (!git_config_get_bool("feature.experimental", &experimental) &&
> +			 experimental)
> +			use_builtin_add_i = 1;
> +	}
>
>  	if (use_builtin_add_i == 1) {
>  		enum add_p_mode mode;
>

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Like this?  If the more specific one is specifically set, we do not
>> look at experimental bit, but otherwise we use the built-in version.
>
> Looks fine to me,
> Dscho

Thanks, with the proposed log message this time...

-- >8 --
Subject: [PATCH] add -i: use the built-in version when feature.experimental is set

We have had parallel implementations of "add -i/-p" since 2.25 and
have been using them from various codepaths since 2.26 days, but
never made the built-in version the default.

We have found and fixed a handful of corner case bugs in the
built-in version, and it may be a good time to start switching over
the user base from the scripted version to the built-in version.
Let's enable the built-in version for those who opt into the
feature.experimental guinea-pig program to give wider exposure.

Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/add.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index b36a99eb7c..26b6ced09e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -192,9 +192,15 @@ int run_add_interactive(const char *revision, const char *patch_mode,
 	int use_builtin_add_i =
 		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
 
-	if (use_builtin_add_i < 0)
-		git_config_get_bool("add.interactive.usebuiltin",
-				    &use_builtin_add_i);
+	if (use_builtin_add_i < 0) {
+		int experimental;
+		if (!git_config_get_bool("add.interactive.usebuiltin",
+					 &use_builtin_add_i))
+			; /* ok */
+		else if (!git_config_get_bool("feature.experimental", &experimental) &&
+			 experimental)
+			use_builtin_add_i = 1;
+	}
 
 	if (use_builtin_add_i == 1) {
 		enum add_p_mode mode;
-- 
2.28.0-539-g66371d8995

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

[Phillip Wood]

On 08/09/2020 23:00, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>>> Like this?  If the more specific one is specifically set, we do not
>>> look at experimental bit, but otherwise we use the built-in version.
>>
>> Looks fine to me,
>> Dscho
> 
> Thanks, with the proposed log message this time...
> 
> -- >8 --
> Subject: [PATCH] add -i: use the built-in version when feature.experimental is set
> 
> We have had parallel implementations of "add -i/-p" since 2.25 and
> have been using them from various codepaths since 2.26 days, but
> never made the built-in version the default.
> 
> We have found and fixed a handful of corner case bugs in the
> built-in version, and it may be a good time to start switching over
> the user base from the scripted version to the built-in version.
> Let's enable the built-in version for those who opt into the
> feature.experimental guinea-pig program to give wider exposure.
> 
> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/add.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/add.c b/builtin/add.c
> index b36a99eb7c..26b6ced09e 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -192,9 +192,15 @@ int run_add_interactive(const char *revision, const char *patch_mode,
>  	int use_builtin_add_i =
>  		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
>  
> -	if (use_builtin_add_i < 0)
> -		git_config_get_bool("add.interactive.usebuiltin",
> -				    &use_builtin_add_i);
> +	if (use_builtin_add_i < 0) {
> +		int experimental;
> +		if (!git_config_get_bool("add.interactive.usebuiltin",
> +					 &use_builtin_add_i))
> +			; /* ok */
> +		else if (!git_config_get_bool("feature.experimental", &experimental) &&
> +			 experimental)
> +			use_builtin_add_i = 1;
> +	}
>  
>  	if (use_builtin_add_i == 1) {
>  		enum add_p_mode mode;
> 

Looks good to me as well, Thanks

Phillip