On Thu, Apr 04, 2013 at 01:31:43PM -0700, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> writes: > > >> If you do this: > >> > >> rm -fr d e > >> mkdir e > >> >e/f > >> ln -s e d > >> git add d/f > >> > >> we do complain that d/f is beyond a symlink (meaning that all you > >> can add is the symlink d that may happen to point at something). > > > > Right, but that is because you are adding a bogus entry to the index; we > > cannot have both 'd' as a symlink and 'd/f' as a path in our git tree. > > But in the removal case, the index manipulation is perfectly reasonable. > > I think you misread me. I am not adding 'd' as a symlink at all. > IIRC, ancient versions of Git got this case wrong and added d/f to > the index, which we later fixed. I think I just spoke sloppily. What is bogus about "d/f" is not that "d" is necessarily in the index right now, but that adding "d/f" implies that "d" is a tree, which it clearly is not. Git maps filesystem symlinks into the index and into its trees without dereferencing them. So whether we have "d" in the index right now or not, "d/f" is wrong conceptually. But I do not think the "we are mis-representing the filesystem" problem applies to this "rm" case. We are not adding anything bogus into the index; on the contrary, we are deleting something that no longer matches the filesystem representation (and is actually the same bogosity that we avoid adding under the rule above). I do agree that it violates git's general behavior with symlinks (i.e., that they are not dereferenced). > I have been hinting that we should do the same safety not to touch > (even compare the contents of) e/f, because the only reason we even > look at it is because it appears beyond a symbolic link 'd'. I can certainly see the safety argument that crossing a symlink at "d" means the resulting "d/f" is not necessarily related to the original "d/f" that is in the index. As I said, I do not mind having the extra protection; my argument was only that the content-check already protects us, so the extra protection is not necessary. And the implication is that I do not feel like working on it. :) I do not mind at all if you drop my third patch (and that is part of the reason I split it out from patch 2, which I do think is a no-brainer), and I am happy to review patches to do the symlink check if you want to work on it. Having made the argument that the content-check is enough, though, I think there is an interesting corner case where it might not be. I don't mind "git rm d/f" deleting "e/f" inside the repository when "d" is a symlink to "e". But what would happen with: rm -rf d ln -s /path/outside/repo d git rm d/f Deleting across symlinks inside the repo can be brushed aside with "eh, well, it is just another way to mention the same name in the filesystem". But deleting anything outside of the repo seems actively wrong. And more on that "brushed aside". I think it is easy in the cases we have been talking about, namely where "d/f" still exists in the index, to think that "git rm d/f" is useful and the question is one of safety: should we delete e/f if it is pointed to? But let us imagine that d/f is _not_ in the index, but "d" is a symlink pointing to some/long/path". The user wants to be lazy and say "git rm d/f", because typing "some/long/path" is too much work. But what happens to the index? We should probably not be removing "some/long/path". Hmm. I think you have convinced me (or perhaps I have convinced myself) that we should generally not be crossing symlink boundaries in translating names between the filesystem and index. I still don't want to work on it, though. :) -Peff -- 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