[MERGE] mv accepts paths containing symlinks #66964

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Aaron Bentley wrote:
> John Arbash Meinel wrote:

...

>>>
>>> Well, I can give you the specifics and tell you that os.path.split is
>>> fine to use, but I understand your hesitancy.
> 
> Okay, I've used os.path.split.  But I suspect the previous inefficiency
> was vanishingly small.


In this specific case, probably. In the general case, we actually call
some of these functions more than we probably should, so I'm trying to
do it right when I see it.

And os.path.split() is actually a little expensive because of some weird
stuff that it does. But yeah, in this case we are probably only calling
it with a few paths (<100 I'm sure)

> 
>>> I'm not 100% positive that 'realpath' always does the right thing under
>>> Mac
> 
> Actually, I've determined that is does the wrong thing on all platforms
> under Python 2.4.1 and previous.
> 

Out of curiosity how did you fund bug #1213894, it doesn't seem like
something you would just happen across.

> 
> I've called it real_parent.

^- Not the best name, since it sounds like you only get the parent back.
Perhaps: path_with_real_parent(). That is a bit long, and I realize this
is a place for bikeshedding. So think about it for a bit, and if the
best we can think of is 'real_parent', we can go with it.
> 
> I've also added the symlink guards, and fixed that "== None".
> 
> What do you think?
> 
> Aaron


v- Ahh a lack of context... I just read this over thinking you were
changing 'cmd_mv', and could not figure out why you would be
re-implementing tree_files() :) I even spent about 5 min reviewing it.

     :param file_list: Filenames to convert.

- -    :param default_branch: Fallback tree path to use if file_list is
empty or None.
+    :param default_branch: Fallback tree path to use if file_list is
empty or
+        None.

     :return: workingtree, [relative_paths]
     """
     if file_list is None or len(file_list) == 0:
         return WorkingTree.open_containing(default_branch)[0], file_list
- -    tree = WorkingTree.open_containing(file_list[0])[0]
+    tree = WorkingTree.open_containing(osutils.realpath(file_list[0]))[0]
     new_list = []
     for filename in file_list:
         try:
- -            new_list.append(tree.relpath(filename))
+            new_list.append(tree.relpath(osutils.real_parent(filename)))
         except errors.PathNotChild:
             raise errors.FileInWrongBranch(tree.branch, filename)
     return tree, new_list



It looks pretty good to me, though. I have the feeling this will also fix:
https://launchpad.net/products/bzr/+bug/32669

It might be nice to add a small test to prove that, and then we can
close both bugs and note it in the NEWS. I won't require it, it just
would be nice. (And I don't really want to close #32669 without adding a
test for it).

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.1 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFP7ooJdeBCYSNAAMRAnZbAKCyjIr8kRnMBTJoRj7CMy0EZGQklwCfUoN5
JvQYFvlu5df1eiHGdEq/PYM=
=DT8r
-----END PGP SIGNATURE-----