[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:
> I will change that, rename the new method and deprecate the old one.

Have a look at bzrlib/symbol_versioning.py, then

>     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)
> 
> 
> Did it that way in my first version, changed it to operate on a list
> instead. There are
> two reasons for it.
> 
> 1) performance. i did not want to call a method inside of a loop if i
> can have
>     the method make that loop.

True, but I think it's unlikely that determine_mv_mode will be a
significant bottleneck.

>     I used dictionaries instead of real tuples, because i augment the
> data in
>     the check-method and tuples are immutable

Ah.  I suppose using a list instead of a tuple would be even worse.

>     ^^^ 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 are right. But this is not the place to raise an error. I must set
> that entry
> to None instead.

I think if from_id is None, you will error eventually, anyhow.  But as
long as it's handled, I'm happy.

>     > -            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.
> 
> 
> I understand the logic expression, but i do not understand why the
> modified flag should not be set if 'len(from_path) <= 0' ??

If from_paths <= 0, the whole WorkingTree.move is a no-op, so it won't
change the inventory.

>     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.
> 
> 
> No, i think the method should handle that case. You never know who will use
> this method in the future. It is only on line of code to make it more
> safe. Thank you
> for identifying that bug. Will fix it.

I'm not sure what you have in mind.  Detecting that situation and
raising an exception would be fairly short.  Correctly applying that
requires doing two renames to avoid renaming something over top of
something else.  That starts to get into bzrlib/transform.py territory.

>     > +    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.
> 
> 
> The only way that comes to my mind now is passing a flag to the
> method. But i consider changing the semantic of a method with a
> flag bad practice, so i will not do that. Maybe you have a better idea?

Inverting behavior with a flag doesn't seem like bad practice to me.
But you could also factor the actual rename code into a separate method.

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

iD8DBQFFT6zD0F+nu1YWqI0RApBKAKCHAv4x63JZS4by62v8Zr5HmXWzCgCdF4jy
PbYKh5SnWMciArxfJOebpKo=
=ErSk
-----END PGP SIGNATURE-----