patchapply: Allow "new file" patches on i-t-a entries

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(-)

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

Please downcase "A"llow.

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

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.

On Tue Aug 4, 2020 at 3:30 PM EDT, Junio C Hamano wrote:

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.

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-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(-)

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

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(-)

On Tue Aug 4, 2020 at 7:49 PM EDT, Junio C Hamano wrote:

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.

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

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(-)

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(+)

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

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(-)

On Thu Aug 6, 2020 at 5:00 PM EDT, Junio C Hamano wrote:

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

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

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

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

chmod +x

OK, this is a good positive test case.

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

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?

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

OK, looking good so far.

Thanks.

On Thu Aug 6, 2020 at 5:07 PM EDT, Junio C Hamano wrote:

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.)

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

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(-)

Hi Raymond

On 08/08/2020 08:49, Raymond E. Pasco wrote:

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

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(-)

Hi Raymond

On 08/08/2020 08:49, Raymond E. Pasco wrote:

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

On Sat Aug 8, 2020 at 9:46 AM EDT, Phillip Wood wrote:

"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.

--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.)

--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.)

Hi Raymond

On 08/08/2020 15:07, Raymond E. Pasco wrote:

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.

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

Thanks

Phillip

On Sat Aug 8, 2020 at 11:48 AM EDT, Phillip Wood wrote:

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.

"--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.

Hi Raymond

On 08/08/2020 16:58, Raymond E. Pasco wrote:

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'

Thanks for clarifying that

Best Wishes

Phillip

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

Well explained.  Thanks.

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(-)

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

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(-)

On 10/08/2020 17:18, Junio C Hamano wrote:

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

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

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

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.

On Wed Aug 12, 2020 at 3:23 PM EDT, Junio C Hamano wrote:

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.

On 10/08/2020 17:18, Junio C Hamano wrote:

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

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

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

Hi Raymond

On 08/08/2020 08:49, Raymond E. Pasco wrote:

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

so 'add -N' does nothing ...

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

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(-)

On Sat, 8 Aug 2020 at 09:55, Raymond E. Pasco <ray@ameretat.dev> wrote:

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

On Sat Aug 8, 2020 at 4:48 AM EDT, Martin Ågren wrote:

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".)

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.

On Sat, 8 Aug 2020 at 13:24, Raymond E. Pasco <ray@ameretat.dev> wrote:

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

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

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

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

;-)  

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.

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

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(-)

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(-)

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

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.

On Mon, Aug 10, 2020 at 4:54 AM Raymond E. Pasco <ray@ameretat.dev> wrote:

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.

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.

On Mon, Aug 10, 2020 at 12:23 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

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

Eric Sunshine <sunshine@sunshineco.com> writes:

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?

On Mon, Aug 10, 2020 at 6:09 PM Junio C Hamano <gitster@pobox.com> wrote:

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).

Eric Sunshine <sunshine@sunshineco.com> writes:

Yeah, perhaps along the lines of

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

On Mon Aug 10, 2020 at 12:23 PM EDT, Eric Sunshine wrote:

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.

On Mon, Aug 10, 2020 at 7:09 PM Raymond E. Pasco <ray@ameretat.dev> wrote:

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.

Eric Sunshine <sunshine@sunshineco.com> writes:

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.

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(-)

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(+)

"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(-)