[MERGE] enhanced mv command

[Steffen Eichenberg]

On 11/16/06, Aaron Bentley <aaron.bentley at utoronto.ca> wrote:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hanns-Steffen Eichenberg wrote:
> > this patch contains an enhanced version of the mv command.
>
> This is very close.  Just a few issues.
>
> > +    takes_options = [Option("after", help="move only the bzr identifier
> of the"
> > +                                          " file (file has already
> been"
> > +                                          " moved). Use this flag if
> bzr is"
> > +                                          " not able to detect this
> itself.")]
>
> This is a style thing, but I think most of us would use a smaller indent
> here:
>
>     takes_options = [Option("after", help="move only the bzr identifier"
>         " of the file (file has already been moved). Use this flag if"
>         " bzr is not able to detect this itself.")]
>
> This is optional, of course.
>
> > === modified file bzrlib/tests/blackbox/test_mv.py //
> last-changed:scameronde at g
> > ... ooglemail.com-20061108154710-5c1b0731ddb9365e // executable:yes
> > --- bzrlib/tests/blackbox/test_mv.py
> > +++ bzrlib/tests/blackbox/test_mv.py
>
> [...]
>
> > +        self.run_bzr('mv', 'a1', 'a2', 'sub1', '--after')
> > +        self.failUnlessExists ('a1')
> > +        self.failUnlessExists('a2')
> > +        self.failUnlessExists('sub1/a1')
> > +        self.failUnlessExists('sub1/a2')
> > +
> >
>
> You have an extra newline here.


This is the end of the file. No newline here? OK. Removed.


> === 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.
>
> Should have a comma after "For each entry"
>
> > +        # create rename entries and tuples
> > +        for from_rel in from_paths:
> > +            from_tail = splitpath(from_rel)[-1]
> > +            from_id = inv.path2id(from_rel)
> > +            if from_id is None:
> > +                raise BzrError("can't rename: old name %r is not
> versioned" %
> > +                               from_rel)
>
> I think this should be a custom error class, because different callers
> might want to handle errors in different ways.  Same for the other
> errors emitted by this method.


Wouldn't that break the API of the methods 'move()' and 'rename_one()' ?

Both methods used to throw BzrErrors. Most of the errors and error messages
are
from the original code.

Or should i use custom error classes for the private methods only and
re-throw them
as BzrErrors in the public methods 'move()' and 'rename_one()' ?


> +                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])
>
> Needs capitals on "rollback", "the", "please", "error".
>
> > +        rename_one has several 'modes' to work. First it can rename a
> physical
>
> 'First' needs a comma.
>
> > +        file and change the file_id. That is the normal mode. Second it
> can
>
> 'Second' needs a comma.
>
> > +    class _RenameEntry(object):
>
> [...]
>
> > +        def set_only_change_inv(self, only_change_inv):
> > +            self.only_change_inv = only_change_inv
>
> This method doesn't seem to have a purpose.



Like most accessors in Java. Just an old habit.
Fixed like all other errors you found.

But before resubmitting the bundle, i would like to have your answer
regarding the custom error classes.

Btw. i asked John a question in the old thread. It is about coding style. I
got no answer from him. Maybe
you can answer that. Here is Johns remark and my question:

> > +    class _RenameEntry(object):
> > +        def __init__(self, from_rel, from_id, from_tail,
from_parent_id,
> > +                     to_rel, to_tail, to_parent_id):
> > +            self.from_rel = from_rel
> > +            self.from_id = from_id
> > +             self.from_tail = from_tail
> > +            self.from_parent_id = from_parent_id
> > +            self.to_rel = to_rel
> > +            self.to_tail = to_tail
> > +            self.to_parent_id = to_parent_id
> > +             self.only_change_inv = False
> > +
> > +        def set_only_change_inv(self, only_change_inv):
> > +            self.only_change_inv = only_change_inv
> > +
> >
> > I think it is better if _RenameEntry is just a private class at module
> > scope, rather than a nested class.
> >
> > Or at the very least, I would define a nested class at the start of the
> > encompassing scope, rather than in the middle.
>
> I would like to have a stronger statement here. What is the desired code
style for bzr?
>   - Is it OK to use nested classes or not?
>   - where *exactly* should nested or private classes be placed in the
source?
>
> I am asking, because in most projects i encountered the rules 'keep
related things together,
> because it is easier to read' and 'define elements in the scope you are
using them'.
>
> Please do not get me wrong. I do not want to question the code style of
bzr, i want
> to understand and learn it.


Steffen



Aaron
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.3 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iD8DBQFFW+QI0F+nu1YWqI0RAhyhAJ954KkBIbue1znNi2hjOYj6O8bZCgCfZspj
> jUXQ3Iswe3oNq87EV+8yG50=
> =m6St
> -----END PGP SIGNATURE-----
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.ubuntu.com/archives/bazaar/attachments/20061116/40393ec4/attachment.htm