Bazaar

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

  1. mv_after
  2. Merge into trunk-old
Proposed by Marius Kruger
Status: Merged
Approved by: Ian Clatworthy
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~amanica/bzr/mv_after
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 68 lines
To merge this branch: bzr merge lp:~amanica/bzr/mv_after
Reviewer Review Type Date Requested Status
Ian Clatworthy Approve
Review via email: mp+8179@code.launchpad.net

This proposal supersedes a proposal from 2009-05-21.

To post a comment you must log in.
Revision history for this message
Marius Kruger (amanica) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Robert Collins (lifeless) wrote : Posted in a previous version of this proposal

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
Revision history for this message
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Marius Kruger (amanica) wrote : Posted in a previous version of this proposal

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

Revision history for this message
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

-----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
Revision history for this message
Ian Clatworthy (ian-clatworthy) wrote :

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
Revision history for this message
Ian Clatworthy (ian-clatworthy) wrote :

I'll go ahead and merge this.

Revision history for this message
Marius Kruger (amanica) wrote :

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!

Revision history for this message
Robert Collins (lifeless) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-07-22 10:01:36 +0000
3+++ NEWS 2009-07-22 23:36:34 +0000
4@@ -325,6 +325,9 @@
5 to 1.1 seconds. The improvement for ``bzr ls -r-1`` is more
6 substantial dropping from 54.3 to 1.1 seconds. (Ian Clatworthy)
7
8+* Can now rename/move files even if they have been removed from the inventory.
9+ (Marius Kruger)
10+
11 * Improve "Path(s) are not versioned" error reporting for some commands.
12 (Benoît PIERRE)
13
14
15=== modified file 'bzrlib/tests/per_workingtree/test_rename_one.py'
16--- bzrlib/tests/per_workingtree/test_rename_one.py 2009-07-10 07:14:02 +0000
17+++ bzrlib/tests/per_workingtree/test_rename_one.py 2009-07-22 23:36:34 +0000
18@@ -220,6 +220,25 @@
19 self.assertTreeLayout([('', root_id), ('a', 'a-id'), ('b', 'b-id')],
20 tree.basis_tree())
21
22+ def test_rename_one_after_source_removed(self):
23+ """Rename even if the source was removed from the inventory already"""
24+ tree = self.make_branch_and_tree('.')
25+ self.build_tree(['a', 'b/'])
26+ tree.add(['a', 'b'], ['a-id', 'b-id'])
27+ tree.commit('initial', rev_id='rev-1')
28+ root_id = tree.get_root_id()
29+ os.rename('a', 'b/foo')
30+ tree.remove(['a'])
31+
32+ self.assertTreeLayout([('', root_id), ('b', 'b-id')], tree)
33+ # We don't need after=True as long as source is missing and target
34+ # exists.
35+ tree.rename_one('a', 'b/foo')
36+ self.assertTreeLayout([('', root_id), ('b', 'b-id'),
37+ ('b/foo', 'a-id')], tree)
38+ self.assertTreeLayout([('', root_id), ('a', 'a-id'), ('b', 'b-id')],
39+ tree.basis_tree())
40+
41 def test_rename_one_after_no_target(self):
42 tree = self.make_branch_and_tree('.')
43 self.build_tree(['a', 'b/'])
44
45=== modified file 'bzrlib/workingtree.py'
46--- bzrlib/workingtree.py 2009-07-06 21:13:30 +0000
47+++ bzrlib/workingtree.py 2009-07-22 23:36:34 +0000
48@@ -1476,9 +1476,17 @@
49 from_tail = splitpath(from_rel)[-1]
50 from_id = inv.path2id(from_rel)
51 if from_id is None:
52- raise errors.BzrRenameFailedError(from_rel,to_rel,
53- errors.NotVersionedError(path=str(from_rel)))
54- from_entry = inv[from_id]
55+ # if file is missing in the inventory maybe it's in the basis_tree
56+ basis_tree = self.branch.basis_tree()
57+ from_id = basis_tree.path2id(from_rel)
58+ if from_id is None:
59+ raise errors.BzrRenameFailedError(from_rel,to_rel,
60+ errors.NotVersionedError(path=str(from_rel)))
61+ # put entry back in the inventory so we can rename it
62+ from_entry = basis_tree.inventory[from_id].copy()
63+ inv.add(from_entry)
64+ else:
65+ from_entry = inv[from_id]
66 from_parent_id = from_entry.parent_id
67 to_dir, to_tail = os.path.split(to_rel)
68 to_dir_id = inv.path2id(to_dir)