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.