[MERGE] enhanced mv command
Wouter van Heyst
larstiq at larstiq.dyndns.org
Thu Nov 16 17:42:37 GMT 2006
On Mon, Nov 13, 2006 at 10:07:11AM +0100, Hanns-Steffen Eichenberg wrote:
> Aloha,
>
>
> this patch contains an enhanced version of the mv command. The mv command
> can now
> operate on already moved files (moved on the file system). I tried to
> incorporate as much
> input from this mailing list as i could.
>
> I will start to implement an auto detect feature for moved files as soon as
> this work can be
> considered 'closed'.
>
>
> Ciao,
> Steffen
...
> self.run_bzr_error(
> - ["^bzr: ERROR: destination u'sub1' is not a versioned directory$"],
> + ["^bzr: ERROR: destination .* is not a versioned directory$"],
> 'mv', 'test.txt', 'sub1')
You have a couple of these changes u'sub1' -> .*, and also employ them
in new code. I'm a bit concerned that will not catch unwanted changes
(or, I'm not a fan of the u'foo' mode and would like to see it gone
entirely someday). Not blocking on that though.
> + 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"""
> + self.build_tree(['a', 'b'])
> + tree = self.make_branch_and_tree('.')
> + tree.add(['a', 'b'])
> + tree.commit('initial commit')
> +
> + os.remove('a')
> + self.run_bzr_error(
> + ["^bzr: ERROR: can't rename: new name .* is already versioned$"],
> + 'mv', 'a', 'b')
> + self.failIfExists('a')
> + self.failUnlessExists('b')
In the docstring you mention the user does 'rm b; mv a b; bzr mv a b',
but in the code you do os.remove('a'). The error string is the same for
both cases, shouldn't they be differentiated?
> + 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"""
> + self.build_tree(['a', 'sub1/'])
> + tree = self.make_branch_and_tree('.')
> + tree.add(['a', 'sub1'])
> + tree.commit('initial commit')
> +
> + os.rename('a', 'sub1/a')
> + self.run_bzr('mv', 'a', 'sub1/a')
> + self.failIfExists('a')
> + self.failUnlessExists('sub1/a')
...
> + 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"""
> + self.build_tree(['a1', 'a2', 'sub1/'])
> + tree = self.make_branch_and_tree('.')
> + tree.add(['a1', 'a2', 'sub1'])
> + tree.commit('initial commit')
> +
> + os.rename('a1', 'sub1/a1')
> + self.run_bzr('mv', 'a1', 'a2', 'sub1')
> + self.failIfExists('a1')
> + self.failIfExists('a2')
> + self.failUnlessExists('sub1/a1')
> + self.failUnlessExists('sub1/a2')
test_mv_already_moved_files_into_subdir is quite like
test_mv_already_moved_file_into_subdir. I'm guessing the latter one is
to test the move modes being determined independently per file? Do we
really need both?
> + 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"""
> + self.build_tree(['a1', 'a2', 'sub1/'])
> + tree = self.make_branch_and_tree('.')
> + tree.add(['a1', 'a2'])
> + tree.commit('initial commit')
> +
> + os.rename('a1', 'sub1/a1')
> + self.run_bzr_error(
> + ["^bzr: ERROR: destination .* is not a versioned directory$"],
> + 'mv', 'a1', 'a2', 'sub1')
> + self.failIfExists('a1')
> + self.failUnlessExists('a2')
> + self.failUnlessExists('sub1/a1')
> + self.failIfExists('sub1/a2')
Same thing for test_mv_already_moved_file_into_unversioned_subdir and
test_mv_already_moved_files_into_unversioned_subdir
I won't repeat it anymore
> +
> + 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"""
> + self.build_tree(['a', 'b'])
> + tree = self.make_branch_and_tree('.')
> + tree.add(['a'])
> + tree.commit('initial commit')
> +
> + self.run_bzr_error(
> + ["^bzr: ERROR: can't rename: both, old name .* and new name .*"
> + " exist. Use option '--after' to force rename."],
> + 'mv', 'a', 'b')
> + self.failUnlessExists('a')
> + self.failUnlessExists('b')
The error message is only clear to me because I've read what is being
tested, if I didn't know that, it would be puzzling.
> === modified file bzrlib/tests/workingtree_implementations/test_workingtree.py
> ... // last-changed:scameronde at googlemail.com-20061113084737-6d184e0e1527168d /
> ... / executable:yes
> --- bzrlib/tests/workingtree_implementations/test_workingtree.py
> +++ bzrlib/tests/workingtree_implementations/test_workingtree.py
> @@ -681,3 +681,50 @@
...
> === modified file bzrlib/workingtree.py // executable:yes
> --- bzrlib/workingtree.py
> +++ bzrlib/workingtree.py
> @@ -959,123 +959,287 @@
> stack.pop()
>
> @needs_tree_write_lock
> - def move(self, from_paths, to_name):
> + def move(self, from_paths, to_dir=None, after=False, **kwargs):
> """Rename files.
>
> - to_name must exist in the inventory.
> + to_dir must exist in the inventory.
>
> - If to_name exists and is a directory, the files are moved into
> + If to_dir exists and is a directory, the files are moved into
> it, keeping their old names.
>
> - Note that to_name is only the last component of the new name;
> + Note that to_dir is only the last component of the new name;
> this doesn't change the directory.
>
> + For each entry in from_paths the move mode will be determined
> + independently.
> +
> + The first mode moves the file in the filesystem and updates the
> + inventory. The second mode only updates the inventory without
> + touching the file on the filesystem. This is the new mode introduced
> + in version 0.13.
> +
> + move uses the second mode if 'after == True' and the target is not
> + versioned but present in the working tree.
> +
> + move uses the second mode if 'after == False' and the source is
> + versioned but no longer in the working tree, and the target is not
> + versioned but present in the working tree.
> +
> + move uses the first mode if 'after == False' and the source is
> + versioned and present in the working tree, and the target is not
> + versioned and not present in the working tree.
> +
> + Everything else results in an error.
> +
> This returns a list of (from_path, to_path) pairs for each
> entry that is moved.
> """
> - result = []
> - ## TODO: Option to move IDs only
> + rename_entries = []
> + rename_tuples = []
> +
> + # check for deprecated use of signature
> + if to_dir is None:
> + to_dir = kwargs.get('to_name', None)
> + if to_dir is None:
> + raise TypeError('You must supply a target directory')
> + else:
> + symbol_versioning.warn('The parameter to_name was deprecated'
> + ' in version 0.13. Use to_dir instead',
> + DeprecationWarning)
Is raising a TypeError the right thing to do?
> + def _determine_mv_mode(self, rename_entries, after=False):
> + """Determines for each from-to pair if both inventory and working tree
> + or only the inventory has to be changed.
> +
> + Also does basic plausability tests.
> + """
> + inv = self.inventory
> +
> + for rename_entry in rename_entries:
> + # store to local variables for easier reference
> + from_rel = rename_entry.from_rel
> + from_id = rename_entry.from_id
> + to_rel = rename_entry.to_rel
> + to_id = inv.path2id(to_rel)
> + only_change_inv = False
> +
> + # check the inventory for source and destination
> + if from_id is None:
> + raise BzrError("can't rename: old name %r is not versioned" %
> + from_rel)
> + if to_id is not None:
> + raise BzrError("can't rename: new name %r is already"
> + " versioned" % to_rel)
> +
> + # try to determine the mode for rename (only change inv or change
> + # inv and file system)
> + if after:
> + if not self.has_filename(to_rel):
> + raise BzrError("can't rename: new working file %r does not"
> + " exist" % to_rel)
You don't seem to test for this possibility.
> + only_change_inv = True
> + elif not self.has_filename(from_rel) and self.has_filename(to_rel):
> + only_change_inv = True
> + elif self.has_filename(from_rel) and not self.has_filename(to_rel):
> + only_change_inv = False
> + else:
> + # something is wrong, so lets determine what exactly
> + if not self.has_filename(from_rel) and \
> + not self.has_filename(to_rel):
> + raise BzrError("can't rename: neither old name %r nor new"
> + " name %r exist" % (from_rel, to_rel))
Nor this one.
> + else:
> + raise BzrError("can't rename: both, old name %r and new"
> + " name %r exist. Use option '--after' to"
> + " force rename." % (from_rel, to_rel))
> + rename_entry.set_only_change_inv(only_change_inv)
> + return rename_entries
> +
> + def _move(self, rename_entries):
> + """Moves a list of files.
> +
> + Depending on the value of the flag 'only_change_inv', the
> + file will be moved on the file system or not.
> + """
> + inv = self.inventory
> + moved = []
> +
> + for entry in rename_entries:
> + try:
> + self._move_entry(entry)
> + except OSError, e:
> + self._rollback_move(moved)
> + raise BzrError("failed to rename %r to %r: %s" %
> + (entry.from_rel, entry.to_rel, e[1]))
> + except BzrError, e:
> + self._rollback_move(moved)
> + raise
> + moved.append(entry)
> +
> + def _rollback_move(self, moved):
> + """Try to rollback a previous move in case of an error in the
> + filesystem.
> + """
> + inv = self.inventory
> + for entry in moved:
> + try:
> + self._move_entry(entry, inverse=True)
> + except OSError, e:
> + raise BzrError("error moving files. rollback failed. the"
> + " working tree is in an inconsistent state."
> + " please consider doing a 'bzr revert'."
> + " error message is: %s" % e[1])
Can this be tested? Also not blocking, but would be good to know it
works. As is, +0.8 with Aaron's concerns addressed.
Wouter van Heyst
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 307 bytes
Desc: Digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061116/43b307a4/attachment.pgp
More information about the bazaar
mailing list