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

On Sun, Oct 31, 2021 at 11:40:05PM -0700, Junio C Hamano wrote:

Johannes Altmanninger [off-list ref] writes:

quoted

Commit cff5dc09ed (apply: add --intent-to-add, 2018-05-26) introduced
"apply -N" plus a test to make sure it behaves exactly as "add -N"
when given equivalent changes.  However, the test only checks working
tree changes. Now "apply -N" forgot to read the index, so it left
all tracked files as deleted, except for the ones it touched.

Fix this by reading the index file, like we do for "apply --cached".
and test that we leave no content changes in the index.

Reported-by: Ryan Hodges <redacted>
Signed-off-by: Johannes Altmanninger <redacted>
---

Sorry I used the wrong Reported-by: address in v1

 apply.c               | 2 +-
 t/t2203-add-intent.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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

@@ -4771,7 +4771,7 @@ static int apply_patch(struct apply_state *state,
 					       LOCK_DIE_ON_ERROR);
 	}
 
-	if (state->check_index && read_apply_cache(state) < 0) {
+	if ((state->check_index || state->ita_only) && read_apply_cache(state) < 0) {
 		error(_("unable to read index file"));
 		res = -128;
 		goto end;

Thanks for an attempt, but I am not sure if it is wise to keep
ita_only independent from check_index like this patch does.

I must confess, I didn't even consider alternative solutions.

There are many safety/correctness related checks that check_index
enables, and that is why not just the "--index" option, but "--3way"
and "--cached" enable it internally.  As "instead of adding the
contents to the index, mark the new path with i-t-a bit" is also
futzing with the index, it should enable the same safety checks by
enabling check_index _much_ earlier.  And if you did so, the above
hunk will become a totally unnecessary change, because by the time
the control gets there, because you accepted ita_only earlier and
enabled check_index, just like you did for "--3way" and "--cached".

One thing that check_index does is that it drops unsafe_paths bit,
which means we would be protected from patch application that tries
to step out of our narrow cone of the directory hierarchy, which is
a safety measure.  There are probably others I am forgetting.

To be clear, check_index *disables* the unsafe_paths check, but it enables
a stronger check: verify_index_match(), which makes sure that the touched
paths exist in the index.