Re: [PATCH 3/3] t3600: test rm of path with changed leading symlinks

From: Jeff King <peff@peff.net>
Date: Apr 4, 2013, 19:55 UTC
Message-ID: <20130404195554.GA20823@sigill.intra.peff.net>
In-Reply-To: <7v6202hykh.fsf@alter.siamese.dyndns.org>

On Thu, Apr 04, 2013 at 12:42:54PM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> Jeff King <peff@peff.net> 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