Re* [PATCH v2] apply: make --intent-to-add not stomp index

Junio C Hamano [off-list ref] writes:

Can you study the code to decide if check_apply_state() is the right
place to do this instead?  I have this feeling that the following
bit in the function

	if (state->ita_only && (state->check_index || is_not_gitdir))
		state->ita_only = 0;

is simply _wrong_ to silently drop the ita_only bit when not in a
repository, or other index-touching options are in effect.  Rather,
I wonder if it should look more like the attached (the other parts
of the implementation of ita_only may be depending on the buggy
construct, which might result in other breakages if we did this
alone, though).

All the existing tests and your new test seem to pass with the "-N
should imply --index" fix.  It could merely be an indication that
our test coverage is horrible, but I _think_ the intent of "-N" is
to behave like "--index" does, but handle creation part slightly
differently.

Of course there is another possible interpretation for "-N", which
is to behave unlike "--index" and touch _only_ the working tree
files, but creations are recorded as if "git add -N" were run for
new paths after such a "working tree only" application was done.

I cannot tell if that is what you wanted to implement; the new test
in your patch seems to pass with the first interpretation.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] apply: --intent-to-add should imply --index

Otherwise we do not read the current index, and more importantly, we
do not check with the current index, losing all the safety.

And the worst part of the story is that we still write the result
out to the index, which loses all the files that are not mentioned
in the incoming patch.

Reported-by: Ryan Hodges <redacted>
Test-by: Johannes Altmanninger [off-list ref]
Signed-off-by: Junio C Hamano <redacted>
---
 apply.c               | 4 ++--
 t/t2203-add-intent.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/apply.c b/apply.c
index 43a0aebf4e..887465347b 100644
--- a/apply.c
+++ b/apply.c

@@ -146,6 +146,8 @@ int check_apply_state(struct apply_state *state, int force_apply)
 	}
 	if (!force_apply && (state->diffstat || state->numstat || state->summary || state->check || state->fake_ancestor))
 		state->apply = 0;
+	if (state->ita_only)
+		state->check_index = 1;
 	if (state->check_index && is_not_gitdir)
 		return error(_("--index outside a repository"));
 	if (state->cached) {

@@ -153,8 +155,6 @@ int check_apply_state(struct apply_state *state, int force_apply)
 			return error(_("--cached outside a repository"));
 		state->check_index = 1;
 	}
-	if (state->ita_only && (state->check_index || is_not_gitdir))
-		state->ita_only = 0;
 	if (state->check_index)
 		state->unsafe_paths = 0;
 

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index cf0175ad6e..035ce3a2b9 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh

@@ -307,7 +307,7 @@ test_expect_success 'apply --intent-to-add' '
 	grep "new file" expected &&
 	git reset --hard &&
 	git apply --intent-to-add expected &&
-	git diff >actual &&
+	(git diff && git diff --cached) >actual &&
 	test_cmp expected actual
 '
 

-- 
2.34.0-rc0-136-gecf67dd964

On Mon, Nov 01, 2021 at 12:07:28AM -0700, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

Can you study the code to decide if check_apply_state() is the right
place to do this instead?  I have this feeling that the following
bit in the function

	if (state->ita_only && (state->check_index || is_not_gitdir))
		state->ita_only = 0;

is simply _wrong_ to silently drop the ita_only bit when not in a
repository, or other index-touching options are in effect.  Rather,
I wonder if it should look more like the attached (the other parts
of the implementation of ita_only may be depending on the buggy
construct, which might result in other breakages if we did this
alone, though).

All the existing tests and your new test seem to pass with the "-N
should imply --index" fix.  It could merely be an indication that
our test coverage is horrible, but I _think_ the intent of "-N" is
to behave like "--index" does, but handle creation part slightly
differently.

Of course there is another possible interpretation for "-N", which
is to behave unlike "--index" and touch _only_ the working tree
files, but creations are recorded as if "git add -N" were run for
new paths after such a "working tree only" application was done.

I cannot tell if that is what you wanted to implement; the new test
in your patch seems to pass with the first interpretation.

I'm still not entirely sure, but the ita-implies-check_index seems simpler
overall, which is a good sign.
It will prevent "apply -N" from modifying untracked files, which seems like
a good safety measure.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] apply: --intent-to-add should imply --index

Otherwise we do not read the current index, and more importantly, we
do not check with the current index, losing all the safety.

(The i-t-a bit should only trigger for added files, so a correct implementation
would preserve the index for all other entries.)

As you suspected earlier, adding "ita_only implies check_index" alone will
break the test case below, because other places assume
"ita_only implies none of --cached/--index/--threeway was given"

test_expect_success 'apply --index --intent-to-add ignores --intent-to-add, so it does not set i-t-a bit of touched file' '
	echo >file &&
	git add file &&
	git apply --index --intent-to-add <<-EOF &&
	diff --git a/file b/file
	deleted file mode 100644
	index f00c965..7e91ed5 100644
	--- a/file
	+++ /dev/null
	@@ -1 +0,0 @@
	-
	EOF
	git ls-files file >actual &&
	test_must_be_empty actual
'

A fix would be to say
"ita_only implies check_index, except if one of its older siblings is present"

	if (state->check_index)
		state->ita_only = 0;
	if (state->ita_only)
		state->check_index = 1;

This matches the documentation of git-apply, and puts ita_only in its place
as early as possible.

Otherwise we do not read the current index, and more importantly, we
do not check with the current index, losing all the safety.

And the worst part of the story is that we still write the result
out to the index, which loses all the files that are not mentioned
in the incoming patch.

Make --intent-to-add imply --index. This means that apply
--intent-to-add will error without a repo, and refuse to modify
untracked files (except for added files). Add a test for the latter, and
another one to make sure that combinations like "--cached -N" keep working.
as documented (-N is ignored, otherwise it would do weird things to the index).

Use --intent-to-add instead of -N because we don't document -N in
git-apply.txt, which might be because it's much more obscure than "add -N".

Reported-by: Ryan Hodges <redacted>
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Johannes Altmanninger <redacted>
---

Not sure about the log message, it feels a bit stitched together.

Most importantly this adds a test to show the difference between v2 (where
ita did not imply check_index).

I wrapped the doc changes to 80 columns, but not the entire paragraph,
since we are inconsistent about that.

 Documentation/git-apply.txt |  7 +++----
 apply.c                     |  9 +++++++--
 t/t2203-add-intent.sh       |  2 +-
 t/t4140-apply-ita.sh        | 29 +++++++++++++++++++++++++++++
 4 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index aa1ae56a25..18ddb4cf8a 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt

@@ -77,10 +77,9 @@ OPTIONS
 --intent-to-add::
 	When applying the patch only to the working tree, mark new
 	files to be added to the index later (see `--intent-to-add`
-	option in linkgit:git-add[1]). This option is ignored unless
-	running in a Git repository and `--index` is not specified.
-	Note that `--index` could be implied by other options such
-	as `--cached` or `--3way`.
+	option in linkgit:git-add[1]). This option has is ignored if `--index`
+	is specified.  Note that `--index` could be implied by other options
+	such as `--cached` or `--3way`.
 
 -3::
 --3way::

diff --git a/apply.c b/apply.c
index 43a0aebf4e..b0239b7482 100644
--- a/apply.c
+++ b/apply.c

@@ -153,8 +153,13 @@ int check_apply_state(struct apply_state *state, int force_apply)
 			return error(_("--cached outside a repository"));
 		state->check_index = 1;
 	}
-	if (state->ita_only && (state->check_index || is_not_gitdir))
+	if (state->ita_only && state->check_index)
 		state->ita_only = 0;
+	if (state->ita_only) {
+		if (is_not_gitdir)
+			return error(_("--intent-to-add outside a repository"));
+		state->check_index = 1;
+	}
 	if (state->check_index)
 		state->unsafe_paths = 0;
 

@@ -4760,7 +4765,7 @@ static int apply_patch(struct apply_state *state,
 	if (state->whitespace_error && (state->ws_error_action == die_on_ws_error))
 		state->apply = 0;
 
-	state->update_index = (state->check_index || state->ita_only) && state->apply;
+	state->update_index = state->check_index && state->apply;
 	if (state->update_index && !is_lock_file_locked(&state->lock_file)) {
 		if (state->index_file)
 			hold_lock_file_for_update(&state->lock_file,

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index cf0175ad6e..035ce3a2b9 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh

@@ -307,7 +307,7 @@ test_expect_success 'apply --intent-to-add' '
 	grep "new file" expected &&
 	git reset --hard &&
 	git apply --intent-to-add expected &&
-	git diff >actual &&
+	(git diff && git diff --cached) >actual &&
 	test_cmp expected actual
 '
 

diff --git a/t/t4140-apply-ita.sh b/t/t4140-apply-ita.sh
index c614eaf04c..4db1ae4e7e 100755
--- a/t/t4140-apply-ita.sh
+++ b/t/t4140-apply-ita.sh

@@ -53,4 +53,33 @@ test_expect_success 'apply deletion patch to ita path (--index)' '
 	git ls-files --stage --error-unmatch test-file
 '
 
+test_expect_success 'apply --intent-to-add is not allowed to modify untracked file' '
+	echo version1 >file &&
+	! git apply --intent-to-add <<-EOF
+	diff --git a/file b/file
+	index 1234567..89abcde 100644
+	--- b/file
+	+++ b/file
+	@@ -1 +1 @@
+	-version1
+	+version2
+	EOF
+'
+
+test_expect_success 'apply --index --intent-to-add ignores --intent-to-add, so it does not set i-t-a bit of touched file' '
+	echo >file &&
+	git add file &&
+	git apply --index --intent-to-add <<-EOF &&
+	diff --git a/file b/file
+	deleted file mode 100644
+	index 1234567..89abcde 100644
+	--- a/file
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-
+	EOF
+	git ls-files file >actual &&
+	test_must_be_empty actual
+'
+
 test_done

-- 
2.33.1

On Sat, Nov 06, 2021 at 12:42:02PM +0100, Johannes Altmanninger wrote:

+test_expect_success 'apply --intent-to-add is not allowed to modify untracked file' '
+	echo version1 >file &&
+	! git apply --intent-to-add <<-EOF

I guess s/!/test_must_fail/