Re: apply --cached --whitespace=fix now failing on items added with "add -N"

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

Duy Nguyen <pclouds@gmail.com> writes:

> On Mon, Jun 22, 2015 at 9:29 PM, Patrick Higgins <phiggins@google.com> wrote:
>> I like to use git to remove trailing whitespace from my files. I use
>> the following ~/.gitconfig to make this convenient:
>>
>> [alias]
>>         wsadd = "!sh -c 'git diff -- \"$@\" | git apply --cached
>> --whitespace=fix;\
>>                 git checkout -- ${1-.} \"$@\"' -"
>>
>> The wsadd alias doesn't work with new files, so I have to use "git add
>> -N" on them first. As of a week or two ago, the "apply --cached" step
>> now fails with the following, assuming a new file named bar containing
>> "foo " has been added with "add -N":
>>
>> $ git diff -- "$@" | git apply --cached --whitespace=fix
>> <stdin>:7: trailing whitespace.
>> foo
>> error: bar: already exists in index
>>
>> The final "git checkout" at the end of wsadd truncates my file. I've
>> changed the ";" to a "&&" to fix the truncation.
>>
>> Were there any recent changes to "git apply" that might have caused this?
>
> Probably fallout from this one, merged to 'master' about 5 weeks ago.
> I'll have a look at it tomorrow unless somebody does it first
>
> d95d728 (diff-lib.c: adjust position of i-t-a entries in diff - 2015-03-16)

Yeah, the world order has changed by that commit, and I would expect
to see some more fallouts.

After "add -N", "git diff" used to pretend that an empty blob was in
the index, and showed a modification between an existing "empty" and
the full file contents, and "git apply --cached" was happy to take
that modification.  In the new world order, "git diff" is instructed
to pretend as if the path that was added by "add -N" does not exist
yet in the index at all, but the index still knows about the path,
which is a strange half-born state.  It shows an addition of the
full file contents as a new file.  "git apply --cached" sees an
addition of a new path, which requires that the path does not exist
in the index.  In the new world order, it should be taught to
pretend that these "add -N" paths do not exist in the index, but
that was not done.

Something like the attached (totally untested) may be a good
starting point.

For another potential fallout, try this:

    $ cp COPYING RENAMING
    $ cp COPYING UNTRACKED
    $ >EMPTY
    $ git add -N RENAMING EMPTY
    $ git rm UNTRACKED
    fatal: pathspec 'UNTRACKED' did not match any files
    $ git rm EMPTY RENAMING
    error: the following file has staged content different from both the
    file and the HEAD:
        EMPTY
        RENAMING
    (use -f to force removal)

One could argue that these three should behave the same way, if the
new world order is "path added by 'add -N' does not exist in the
index".

I however think the new world order should be slightly different
from "does not exist in the index", but should be more like "the
index is aware of its presence but has not been told about its
contents yet".  The consequences of this are:

 - "git rm RENAMING" shouldn't say 'did not match any files'.

 - "git rm RENAMING" does not know about 'staged content', so
   complaining about "staged contents different from file and HEAD"
   feels wrong.

Having said that, I do think erroring out and requiring -f is the
right thing when remiving RENAMING and EMPTY.  Unlike contents added
by "git add" without "-N", we do not know what is in the working
tree file at all.  Given that we check and refuse when the contents
are different between the working tree file, the index and the HEAD
even when we know what was in the working tree when "git add"
without "-N" was done, we should keep the safety to prevent
accidental removal of the working tree file, which has the only
existing copy of the user content.

On the other hand, I am also OK if the behaviour was like this:

    $ git rm --cached RENAMING
    ... removed without complaints ...
    $ git rm RENAMING
    error: file 'RENAMING' does not have staged content yet.
    (use -f to force removal)

In any case, here is a "how about this" weather-balloon patch for
"git apply"

 builtin/apply.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 146be97..653191e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3546,12 +3546,23 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 #define EXISTS_IN_INDEX 1
 #define EXISTS_IN_WORKTREE 2
 
+static int exists_in_index(const char *new_name)
+{
+	int pos = cache_name_pos(new_name, strlen(new_name));
+
+	if (pos < 0)
+		return 0;
+	if (active_cache[pos]->ce_flags & CE_INTENT_TO_ADD)
+		return 0;
+	return 1;
+}
+
 static int check_to_create(const char *new_name, int ok_if_exists)
 {
 	struct stat nst;
 
 	if (check_index &&
-	    cache_name_pos(new_name, strlen(new_name)) >= 0 &&
+	    exists_in_index(new_name) &&
 	    !ok_if_exists)
 		return EXISTS_IN_INDEX;
 	if (cached)