[MERGE] enhanced mv command
Aaron Bentley
aaron.bentley at utoronto.ca
Wed Jan 3 04:46:54 GMT 2007
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Marius Kruger wrote:
> thanks for the review John,
> I incorporated you requests as best I could.
+0. I think we're close, though.
=== modified file 'bzrlib/builtins.py' (properties changed)
^^^ I don't think it makes sense to set builtins.py executable.
=== modified file 'bzrlib/errors.py'
- --- bzrlib/errors.py 2006-12-21 20:07:34 +0000
+++ bzrlib/errors.py 2007-01-03 02:46:14 +0000
@@ -18,7 +18,8 @@
"""
- -from bzrlib import symbol_versioning
+from bzrlib import (symbol_versioning,
+ osutils,)
from bzrlib.patches import (PatchSyntax,
PatchConflict,
MalformedPatchHeader,
@@ -319,6 +320,37 @@
_fmt = "File exists: %(path)r%(extra)s"
+class FilesExist(PathError):
+ """Used when reporting that files do exist"""
+
+ _fmt = "File%(plural)s exist: %(paths_as_string)s%(extra)s"
+
+ def __init__(self, paths, extra=None):
+ # circular import
+ from bzrlib.osutils import quotefn
+ BzrError.__init__(self)
+ self.paths = paths
+ self.paths_as_string = ' '.join(paths)
+ if extra:
+ self.extra = ': ' + str(extra)
+ else:
+ self.extra = ''
+ if len(paths) > 1:
+ self.plural = 's'
+ else:
+ self.plural = ''
^^^ I'm dubious about trying to generate correct English plurals for
error strings -- we've always just done "File(s) exist". You don't have
to change this, it's just my reaction.
+class NotInWorkingDirectory(PathError):
+
+ _fmt = "%(path)r is not a the working directory %(extra)s"
^^^ There's something wrong with this. "is not *in* the working directory"?
+class BzrMoveFailedError(BzrError):
+
+ _fmt = "Could not move %(from_path)s%(operator)s %(to_path)s%(extra)s"
+
+ def __init__(self, from_path, to_path, extra=None):
+ BzrError.__init__(self)
+ if extra:
+ self.extra = ': ' + str(extra)
+ else:
+ self.extra = ''
+
+ has_from = len(from_path) > 0
+ has_to = len(to_path) > 0
The API is "you must supply from_path and to_path, but they may be empty
strings"? Why not just allow them to be omitted?
@@ -147,3 +152,151 @@
self.run_bzr('mv', 'c/b', 'b')
tree = workingtree.WorkingTree.open('.')
self.assertEqual('b-id', tree.path2id('b'))
+
+ def test_mv_already_moved_file(self):
+ """a is in repository, b does not exist. User does
+ mv a b; bzr mv a b"""
^^^ Docstrings should have a one-line summary, then an empty line, then
a longer description. Also, we don't talk about files being 'in the
repository'. I'd say 'in the working tree' here.
Also, these tests don't seem to be checking whether the inventory is
updated. You can do that by doing e.g.
self.assertFalse(WorkingTree.open('foo').path2id('bar/baz') is None)
Finally, I'm not sure what the point of committing the files is. 'bzr
mv' doesn't and shouldn't care whether or not the files have been committed.
+ def test_mv_already_moved_file_to_versioned_target(self):
+ """a and b are in the repository. User does
+ rm b; mv a b; bzr mv a b"""
^^^ ditto. Also, the docstring doesn't match; here, the user removes
'a', not 'b'.
+ def test_mv_already_moved_file_into_subdir(self):
+ """a and sub1/ are in the repository. User does
+ mv a sub1/a; bzr mv a sub1/a"""
^^^ ditto.
+ def test_mv_already_moved_file_into_unversioned_subdir(self):
+ """a is in the repository, sub1/ is not. User does
+ mv a sub1/a; bzr mv a sub1/a"""
^^^ ditto.
+ def test_mv_already_moved_files_into_subdir(self):
+ """a1, a2, sub1 are in the repository. User does
+ mv a1 sub1/.; bzr mv a1 a2 sub1"""
^^^ ditto.
+ def test_mv_already_moved_files_into_unversioned_subdir(self):
+ """a1, a2 are in the repository, sub1 is not. User does
+ mv a1 sub1/.; bzr mv a1 a2 sub1"""
^^^ ditto.
+ def test_mv_already_moved_file_forcing_after(self):
+ """a is in the repository, b does not exist. User does
+ mv a b; touch a; bzr mv a b"""
^^^ ditto
+ self.build_tree(['a', 'b'])
+ tree = self.make_branch_and_tree('.')
+ tree.add(['a'])
+ tree.commit('initial commit')
+
+ self.run_bzr_error(
+ ["^bzr: ERROR: Could not rename a => b: Files exist: a b:"
+ " \(Use option '--after' to force rename\)$"],
+ 'mv', 'a', 'b')
- --after doesn't cause bzr to do "os.rename('a', 'b')", but this message
could be misinterpreted to mean that. Perhaps it would be clearer to
say "(Use --after to update just the Bazaar id)" ?
+ def test_mv_already_moved_file_using_after(self):
+ """a is in the repository, b does not exist. User does
+ mv a b; touch a; bzr mv a b --after"""
^^^ ditto.
+ def test_mv_already_moved_files_forcing_after(self):
+ """a1, a2, sub1 are in the repository, sub1/a1, sub1/a2
+ do not exist. User does
+ mv a* sub1; touch a1; touch a2; bzr mv a1 a2 sub1"""
^^^ ditto.
+ def test_mv_already_moved_files_using_after(self):
+ """a1, a2, sub1 are in the repository, sub1/a1, sub1/a2
+ do not exist. User does
+ mv a* sub1; touch a1; touch a2; bzr mv a1 a2 sub1 --after"""
^^^ ditto
=== modified file
'bzrlib/tests/workingtree_implementations/test_workingtree.py'
(properties changed)
^^^ I don't think it makes sense to make test_workingtree.py executable.
- --- bzrlib/tests/workingtree_implementations/test_workingtree.py
2006-09-10 19:04:48 +0000
+++ bzrlib/tests/workingtree_implementations/test_workingtree.py
2007-01-03 02:46:14 +0000
@@ -681,3 +681,50 @@
tree.add, [u'a\u030a'])
finally:
osutils.normalized_filename = orig
+ def test_move_deprecated_wrong_call(self):
+ """tree.move has the deprecated parameter 'to_name'.
+ It has been replaced by 'to_dir' for consistency.
+ Test the new API using wrong parameter"""
+ self.build_tree(['a1', 'sub1/'])
+ tree = self.make_branch_and_tree('.')
+ tree.add(['a1', 'sub1'])
+ tree.commit('initial commit')
+ self.assertRaises(TypeError, tree.move, ['a1'],
+ to_this_parameter_does_not_exist='sub1',
+ after=False)
^^^ This is probably superfluous with the following test. I must admit
I thought you were obsoleting (not just deprecating) the parameter, at
first.
+ def test_move_deprecated_deprecated_call(self):
=== modified file 'bzrlib/workingtree.py' (properties changed)
^^^ I don't think it makes sense to set workingtree.py executable.
- --- bzrlib/workingtree.py 2006-12-19 17:06:42 +0000
+++ bzrlib/workingtree.py 2007-01-03 02:46:14 +0000
@@ -265,7 +256,8 @@
# if needed, or, when the cache sees a change, append it to the
hash
# cache file, and have the parser take the most recent entry for a
# given path only.
- - cache_filename =
self.bzrdir.get_workingtree_transport(None).local_abspath('stat-cache')
+ cache_filename = self.bzrdir.get_workingtree_transport(None) \
+ .local_abspath('stat-cache')
self._hashcache = hashcache.HashCache(basedir, cache_filename,
^^^ the preferred method of splitting long lines like this is to use a
temporary variable. e.g.:
wt_trans = self.bzrdir.get_workingtree_transport(None)
cache_filename = wt_trans.local_abspath('stat-cache')
@@ -830,14 +822,14 @@
def merge_modified(self):
try:
hashfile = self._control_files.get('merge-hashes')
- - except NoSuchFile:
+ except errors.NoSuchFile:
I have to say that it's kinda frustrating plowing through all these
unrelated changes in order to review this new mv thing.
+ This is the new mode introduced
+ in version 0.13.
More like version 0.14. I think. Hey, who's flying this plane, anyway?
This returns a list of (from_path, to_path) pairs for each
- - entry that is moved.
+ entry, that is moved.
^^^ The original phrasing is correct.
As code: [pairs(entry) for entry in moved_entries()]
+ # determine which move mode to use. checks also for movability
+ rename_entries = self._determine_mv_mode(rename_entries, after)
+
+ # move pairs.
^^^ What does "move pairs" mean?
original_modified = self._inventory_is_modified
try:
- - if len(from_paths):
+ if len(from_paths):
^^^ All this does is insert a trailing space. Not an improvement :-)
+ for entry in rename_entries:
+ try:
+ self._move_entry(entry)
+ except OSError, e:
+ self._rollback_move(moved)
+ raise errors.BzrRenameFailedError(entry.from_rel,
entry.to_rel,
+ e[1])
+ except errors.BzrError, e:
+ self._rollback_move(moved)
+ raise
^^^ Is it possible to catch more specific errors here? BzrError and
OSError are very broad.
Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFFmzU+0F+nu1YWqI0RAm5lAKCD2iQWbbN/gveHHGJl9aB+yhl4UwCfXxYY
2ZkwJlfN8FWyw6Q+Nat8Q/4=
=QMPn
-----END PGP SIGNATURE-----
More information about the bazaar
mailing list