On Thu, Apr 04, 2013 at 12:42:54PM -0700, Junio C Hamano wrote:
> Jeff King <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
