[PATCH] Re: Feature Request: 'bzr mv --after' to tell bzr that file(s) have already been moved in the working tree

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hanns-Steffen Eichenberg wrote:

> Here it is. Since this is my first work for bzr and my first lines of
> python for more than 3 years i would be happy if
> you can give the code a thorough review.

Here's a review of the actual content.  I hope it doesn't come off as
too nitpicky.

Some style things:
- - please wrap your lines at 79 characters.
- - "independent" has no "a" in it
- - the "after" option should be described in terms of the resulting
behavior, e.g. "move only the bzr identifier of the file (file has
already been moved)"

> === modified file .bzrignore // last-changed:eichenbs at gilpnb051b-20061103151123
> ... -dcb18125ed861bdc
> --- .bzrignore
> +++ .bzrignore
> @@ -30,4 +30,6 @@
>  ./tools/win32/bzr.iss
>  ./dist
>  # performance history data file
> -./.perf_history
> \ No newline at end of file
> +./.perf_history
> +make_bat.last
> +make_bat.py

What are make_bat.last and make_bat.py?

> +            for tuple in tree.move(rel_names[:-1], rel_names[-1], after):
> +                self.outf.write("%s => %s\n" % (tuple['from_rel'], tuple['to_rel']))

John already pointed out there's a built-in tuple type, but I don't
think he mentioned that "tuple" is the name of a built-in function.
Assigning to "tuple" overides that built-in function in the "run"
method's scope.  In this case, it's not dangerous, but can be considered
bad form.

Also, because we try to preserve API compatibility, we can't just change
the return type of tree.move.  If a new return type is desired, we can
create a new method, or add a parameter to tree.move to enable the new
output.  There needs to be at least one release that supports the old
and new behaviors, with the old behavior causing a deprecation warning
to be emitted.

> === modified file bzrlib/workingtree.py
> --- bzrlib/workingtree.py
> +++ bzrlib/workingtree.py
> @@ -989,7 +989,7 @@
>                  stack.pop()
>  
>      @needs_tree_write_lock
> -    def move(self, from_paths, to_name):
> +    def move(self, from_paths, to_dir, after=False):

I wonder whether after should support 3 values:
True: move only the inventory
False: always move both inventory and file
None: auto-detect whether to move file

>          """Rename files.
>  
>          to_name must exist in the inventory.
> @@ -1000,112 +1000,243 @@
>          Note that to_name is only the last component of the new name;
>          this doesn't change the directory.
>  
> -        This returns a list of (from_path, to_path) pairs for each
> -        entry that is moved.
> +        for each entry in from_paths the move mode will be determined independantly.
> +
> +        The first mode is moving the file in the filesystem and update the inventory.
> +        The second mode is only update 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.
> +
> +        rename_one 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.


I don't think the doctext for WorkingTree.move should describe
WorkingTree.rename_one.  (Cut and paste error?)

> +        # create from-to tuples

I think a lot of the need for from-to tuples would be eliminated if
_determine_mv_mode worked on a single entry at a time.

i.e. its signature would be:
_determine_mv_mode(from_rel, from_id, to_rel)

> +        for from_rel in from_paths:
> +            from_tail = splitpath(from_rel)[-1]
> +            from_id = inv.path2id(from_rel)
> +            from_entry = inv[from_id]

^^^ This looks like it would fail messily whenever from_id is None.  You
probably want to raise an error if from_id is None.

You have failing test cases, and this is one reason why.

Specifically,

bzrlib.tests.blackbox.test_mv.TestMove.test_mv_unversioned
bzrlib.tests.blackbox.test_mv.TestMove.test_mv_nonexisting

are failing because they're raising the wrong error now.


You should be able to run just those tests with "bzr selftest
test_mv_unversioned" and "test_mv_nonexisting".
> +        # move pairs.
>          original_modified = self._inventory_is_modified
>          try:
> -            if len(from_paths):
> +            if len(from_paths):  # i do not understand this line (scameronde)

The line is equivalent to 'if len(from_paths) > 0:'.  I think we could
actually exit early in that case.

> +    def _move(self, from_to_tuples):
> +        """Moves a list of files.
> +
> +        The flag 'only_change_inv' is expected for each file to move
> +        determines if only the inventory or if both,
> +        inventory and filesystem, have to be updated.
> +        """

This method is not entirely general, in that it's possible for the
from_to_tuples to name the same file as both a source and a target,
which would cause data loss.  But since it's a private method and none
of the callers can accept input like that, it's fine.

> +        for tuple in from_to_tuples:
> +            from_rel        = tuple['from_rel']
> +            from_id         = tuple['from_id']
> +            to_rel          = tuple['to_rel']
> +            to_tail         = tuple['to_tail']
> +            to_parent_id    = tuple['to_parent_id']
> +            only_change_inv = tuple['only_change_inv']

I am seeing this pattern a wee bit too often.  If this were a real
tuple, you could do:

for from_rel, from_id, to_rel, to_tail, to_parent_id, only_change_inv\
    in from_to_tuples:

and then skip right to the good part.

> +    def _rollback_move(self, moved):
> +        """Try to rollback a previous move in case of an error in the filesystem.
> +        """

I don't have a specific refactoring in mind, but this looks awfully
similar to _move, and there must be a way to unify them.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFFT2U10F+nu1YWqI0RAunUAJ0QRBsnqZiON+SaW5t2W6WnOFRPDgCfb9Vc
+d2+AUICFT/kutXlcSBuTjI=
=xgWH
-----END PGP SIGNATURE-----