Sven Verdoolaege <skimo@xxxxxxxxxx> writes:
> @@ -2096,8 +2142,15 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
> lstat(old_name, &st))
> return -1;
> }
> - if (!cached)
> + if (!cached) {
> changed = ce_match_stat(ce, &st, 1);
> + if (S_ISGITLINK(patch->old_mode)) {
> + changed &= TYPE_CHANGED;
> + if (!changed &&
> + verify_gitlink_clean(patch->old_name))
> + changed |= TYPE_CHANGED;
> + }
> + }
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);
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
