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

Proposed by Marius Kruger on 2009-05-21
Status: Superseded
Proposed branch: lp:~amanica/bzr/mv_after
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 78 lines (has conflicts)
Text conflict in NEWS
To merge this branch: bzr merge lp:~amanica/bzr/mv_after
Reviewer Review Type Date Requested Status
John A Meinel Resubmit on 2009-07-03
Robert Collins (community) 2009-05-21 Needs Fixing on 2009-06-01
Review via email: mp+6733@code.launchpad.net

This proposal has been superseded by a proposal from 2009-07-03.

To post a comment you must log in.
Marius Kruger (amanica) wrote :

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.

lp:~amanica/bzr/mv_after updated on 2009-05-21
4374. By Marius Kruger on 2009-05-21

merge with bzr.dev to get news in its place

Robert Collins (lifeless) wrote :

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) wrote :

-----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-----

lp:~amanica/bzr/mv_after updated on 2009-07-03
4375. By Marius Kruger on 2009-07-03

merge with bzr.dev moving my news up

4376. By Marius Kruger on 2009-07-03

clone the the inventory entry when putting it back

Marius Kruger (amanica) 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))

John A Meinel (jameinel) wrote :

-----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: Resubmit

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-06-15 15:20:24 +0000
3+++ NEWS 2009-06-16 02:37:37 +0000
4@@ -81,6 +81,7 @@
5 Improvements
6 ************
7
8+<<<<<<< TREE
9
10 * ``--development6-rich-root`` can now stack. (Modulo some smart-server
11 bugs with stacking and non default formats.)
12@@ -110,6 +111,11 @@
13 (Alexander Belchenko, Jelmer Vernooij, John Arbash Meinel)
14
15
16+=======
17+* Can now rename/move files even if they have been removed from the inventory.
18+ (Marius Kruger)
19+
20+>>>>>>> MERGE-SOURCE
21 Bug Fixes
22 *********
23
24
25=== modified file 'bzrlib/tests/workingtree_implementations/test_rename_one.py'
26--- bzrlib/tests/workingtree_implementations/test_rename_one.py 2009-03-23 14:59:43 +0000
27+++ bzrlib/tests/workingtree_implementations/test_rename_one.py 2009-06-16 02:37:37 +0000
28@@ -220,6 +220,25 @@
29 self.assertTreeLayout([('', root_id), ('a', 'a-id'), ('b', 'b-id')],
30 tree.basis_tree())
31
32+ def test_rename_one_after_source_removed(self):
33+ """Rename even if the source was removed from the inventory already"""
34+ tree = self.make_branch_and_tree('.')
35+ self.build_tree(['a', 'b/'])
36+ tree.add(['a', 'b'], ['a-id', 'b-id'])
37+ tree.commit('initial', rev_id='rev-1')
38+ root_id = tree.get_root_id()
39+ os.rename('a', 'b/foo')
40+ tree.remove(['a'])
41+
42+ self.assertTreeLayout([('', root_id), ('b', 'b-id')], tree)
43+ # We don't need after=True as long as source is missing and target
44+ # exists.
45+ tree.rename_one('a', 'b/foo')
46+ self.assertTreeLayout([('', root_id), ('b', 'b-id'),
47+ ('b/foo', 'a-id')], tree)
48+ self.assertTreeLayout([('', root_id), ('a', 'a-id'), ('b', 'b-id')],
49+ tree.basis_tree())
50+
51 def test_rename_one_after_no_target(self):
52 tree = self.make_branch_and_tree('.')
53 self.build_tree(['a', 'b/'])
54
55=== modified file 'bzrlib/workingtree.py'
56--- bzrlib/workingtree.py 2009-06-15 15:47:45 +0000
57+++ bzrlib/workingtree.py 2009-06-16 02:37:37 +0000
58@@ -1462,9 +1462,17 @@
59 from_tail = splitpath(from_rel)[-1]
60 from_id = inv.path2id(from_rel)
61 if from_id is None:
62- raise errors.BzrRenameFailedError(from_rel,to_rel,
63- errors.NotVersionedError(path=str(from_rel)))
64- from_entry = inv[from_id]
65+ # if file is missing in the inventory maybe it's in the basis_tree
66+ basis_tree = self.branch.basis_tree()
67+ from_id = basis_tree.path2id(from_rel)
68+ if from_id is None:
69+ raise errors.BzrRenameFailedError(from_rel,to_rel,
70+ errors.NotVersionedError(path=str(from_rel)))
71+ # put entry back in the inventory so we can rename it
72+ from_entry = basis_tree.inventory[from_id]
73+ inv.add(from_entry)
74+ else:
75+ from_entry = inv[from_id]
76 from_parent_id = from_entry.parent_id
77 to_dir, to_tail = os.path.split(to_rel)
78 to_dir_id = inv.path2id(to_dir)