[PATCH] Re: Feature Request: 'bzr mv --after' to tell bzr that file(s) have already been moved in the working tree
John Arbash Meinel
john at arbash-meinel.com
Tue Nov 7 23:16:15 GMT 2006
Hanns-Steffen Eichenberg wrote:
> Hello,
>
>
> This patch is still a draft, but should be better then the first version.
>
It is starting to look pretty good.
>
> what has changed between version 1 and 2?
> - removed all \t
> - reformattet to 79 columns
Personally, I would rather see:
takes_options = [Option("after",
help="move only the bzr identifier of the file"
" (file has already been moved). Use this"
" flag if bzr is not able to detect this"
" itself."
),
]
But that may just be me. So it isn't anything I would spend too much
time on.
For parameters like 'after' I usually prefer to see:
tree.rename_one(rel_names[0], rel_names[1], after=after)
^^^-named parameter.
> - have read PEP 8 :-)
> - removed an unnessecary API change
> - deprecated one API
> - replaced the dictionaries that i called 'tuples' with a ValueObject
> - fixed some bugs
> - made the code a little bit more robust (i hope so)
> - written some new blackbox tests for mv
> - switched from pure Win32 to cygwin for development (feels more
> comfortable)
>
Interesting. It probably is a little bit more of a match. I always
avoided it because it runs about 50% slower. At least on my machine
cygwin adds a lot of overhead.
>
> what i have not done:
> - run the complete testsuite. i still have problems running the
> selftest. I need to spend some time on it.
>
>
> Again, thank you for the first review. I hope this version is an
> improvement.
>
>
> Ciao,
> Steffen
>
For tests, we are trying to simplify them, so that each test only is
testing 1 thing, rather than testing several things in each test. It's
something that I'm not always good at, but I'm learning from Robert. So
I would recommend a few things here:
v- You call 'build_tree([..., 'b'...])' but then later you os.rename()
over top of it. At the very least you probably don't want to create a
file you overwrite.
+
+ def test_mv_inv_simple(self):
+ self.build_tree(['a', 'b', 'c', 'd', 'e', 'sub1/', 'sub2/'])
+ tree = self.make_branch_and_tree('.')
+ tree.add(['a', 'c', 'd', 'e', 'sub1'])
+ tree.commit('initial commit')
+
+ os.rename('a', 'b')
+ self.run_bzr('mv', 'a', 'b')
+ self.failIfExists('a')
+ self.failUnlessExists('b')
+
+ os.remove('c')
+ self.run_bzr_error(
+ ["^bzr: ERROR: can't rename: new name .* is already
versioned$"],
+ 'mv', 'c', 'd')
+ self.failIfExists('c')
+ self.failUnlessExists('d')
+
+ os.rename('d', 'sub1/d')
+ self.run_bzr('mv', 'd', 'sub1/d')
+ self.failIfExists('d')
+ self.failUnlessExists('sub1/d')
+
+ os.rename('e', 'sub2/e')
+ self.run_bzr_error(
+ ["^bzr: ERROR: can't determine destination directory id for
.*$"],
+ 'mv', 'e', 'sub2/e')
+ self.failIfExists('e')
+ self.failUnlessExists('sub2/e')
^- I would break this up into multiple tests. Something like:
def test_mv_already_moved(self):
self.build_tree(['a'])
tree = self.make_branch_and_tree('.')
# By passing in a file id to use, we can later check that the
# internals are aware the file has (or hasn't) moved.
tree.add('a', 'a-id')
tree.commit('a')
os.rename('a', 'b')
# Renaming a => b will detect that it has already been done and update
# the inventory entry.
self.run_bzr('mv', 'a', 'b')
self.assertEqual('a-id', tree.path2id('b'))
# I'm not sure these are needed
self.failIfExists('a')
self.failUnlessExists('b')
def test_mv_source_missing_target_versioned(self):
self.build_tree(['a', 'b'])
tree = self.make_branch_and_tree('.')
tree.add(['a', 'b'], ['a-id', 'b-id'])
tree.commit('a and b')
os.remove('a')
self.run_bzr_error(
["^bzr: ERROR: can't rename: new name .* is already versioned$"],
'mv', 'a', 'b')
# The inventory should not be modified.
self.assertEqual('a-id', tree.path2id('a'))
self.assertEqual('b-id', tree.path2id('b'))
By the way, it is a bug that the "u" of u'b' is being exposed, so you
are welcome to use .* for now, but we really should fix that. But making
exceptions Unicode safe is outside of what we are asking you to do here.
def test_mv_already_moved_into_versioned_subdir(self):
and
def test_mv_already_moved_unversioned_subdir(self):
That way each test is testing exactly 1 piece of the move logic, which
makes it clearer what is going on, and easier to track down an fix bugs.
v- You had 2 spaces between the functions, PEP8 has 1 space between
class scope definitions, and 2 spaces at the module scope. So one less
here (you have it right later) I would also refactor this test a bit
into smaller tests.
+
+
+ def test_mv_inv_dir(self):
+ self.build_tree(['a1', 'a2', 'b1', 'b2', 'c1', 'c2', 'd1', 'd2',
+ 'e1', 'e2', 'sub1/', 'sub2/'])
+ tree = self.make_branch_and_tree('.')
+ tree.add(['a1', 'a2', 'b1', 'b2', 'sub1'])
+ tree.commit('initial commit')
+
+ os.rename('a1', 'sub1/a1')
+ self.run_bzr('mv', 'a1', 'a2', 'sub1')
+ self.failIfExists('a1')
+ self.failIfExists('a2')
+ self.failUnlessExists('sub1/a1')
+ self.failUnlessExists('sub1/a2')
+
+ os.rename('b1', 'sub2/b1')
+ self.run_bzr_error(
+ ["^bzr: ERROR: destination .* is not a versioned directory$"],
+ 'mv', 'b1', 'b2', 'sub2')
+ self.failIfExists('b1')
+ self.failUnlessExists('b2')
+ self.failUnlessExists('sub2/b1')
+ self.failIfExists('sub2/b2')
+
v- A doc string or comment here indicating this is simulating where the
user does:
"mv a b; touch a; bzr mv a b" might be helpful here.
+ def test_mv_inv_simple_touched(self):
+ self.build_tree(['a', 'b'])
+ tree = self.make_branch_and_tree('.')
+ tree.add(['a'])
+ tree.commit('initial commit')
+
+ self.run_bzr_error(
+ ["^bzr: ERROR: can't rename: both, old name .* and new name .*"
+ " exist. Use option '--after' to force rename."],
+ 'mv', 'a', 'b')
+ self.failUnlessExists('a')
+ self.failUnlessExists('b')
+
+ self.run_bzr('mv', 'a', 'b', '--after')
+ self.failUnlessExists('a')
+ self.failUnlessExists('b')
+
...
@needs_tree_write_lock
- def move(self, from_paths, to_name):
+ def move(self, from_paths, to_dir, after=False, **kwargs):
"""Rename files.
- to_name must exist in the inventory.
+ to_dir must exist in the inventory.
- If to_name exists and is a directory, the files are moved into
+ If to_dir exists and is a directory, the files are moved into
it, keeping their old names.
- Note that to_name is only the last component of the new name;
+ Note that to_dir is only the last component of the new name;
this doesn't change the directory.
+ For each entry in from_paths the move mode will be determined
+ independently.
+
+ The first mode moves the file in the filesystem and updates the
+ inventory. The second mode only updates 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.
+
+ move 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.
+
+ Everything else results in an error.
+
This returns a list of (from_path, to_path) pairs for each
entry that is moved.
"""
- result = []
- ## TODO: Option to move IDs only
+ rename_entries = []
+ rename_tuples = []
+
+ # check for deprecated use of signature
+ if to_dir is None:
+ to_dir = kwargs.get('to_name', None)
+ if to_dir is None:
+ raise TypeError('You must supply a target directory')
+ else:
+ symbol_versioning.warn('The parameter to_name was
deprecated'
+ ' in version 0.13. Use to_dir
instead',
+ DeprecationWarning)
We should also have a test that to_name is deprecated but still works.
That would probably go into bzrlib/test_workingtree.py. You can use
self.callDeprecated() to catch the warning, and make sure the right
warning is issued, and then check the function still has the right effect.
...
+ class _RenameEntry(object):
+ def __init__(self, from_rel, from_id, from_tail, from_parent_id,
+ to_rel, to_tail, to_parent_id):
+ self.from_rel = from_rel
+ self.from_id = from_id
+ self.from_tail = from_tail
+ self.from_parent_id = from_parent_id
+ self.to_rel = to_rel
+ self.to_tail = to_tail
+ self.to_parent_id = to_parent_id
+ self.only_change_inv = False
+
+ def set_only_change_inv(self, only_change_inv):
+ self.only_change_inv = only_change_inv
+
I think it is better if _RenameEntry is just a private class at module
scope, rather than a nested class.
Or at the very least, I would define a nested class at the start of the
encompassing scope, rather than in the middle.
Oh, and I would like to see the error messages unified a little bit
more. I really don't see why we need move() and rename_one() to be
separate functions. I saw that 'rename_one' seems to raise:
"can't determine destination directory id for"
rather than
"destination %r is not a versioned"
I think having them both say 'destination not versioned' would be the best.
Also, the check:
if to_dir_id is None and to_dir != '':
should probably be:
if to_dir_id is None:
At one point, the root of a tree didn't have a file id, but it should
now, so we don' need to special case "to_dir = ''".
John
=:->
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061107/622c429e/attachment.pgp
More information about the bazaar
mailing list