[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