Re: [PATCH] apply: fix adding new files on i-t-a entries

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

Since d95d728 (diff-lib.c: adjust position of i-t-a entries in diff -
2015-03-16), a normal "git diff" on i-t-a entries would produce a diff
that _adds_ those files, not just adding content to existing and empty
files like before.

This is correct. Unfortunately, applying such a patch on the same
repository that has the same i-t-a entries will fail with message
"already exists in index" because git-apply checks, sees those i-t-a
entries and aborts. git-apply does not realize those are for
bookkeeping only, they do not really exist in the index.

This patch tightens the "exists in index" check, ignoring i-t-a
entries. For fixing the above problem, only the change in
check_to_create() is needed.

And the first thing I noticed and found a bit disturbing was that
this change (which I think is correct, and happens to match what I
sent out earlier) was the only thing necessary to make your new test
pass.  IOW, the other changes in this patch have no test coverage.

For other changes,

 - load_current(), reporting "not exists in index" is better than "does
   not match index"

Is that error reporting the only side effect from this change?

This is only used when falling back to three-way merge while
applying a creation patch.

 - check_preimage(), similar to load_current(), but it may also use
   ce_mode from i-t-a entry which is always zero

This is for the normal (non three-way) application and the idea is
the same as load_current() as you said above.

 - get_current_sha1(), or actually build_fake_ancestor(), we should not
   add i-t-a entries to the temporary index, at least not without also
   adding i-t-a flag back

This is part of "am" three-way fallback codepath.  I do not think
the merge-recursive three-way merge code knows and cares about, is
capable of handling, or would even want to deal with i-t-a entries
in the first place, so adding an entry as i-t-a bit would not help.
What the ultimate caller wants from us in this codepath is a tree
object, and that is written out from the temporary index---and that
codepath ignores i-t-a entries, so it is correct to omit them from
the temporary index in the first place.  Unlike the previous two
changes, I think this change deserves a new test.

 I think I'm opening a can of worms with d95d728. There's nothing
 wrong with that patch per se, but with this issue popping up, I need
 to go over all {cache,index}_name_pos call sites and see what would be
 the sensible behavior when i-t-a entries are involved.

Yeah, I agree that d95d728 should have been a part of a larger
series that changes the world order, instead of a single change that
brings inconsistency to the system.

I cannot offhand convince myself that "apply" is the only casualty;
assuming it is, I think a reasonable way forward is to keep d95d728
and adjust "apply" to the new world order.  Otherwise, i.e. if there
are wider fallouts from d95d728, we may instead want to temporarily
revert it off from 'master', deal with fallouts to "apply" and other
things, before resurrecting it.

Anything that internally uses "diff-index" is suspect, I think.

What do others think?  You seem to ...

 So far blame, rm and checkout-entry and "checkout <paths>" are on my
 to-think-or-fix list. But this patch can get in early to fix a real
 regression instead of waiting for one big series. A lot more
 discussions will be had before that series gets in good shape.

... think that the damage could be quite extensive, so I am inclined
to say that we first revert d95d728 before going forward.

Something that is named as if it would return yes/no that returns a
real value is not a very welcome idea.

+/* This is the same as index_name_pos, except that i-t-a entries are invisible */
+int exists_in_index(const struct index_state *istate, const char *name, int namelen)
+{
+	int pos = index_name_stage_pos(istate, name, namelen, 0);
+
+	if (pos < 0)
+		return pos;
+	if (istate->cache[pos]->ce_flags & CE_INTENT_TO_ADD)
+		return -pos-1;

This is a useless complexity.  Your callers cannot use the returned
value like this:

	pos = exists_in_cache(...);
        if (pos < 0) {
        	if (active_cache[-pos-1]->ce_flags & CE_INTENT_TO_ADD)
			; /* ah it actually exists but it is i-t-a */
		else
                        ; /* no it does not really exist */
	} else {
        	; /* yes it is really there at pos */
	}

because they cannot tell two cases apart: (1) you do have i-t-a with
the given name, (2) you do not have the entry but the location you
would insert an entry with such a name is occupied by an unrelated
entry (i.e. with a name that sorts adjacent) that happens to be
i-t-a.

+	return pos;
+}