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

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