Thread (4 messages) 4 messages, 2 authors, 2021-11-06

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

From: Junio C Hamano <hidden>
Date: 2021-11-01 07:07:34
Subsystem: the rest · Maintainer: Linus Torvalds

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help