[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