[MERGE] enhanced mv command
Steffen Eichenberg
scameronde at googlemail.com
Mon Nov 20 14:19:01 GMT 2006
Hello,
first of all, i am very busy at the moment. I will not be able to polish the
code
for the deadline today. Sorry for that. I will have more time at the end of
the week.
So i guess the code will be incorporated in the next sprint. Will this be
version 0.14?
(have to change some text then)
On 11/16/06, Wouter van Heyst <larstiq at larstiq.dyndns.org> wrote:
>
> 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.
I encountered that pattern so often, that i thought it was the way to go. I
will
happily change that.
> + 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?
The test is not that explicit. I could have coded
os.remove("b")
os.move("a", "b")
but since the files a and b do not have any content,
os.remove("a")
has the same effect.
> + 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?
aaah, yes and no. If this would have been a real black box test, then no.
But
the implementation of bzr mv uses two different methods for the two use
cases
and i want both to be tested.
This has not been my idea. I do not like the differentiation, but the
original code
was written that way and i do not want to break any API.
> > +
> > + 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.
I seem to be too involved into the functionality to understand what exactly
is not
clear. Can you give me a hint, or even better, can you give me a better
error message ;-)
> === 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?
That is what i have been told to do. I am not quite sure at the moment who
it was,
maybe John. I am looking for something more like an IllegalArgumentError or
a
MissingArgumentError but there seem to be no standard errors for this use
case.
Any suggestions?
> + 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.
no, i don't. will do.
> + 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.
will do.
> + 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.
yes, but only with a white box test. I haven't done that, becaus i had not
the
time. Because i seem to have missed the deadline for this sprint, i will
have
enough time to write a test for this, too.
Wouter van Heyst
>
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.5 (GNU/Linux)
>
> iQCVAwUBRVyjDOnTGucjhaP+AQKR3AP/SiAUMHELruoVwoKy6JO4cnaamvxXHIqo
> 006WVqvSgpshXYVTn3WEAwpbwWhNpZ9/ZFOHeRJ6bqhRk7K66ehLa/gWTwqzJCZ4
> qgdojsEnENyoae6X9tJCWT2mCPnpA7WYSyP5Zt+bTsRAni+o0JJPEpjk0UNVdkHA
> B4RFRro8U7A=
> =suOw
> -----END PGP SIGNATURE-----
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.ubuntu.com/archives/bazaar/attachments/20061120/a23532d8/attachment.htm
More information about the bazaar
mailing list