[MERGE] mv accepts paths containing symlinks #66964

John Arbash Meinel john at arbash-meinel.com
Wed Oct 25 20:25:28 BST 2006

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:
- -            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:

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).


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


More information about the bazaar mailing list