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

Sven Verdoolaege <skimo@kotnet.org> 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);