[PATCH] apply: Allow "new file" patches on i-t-a entries

[PATCH] apply: Allow "new file" patches on i-t-a entries

[Raymond E. Pasco]

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.

This adds an additional check to check_to_create() which tests if the
CE_INTENT_TO_ADD flag is set on an existing index entry, and allows the
apply to proceed if so.
---
cf. <5BDF4B85-7AC1-495F-85C3-D429E3E51106@gmail.com>
 apply.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/apply.c b/apply.c
index 8bff604dbe..b31bd0e866 100644
--- a/apply.c
+++ b/apply.c
@@ -3747,10 +3747,20 @@ 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) {
+		struct cache_entry *ce = NULL;
+		int intent_to_add;
+		int pos = index_name_pos(state->repo->index, new_name, strlen(new_name));
+		if (pos >= 0)
+			ce = state->repo->index->cache[pos];
+		if (ce && (ce->ce_flags & CE_INTENT_TO_ADD))
+			intent_to_add = 1;
+		else
+			intent_to_add = 0;
+		if (pos >= 0 && !intent_to_add && !ok_if_exists)
+			return EXISTS_IN_INDEX;
+	}
+
 	if (state->cached)
 		return 0;
 
-- 
2.28.0

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

[Junio C Hamano]

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

> Subject: Re: [PATCH] apply: Allow "new file" patches on i-t-a entries

Please downcase "A"llow.

> 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.
>
> This adds an additional check to check_to_create() which tests if the
> CE_INTENT_TO_ADD flag is set on an existing index entry, and allows the
> apply to proceed if so.
> ---

Please sign-off your patch (see Documentation/SubmittingPatches)

> cf. <5BDF4B85-7AC1-495F-85C3-D429E3E51106@gmail.com>
>  apply.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 8bff604dbe..b31bd0e866 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3747,10 +3747,20 @@ 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) {
> +		struct cache_entry *ce = NULL;
> +		int intent_to_add;
> +		int pos = index_name_pos(state->repo->index, new_name, strlen(new_name));
> +		if (pos >= 0)
> +			ce = state->repo->index->cache[pos];
> +		if (ce && (ce->ce_flags & CE_INTENT_TO_ADD))
> +			intent_to_add = 1;
> +		else
> +			intent_to_add = 0;
> +		if (pos >= 0 && !intent_to_add && !ok_if_exists)
> +			return EXISTS_IN_INDEX;
> +	}
> +

I think the new logic looks sound.  When we are applying a patch
that adds a new path, we do not want the path to already exist, so
we used to see if there is an existign cache entry with that name
and barfed if there is.  The spirit of the new code is the same,
except that we want to treat an i-t-a entry as "not yet exist".

How often do we pass ok_if_exists, I have to wonder.  If it is often
enough, then we can check that first way before we even check to see
if a cache entry for the path even exists or what its i-t-a flag
says.  Something along the lines of this untested code:

	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;
	}

That is, only if we are told to make sure the path does not already exist,
we see if the path is in the index, and if the cache entry for the
path in the index is a real entry (as opposed to i-t-a aka "not
added yet"), we complain.  Otherwise we'd happily take the patch.

Whether ok_if_exists is frequently used or not, the resulting code
may be easier to understand, but I am of course biased, as I just
wrote it ;-)

Hmm?

Thanks.

>  	if (state->cached)
>  		return 0;

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

[Raymond E. Pasco]

On Tue Aug 4, 2020 at 3:30 PM EDT, Junio C Hamano wrote:
> How often do we pass ok_if_exists, I have to wonder. If it is often
> enough, then we can check that first way before we even check to see
> if a cache entry for the path even exists or what its i-t-a flag
> says. Something along the lines of this untested code:
>
> 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;
> }
>
> That is, only if we are told to make sure the path does not already
> exist,
> we see if the path is in the index, and if the cache entry for the
> path in the index is a real entry (as opposed to i-t-a aka "not
> added yet"), we complain. Otherwise we'd happily take the patch.
>
> Whether ok_if_exists is frequently used or not, the resulting code
> may be easier to understand, but I am of course biased, as I just
> wrote it ;-)

ok_if_exists gets passed in cases where a real entry does exist but
we're okay with a new file diff anyway due to other patches in the set
being applied making it valid (type-change diffs and rename diffs) - for
this reason, I didn't pass ok_if_exists, but instead checked here. I
think we're in agreement on this and your logic makes sense in that
light. I'll send an updated patch.

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

[Raymond E. Pasco]

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.

This changes the logic in check_to_create() which checks if an entry
already exists in an index in two ways: first, we only search for an
index entry at all if ok_if_exists is false; second, we check for the
CE_INTENT_TO_ADD flag on any index entries we find and allow the apply
to proceed if it is set.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 apply.c | 11 +++++++----
 1 file changed, 7 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;
 
-- 
2.28.0.1.g70e0b8363a

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

[Raymond E. Pasco]

diff-files recently changed to treat changes to paths marked "intent to
add" in the index 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.

This changes the logic in check_to_create() which checks if an entry
already exists in an index in two ways: first, we only search for an
index entry at all if ok_if_exists is false; second, we check for the
CE_INTENT_TO_ADD flag on any index entries we find and allow the apply
to proceed if it is set.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
No substantive changes - fixed a poor explanation in the commit message.

 apply.c | 11 +++++++----
 1 file changed, 7 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;
 
-- 
2.28.0.1.g70e0b8363a

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

[Junio C Hamano]

"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

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

[Raymond E. Pasco]

On Tue Aug 4, 2020 at 7:49 PM EDT, Junio C Hamano wrote:
> 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?

Yes, "add -p" uses "apply --cached". I do believe this belongs in apply,
both because all "add -p" really does is assemble things to be fed to
apply and also for the more detailed reasons below.

The index and the filesystem are both able to represent "no file" and "a
file exists" states, but the index has an additional state (i-t-a) with
no direct representation in the worktree. By (correctly) emitting "new
file" patches when comparing a file to an i-t-a index entry, we are
setting down the rule that a "new file" patch is not merely the diff
between "no file" and "a file exists", but also the diff between i-t-a
and "a file exists".

Similarly, "deleted file" patches are the diff between "a file exists"
and "no file exists", but they are also the diff between i-t-a and "no
file exists" - if you add -N a file and then delete it from the
worktree, "deleted file" is what git diff (correctly) shows. As a
consequence of these rules, "new file" and "deleted file" diffs are now
the only diffs that validly apply to an i-t-a entry. So apply needs to
handle them (in "--cached" mode, anyway).

But the worktree lives in the filesystem, where there are no i-t-a
entries. So the question seems to me to be whether "no file" in the
worktree matches an i-t-a entry in the index for the purposes of "add
--index". I count a couple options here:

- Nothing on the filesystem can accurately match an i-t-a entry in the
  index, so all attempts at "apply --index" when there is an i-t-a in
  the index fail with "file: does not match index". "apply --cached",
  which "add -p" uses, applies only to the index and continues to work.
  I think I prefer this one; additionally, the comment in read-cache.c
  indicate that this is supposed to be the case already, so I just need
  to make sure this check is not skipped on "new file" patches.

- The current (as of this patch) behavior: a "new file" patch applies
  both to an i-t-a in the index, and to the lack of a file in the
  worktree. This may seem strange, but it may also seem strange that an
  identical new file patch, which can be applied either to just the
  worktree or just the index successfully, fails when applied to both at
  the same time with "apply --index". However, this is precisely what is
  done anyway by "apply --index" when there are no i-t-a entries
  involved, so I lean towards i-t-a entries never matching the worktree.

Patch for the first option in progress.

[PATCH v4 0/3] apply: handle i-t-a entries in index

[Raymond E. Pasco]

I've finished this up and completed the tests as well. Just as
read-cache.c says, i-t-a entries should never match the worktree -
therefore, apply --index always fails.

Raymond E. Pasco (3):
  apply: allow "new file" patches on i-t-a entries
  apply: make i-t-a entries never match worktree
  t4140: test apply with i-t-a paths

 apply.c              | 26 ++++++++++++++++----
 t/t4140-apply-ita.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 4 deletions(-)
 create mode 100644 t/t4140-apply-ita.sh

-- 
2.28.0.2.g72bf77540a.dirty

[PATCH v4 1/3] apply: allow "new file" patches on i-t-a entries

[Raymond E. Pasco]

diff-files recently changed to treat changes to paths marked "intent to
add" in the index 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.

This changes the logic in check_to_create() which checks if an entry
already exists in an index in two ways: first, we only search for an
index entry at all if ok_if_exists is false; second, we check for the
CE_INTENT_TO_ADD flag on any index entries we find and allow the apply
to proceed if it is set.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 apply.c | 11 +++++++----
 1 file changed, 7 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;
 
-- 
2.28.0.2.g72bf77540a.dirty

[PATCH v4 2/3] apply: make i-t-a entries never match worktree

[Raymond E. Pasco]

By definition, an intent-to-add index entry can never match the
worktree, because worktrees have no concept of intent-to-add entries.
Therefore, "apply --index" should always fail on intent-to-add paths.

Because check_preimage() calls verify_index_match(), it already fails
for patches other than creation patches, which check_preimage() ignores.
This patch adds a check to check_preimage()'s rough equivalent for
creation patches, check_to_create().

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 apply.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/apply.c b/apply.c
index 4cba4ce71a..6328591489 100644
--- a/apply.c
+++ b/apply.c
@@ -3754,6 +3754,21 @@ static int check_to_create(struct apply_state *state,
 			return EXISTS_IN_INDEX;
 	}
 
+	/* If the new path to be added has an intent-to-add entry, then
+	 * by definition it does not match what is in the work tree. So
+	 * "apply --index" should always fail in this case. Patches other
+	 * than creation patches are already held to this standard by
+	 * check_preimage() calling verify_index_match().
+	 */
+	if (state->check_index && !state->cached) {
+		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 error(_("%s: does not match index"), new_name);
+	}
+
+
 	if (state->cached)
 		return 0;
 
-- 
2.28.0.2.g72bf77540a.dirty

Re: [PATCH v4 2/3] apply: make i-t-a entries never match worktree

[Junio C Hamano]

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

> By definition, an intent-to-add index entry can never match the
> worktree, because worktrees have no concept of intent-to-add entries.
> Therefore, "apply --index" should always fail on intent-to-add paths.
>
> Because check_preimage() calls verify_index_match(), it already fails
> for patches other than creation patches, which check_preimage() ignores.
> This patch adds a check to check_preimage()'s rough equivalent for
> creation patches, check_to_create().
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
> ---
>  apply.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)

At first glance, it feels somewhat sad that this check is not done
in check_preimage(); after all, the caller of check_preimage() feeds
it to all kind of patches, without excluding path creation, so the
helper should be allowed to say "heh, you are trying to create path
F with this patch, but there already is F in the index", "you are
renaming into F but there is F already", etc.

But ensuring the state of the path to be patched is done by
check_to_create() for paths that are to be created, even before this
patch, because it simply needs to do different checks from what is
done on modification patch, and also because we need to resolve
patch->is_new bit for non-git patches before being able to perform
the checks done in check_to_create().

So I agree with the location of this additonal logic.

It is somewhat unsatisfactory that we need to do the same
index_name_pos probing twice.  I wonder if we somehow can
consolidate them?

Perhaps something along this line, instead of this patch?

 apply.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/apply.c b/apply.c
index 4cba4ce71a..c5ecb64102 100644
--- a/apply.c
+++ b/apply.c
@@ -3740,6 +3740,7 @@ static int check_preimage(struct apply_state *state,
 
 #define EXISTS_IN_INDEX 1
 #define EXISTS_IN_WORKTREE 2
+#define EXISTS_IN_INDEX_AS_ITA 3
 
 static int check_to_create(struct apply_state *state,
 			   const char *new_name,
@@ -3747,11 +3748,21 @@ static int check_to_create(struct apply_state *state,
 {
 	struct stat nst;
 
-	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->check_index && (!ok_if_exists || !state->cached)) {
+		int pos;
+
+		pos = index_name_pos(state->repo->index, new_name, strlen(new_name));
+		if (pos >= 0) {
+			struct cache_entry *ce = state->repo->index->cache[pos];
+
+			/* allow ITA, as they do not yet exist in the index */
+			if (!ok_if_exists && !(ce->ce_flags & CE_INTENT_TO_ADD))
+				return EXISTS_IN_INDEX;
+
+			/* ITA entries can never match working tree files */
+			if (!state->cached && (ce->ce_flags & CE_INTENT_TO_ADD))
+				return EXISTS_IN_INDEX_AS_ITA;
+		}
 	}
 
 	if (state->cached)
@@ -3938,6 +3949,9 @@ static int check_patch(struct apply_state *state, struct patch *patch)
 		case EXISTS_IN_INDEX:
 			return error(_("%s: already exists in index"), new_name);
 			break;
+		case EXISTS_IN_INDEX_AS_ITA:
+			return error(_("%s: does not match index"), new_name);
+			break;
 		case EXISTS_IN_WORKTREE:
 			return error(_("%s: already exists in working directory"),
 				     new_name);

Re: [PATCH v4 2/3] apply: make i-t-a entries never match worktree

[Raymond E. Pasco]

On Thu Aug 6, 2020 at 5:00 PM EDT, Junio C Hamano wrote:
> At first glance, it feels somewhat sad that this check is not done
> in check_preimage(); after all, the caller of check_preimage() feeds
> it to all kind of patches, without excluding path creation, so the
> helper should be allowed to say "heh, you are trying to create path
> F with this patch, but there already is F in the index", "you are
> renaming into F but there is F already", etc.

I spent some time trying to put it in there before deciding it was
better off in check_to_create().

> It is somewhat unsatisfactory that we need to do the same
> index_name_pos probing twice. I wonder if we somehow can
> consolidate them?
>
> Perhaps something along this line, instead of this patch?

I think this logic can be consolidated and still readable, yeah. I'll
send a patch.

[PATCH v4 3/3] t4140: test apply with i-t-a paths

[Raymond E. Pasco]

apply --cached (as used by add -p) should accept creation and deletion
patches to intent-to-add paths in the index. apply --index, however,
should always fail because an intent-to-add path never matches the
worktree (by definition).

Based-on-patch-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 t/t4140-apply-ita.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 t/t4140-apply-ita.sh

diff --git a/t/t4140-apply-ita.sh b/t/t4140-apply-ita.sh
new file mode 100644
index 0000000000..c614eaf04c
--- /dev/null
+++ b/t/t4140-apply-ita.sh
@@ -0,0 +1,56 @@
+#!/bin/sh
+
+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 &&
+
+	rm -f test-file &&
+	git diff >deletion-patch &&
+	grep "deleted file mode 100644" deletion-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 &&
+
+	test_must_fail git apply --index creation-patch
+'
+
+test_expect_success 'apply deletion patch to ita path (--cached)' '
+	git rm -f test-file &&
+	cat blueprint >test-file &&
+	git add -N test-file &&
+
+	git apply --cached deletion-patch &&
+	test_must_fail git ls-files --stage --error-unmatch test-file
+'
+
+test_expect_success 'apply deletion patch to ita path (--index)' '
+	cat blueprint >test-file &&
+	git add -N test-file &&
+
+	test_must_fail git apply --index deletion-patch &&
+	git ls-files --stage --error-unmatch test-file
+'
+
+test_done
-- 
2.28.0.2.g72bf77540a.dirty

Re: [PATCH v4 3/3] t4140: test apply with i-t-a paths

[Junio C Hamano]

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

> apply --cached (as used by add -p) should accept creation and deletion
> patches to intent-to-add paths in the index. apply --index, however,
> should always fail because an intent-to-add path never matches the
> worktree (by definition).
>
> Based-on-patch-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
> ---
>  t/t4140-apply-ita.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 t/t4140-apply-ita.sh

chmod +x

> diff --git a/t/t4140-apply-ita.sh b/t/t4140-apply-ita.sh
> new file mode 100644
> index 0000000000..c614eaf04c
> --- /dev/null
> +++ b/t/t4140-apply-ita.sh
> @@ -0,0 +1,56 @@
> +#!/bin/sh
> +
> +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 &&
> +
> +	rm -f test-file &&
> +	git diff >deletion-patch &&
> +	grep "deleted file mode 100644" deletion-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
> +'

OK, this is a good positive test case.

> +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 &&
> +
> +	test_must_fail git apply --index creation-patch
> +'

"--index" (not "--cached") notices that test-file does not match
between the index and the working tree, and fails.  OK.

> +test_expect_success 'apply deletion patch to ita path (--cached)' '
> +	git rm -f test-file &&
> +	cat blueprint >test-file &&
> +	git add -N test-file &&
> +
> +	git apply --cached deletion-patch &&
> +	test_must_fail git ls-files --stage --error-unmatch test-file
> +'

We can delete an I-T-A entry.  I wonder if

	git apply --cached -R creation-patch

also serves as a way to remove the path?  It should succeed, right?

> +test_expect_success 'apply deletion patch to ita path (--index)' '
> +	cat blueprint >test-file &&
> +	git add -N test-file &&
> +
> +	test_must_fail git apply --index deletion-patch &&
> +	git ls-files --stage --error-unmatch test-file
> +'

Again "--index" notices that the path has ITA bit on, and refuses to
remove it.

OK, looking good so far.

Thanks.

Re: [PATCH v4 3/3] t4140: test apply with i-t-a paths

[Raymond E. Pasco]

On Thu Aug 6, 2020 at 5:07 PM EDT, Junio C Hamano wrote:
> We can delete an I-T-A entry. I wonder if
>
> git apply --cached -R creation-patch
>
> also serves as a way to remove the path? It should succeed, right?

It fails to apply because the inverse patch to creation-patch removes
lines, while the i-t-a entry has no lines. (Looking at it from the other
angle, deletion-patch doesn't delete lines, because there are none.)

(Of course, -R creation-patch after creation-patch removes the path
cleanly.)

[PATCH v5 0/3] apply: handle i-t-a entries in index

[Raymond E. Pasco]

While testing the logic, I encountered some other issues, to be
addressed in related patchsets.

Raymond E. Pasco (3):
  apply: allow "new file" patches on i-t-a entries
  apply: make i-t-a entries never match worktree
  t4140: test apply with i-t-a paths

 apply.c              | 25 ++++++++++++++++----
 t/t4140-apply-ita.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 4 deletions(-)
 create mode 100755 t/t4140-apply-ita.sh

-- 
2.28.0.5.gfc8e108108

[PATCH v5 1/3] apply: allow "new file" patches on i-t-a entries

[Raymond E. Pasco]

diff-files recently changed to treat changes to paths marked "intent to
add" in the index 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.

This changes the logic in check_to_create() which checks if an entry
already exists in an index in two ways: first, we only search for an
index entry at all if ok_if_exists is false; second, we check for the
CE_INTENT_TO_ADD flag on any index entries we find and allow the apply
to proceed if it is set.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 apply.c | 11 +++++++----
 1 file changed, 7 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;
 
-- 
2.28.0.5.gfc8e108108

Re: [PATCH v5 1/3] apply: allow "new file" patches on i-t-a entries

[Phillip Wood]

Hi Raymond

On 08/08/2020 08:49, Raymond E. Pasco wrote:
> diff-files recently changed to treat changes to paths marked "intent to
> add" in the index 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.
> 
> This changes the logic in check_to_create() which checks if an entry
> already exists in an index in two ways: first, we only search for an
> index entry at all if ok_if_exists is false; second, we check for the
> CE_INTENT_TO_ADD flag on any index entries we find and allow the apply
> to proceed if it is set.

Thanks for working on this, I got stung by not being able to apply a 
patch because the path was marked i-t-a recently

Best Wishes

Phillip

> 
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
> ---
>   apply.c | 11 +++++++----
>   1 file changed, 7 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;
>   
> 

[PATCH v5 2/3] apply: make i-t-a entries never match worktree

[Raymond E. Pasco]

By definition, an intent-to-add index entry can never match the
worktree, because worktrees have no concept of intent-to-add entries.
Therefore, "apply --index" should always fail on intent-to-add paths.

Because check_preimage() calls verify_index_match(), it already fails
for patches other than creation patches, which check_preimage() ignores.
This patch adds a check to check_preimage()'s rough equivalent for
creation patches, check_to_create().

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 apply.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/apply.c b/apply.c
index 4cba4ce71a..c5ecb64102 100644
--- a/apply.c
+++ b/apply.c
@@ -3740,6 +3740,7 @@ static int check_preimage(struct apply_state *state,
 
 #define EXISTS_IN_INDEX 1
 #define EXISTS_IN_WORKTREE 2
+#define EXISTS_IN_INDEX_AS_ITA 3
 
 static int check_to_create(struct apply_state *state,
 			   const char *new_name,
@@ -3747,11 +3748,21 @@ static int check_to_create(struct apply_state *state,
 {
 	struct stat nst;
 
-	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->check_index && (!ok_if_exists || !state->cached)) {
+		int pos;
+
+		pos = index_name_pos(state->repo->index, new_name, strlen(new_name));
+		if (pos >= 0) {
+			struct cache_entry *ce = state->repo->index->cache[pos];
+
+			/* allow ITA, as they do not yet exist in the index */
+			if (!ok_if_exists && !(ce->ce_flags & CE_INTENT_TO_ADD))
+				return EXISTS_IN_INDEX;
+
+			/* ITA entries can never match working tree files */
+			if (!state->cached && (ce->ce_flags & CE_INTENT_TO_ADD))
+				return EXISTS_IN_INDEX_AS_ITA;
+		}
 	}
 
 	if (state->cached)
@@ -3938,6 +3949,9 @@ static int check_patch(struct apply_state *state, struct patch *patch)
 		case EXISTS_IN_INDEX:
 			return error(_("%s: already exists in index"), new_name);
 			break;
+		case EXISTS_IN_INDEX_AS_ITA:
+			return error(_("%s: does not match index"), new_name);
+			break;
 		case EXISTS_IN_WORKTREE:
 			return error(_("%s: already exists in working directory"),
 				     new_name);
-- 
2.28.0.5.gfc8e108108

Re: [PATCH v5 2/3] apply: make i-t-a entries never match worktree

[Phillip Wood]

Hi Raymond

On 08/08/2020 08:49, Raymond E. Pasco wrote:
> By definition, an intent-to-add index entry can never match the
> worktree, because worktrees have no concept of intent-to-add entries.
> Therefore, "apply --index" should always fail on intent-to-add paths.

I'm not sure I understand the logic for this. If I run 'git add -N 
<path>' and <path> does not exist in the worktree what's the reason to 
stop a patch that creates <path> from applying?

I was relieved to see from the next patch that this does not affect 
--cached even though the documentation says it implies --index. It might 
be worth mentioning that in the commit message. Also it would be easier 
to follow if the tests were in the same patch (this is what we usually do).

How this does it affect --check? `git add -p` uses --check to verify 
that hunks that the user has edited still apply. It does not let the 
user edit the hunk for a newly added file at the moment but that is 
something I'm thinking of adding.

Best Wishes

Phillip

> Because check_preimage() calls verify_index_match(), it already fails
> for patches other than creation patches, which check_preimage() ignores.
> This patch adds a check to check_preimage()'s rough equivalent for
> creation patches, check_to_create().
> 
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
> ---
>   apply.c | 24 +++++++++++++++++++-----
>   1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/apply.c b/apply.c
> index 4cba4ce71a..c5ecb64102 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3740,6 +3740,7 @@ static int check_preimage(struct apply_state *state,
>   
>   #define EXISTS_IN_INDEX 1
>   #define EXISTS_IN_WORKTREE 2
> +#define EXISTS_IN_INDEX_AS_ITA 3
>   
>   static int check_to_create(struct apply_state *state,
>   			   const char *new_name,
> @@ -3747,11 +3748,21 @@ static int check_to_create(struct apply_state *state,
>   {
>   	struct stat nst;
>   
> -	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->check_index && (!ok_if_exists || !state->cached)) {
> +		int pos;
> +
> +		pos = index_name_pos(state->repo->index, new_name, strlen(new_name));
> +		if (pos >= 0) {
> +			struct cache_entry *ce = state->repo->index->cache[pos];
> +
> +			/* allow ITA, as they do not yet exist in the index */
> +			if (!ok_if_exists && !(ce->ce_flags & CE_INTENT_TO_ADD))
> +				return EXISTS_IN_INDEX;
> +
> +			/* ITA entries can never match working tree files */
> +			if (!state->cached && (ce->ce_flags & CE_INTENT_TO_ADD))
> +				return EXISTS_IN_INDEX_AS_ITA;
> +		}
>   	}
>   
>   	if (state->cached)
> @@ -3938,6 +3949,9 @@ static int check_patch(struct apply_state *state, struct patch *patch)
>   		case EXISTS_IN_INDEX:
>   			return error(_("%s: already exists in index"), new_name);
>   			break;
> +		case EXISTS_IN_INDEX_AS_ITA:
> +			return error(_("%s: does not match index"), new_name);
> +			break;
>   		case EXISTS_IN_WORKTREE:
>   			return error(_("%s: already exists in working directory"),
>   				     new_name);
> 

Re: [PATCH v5 2/3] apply: make i-t-a entries never match worktree

[Raymond E. Pasco]

On Sat Aug 8, 2020 at 9:46 AM EDT, Phillip Wood wrote:
> > By definition, an intent-to-add index entry can never match the
> > worktree, because worktrees have no concept of intent-to-add entries.
> > Therefore, "apply --index" should always fail on intent-to-add paths.
>
> I'm not sure I understand the logic for this. If I run 'git add -N
> <path>' and <path> does not exist in the worktree what's the reason to
> stop a patch that creates <path> from applying?

"apply --index" requires the index and worktree to match, and applies
the same path to both to get the same result in both. I brainstormed the
logic a few emails upthread, and that's what's consistent with
everything else.

> I was relieved to see from the next patch that this does not affect
> --cached even though the documentation says it implies --index. It might
> be worth mentioning that in the commit message. Also it would be easier
> to follow if the tests were in the same patch (this is what we usually
> do).

--cached doesn't really imply --index - the docs are wrong and should be
changed. If anything, --index is closer to implying --cached - but
really, [no flags], --cached, and --index are three different modes with
different behavior. (Just removing "this implies --index" would be
sufficient to make the docs correct.)

> How this does it affect --check? `git add -p` uses --check to verify
> that hunks that the user has edited still apply. It does not let the
> user edit the hunk for a newly added file at the moment but that is
> something I'm thinking of adding.

--check goes through all the same code, it just doesn't actually touch
anything in the index or worktree. Splittable/editable new file patches
are a logical related feature, IMO. (This is just to squash an error
that shouldn't happen.)

Re: [PATCH v5 2/3] apply: make i-t-a entries never match worktree

[Phillip Wood]

Hi Raymond

On 08/08/2020 15:07, Raymond E. Pasco wrote:
> On Sat Aug 8, 2020 at 9:46 AM EDT, Phillip Wood wrote:
>>> By definition, an intent-to-add index entry can never match the
>>> worktree, because worktrees have no concept of intent-to-add entries.
>>> Therefore, "apply --index" should always fail on intent-to-add paths.
>>
>> I'm not sure I understand the logic for this. If I run 'git add -N
>> <path>' and <path> does not exist in the worktree what's the reason to
>> stop a patch that creates <path> from applying?
> 
> "apply --index" requires the index and worktree to match, and applies
> the same path to both to get the same result in both. I brainstormed the
> logic a few emails upthread, and that's what's consistent with
> everything else.

I had a quick scan of the earlier email and found

 > The index and the filesystem are both able to represent "no file"
 > and "a file exists" states, but the index has an additional
 > state (i-t-a) with no direct representation in the
 > worktree. By (correctly) emitting "new file" patches when
 > comparing a file to an i-t-a index entry, we are setting down the
 > rule that a "new file" patch is not merely the diff between "no
 > file" and "a file exists", but also the diff between i-t-a and "a
 > file exists".
 >
 > Similarly, "deleted file" patches are the diff between "a file
 > exists" and "no file exists", but they are also the diff between
 > i-t-a and "no file exists" - if you add -N a file and then delete
 > it from the worktree, "deleted file" is what git diff (correctly)
 > shows. As a consequence of these rules, "new file" and "deleted
 > file" diffs are now the only diffs that validly apply to an i-t-a
 > entry. So apply needs to handle them (in "--cached" mode,
 > anyway).

If I've understood correctly an i-t-a entry in the index combined with 
nothing in the worktree is a deletion and that is why we don't want 
--index to succeed when applying a creation patch? If so an expanded 
explanation in the commit message to this patch would help rather than 
just saying 'by definition'. I'm still a bit confused as we don't count 
it as a deletion when using --cached or applying to the worktree.

>> I was relieved to see from the next patch that this does not affect
>> --cached even though the documentation says it implies --index. It might
>> be worth mentioning that in the commit message. Also it would be easier
>> to follow if the tests were in the same patch (this is what we usually
>> do).
> 
> --cached doesn't really imply --index - the docs are wrong and should be
> changed. If anything, --index is closer to implying --cached - but
> really, [no flags], --cached, and --index are three different modes with
> different behavior. (Just removing "this implies --index" would be
> sufficient to make the docs correct.)
> 
>> How this does it affect --check? `git add -p` uses --check to verify
>> that hunks that the user has edited still apply. It does not let the
>> user edit the hunk for a newly added file at the moment but that is
>> something I'm thinking of adding.
> 
> --check goes through all the same code, 

The same code as --cached or --index? (I assume it's the former but 
wanted to be sure)

Thanks

Phillip

>it just doesn't actually touch
> anything in the index or worktree. Splittable/editable new file patches
> are a logical related feature, IMO. (This is just to squash an error
> that shouldn't happen.)
> 

Re: [PATCH v5 2/3] apply: make i-t-a entries never match worktree

[Raymond E. Pasco]

On Sat Aug 8, 2020 at 11:48 AM EDT, Phillip Wood wrote:
> If I've understood correctly an i-t-a entry in the index combined with
> nothing in the worktree is a deletion and that is why we don't want
> --index to succeed when applying a creation patch? If so an expanded
> explanation in the commit message to this patch would help rather than
> just saying 'by definition'. I'm still a bit confused as we don't count
> it as a deletion when using --cached or applying to the worktree.

Nothing that complicated - --index requires that the index and worktree
be identical, and nothing that can possibly be in a worktree is
identical to an i-t-a entry.

> > --check goes through all the same code,
>
> The same code as --cached or --index? (I assume it's the former but
> wanted to be sure)

"--cached --check" goes through the same code paths as "--cached",
"--cached --index" goes through the same code paths as "--index",
"--check" goes through the same code paths as [no options]. The option
just makes it skip the part where it writes things out.

Re: [PATCH v5 2/3] apply: make i-t-a entries never match worktree

[Phillip Wood]

Hi Raymond

On 08/08/2020 16:58, Raymond E. Pasco wrote:
> On Sat Aug 8, 2020 at 11:48 AM EDT, Phillip Wood wrote:
>> If I've understood correctly an i-t-a entry in the index combined with
>> nothing in the worktree is a deletion and that is why we don't want
>> --index to succeed when applying a creation patch? If so an expanded
>> explanation in the commit message to this patch would help rather than
>> just saying 'by definition'. I'm still a bit confused as we don't count
>> it as a deletion when using --cached or applying to the worktree.
> 
> Nothing that complicated - --index requires that the index and worktree
> be identical, and nothing that can possibly be in a worktree is
> identical to an i-t-a entry.

Oh, so --index requires the index and worktree to match and the worktree 
cannot represent i-t-a so they don't match. I'm still not totally 
convinced that it is useful to prevent a creation patch from applying 
when the path is missing from the worktree but is marked as i-t-a in the 
index but I guess it matches the behavior the general behavior where a 
patch can be successfully applied with 'apply' and 'apply --cached' but 
not with 'apply --index'

>>> --check goes through all the same code,
>>
>> The same code as --cached or --index? (I assume it's the former but
>> wanted to be sure)
> 
> "--cached --check" goes through the same code paths as "--cached",
> "--cached --index" goes through the same code paths as "--index",
> "--check" goes through the same code paths as [no options]. The option
> just makes it skip the part where it writes things out.

Thanks for clarifying that

Best Wishes

Phillip

Re: [PATCH v5 2/3] apply: make i-t-a entries never match worktree

[Junio C Hamano]

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

> On Sat Aug 8, 2020 at 11:48 AM EDT, Phillip Wood wrote:
>> If I've understood correctly an i-t-a entry in the index combined with
>> nothing in the worktree is a deletion and that is why we don't want
>> --index to succeed when applying a creation patch? If so an expanded
>> explanation in the commit message to this patch would help rather than
>> just saying 'by definition'. I'm still a bit confused as we don't count
>> it as a deletion when using --cached or applying to the worktree.
>
> Nothing that complicated - --index requires that the index and worktree
> be identical, and nothing that can possibly be in a worktree is
> identical to an i-t-a entry.
>
>> > --check goes through all the same code,
>>
>> The same code as --cached or --index? (I assume it's the former but
>> wanted to be sure)
>
> "--cached --check" goes through the same code paths as "--cached",
> "--cached --index" goes through the same code paths as "--index",
> "--check" goes through the same code paths as [no options]. The option
> just makes it skip the part where it writes things out.

Well explained.  Thanks.

[PATCH] git-apply.txt: correct description of --cached

[Raymond E. Pasco]

The blurb for "--cached" says it implies "--index", but in reality
"--cached" and "--index" are distinct modes with different behavior.

Remove the sentence "This implies `--index`." to make the description
accurate.

Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 Documentation/git-apply.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index b9aa39000f..373a9354b5 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -72,7 +72,7 @@ OPTIONS
 --cached::
 	Apply a patch without touching the working tree. Instead take the
 	cached data, apply the patch, and store the result in the index
-	without using the working tree. This implies `--index`.
+	without using the working tree.
 
 --intent-to-add::
 	When applying the patch only to the working tree, mark new
-- 
2.28.0.10.gc1c9059842

Re: [PATCH] git-apply.txt: correct description of --cached

[Junio C Hamano]

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

> The blurb for "--cached" says it implies "--index", but in reality
> "--cached" and "--index" are distinct modes with different behavior.
>
> Remove the sentence "This implies `--index`." to make the description
> accurate.
>
> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
> ---
>  Documentation/git-apply.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> index b9aa39000f..373a9354b5 100644
> --- a/Documentation/git-apply.txt
> +++ b/Documentation/git-apply.txt
> @@ -72,7 +72,7 @@ OPTIONS
>  --cached::
>  	Apply a patch without touching the working tree. Instead take the
>  	cached data, apply the patch, and store the result in the index
> -	without using the working tree. This implies `--index`.
> +	without using the working tree.

The updated text is not wrong per-se, but I have a feeling that this
is barking up a wrong tree.  The implication is probably referring
to the fact that "--index" does certain verification and "--cached"
does the same (i.e. the patch must be applicable to what is in the
index).  We may want to update the description for both options.

How about simplifying them like this, perhaps?

 Documentation/git-apply.txt | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index b9aa39000f..92b5f0ae22 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -58,21 +58,18 @@ OPTIONS
 --check::
 	Instead of applying the patch, see if the patch is
 	applicable to the current working tree and/or the index
-	file and detects errors.  Turns off "apply".
+	file and detects errors.  Turns off `--apply`.
 
 --index::
-	When `--check` is in effect, or when applying the patch
-	(which is the default when none of the options that
-	disables it is in effect), make sure the patch is
-	applicable to what the current index file records.  If
-	the file to be patched in the working tree is not
-	up to date, it is flagged as an error.  This flag also
-	causes the index file to be updated.
+	Apply the patch to both the contents in the index and in the
+	working tree.  It is an error if the patched file in the
+	working tree is not up to date.
 
 --cached::
-	Apply a patch without touching the working tree. Instead take the
-	cached data, apply the patch, and store the result in the index
-	without using the working tree. This implies `--index`.
+	Apply the patch only to the contents in the index but not to
+	the working tree.  It is OK if the contents in the index
+	and in the working tree are different, as the latter is
+	never looked at.
 
 --intent-to-add::
 	When applying the patch only to the working tree, mark new

Re: [PATCH] git-apply.txt: correct description of --cached

[Phillip Wood]

On 10/08/2020 17:18, Junio C Hamano wrote:
> "Raymond E. Pasco" <ray@ameretat.dev> writes:
> 
>> The blurb for "--cached" says it implies "--index", but in reality
>> "--cached" and "--index" are distinct modes with different behavior.
>>
>> Remove the sentence "This implies `--index`." to make the description
>> accurate.
>>
>> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
>> ---
>>  Documentation/git-apply.txt | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
>> index b9aa39000f..373a9354b5 100644
>> --- a/Documentation/git-apply.txt
>> +++ b/Documentation/git-apply.txt
>> @@ -72,7 +72,7 @@ OPTIONS
>>  --cached::
>>  	Apply a patch without touching the working tree. Instead take the
>>  	cached data, apply the patch, and store the result in the index
>> -	without using the working tree. This implies `--index`.
>> +	without using the working tree.
> 
> The updated text is not wrong per-se, but I have a feeling that this
> is barking up a wrong tree.  The implication is probably referring
> to the fact that "--index" does certain verification and "--cached"
> does the same (i.e. the patch must be applicable to what is in the
> index).  We may want to update the description for both options.
> 
> How about simplifying them like this, perhaps?

I think this is clearer, I've got one comment below

> 
>  Documentation/git-apply.txt | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> index b9aa39000f..92b5f0ae22 100644
> --- a/Documentation/git-apply.txt
> +++ b/Documentation/git-apply.txt
> @@ -58,21 +58,18 @@ OPTIONS
>  --check::
>  	Instead of applying the patch, see if the patch is
>  	applicable to the current working tree and/or the index
> -	file and detects errors.  Turns off "apply".
> +	file and detects errors.  Turns off `--apply`.
>  
>  --index::
> -	When `--check` is in effect, or when applying the patch
> -	(which is the default when none of the options that
> -	disables it is in effect), make sure the patch is
> -	applicable to what the current index file records.  If
> -	the file to be patched in the working tree is not
> -	up to date, it is flagged as an error.  This flag also
> -	causes the index file to be updated.
> +	Apply the patch to both the contents in the index and in the
> +	working tree.  It is an error if the patched file in the
> +	working tree is not up to date.

I wonder if it would be clearer to say "This option requires the index
entry for the patched file to match the working tree". Saying "if the
patched file in the working tree is not up to date" does not say up to
date with what and one could argue that it is the index that is out of
date rather than the working tree if they don't match.

Best Wishes

Phillip

>  --cached::
> -	Apply a patch without touching the working tree. Instead take the
> -	cached data, apply the patch, and store the result in the index
> -	without using the working tree. This implies `--index`.
> +	Apply the patch only to the contents in the index but not to
> +	the working tree.  It is OK if the contents in the index
> +	and in the working tree are different, as the latter is
> +	never looked at.
>  
>  --intent-to-add::
>  	When applying the patch only to the working tree, mark new
> 

Re: [PATCH] git-apply.txt: correct description of --cached

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>>  --index::
>> -	When `--check` is in effect, or when applying the patch
>> -	(which is the default when none of the options that
>> -	disables it is in effect), make sure the patch is
>> -	applicable to what the current index file records.  If
>> -	the file to be patched in the working tree is not
>> -	up to date, it is flagged as an error.  This flag also
>> -	causes the index file to be updated.
>> +	Apply the patch to both the contents in the index and in the
>> +	working tree.  It is an error if the patched file in the
>> +	working tree is not up to date.
>
> I wonder if it would be clearer to say "This option requires the index
> entry for the patched file to match the working tree".

Perhaps.  But "the index entry to match the working tree" leaves the
definition of what is to "match" open to interpretation, so it may
need to be further tightened.

In the olden days, everybody knew "up to date", used to describe the
state of a file in the working tree, is a technical term with a
specific meaning (i.e. the contents has not changed since it was
added to the index, and also cached stat information in the index is
fresh), and that is why the original description used that wording.
But I agree that we should be able to express this without resorting
to a term of art.

    An error is raised if the file in the working tree being patched
    has contents different from what is registered in the index.

captures most of it, but still misses the "cached stat information
also must match" part.

Re: [PATCH] git-apply.txt: correct description of --cached

[Raymond E. Pasco]

On Wed Aug 12, 2020 at 3:23 PM EDT, Junio C Hamano wrote:
> An error is raised if the file in the working tree being patched
> has contents different from what is registered in the index.
>
> captures most of it, but still misses the "cached stat information
> also must match" part.

As a point of pedantry, it checks the preimages (don't want docs readers
to be worried it might somehow touch something and *then* error out).

I'll see if I can think of a succinct wording while I'm updating some of
the other patches.

Re: [PATCH] git-apply.txt: correct description of --cached

[Phillip Wood]

On 10/08/2020 17:18, Junio C Hamano wrote:
> "Raymond E. Pasco" <ray@ameretat.dev> writes:
> 
>> The blurb for "--cached" says it implies "--index", but in reality
>> "--cached" and "--index" are distinct modes with different behavior.
>>
>> Remove the sentence "This implies `--index`." to make the description
>> accurate.
>>
>> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
>> ---
>>   Documentation/git-apply.txt | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
>> index b9aa39000f..373a9354b5 100644
>> --- a/Documentation/git-apply.txt
>> +++ b/Documentation/git-apply.txt
>> @@ -72,7 +72,7 @@ OPTIONS
>>   --cached::
>>   	Apply a patch without touching the working tree. Instead take the
>>   	cached data, apply the patch, and store the result in the index
>> -	without using the working tree. This implies `--index`.
>> +	without using the working tree.
> 
> The updated text is not wrong per-se, but I have a feeling that this
> is barking up a wrong tree.  The implication is probably referring
> to the fact that "--index" does certain verification and "--cached"
> does the same (i.e. the patch must be applicable to what is in the
> index).  We may want to update the description for both options.
> 
> How about simplifying them like this, perhaps?

I think this is clearer, I've got one comment below

>   Documentation/git-apply.txt | 19 ++++++++-----------
>   1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> index b9aa39000f..92b5f0ae22 100644
> --- a/Documentation/git-apply.txt
> +++ b/Documentation/git-apply.txt
> @@ -58,21 +58,18 @@ OPTIONS
>   --check::
>   	Instead of applying the patch, see if the patch is
>   	applicable to the current working tree and/or the index
> -	file and detects errors.  Turns off "apply".
> +	file and detects errors.  Turns off `--apply`.
>   
>   --index::
> -	When `--check` is in effect, or when applying the patch
> -	(which is the default when none of the options that
> -	disables it is in effect), make sure the patch is
> -	applicable to what the current index file records.  If
> -	the file to be patched in the working tree is not
> -	up to date, it is flagged as an error.  This flag also
> -	causes the index file to be updated.
> +	Apply the patch to both the contents in the index and in the
> +	working tree.  It is an error if the patched file in the
> +	working tree is not up to date.

I wonder if it would be clearer to say "This option requires the index 
entry for the patched file to match the working tree". Saying "if the 
patched file in the working tree is not up to date" does not say up to 
date with what and one could argue that it is the index that is out of 
date rather than the working tree if they don't match.

Best Wishes

Phillip

>   --cached::
> -	Apply a patch without touching the working tree. Instead take the
> -	cached data, apply the patch, and store the result in the index
> -	without using the working tree. This implies `--index`.
> +	Apply the patch only to the contents in the index but not to
> +	the working tree.  It is OK if the contents in the index
> +	and in the working tree are different, as the latter is
> +	never looked at.
>   
>   --intent-to-add::
>   	When applying the patch only to the working tree, mark new
> 

[PATCH v5 3/3] t4140: test apply with i-t-a paths

[Raymond E. Pasco]

apply --cached (as used by add -p) should accept creation and deletion
patches to intent-to-add paths in the index. apply --index, however,
should always fail because an intent-to-add path never matches the
worktree (by definition).

Based-on-patch-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 t/t4140-apply-ita.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100755 t/t4140-apply-ita.sh

diff --git a/t/t4140-apply-ita.sh b/t/t4140-apply-ita.sh
new file mode 100755
index 0000000000..c614eaf04c
--- /dev/null
+++ b/t/t4140-apply-ita.sh
@@ -0,0 +1,56 @@
+#!/bin/sh
+
+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 &&
+
+	rm -f test-file &&
+	git diff >deletion-patch &&
+	grep "deleted file mode 100644" deletion-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 &&
+
+	test_must_fail git apply --index creation-patch
+'
+
+test_expect_success 'apply deletion patch to ita path (--cached)' '
+	git rm -f test-file &&
+	cat blueprint >test-file &&
+	git add -N test-file &&
+
+	git apply --cached deletion-patch &&
+	test_must_fail git ls-files --stage --error-unmatch test-file
+'
+
+test_expect_success 'apply deletion patch to ita path (--index)' '
+	cat blueprint >test-file &&
+	git add -N test-file &&
+
+	test_must_fail git apply --index deletion-patch &&
+	git ls-files --stage --error-unmatch test-file
+'
+
+test_done
-- 
2.28.0.5.gfc8e108108

Re: [PATCH v5 3/3] t4140: test apply with i-t-a paths

[Phillip Wood]

Hi Raymond

On 08/08/2020 08:49, Raymond E. Pasco wrote:
> apply --cached (as used by add -p) should accept creation and deletion
> patches to intent-to-add paths in the index. apply --index, however,
> should always fail because an intent-to-add path never matches the
> worktree (by definition).
> 
> Based-on-patch-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
> ---
>   t/t4140-apply-ita.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 56 insertions(+)
>   create mode 100755 t/t4140-apply-ita.sh
> 
> diff --git a/t/t4140-apply-ita.sh b/t/t4140-apply-ita.sh
> new file mode 100755
> index 0000000000..c614eaf04c
> --- /dev/null
> +++ b/t/t4140-apply-ita.sh
> @@ -0,0 +1,56 @@
> +#!/bin/sh
> +
> +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 &&
> +
> +	rm -f test-file &&
> +	git diff >deletion-patch &&
> +	grep "deleted file mode 100644" deletion-patch

test-file is still i-t-a in the index ...

> +'
> +
> +test_expect_success 'apply creation patch to ita path (--cached)' '
> +	git rm -f test-file &&
> +	cat blueprint >test-file &&
> +	git add -N test-file &&

so 'add -N' does nothing ...

> +
> +	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 &&

If the last test was successful then test-file is already in the index 
and 'add -N' has no effect, 'apply --index' will fail wether or not it 
rejects i-t-a entries.

I think you should fix this by adding

   test_when_finished git read-tree --empty

(or possibly 'git reset' if that works correctly when HEAD is invalid) 
to each test in this file

Best Wishes

Phillip

> +	rm -f test-file &&
> +	test_must_fail git apply --index creation-patch
> +'
> +
> +test_expect_success 'apply deletion patch to ita path (--cached)' '
> +	git rm -f test-file &&
> +	cat blueprint >test-file &&
> +	git add -N test-file &&
> +
> +	git apply --cached deletion-patch &&
> +	test_must_fail git ls-files --stage --error-unmatch test-file
> +'
> +
> +test_expect_success 'apply deletion patch to ita path (--index)' '
> +	cat blueprint >test-file &&
> +	git add -N test-file &&
> +
> +	test_must_fail git apply --index deletion-patch &&
> +	git ls-files --stage --error-unmatch test-file
> +'
> +
> +test_done
> 

[PATCH 1/1] diff-lib: use worktree mode in diffs from i-t-a entries

[Raymond E. Pasco]

When creating "new file" diffs against i-t-a index entries, diff-lib
erroneously used the mode of the cache entry rather than the mode of the
file in the worktree. This changes run_diff_files() to correctly use the
mode of the worktree file in this case.

Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
This is a distinct bugfix from the other changes, so I've sent it as a
separate patch also based on v2.28.0.

 diff-lib.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/diff-lib.c b/diff-lib.c
index 25fd2dee19..50521e2093 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -219,7 +219,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 				continue;
 			} else if (revs->diffopt.ita_invisible_in_index &&
 				   ce_intent_to_add(ce)) {
-				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
+				newmode = ce_mode_from_stat(ce, st.st_mode);
+				diff_addremove(&revs->diffopt, '+', newmode,
 					       &null_oid, 0, ce->name, 0);
 				continue;
 			}
-- 
2.28.0.5.gfc8e108108

Re: [PATCH 1/1] diff-lib: use worktree mode in diffs from i-t-a entries

[Martin Ågren]

On Sat, 8 Aug 2020 at 09:55, Raymond E. Pasco <ray@ameretat.dev> wrote:
>
> When creating "new file" diffs against i-t-a index entries, diff-lib
> erroneously used the mode of the cache entry rather than the mode of the
> file in the worktree. This changes run_diff_files() to correctly use the
> mode of the worktree file in this case.

Good catch!

Describing the current state of affairs and using imperative mode, it
could be something like:

  When creating "new file" diffs against i-t-a index entries, diff-lib
  erroneously uses the mode of the cache entry rather than the mode of
  the file in the worktree. Change run_diff_files() to correctly use the
  mode of the worktree file in this case.

More importantly:

I can confirm that the bug is there before your patch and that your
patch fixes it. Could you add a test in this patch so we can trust that
this stays fixed?

Martin

Re: [PATCH 1/1] diff-lib: use worktree mode in diffs from i-t-a entries

[Raymond E. Pasco]

On Sat Aug 8, 2020 at 4:48 AM EDT, Martin Ågren wrote:
> Describing the current state of affairs and using imperative mode, it
> could be something like:
>
> When creating "new file" diffs against i-t-a index entries, diff-lib
> erroneously uses the mode of the cache entry rather than the mode of
> the file in the worktree. Change run_diff_files() to correctly use the
> mode of the worktree file in this case.

I see both styles around in the tree (past for the state of the world
before this patch, present for the state of the world as of this patch,
vs. present for the state of the world just before and just after
applying this patch). Neither is unreadable to me so I just want to do
whatever's the standard around here.

(I'm not convinced, as a matter of grammar, that the
commit-message-present verb form is really in the imperative mood; I
think the freeform nature of English grammar obscures that it's the
present active infinitive, analogous to, say, the fact that a French
software program with an "open file" button will say "ouvrir" and not
"ouvrez".)

> I can confirm that the bug is there before your patch and that your
> patch fixes it. Could you add a test in this patch so we can trust that
> this stays fixed?

The whole set of i-t-a diffing behaviors needs a test suite (unless I've
grepped very poorly), which will come in another patchset. t4140 in this
thread's main patchset assumes they work.

Re: [PATCH 1/1] diff-lib: use worktree mode in diffs from i-t-a entries

[Martin Ågren]

On Sat, 8 Aug 2020 at 13:24, Raymond E. Pasco <ray@ameretat.dev> wrote:
>
> On Sat Aug 8, 2020 at 4:48 AM EDT, Martin Ågren wrote:
> > Describing the current state of affairs and using imperative mode, it
> > could be something like:
> >
> > When creating "new file" diffs against i-t-a index entries, diff-lib
> > erroneously uses the mode of the cache entry rather than the mode of
> > the file in the worktree. Change run_diff_files() to correctly use the
> > mode of the worktree file in this case.
>
> I see both styles around in the tree (past for the state of the world
> before this patch, present for the state of the world as of this patch,
> vs. present for the state of the world just before and just after
> applying this patch). Neither is unreadable to me so I just want to do
> whatever's the standard around here.

Yeah, there are all kinds of log messages in the history.
SubmittingPatches (search for "imperative") recommends this way of
writing.

> (I'm not convinced, as a matter of grammar, that the
> commit-message-present verb form is really in the imperative mood; I
> think the freeform nature of English grammar obscures that it's the
> present active infinitive, analogous to, say, the fact that a French
> software program with an "open file" button will say "ouvrir" and not
> "ouvrez".)

When you put it that way, I'm also not sure. :-)

> The whole set of i-t-a diffing behaviors needs a test suite (unless I've
> grepped very poorly), which will come in another patchset. t4140 in this
> thread's main patchset assumes they work.

I did grep a little when I wrote my previous reply and I didn't find
anything either.

I guess you could add a very small testcase here and then base that
future series on top of this commit. Or in lieu of a test, maybe this
could be used:

  Tested-by: Martin Ågren <martin.agren@gmail.com>

Martin

Re: [PATCH 1/1] diff-lib: use worktree mode in diffs from i-t-a entries

[Junio C Hamano]

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

> When creating "new file" diffs against i-t-a index entries, diff-lib
> erroneously used the mode of the cache entry rather than the mode of the
> file in the worktree. This changes run_diff_files() to correctly use the
> mode of the worktree file in this case.
>
> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
> ---
> This is a distinct bugfix from the other changes, so I've sent it as a
> separate patch also based on v2.28.0.
>
>  diff-lib.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/diff-lib.c b/diff-lib.c
> index 25fd2dee19..50521e2093 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -219,7 +219,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>  				continue;
>  			} else if (revs->diffopt.ita_invisible_in_index &&
>  				   ce_intent_to_add(ce)) {
> -				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
> +				newmode = ce_mode_from_stat(ce, st.st_mode);
> +				diff_addremove(&revs->diffopt, '+', newmode,
>  					       &null_oid, 0, ce->name, 0);

;-)  

The ita-invisible-in-index option means that Git must ignore
anything in the index about the entry, other than just the fact that
the path is subject to comparison, so use of ce->ce_mode here is
wrong by definition.  Makes sense.

>  				continue;
>  			}

[PATCH] t4069: test diff behavior with i-t-a paths

[Raymond E. Pasco]

Add a small test suite to test the behavior of diff with intent-to-add
paths. Specifically, the diff between an i-t-a entry and a file in the
worktree should be a "new file" diff, and the diff between an i-t-a
entry and no file in the worktree should be a "deleted file" diff.
However, if --ita-visible-in-index is passed, the former should instead
be a diff from the empty blob.

Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 t/t4069-diff-intent-to-add.sh | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 t/t4069-diff-intent-to-add.sh

diff --git a/t/t4069-diff-intent-to-add.sh b/t/t4069-diff-intent-to-add.sh
new file mode 100644
index 0000000000..85c1a35ca7
--- /dev/null
+++ b/t/t4069-diff-intent-to-add.sh
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+test_description='behavior of diff with intent-to-add entries'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	test_write_lines 1 2 3 4 5 >blueprint
+'
+
+test_expect_success 'diff between i-t-a and file should be new file' '
+	cat blueprint >test-file &&
+	git add -N test-file &&
+	git diff >output &&
+	grep "new file mode 100644" output
+'
+
+test_expect_success 'diff between i-t-a and no file should be deletion' '
+	rm -f test-file &&
+	git diff >output &&
+	grep "deleted file mode 100644" output
+'
+
+test_expect_success '--ita-visible-in-index diff should be from empty blob' '
+	cat blueprint >test-file &&
+	git diff --ita-visible-in-index >output &&
+	grep "index e69de29" output
+'
+
+test_done
-- 
2.28.0.3.gc4aba908ca

[PATCH] diff-lib: use worktree mode in diffs from i-t-a entries

[Raymond E. Pasco]

When creating "new file" diffs against i-t-a index entries, diff-lib
erroneously uses the placeholder mode in the cache entry rather than the
mode of the file in the worktree.

Change run_diff_files() to correctly use the mode of the worktree file
in this case, and add a test verifying that diff uses the worktree mode.

Tested-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
Changed wording based on suggestion from Martin Ågren, although if
someone else shows up and says the other style is preferred I will be
cross.

The addition of a test makes this patch based on the patch I sent just
now creating the test file, which is itself based on v2.28.0.

 diff-lib.c                    | 3 ++-
 t/t4069-diff-intent-to-add.sh | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/diff-lib.c b/diff-lib.c
index 25fd2dee19..50521e2093 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -219,7 +219,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 				continue;
 			} else if (revs->diffopt.ita_invisible_in_index &&
 				   ce_intent_to_add(ce)) {
-				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
+				newmode = ce_mode_from_stat(ce, st.st_mode);
+				diff_addremove(&revs->diffopt, '+', newmode,
 					       &null_oid, 0, ce->name, 0);
 				continue;
 			}
diff --git a/t/t4069-diff-intent-to-add.sh b/t/t4069-diff-intent-to-add.sh
index 85c1a35ca7..cdb41ba89d 100644
--- a/t/t4069-diff-intent-to-add.sh
+++ b/t/t4069-diff-intent-to-add.sh
@@ -15,6 +15,12 @@ test_expect_success 'diff between i-t-a and file should be new file' '
 	grep "new file mode 100644" output
 '
 
+test_expect_success 'new file diff should use worktree mode' '
+        chmod 755 test-file &&
+        git diff >output &&
+        grep "new file mode 100755" output
+'
+
 test_expect_success 'diff between i-t-a and no file should be deletion' '
 	rm -f test-file &&
 	git diff >output &&
-- 
2.28.0.3.gc4aba908ca

[RESEND PATCH v2] diff-lib: use worktree mode in diffs from i-t-a entries

[Raymond E. Pasco]

When creating "new file" diffs against i-t-a index entries, diff-lib
erroneously uses the placeholder mode in the cache entry rather than the
mode of the file in the worktree.

Change run_diff_files() to correctly use the mode of the worktree file
in this case, and add a test verifying that diff uses the worktree mode.

Tested-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
Whoops - this is a PATCH v2 (passed -v2 on the wrong command line).
Apologies!

 diff-lib.c                    | 3 ++-
 t/t4069-diff-intent-to-add.sh | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/diff-lib.c b/diff-lib.c
index 25fd2dee19..50521e2093 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -219,7 +219,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 				continue;
 			} else if (revs->diffopt.ita_invisible_in_index &&
 				   ce_intent_to_add(ce)) {
-				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
+				newmode = ce_mode_from_stat(ce, st.st_mode);
+				diff_addremove(&revs->diffopt, '+', newmode,
 					       &null_oid, 0, ce->name, 0);
 				continue;
 			}
diff --git a/t/t4069-diff-intent-to-add.sh b/t/t4069-diff-intent-to-add.sh
index 85c1a35ca7..cdb41ba89d 100644
--- a/t/t4069-diff-intent-to-add.sh
+++ b/t/t4069-diff-intent-to-add.sh
@@ -15,6 +15,12 @@ test_expect_success 'diff between i-t-a and file should be new file' '
 	grep "new file mode 100644" output
 '
 
+test_expect_success 'new file diff should use worktree mode' '
+        chmod 755 test-file &&
+        git diff >output &&
+        grep "new file mode 100755" output
+'
+
 test_expect_success 'diff between i-t-a and no file should be deletion' '
 	rm -f test-file &&
 	git diff >output &&
-- 
2.28.0.3.gc4aba908ca

Re: [PATCH] t4069: test diff behavior with i-t-a paths

[Junio C Hamano]

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

> Add a small test suite to test the behavior of diff with intent-to-add
> paths. Specifically, the diff between an i-t-a entry and a file in the
> worktree should be a "new file" diff, and the diff between an i-t-a
> entry and no file in the worktree should be a "deleted file" diff.
> However, if --ita-visible-in-index is passed, the former should instead
> be a diff from the empty blob.
>
> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
> ---
>  t/t4069-diff-intent-to-add.sh | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 t/t4069-diff-intent-to-add.sh

It indeed is that "add -N" appears only once in our test suite and
tests around it is lacking, but I'd prefer to see i-t-a to be taken
as just one of the normal things by not adding a special test for
it.  I wonder if there is a reason why these are not part of say
t4013 (diff-various)?

By adjusting and adding to existing test, we'd avoid a mistake of
adding a test script that is not executable (didn't your "make
DEVELOPER=1 test" catch the error?) ;-)

Thanks.


> diff --git a/t/t4069-diff-intent-to-add.sh b/t/t4069-diff-intent-to-add.sh
> new file mode 100644
> index 0000000000..85c1a35ca7
> --- /dev/null
> +++ b/t/t4069-diff-intent-to-add.sh
> @@ -0,0 +1,30 @@
> +#!/bin/sh
> +
> +test_description='behavior of diff with intent-to-add entries'
> +
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +	test_write_lines 1 2 3 4 5 >blueprint
> +'
> +
> +test_expect_success 'diff between i-t-a and file should be new file' '
> +	cat blueprint >test-file &&
> +	git add -N test-file &&
> +	git diff >output &&
> +	grep "new file mode 100644" output
> +'
> +
> +test_expect_success 'diff between i-t-a and no file should be deletion' '
> +	rm -f test-file &&
> +	git diff >output &&
> +	grep "deleted file mode 100644" output
> +'
> +
> +test_expect_success '--ita-visible-in-index diff should be from empty blob' '
> +	cat blueprint >test-file &&
> +	git diff --ita-visible-in-index >output &&
> +	grep "index e69de29" output
> +'
> +
> +test_done

Re: [PATCH] t4069: test diff behavior with i-t-a paths

[Eric Sunshine]

On Mon, Aug 10, 2020 at 4:54 AM Raymond E. Pasco <ray@ameretat.dev> wrote:
> Add a small test suite to test the behavior of diff with intent-to-add
> paths. Specifically, the diff between an i-t-a entry and a file in the
> worktree should be a "new file" diff, and the diff between an i-t-a
> entry and no file in the worktree should be a "deleted file" diff.
> However, if --ita-visible-in-index is passed, the former should instead
> be a diff from the empty blob.
>
> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
> ---
> diff --git a/t/t4069-diff-intent-to-add.sh b/t/t4069-diff-intent-to-add.sh
> @@ -0,0 +1,30 @@
> +test_expect_success 'diff between i-t-a and file should be new file' '
> +       cat blueprint >test-file &&
> +       git add -N test-file &&
> +       git diff >output &&
> +       grep "new file mode 100644" output
> +'

If someone comes along and inserts new tests above this one and those
new tests make their own changes to the index or worktree, how can
this test be sure that the "new file mode" line is about 'test-file'
rather than some other entry? It might be better to tighten this test,
perhaps like this:

    git diff -- test-file &&

Same comment applies to the other tests.

> +test_expect_success 'diff between i-t-a and no file should be deletion' '
> +       rm -f test-file &&
> +       git diff >output &&
> +       grep "deleted file mode 100644" output
> +'
> +
> +test_expect_success '--ita-visible-in-index diff should be from empty blob' '
> +       cat blueprint >test-file &&
> +       git diff --ita-visible-in-index >output &&
> +       grep "index e69de29" output
> +'

The hard-coded SHA-1 value in the "index" line is going to cause the
test to fail when the test suite is configured to run with SHA-256.
You could fix it by preparing two hash values -- one for SHA-1 and one
for SHA-256 -- and then looking up the value with test_oid() for use
with grep. On the other hand, if you're not interested in the exact
value, but care only that _some_ hash value is present, then you could
just grep for a hex-string.

But what is this test actually checking? In my experiments, this grep
expression will also successfully match the output from the test
preceding this one, which means that the conditions of this test are
too loose.

To tighten this test, perhaps it makes sense to take a different
approach and check the exact output rather than merely grepping for a
particular string. In other words, something like this might be better
(typed in email, so untested):

    cat >expect <<-\EOF &&
    diff --git a/test-file b/test-file
    index HEX..HEX HEX
    --- a/test-file
    +++ b/test-file
    EOF
    cat blueprint >test-file &&
    git diff --ita-visible-in-index -- test-file >raw &&
    sed "s/[0-9a-f][0-9a-f]*/HEX/g' raw >actual &&
    test_cmp expect actual

In fact, this likely would be a good model to use for all the tests.

Re: [PATCH] t4069: test diff behavior with i-t-a paths

[Eric Sunshine]

On Mon, Aug 10, 2020 at 12:23 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> To tighten this test, perhaps it makes sense to take a different
> approach and check the exact output rather than merely grepping for a
> particular string. In other words, something like this might be better
> (typed in email, so untested):
>
>     cat >expect <<-\EOF &&
>     diff --git a/test-file b/test-file
>     index HEX..HEX HEX
>     --- a/test-file
>     +++ b/test-file
>     EOF
>     cat blueprint >test-file &&
>     git diff --ita-visible-in-index -- test-file >raw &&
>     sed "s/[0-9a-f][0-9a-f]*/HEX/g' raw >actual &&
>     test_cmp expect actual

This can be improved by taking advantage of the OID_REGEX variable
defined by the test suite for matching an OID. So something like this
would be even better:

    cat >expect <<-\EOF &&
    diff --git a/test-file b/test-file
    index OID..OID 100644
    --- a/test-file
    +++ b/test-file
    EOF
    cat blueprint >test-file &&
    git diff --ita-visible-in-index -- test-file >raw &&
    sed "s/$OID_REGEX/OID/g" raw >actual &&
    test_cmp expect actual

Re: [PATCH] t4069: test diff behavior with i-t-a paths

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Aug 10, 2020 at 12:23 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>> To tighten this test, perhaps it makes sense to take a different
>> approach and check the exact output rather than merely grepping for a
>> particular string. In other words, something like this might be better
>> (typed in email, so untested):
>>
>>     cat >expect <<-\EOF &&
>>     diff --git a/test-file b/test-file
>>     index HEX..HEX HEX
>>     --- a/test-file
>>     +++ b/test-file
>>     EOF
>>     cat blueprint >test-file &&
>>     git diff --ita-visible-in-index -- test-file >raw &&
>>     sed "s/[0-9a-f][0-9a-f]*/HEX/g' raw >actual &&
>>     test_cmp expect actual
>
> This can be improved by taking advantage of the OID_REGEX variable
> defined by the test suite for matching an OID. So something like this
> would be even better:
>
>     cat >expect <<-\EOF &&
>     diff --git a/test-file b/test-file
>     index OID..OID 100644
>     --- a/test-file
>     +++ b/test-file
>     EOF
>     cat blueprint >test-file &&
>     git diff --ita-visible-in-index -- test-file >raw &&
>     sed "s/$OID_REGEX/OID/g" raw >actual &&
>     test_cmp expect actual

OID_REGEX is [0-9a-f]{40} while what is used here is [0-9a-f]{1,}.
Unless --full-index is in use, they mean different things, no?

Re: [PATCH] t4069: test diff behavior with i-t-a paths

[Eric Sunshine]

On Mon, Aug 10, 2020 at 6:09 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > This can be improved by taking advantage of the OID_REGEX variable
> > defined by the test suite for matching an OID. So something like this
> > would be even better:
> >
> >     cat >expect <<-\EOF &&
> >     diff --git a/test-file b/test-file
> >     index OID..OID 100644
> >     --- a/test-file
> >     +++ b/test-file
> >     EOF
> >     cat blueprint >test-file &&
> >     git diff --ita-visible-in-index -- test-file >raw &&
> >     sed "s/$OID_REGEX/OID/g" raw >actual &&
> >     test_cmp expect actual
>
> OID_REGEX is [0-9a-f]{40} while what is used here is [0-9a-f]{1,}.
> Unless --full-index is in use, they mean different things, no?

You're right, of course. The regex in the original example I gave was
too loose, matching even single hex letters in words like "index" and
"test-file", so I wanted to tighten it up, but I botched it with
OID_REGEX. Anyhow, I hope my examples convey the general idea (which
needs a bit of tweaking from what I showed).

Re: [PATCH] t4069: test diff behavior with i-t-a paths

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Aug 10, 2020 at 6:09 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>> > This can be improved by taking advantage of the OID_REGEX variable
>> > defined by the test suite for matching an OID. So something like this
>> > would be even better:
>> >
>> >     cat >expect <<-\EOF &&
>> >     diff --git a/test-file b/test-file
>> >     index OID..OID 100644
>> >     --- a/test-file
>> >     +++ b/test-file
>> >     EOF
>> >     cat blueprint >test-file &&
>> >     git diff --ita-visible-in-index -- test-file >raw &&
>> >     sed "s/$OID_REGEX/OID/g" raw >actual &&
>> >     test_cmp expect actual
>>
>> OID_REGEX is [0-9a-f]{40} while what is used here is [0-9a-f]{1,}.
>> Unless --full-index is in use, they mean different things, no?
>
> You're right, of course. The regex in the original example I gave was
> too loose, matching even single hex letters in words like "index" and
> "test-file", so I wanted to tighten it up, but I botched it with
> OID_REGEX. Anyhow, I hope my examples convey the general idea (which
> needs a bit of tweaking from what I showed).

Yeah, perhaps along the lines of

HEX_RX="_x05*"
sed -e "s/index $HEX_RX..$HEX_RX /index OID..OID /"

Re: [PATCH] t4069: test diff behavior with i-t-a paths

[Raymond E. Pasco]

On Mon Aug 10, 2020 at 12:23 PM EDT, Eric Sunshine wrote:
> The hard-coded SHA-1 value in the "index" line is going to cause the
> test to fail when the test suite is configured to run with SHA-256.
> You could fix it by preparing two hash values -- one for SHA-1 and one
> for SHA-256 -- and then looking up the value with test_oid() for use
> with grep. On the other hand, if you're not interested in the exact
> value, but care only that _some_ hash value is present, then you could
> just grep for a hex-string.

Loosely, all this test cares about is that it's not "index 0000000"; or
perhaps, taking another tack, that it doesn't have a "new file" line.

More rigidly, it's nice to confirm that it's a diff from the empty blob
and not from some other random blob.

Re: [PATCH] t4069: test diff behavior with i-t-a paths

[Eric Sunshine]

On Mon, Aug 10, 2020 at 7:09 PM Raymond E. Pasco <ray@ameretat.dev> wrote:
> More rigidly, it's nice to confirm that it's a diff from the empty blob
> and not from some other random blob.

The tests can get access to the correct OID of the empty blob without
hardcoding it either via $(test_oid empty_blob) or just as
$EMPTY_BLOB, however, that's the full length hex string, so you'd
still need to do some tweaking.

Re: [PATCH] t4069: test diff behavior with i-t-a paths

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Aug 10, 2020 at 7:09 PM Raymond E. Pasco <ray@ameretat.dev> wrote:
>> More rigidly, it's nice to confirm that it's a diff from the empty blob
>> and not from some other random blob.
>
> The tests can get access to the correct OID of the empty blob without
> hardcoding it either via $(test_oid empty_blob) or just as
> $EMPTY_BLOB, however, that's the full length hex string, so you'd
> still need to do some tweaking.

If the tests do truly care about these exact objects, then using
"--full-index" would be a good way to compare with $EMPTY_BLOB
and/or $ZERO_OID and such.

[HYPOTHETICAL PATCH 0/2] apply: reject modification diffs to i-t-a entries

[Raymond E. Pasco]

This is based on the main patchset of this thread, and makes apply
reject diffs other than creation or deletion diffs to i-t-a entries. I
think that as long as --ita-visible-in-index exists as an option this
probably doesn't make sense to merge into the tree - I am just including
it for the record.

Raymond E. Pasco (2):
  apply: reject modification diffs to i-t-a entries
  t4140: test failure of diff from empty blob to i-t-a path

 apply.c              |  3 +++
 t/t4140-apply-ita.sh | 16 +++++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

-- 
2.28.0.5.gfc8e108108

[HYPOTHETICAL PATCH 1/2] apply: reject modification diffs to i-t-a entries

[Raymond E. Pasco]

Because the correct diff between an i-t-a entry and a new file is now a
creation diff, reject diffs from the empty blob as not applying to the
preimage.

Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 apply.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/apply.c b/apply.c
index c5ecb64102..656f00c113 100644
--- a/apply.c
+++ b/apply.c
@@ -3637,6 +3637,9 @@ static int apply_data(struct apply_state *state, struct patch *patch,
 	if (load_preimage(state, &image, patch, st, ce) < 0)
 		return -1;
 
+	if (!(patch->is_new || patch->is_delete) && ce->ce_flags & CE_INTENT_TO_ADD)
+		return -1;
+
 	if (patch->direct_to_threeway ||
 	    apply_fragments(state, &image, patch) < 0) {
 		/* Note: with --reject, apply_fragments() returns 0 */
-- 
2.28.0.5.gfc8e108108

[HYPOTHETICAL PATCH 2/2] t4140: test failure of diff from empty blob to i-t-a path

[Raymond E. Pasco]

"git apply" should only accept new file or deleted file patches to an
i-t-a index entry.

Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
---
 t/t4140-apply-ita.sh | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/t/t4140-apply-ita.sh b/t/t4140-apply-ita.sh
index c614eaf04c..898dd251b4 100755
--- a/t/t4140-apply-ita.sh
+++ b/t/t4140-apply-ita.sh
@@ -14,7 +14,13 @@ test_expect_success setup '
 
 	rm -f test-file &&
 	git diff >deletion-patch &&
-	grep "deleted file mode 100644" deletion-patch
+	grep "deleted file mode 100644" deletion-patch &&
+
+	git rm -f test-file &&
+	touch test-file &&
+	git add test-file &&
+	cat blueprint >test-file &&
+	git diff >incorrect-patch
 '
 
 test_expect_success 'apply creation patch to ita path (--cached)' '
@@ -27,6 +33,14 @@ test_expect_success 'apply creation patch to ita path (--cached)' '
 	test_cmp blueprint actual
 '
 
+test_expect_success 'apply diff from empty blob to ita path (--cached)' '
+	git rm -f test-file &&
+	cat blueprint >test-file &&
+	git add -N test-file &&
+
+	test_must_fail git apply --cached incorrect-patch
+'
+
 test_expect_success 'apply creation patch to ita path (--index)' '
 	git rm -f test-file &&
 	cat blueprint >test-file &&
-- 
2.28.0.5.gfc8e108108