[MERGE] mv accepts paths containing symlinks #66964

[John Arbash Meinel]

Aaron Bentley wrote:
> Hi all,
> 
> Currentley, bzr commands that accept filenames don't traverse symlinks.
> 
> So if you have
> 
> foo/bar (directory)
> baz (symlink -> foo)
> 
> you cannot specify "bzr mv baz/mar", even though you can with normal
> UNIX mv.  Futhermore, the crash is really ugly.  This bundle
> 
> 1. changes path2id so that it returns None if you specify a path that
> contains a non-directory as a parent directory, because path2id returns
> None if the path doesn't exist.
> 
> 2. runs osutils.realpath on all input paths, in such a way that paths
> *to* symlinks are not adjusted, but paths *through* symlinks are adjusted.
> 
> This only works for the current working tree, not for RevisionTrees,
> etc.  Attempting to solve all of them at once proved very difficult, so
> I think this is the best solution for now, and a considerable
> improvement over what we had.
> 
> Aaron

What about:
https://launchpad.net/products/bzr/+bug/48444

Do you think that bug would be affected by this fix?



...

v- This should probably be:

dirname, basename = os.path.split(path)
osutils.pathjoin(osutils.realpath(dirname), basename)

dirname and basename both work by calling split() and throwing away the
second half, so it doesn't really make sense to do that 2 times.

Perhaps we should change osutils.??? so that it implements this
behavior, because it would have an effect for lots of other bugs as well.

+    def realpath(path):
+        return osutils.pathjoin(osutils.realpath(osutils.dirname(path)),
+                                osutils.basename(path))
+


v- Don't you mean 'if children is None:'?

             try:
-                cie = parent.children[f]
+                children = getattr(parent, 'children', None)
+                if children == None:
+                    return None
+                cie = children[f]
                 assert cie.name == f
                 assert cie.parent_id == parent.file_id
                 parent = cie


v- I think you need a trap to not run this test if you are on a platform
that doesn't support symlinks. At the very least something like:

  if not osutils.has_symlinks():
      raise TestSkipped('Symlinks are not supported on this platform')

+
+    def test_mv_through_symlinks(self):
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['a/', 'a/b'])
+        os.symlink('a', 'c')
+        os.symlink('.', 'd')
+        tree.add(['a', 'a/b', 'c'], ['a-id', 'b-id', 'c-id'])
+        self.run_bzr('mv', 'c/b', 'b')
+        tree = workingtree.WorkingTree.open('.')
+        self.assertEqual('b-id', tree.path2id('b'))


So I think the change could probably be expanded, and the test needs a
has_symlinks() guard, but I think having support for when a file is
referenced through a symlink would be nice.

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061023/17b8cac4/attachment.pgp