Re: [PATCH v4] git-apply: apply submodule changes

Sven Verdoolaege [off-list ref] writes:

In this codepath, we know the patch wants to either modify the
path at old_name or remove old_name.  If we are going to affect
the work tree, we have run lstat on it, and ran checkout_entry() 
if we did not have anything there and did lstat() again.

I think the check "S_ISGITLINK(patch->old_mode)" is wrong
(that's where my confusion while reading your patch came from).
It has to check ce's mode, not patch->old_mode, because we are
verifying if the index matches with the work tree in this
codepath.  If you fix it to S_ISGITLINK(ntohl(ce->ce_mode)),
I think I can see what you are trying to do.

When ce is not a gitlink, you keep the original behaviour, which
is assuring that you did not break things for people who do not
use gitlink.

I am still having trouble with the TYPE_CHANGED bits.  You
discard everything other than TYPE_CHANGED, and 

 - if ce_match_stat() returned TYPE_CHANGED, then that is given
   to later processing to cause us to fail "oops, path is not up
   to date";

 - if ce_match_stat() did not return TYPE_CHANGED, that means we
   found a directory at the path (ce_match_stat_basic() says
   so).  In such a case you call verify_gitlink_clean(), but it
   essentially says "make sure there is either an empty
   directory or some repository".  Maybe we do not even have to
   have this extra check?

When ce is a gitlink, ce_match_stat() says DATA_CHANGED if the
commit in the work tree of the subproject is different.  From
the earlier discussions, we do want to discard DATA_CHANGED for
this codepath.

So it looks almost Ok after spending a few days looking at this
code.  Finally.

However, if it takes _me_ three days to understand this hunk,
(admittably, the parameter to S_ISGITLINK() completely confused
me originally, and I also had other things to do, so it was not
"72 hours"), I do not think the code with your patch is
maintainable by anybody.  At least we would need to have a few
words of comment to describe what is going on there.

	if (!cached) {
        	changed = ce_match_stat(ce, &st, 1);
                if (S_ISGITLINK(ntohl(ce->ce_mode)))
                	/*
			 * ce_match_stat() reports the
			 * difference between the commit object
                         * name in the index and what is checked
			 * out in the work tree of subproject;
                         * because we do not recurse, we do not
			 * want to insist on them matching with
                         * each other.
                         */
                	changed &= ~DATA_CHANGED;
	}
        if (changed)
        	return error("%s: does not match index", old_name);