On Thu, Apr 04, 2013 at 12:42:54PM -0700, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> writes: > > > +test_expect_success SYMLINKS 'replace dir with symlink to dir (same content)' ' > > + git reset --hard && > > + rm -rf d e && > > + mkdir e && > > + echo content >e/f && > > + ln -s e d && > > + git rm d/f && > > + test_must_fail git rev-parse --verify :d/f && > > + test -h d && > > + test_path_is_dir e > > +' > > This does not check if e/f still exists in the working tree, and I > suspect "git rm d/f" removes it. I guess I should have been more clear in my test; I think it _should_ be removed (and it is). You do not necessarily care that "d" is now the symlink and not the actual path; it is safe to remove d/f even though it is behind a symlink now, because it has the exact same content that it had before (it is of course important that we still remove the actual d/f index entry, but as far as the working tree goes, we only care that it is safe to remove, and that we remove it). IOW, I should have been more explicit like this: diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index 9eaec08..3b51a63 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -687,7 +687,8 @@ test_expect_success SYMLINKS 'replace dir with symlink to dir (same content)' ' git rm d/f && test_must_fail git rev-parse --verify :d/f && test -h d && - test_path_is_dir e + test_path_is_dir e && + test_path_is_missing e/f ' test_expect_success SYMLINKS 'replace dir with symlink to dir (new content)' ' > 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. You are deleting the existing "d/f" entry. The only confusion comes from the fact that the working tree does not match that anymore. > Silent removal of e/f that is unrelated to the current project's > tracked contents feels very wrong, and at the same time it looks to > me that it is inconsistent with what we do when adding. > > I need a bit more persuading to understand why it is not a bug, I > think. But that's the point of the two content tests. It _isn't_ unrelated to the current project's tracked contents; it's the exact same content at the same path (albeit accessed via symlinks now). The likely case for this is something like: mv dir somewhere/else ln -s somewhere/else/dir dir I do not mind if you want to insert extra protection to not cross symlink boundaries (which would obviously invalidate my test). But I don't think it is necessary because of the existing content-level protections. Adding extra protections would disallow "git rm dir/file" in the above case, but I don't think it's that inconvenient; the user just has to make the index aware of the typechange first via "git add". -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