[patch] fix bug 30190 "'bzr cat' requires full path in old revision"

[Robert Collins]

On Mon, 2006-10-16 at 12:09 +0800, Cheuksan Edward Wang wrote:
> 
> Thanks. I have modified the code according to your suggestions and
> added more tests for the exceptions. The new changeset is attached.

I've reviewed this, and I have some UI concerns, as well as a bunch of
more mechanical 'this is how we do things' requests. At the moment, I'm
voting -1 on this.

-Rob

>=== modified file bzrlib/builtins.py
>--- bzrlib/builtins.py
>+++ bzrlib/builtins.py
>@@ -1675,13 +1675,20 @@
> class cmd_cat(Command):
>     """Write a file's text from a previous revision."""
> 
>-    takes_options = ['revision']
>+    takes_options = ['revision',
>+                     Option('old', help='The path name in the old tree.'),
>+                     Option('new', help='The path name in the current tree.')]
>     takes_args = ['filename']

These are likely to need to be standard options and should be globally
registered in options.py. However, I dislike the names as I think they
are too sensitive to the context.
For instance, I can have a tree which I have reverted to revision 50,
and pass in -r 100. In this case, the code will consider 'old' to be
100, and 'new' to be the current working tree, but I might well
internally think of 'old' as the current working tree and 'new' as
revision 100. Lets try to find better options names here.

>     @display_command
>-    def run(self, filename, revision=None):
>+    def run(self, filename, revision=None, old=False, new=False):

I think that having these two parameters which are mutually exclusive is
unneeded : surely one can be the default, and the other selected by
providing it?

We do have a helper that allows this to happen automatically, which I'd
also like to see used - I see it working like this:
 * If the user supplies --old, the file is looked up in the old tree
 else
 * If the file is in the current tree, it is used
 else
 * If the file is in the old tree it is used

 if no file is found, it is an error. This could be done easily by just
searching for the ids in either the (current, old) trees, or the (old, )
tree alone.

I suggest this because while I can imagine wanting to force selection of
something from the selected revision - i.e. the file that was at NEWS in
the last commit, but is not now, and I've replaced the file since... I
can't seem to come up with a sensible use case for 'the file path is in
both the working tree and the specified revision, with different
fileids, and cause an error rather than taking the copy from the working
tree'.

Removing the second option will make the code simpler, *and the UI
simpler too*. (And thats most important IMNSHO).

>=== modified file bzrlib/tests/blackbox/test_cat.py
>--- bzrlib/tests/blackbox/test_cat.py
>+++ bzrlib/tests/blackbox/test_cat.py
>@@ -61,3 +61,36 @@
>         bzr('cat', 'a', retcode=3)
>         bzr('cat', 'a', '-r', 'revno:1:branch-that-does-not-exist', retcode=3)
>         
>+    def test_cat_different_id(self): 
>+        """'cat' works with old and new files"""
>+
>+        def bzr(*args, **kwargs):
>+            return self.run_bzr(*args, **kwargs)[0]
>+

Please use self.run_bzr() rather than a inner function here - this lets
you check stderr and stdout for correctness.

>+        bzr('init')
>+        open('a', 'wb').write('foo\n')

This idiom is not safe because a failure will leave the file object
open, and referenced in the python stack, leading to an inability to
cleanup the test case on windows. Please use self.build_tree or
self.build_tree_contents instead.

>+        open('c', 'wb').write('baz\n')
>+        open('d', 'wb').write('bar\n')
>+        bzr('add', 'a', 'c', 'd')
>+        bzr('commit', '-m', '1')
>+
>+        os.remove('d')
>+        bzr('remove', 'd');
>+        bzr('rename', 'a', 'b');
>+        bzr('rename', 'c', 'a');
>+        bzr('commit', '-m', '2')

I dont think we need to commit here, as the working tree is already
setup after the last rename. That allows us to skip the -r parameter in
the test.
 
I also think that the file names could be more descriptive, so the test
is easier to read.

>+        # new file is not present
>+        bzr('cat', 'c', '-r', '1', '--new', retcode=3)

Please use run_bzr_error here - it checks the error output looks as
desired to prevent regressions in error messages emitted by the UI
layer.

>+        # old file is not present
>+        bzr('cat', 'b', '-r', '1', '--old', retcode=3)
>+
>+        # files are different
>+        bzr('cat', 'a', '-r', '1', retcode=3)
>+
>+        # get to the old file automatically
>+        self.assertEquals(bzr('cat', 'd', '-r', '1'), 'bar\n')

Please put the expected value on the left in assert statements. Also we
tend to use 'assertEqual' not 'assertEquals' - it reads more nicely (to
me at least!).

>+        self.assertEquals(bzr('cat', 'a', '-r', '1', '--old'), 'foo\n')
>+        self.assertEquals(bzr('cat', 'a', '-r', '1', '--new'), 'baz\n')

Please try to use the library api to construct the test case, this gives
better performance and usually a clearer test. I realise much of the
blackbox tests do not look use that today, but thats historical not
current practice.

I've done a quick rewrite of this test as an example - but its
[deliberately] not quite right :).

+    def test_cat_different_id(self): 
+        """'cat' works with old and new files"""
+        tree = self.make_branch_and_tree('a tree')
+        # the files are named after their path in the revision and 
+        # current trees later in the test case
+        # a-rev-tree is special because it appears in both the revision
+        # tree and the working tree
+        self.build_tree_contents([('a-rev-tree', 'foo\n'),
+            ('c-rev', 'baz\n'), ('d-rev', 'bar\n')])
+        tree.lock_write()
+        try:
+            tree.add(['a-rev-tree', 'c-rev', 'd-rev'])
+            tree.commit('add test files')
+            tree.remove(['d-rev'])
+            tree.rename_one('a-rev-tree', 'b-tree')
+            tree.rename_one('c-rev', 'a-rev-tree')
+
+            # 'b-tree' is not present in the old tree.
+            self.run_bzr_error([], 'cat', 'b-tree', '--old')
+
+            # files are different
+            self.run_bzr_error([], 'cat', 'a-rev-tree')
+
+            # get to the old file automatically
+            out, err = self.run_bzr('cat', 'd')
+            self.assertEqual('bar\n', out)
+            self.assertEqual('', err)
+
+            out, err = self.run_bzr('cat', 'a', '--old')
+            self.assertEqual('foo\n', out)
+            self.assertEqual('', err)
+
+            out, err = self.run_bzr('cat', 'a', '--new')
+            self.assertEqual('baz\n', out)
+            self.assertEqual('', err)
+        finally:
+            tree.unlock()


-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061016/c83660e7/attachment.pgp