Merge lp:~amanica/bzr/mv_after into lp:~bzr/bzr/trunk-old

[Marius Kruger]

[Marius Kruger (amanica)]

Allow renaming of files already removed from the inventory,
by quickly copying the inventory entry from the basis_tree before doing the rename.
(I'm not too clued up with the inventory so I hope I didn't commit any grave sins).

== history/motivation ==
It happened to me again today that I tried to do a `bzr mv --after old new` but bzr insists that `old is not versioned`. So I debuged it and realised that indeed `old` was
not present in the inventory. This can typically happen if the user copies the file
to the new location an then `bzr remove old`. This happens quite easily if you deleted
it from for eg. eclipse with the bzr plugin installed where you don't even know that it
automatically does a bzr remove for you in the backend.

Once you are in this situation though you start to bang your head against the keyboard
out of frustration after a while. Because according to `bzr status` a file that has
been removed or `bzr remove`d looks the same but for the subtle difference that in the
latter case it is actually removed from the inventory already.

I think in this case we should just go ahead and do what the user means, because the workaround it messy and totally undiscoverable:
`bzr revert old; bzr mv --after old new; rm old`
(especially if your file paths are longer than 80 characters)

I can't find a bug related to this, but I think I've seen this problem on the mailing list.

[Robert Collins (lifeless)]

Its not safe to mutate the inventory in this way. You're aliasing the entry in the basis with the working tree, and later mutations will alter both inventories. It will also be copying all the children in which will fail badly if the children weren't all removed.

So the right way to do this is to create a new inventory entry with the right file id, parent id, kind and basename.

review: Needs Fixing

[John A Meinel (jameinel)]

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

Robert Collins wrote:
> Review: Needs Fixing
> Its not safe to mutate the inventory in this way. You're aliasing the entry in the basis with the working tree, and later mutations will alter both inventories. It will also be copying all the children in which will fail badly if the children weren't all removed.
>
> So the right way to do this is to create a new inventory entry with the right file id, parent id, kind and basename.

Isn't InventoryEntry.copy() the right way to do that? I know it
preserves file_id, revision, and *doesn't* preserve .children for
directories....

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkoj+ioACgkQJdeBCYSNAAPWDgCffuwm7b1ai26o4oXTkrAr7rgL
PKwAn3nUPFHL7Sg/TmojVSok1/HyBK/o
=Tr6D
-----END PGP SIGNATURE-----

[Marius Kruger (amanica)]

> Robert Collins wrote:
> > Review: Needs Fixing
> > Its not safe to mutate the inventory in this way. You're aliasing the entry
> in the basis with the working tree, and later mutations will alter both
> inventories. It will also be copying all the children in which will fail badly
> if the children weren't all removed.
> >
> > So the right way to do this is to create a new inventory entry with the
> right file id, parent id, kind and basename.
>
> Isn't InventoryEntry.copy() the right way to do that? I know it
> preserves file_id, revision, and *doesn't* preserve .children for
> directories....

I followed John's advice and just .copy() the InventoryEntry now before re-inserting it, which seems correct.
(I tried to add a test to check if it was really cloned, but could not figure out how to compare them in the end. It may be a little overkill in any case.)

I pushed my changes to this branch now, but I can't figure out how to re-submit for review and to update the diff.

(Sorry for being quiet on this. I've been sick, on holiday and busy (in that order))

[John A Meinel (jameinel)]

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

Marius Kruger wrote:
>> Robert Collins wrote:
>>> Review: Needs Fixing
>>> Its not safe to mutate the inventory in this way. You're aliasing the entry
>> in the basis with the working tree, and later mutations will alter both
>> inventories. It will also be copying all the children in which will fail badly
>> if the children weren't all removed.
>>> So the right way to do this is to create a new inventory entry with the
>> right file id, parent id, kind and basename.
>>
>> Isn't InventoryEntry.copy() the right way to do that? I know it
>> preserves file_id, revision, and *doesn't* preserve .children for
>> directories....
>
> I followed John's advice and just .copy() the InventoryEntry now before re-inserting it, which seems correct.
> (I tried to add a test to check if it was really cloned, but could not figure out how to compare them in the end. It may be a little overkill in any case.)
>
> I pushed my changes to this branch now, but I can't figure out how to re-submit for review and to update the diff.
>
> (Sorry for being quiet on this. I've been sick, on holiday and busy (in that order))

 review resubmit
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkpNc/YACgkQJdeBCYSNAANSaQCfVoH5L72yOiBPXwbAgffDRmWx
FQ8AoIfiRDqucFVlxfTlyWtrqF9UcbIF
=RGNC
-----END PGP SIGNATURE-----

review: Needs Resubmitting

[Ian Clatworthy (ian-clatworthy)]

This looks ok to me but I wouldn't mind a second opinion. My main concern is that we've been moving away from methods that mutate inventories (like inv.add()) in favour of generating new ones via create_by_apply_delta(). I *think* the code is all ok in this context but I'd like John and/or Robert (say) to confirm that.

review: Approve

[Ian Clatworthy (ian-clatworthy)]

I'll go ahead and merge this.

[Marius Kruger (amanica)]

2009/7/22 Ian Clatworthy <email address hidden>:
> I'll go ahead and merge this.
> --
> https://code.edge.launchpad.net/~amanica/bzr/mv_after/+merge/8179
> You are the owner of lp:~amanica/bzr/mv_after.
>

thanks!

[Robert Collins (lifeless)]

On Tue, 2009-07-14 at 06:18 +0000, Ian Clatworthy wrote:
> Review: Approve
> This looks ok to me but I wouldn't mind a second opinion. My main concern is that we've been moving away from methods that mutate inventories (like inv.add()) in favour of generating new ones via create_by_apply_delta(). I *think* the code is all ok in this context but I'd like John and/or Robert (say) to confirm that.

This function already works with full inventories.

It would be good to make it issue an inventory delta rather than direct
mutation, but this patch doesn't need to block on that.

-Rob

=== modified file 'NEWS'
--- NEWS	2009-07-22 10:01:36 +0000
+++ NEWS	2009-07-22 23:36:34 +0000
@@ -325,6 +325,9 @@
   to 1.1 seconds. The improvement for ``bzr ls -r-1`` is more
   substantial dropping from 54.3 to 1.1 seconds. (Ian Clatworthy)
 
+* Can now rename/move files even if they have been removed from the inventory.
+  (Marius Kruger)
+
 * Improve "Path(s) are not versioned" error reporting for some commands.
   (Benoît PIERRE)
 
 
=== modified file 'bzrlib/tests/per_workingtree/test_rename_one.py'
--- bzrlib/tests/per_workingtree/test_rename_one.py	2009-07-10 07:14:02 +0000
+++ bzrlib/tests/per_workingtree/test_rename_one.py	2009-07-22 23:36:34 +0000
@@ -220,6 +220,25 @@
         self.assertTreeLayout([('', root_id), ('a', 'a-id'), ('b', 'b-id')],
                               tree.basis_tree())
 
+    def test_rename_one_after_source_removed(self):
+        """Rename even if the source was removed from the inventory already"""
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['a', 'b/'])
+        tree.add(['a', 'b'], ['a-id', 'b-id'])
+        tree.commit('initial', rev_id='rev-1')
+        root_id = tree.get_root_id()
+        os.rename('a', 'b/foo')
+        tree.remove(['a'])
+
+        self.assertTreeLayout([('', root_id), ('b', 'b-id')], tree)
+        # We don't need after=True as long as source is missing and target
+        # exists.
+        tree.rename_one('a', 'b/foo')
+        self.assertTreeLayout([('', root_id), ('b', 'b-id'),
+                               ('b/foo', 'a-id')], tree)
+        self.assertTreeLayout([('', root_id), ('a', 'a-id'), ('b', 'b-id')],
+                              tree.basis_tree())
+
     def test_rename_one_after_no_target(self):
         tree = self.make_branch_and_tree('.')
         self.build_tree(['a', 'b/'])
 
=== modified file 'bzrlib/workingtree.py'
--- bzrlib/workingtree.py	2009-07-06 21:13:30 +0000
+++ bzrlib/workingtree.py	2009-07-22 23:36:34 +0000
@@ -1476,9 +1476,17 @@
         from_tail = splitpath(from_rel)[-1]
         from_id = inv.path2id(from_rel)
         if from_id is None:
-            raise errors.BzrRenameFailedError(from_rel,to_rel,
-                errors.NotVersionedError(path=str(from_rel)))
-        from_entry = inv[from_id]
+            # if file is missing in the inventory maybe it's in the basis_tree
+            basis_tree = self.branch.basis_tree()
+            from_id = basis_tree.path2id(from_rel)
+            if from_id is None:
+                raise errors.BzrRenameFailedError(from_rel,to_rel,
+                    errors.NotVersionedError(path=str(from_rel)))
+            # put entry back in the inventory so we can rename it
+            from_entry = basis_tree.inventory[from_id].copy()
+            inv.add(from_entry)
+        else:
+            from_entry = inv[from_id]
         from_parent_id = from_entry.parent_id
         to_dir, to_tail = os.path.split(to_rel)
         to_dir_id = inv.path2id(to_dir)