[MERGE] Fix WorkingTree.remove for tree-references and non-cwd trees
Aaron Bentley
aaron.bentley at utoronto.ca
Thu May 24 16:39:35 BST 2007
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi all,
WorkingTree.remove doesn't work properly with tree references, because
it uses isdir to determine whether a path is a directory, and then calls
tree.walkdirs. That mixes "inventory kind" and "disk kind" inappropriately.
tree.walkdirs operates in terms of inventory kinds, where possible kinds
are "file", "symlink", "directory" and "tree-reference". It's not meant
to recurse into tree references, and if a tree reference is passed as a
directory, an AttributeError is raised, because a TreeReference has no
children.
Since recursion is always desired, I changed it to use osutils.walkdirs,
which *always* operates in terms of disk kinds. This means that tree
references have the kind "directory", and are recursed into.
I also fixed it to use absolute paths in the appropriate places, so that
it works properly when the tree root is not the cwd.
Finally, I removed all use of Tree.inventory from the implementation,
because we're trying to make the Inventory object an implementation
detail of WorkingTree3 objects.
Even if I agreed that it made sense for WorkingTree.remove to delete
files, I don't think this is a good implementation.
- - keep_files and force are boolean, but clearly keep_files=True,
force=True is nonsensical. This should be tri-modal, e.g.
keep_files= "always", "never", or "auto".
- - Instead of taking to_file, it should take a reporter object, so we
aren't stuck with a particular output format.
- - BzrRemoveChangedFilesError should not take a delta object as an
argument. It would make more sense to take a list of
modified/unversioned files. Requiring a delta restricts the
implementation options.
- - It reports on children of unversioned files. We never do that, and
I'm not convinced we should start.
- - It uses "unknown" where it means "unversioned", e.g. the
"unknown_files_in_directory" set.
- - It appears to ignore symlinks in some places, and it appears to
traverse them in others.
- - There are PEP8 issues, e.g. lack of spaces around equal signs.
- - It uses the format '"%s does not exist." % (f,)', when our house style
is to omit the tuple, and use f directly.
- - It reimplements TreeDelta.has_changed
I spent all afternoon yesterday trying to reimplement it from scratch,
but the desired behavior specified in the test cases is just too weird
for it to come easily.
So I proffer this bugfix instead.
Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFGVbG30F+nu1YWqI0RAhnpAJ4rk9TtDq9w2JzbM7JQP4Geefa/yQCfQD9Q
IF20DYZPp3hD11cZ8liWM5k=
=88kZ
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix-remove.patch
Type: text/x-patch
Size: 20105 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070524/d3054910/attachment-0001.bin
More information about the bazaar
mailing list