[MERGE] --undelete option for revert
Martin Pool
mbp at canonical.com
Wed Mar 25 06:06:40 GMT 2009
Martin Pool has voted resubmit.
Status is now: Resubmit
Comment:
Sorry for the delay in further review of this. I'm still keen to get it
in.
>Also, for my understanding, is it possible to do this by sending this
>using the "propose merge" feature of launchpad or does it need to be a
>mail to the list. Thanks.
Yes, you can do that. The bulk of them are here on the list but the
Launchpad review support is looking increasingly good and it's worth
trying.
- forget_merges=None):
+ forget_merges=None, limit = None, undelete = None):
For future reference, in keyword arguments there should be no space
around the equals sign.
+ The --undelete option can be used to undelete a previously removed
file.
+ This option searches from new revision to old for the specified
file(s)
+ and reverts to the last committed revision in the specified range.
+ The range of search may be specified using the standard revision
spec
+ (see: "help revisionspec") or the --limit option. The limit option
+ searches the last N revision starting with the latest. If no range
is
+ specified, last 20 revisons are checked.
"""
+ def _undelete_all_files(self, file_list, revision, limit):
+ DEFAULT_LIMIT = -21
+ if (limit != None) and (revision != None):
+ raise errors.BzrCommandError(
+ "--limit and --revision are mutually exclusive"
+ " with --undelete")
OK, that sounds good. But then why have a special option for --limit --
why not just let people say -r-50 if they want to check the last 50?
That would also let you get rid of the code for _get_revision_from_limit
and similar stuff that seems a bit inelegant to have be specific to this
command, and it will cut out a lot of tests that cover both.
+ def _undelete_all_files(self, file_list, revision, limit):
This method is a bit strangely named; it doesn't undelete all files in
the tree as you might expect but rather deletes named files. But if you
get rid of the special meaning of limit there won't be much left and so
you can just remove it, and call from cmd_revert into say
_undelete_named_files.
+ bd = bzrdir.BzrDir.open_containing('.')[0]
+ br = bd.open_branch()
+
+ for filename in file_list:
+ self._undelete_file(filename, revision, br)
+
+ def _undelete_file(self, filename, revision, br):
+ wt = WorkingTree.open_containing('.')[0]
+ filepath = wt.relpath(os.path.abspath(filename))
+
Generally you shouldn't be opening '.', but rather using one of the
methods that opens the tree containing the first named file. That lets
people pass a full path to the file even if they're standing outside the
tree. Also, you shouldn't need to open the branch and workingtree
separately - just get the workingtree's branch.
You should write_lock the workingtree and read lock the branch for the
whole scope of the command, rather than locking the branch for every
file you search. That will let bzr cache some information.
+ startno = start_rev.revno
+ endno = end_rev.revno - 1
+ revs = range(startno, endno, -1)
+ pb = progress.ProgressBar()
+ for count, i in enumerate(revs):
+ pb.update(filename, count, len(revs))
+
+ revision_spec = RevisionSpec.from_string(str(i))
+
+ try:
+ revision_id = revision_spec.as_revision_id(br)
+ except errors.InvalidRevisionSpec:
+ self.outf.write("revisions not found for '" + filename
+ "'\n")
+ return
+
+ tree = revision_spec.as_tree(br)
It seems like the branch ought to be able to give you the list of
revision ids within that range rather than you needing to roundtrip it
like this. I don't recall how off hand but if you can't find it we can
look.
You should now get the progress bar from ui.nested_progress_bar.
+ wt.revert(filenames = [filepath], old_tree=tree)
Again no spaces.
I wouldn't necessarily block this on it, but I think this would be a bit
more cleanly written as a method that walks back through history looking
for the last occurrence of files with a particular name. Ideally this
would take a list of filenames and return a list of (filename, fileid,
revision-where-present) tuples. Then you can hook this up to undelete
but it could be used elsewhere, and it can be tested in isolation.
Thanks for adding tests.
+
+from unittest import makeSuite
You should generally use bzr's unittest not python's one. However you
don't seem to actually use makeSuite. :)
+def get_random_name(length = 15):
+ CHARS='0123456789abcdefghijklmnopqrstuvwxyz_.'
+ name = ''
+ for i in range(length):
+ name += random.choice(CHARS)
+ return name
Using random variables in tests is a bad idea because it's likely to
make tests intermittently pass or fail depending on what values they
happen to hit. There is maybe room for pseudorandom data in other kinds
of testing, if you're trying to check many different inputs to see if
anything fails but that's not really what we do in unit tests.
+
+class TestUndelete(TestCaseWithTransport):
+ def touch(self, filename, content = ''):
+ file(filename, 'wb').write(content)
+
+ def add_and_commit_file(self, filename, content = ''):
+ self.touch(filename, content)
+ self.run_bzr('add ' + filename)
+ self.run_bzr('commit -m "added ' + filename + '"')
+
+ def add_new_versions(self, max_revs = 10):
+ """method to add a few checkins to create deeper history"""
+ for i in range(max_revs):
+ name = get_random_name()
+ self.touch(name)
+ self.run_bzr('add ' + name)
+ self.run_bzr('commit -m "added ' + name + '"')
I think you can do this more easily and faster using branchbuilder, see
eg test_ancestry. Generally speaking we like to set up for tests using
apis rather than run_bzr because it's faster and gives better
programmatic error reporting.
+ def test_undelete_basic(self):
+ """add file and root. delete and undelete"""
+ self.run_bzr('init')
+
+ # add initial data
+ self.touch('a')
+ self.touch('b')
+ self.run_bzr('add .')
+
+ self.run_bzr('commit -m initial')
+ self.run_bzr('rm a')
+ self.run_bzr('commit -m "removed a"')
+
+ # ensure 'a' is removed
+ assert not os.path.lexists('a')
+
+ self.add_new_versions()
+
+ # undelete a
+ self.run_bzr('revert --undelete a')
+ assert os.path.lexists('a')
+
It would be good if this also checked that the file has the correct text
and file id, which is a bit easier to do if you use the tree api. Also,
you can do
out, err = run_bzr(...)
and then check the expected output is given.
There is also an assertFileExists in our TestCase which gives a better
message if it's not there.
+ # check contents of file are intact
+ assert DATA == open('a','r').read()
+
You shouldn't use assert statements because they don't run when Python
is run with optimization turned on (-O). And in fact trying to use them
should cause a test_source test to fail.
These docstrings would look better if they're in regular sentence case,
as the rest of them are.
+ def test_undelete_dot(self):
+ """if user does 'revert --undelete .' is should not do anything
+ remove a file w/o commit. this removes the file from disk.
+ undelete .
+ ensure file does not exist (revert would have brought
+ the file back)
+ """
I can't completely understand that comment, can you clarify it?
+
+ # do some writes to 'a'
+ DATA = 'hello'
+ f = open('a', 'w')
+ f.write(DATA)
+ f.close()
+ # note that this change is not committed
That's better done with build_tree. And file handles, unlike in
perl(?), shouldn't be in uppercase.
+ # undelete a
+ self.run_bzr('revert --undelete a')
+ assert not os.path.lexists('a')
Maybe it should return retcode=1 in this case?
+ # undelete a
+ self.assertRaises(errors.BzrCommandError,
+ commands.run_bzr, ['revert --undelete -l 10 -r -5.. a'])
I think you can do this more cleanly with self.run_bzr(...., retcode=3)
So, that's quite a lot of comments, but I would be happy to look at an
update or answer questions about what to do next.
For details, see:
http://bundlebuggy.aaronbentley.com/project/bzr/request/%3Cb87b89280902251019u212be32bv5e1b33fbe3dea83c%40mail.gmail.com%3E
Project: Bazaar
More information about the bazaar
mailing list