Re: [PATCH v2] apply: allow "new file" patches on i-t-a entries

[Junio C Hamano <gitster@pobox.com>]

"Raymond E. Pasco" <ray@ameretat.dev> writes:

> diff-files recently changed to treat "intent to add" entries as new file
> diffs rather than diffs from the empty blob. However, apply refuses to
> apply new file diffs on top of existing index entries, except in the
> case of renames. This causes "git add -p", which uses apply, to fail
> when attempting to stage hunks from a file when intent to add has been
> recorded.

As this is supposed to be a "bugfix", there shouldn't be any need to
update documentation (otherwise, we are either fixing documentation
in addition to the bug, or we are changing the documented behaviour
in the name of bugfix---which we need to be very careful to see if
we are not breaking existing users).  But we do need to document
what behaviour we want with tests, which will also serve as a way to
protect the current behaviour from future bugs.

So I started writing the attached, but I have strong doubts about
the updated behaviour.

 - The first one (setup).  We create a sample file and keep it as
   the master copy, and express our intention to add test-file with
   its contents sometime in the future.  And then we take a patch
   out of the index.  We make sure that the produced patch is a
   creation patch.

   This should be straight-forward and uncontroversial.


 - The second one.  We make sure that we have i-t-a and not real
   entry for test-file in the index.  We try to apply the patch we
   took in the first test to (and only to) the index.  This must
   succeed, thanks to your fix---the i-t-a entry in the index should
   not prevent "new file mode 100644" from created at test-file.  We
   make sure what is in the index matches the master copy.

   This should be straight-forward and uncontroversial.


 - The third one.  We do the same set-up as the previous one, but in
   addition, we remove the working tree copy before attempting to
   apply the patch both to the index and to the working tree.  That
   way, "when creating a new file, it must not exist yet" rule on
   the working tree side would not trigger.

   This I find troublesome.  The real use case you had trouble with
   (i.e. "git add -p") would not remove any working tree files
   before attempting to apply any patch, I would imagine.  Are we
   expecting the right behaviour with this test?  I cannot tell.  

   It feels like it is even pointless to allow i-t-a entry to exist
   in the index for the path, if we MUST remove the path from the
   working tree anyway, to be able to apply.


 - The fourth one.  If we have test-file on the working tree, "when
   creating a new file, it must not exist yet" rule on the working
   tree side should trigger and prevent the operation.  I think this
   is a reasonable expectation.


What I am wondering primarily is if we should actually FAIL the
third one.  The patch tries to create a path, for which there is an
i-t-a entry in the index.  But a path with i-t-a entry in the index
means the user at least must have had a file on the working tree to
register that intent-to-add the path.  Removed working tree file
would then mean that the path _has_ a local modification, so "git
apply --index" should *not* succeed for the usual reasons of having
differences between the index and the working tree.

And without your "fix" to apply.c, "git apply" in the the third test
fails, so we may need a bit more work to make sure it keeps failing.

I dunno.  It almost feels that this approach to fix "git add -p"
might be barking up a wrong tree.  After all, the user, by having an
i-t-a entry for the path in the index, expressed the desire to add
real contents later to the path, so being able to use "git apply"
with either "--cached" or "--index" options to clobber the path with
a creation patch feels wrong _unless_ the user then rescinded the
previous intent to add to the path (with "git rm --cached" or an
equivalent).

How exactly does "git add -p" fail for such a patch?  What operation
does it exactly want to do ("apply --cached"???) and is it "apply"
that is wrong, or is it "git add -p" that fails to remove the i-t-a
entry from the index before running "git apply" that is at fault?

Thanks.

 apply.c              | 11 +++++++----
 t/t4140-apply-ita.sh | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/apply.c b/apply.c
index 8bff604dbe..4cba4ce71a 100644
--- a/apply.c
+++ b/apply.c
@@ -3747,10 +3747,13 @@ static int check_to_create(struct apply_state *state,
 {
 	struct stat nst;
 
-	if (state->check_index &&
-	    index_name_pos(state->repo->index, new_name, strlen(new_name)) >= 0 &&
-	    !ok_if_exists)
-		return EXISTS_IN_INDEX;
+	if (state->check_index && !ok_if_exists) {
+		int pos = index_name_pos(state->repo->index, new_name, strlen(new_name));
+		if (pos >= 0 &&
+		    !(state->repo->index->cache[pos]->ce_flags & CE_INTENT_TO_ADD))
+			return EXISTS_IN_INDEX;
+	}
+
 	if (state->cached)
 		return 0;
 
diff --git a/t/t4140-apply-ita.sh b/t/t4140-apply-ita.sh
new file mode 100755
index 0000000000..e9f3749e65
--- /dev/null
+++ b/t/t4140-apply-ita.sh
@@ -0,0 +1,51 @@
+#!/bin/sh
+#
+# Copyright 2020 Google LLC
+#
+
+test_description='git apply of i-t-a file'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	test_write_lines 1 2 3 4 5 >blueprint &&
+
+	cat blueprint >test-file &&
+	git add -N test-file &&
+	git diff >creation-patch &&
+	grep "new file mode 100644" creation-patch
+'
+
+test_expect_success 'apply creation patch to ita path (--cached)' '
+	git rm -f test-file &&
+	cat blueprint >test-file &&
+	git add -N test-file &&
+
+	git apply --cached creation-patch &&
+	git cat-file blob :test-file >actual &&
+	test_cmp blueprint actual
+'
+
+test_expect_success 'apply creation patch to ita path (--index)' '
+	git rm -f test-file &&
+	cat blueprint >test-file &&
+	git add -N test-file &&
+	rm -f test-file &&
+
+	# NEEDSWORK: this should fail as test-file does not
+	# agree between index and the working tree, no?????
+	git apply --index creation-patch &&
+	git cat-file blob :test-file >actual &&
+	test_cmp blueprint actual &&
+	test_cmp blueprint test-file
+'
+
+test_expect_success 'apply creation patch to ita path (--index)' '
+	git rm -f test-file &&
+	cat blueprint >test-file &&
+	git add -N test-file &&
+
+	test_must_fail git apply --index creation-patch
+'
+
+test_done