[patch] 'bzr mv' with no arguments breaks

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Wouter van Heyst wrote:

...

> I've cleaned the patch up a bit, I'll see if I can find out where the
> bug in workingtree is, but rereading the inventory works for now.
> 
> Wouter van Heyst
> 
> 

...

> +class TestMove(TestCaseWithTransport):
> +
> +    def test_mv_modes(self):
> +        """Test two modes of operation for mv"""
> +        tree = self.make_branch_and_tree('.')
> +        files = self.build_tree(['a', 'c', 'subdir/'])
> +        tree.add(['a', 'c', 'subdir'])
> +
> +        self.run_bzr('mv', 'a', 'b')
> +        self.run_bzr('mv', 'b', 'subdir')
> +        self.run_bzr('mv', 'subdir/b', 'a')
> +        self.run_bzr('mv', 'a', 'c', 'subdir')
> +        self.run_bzr('mv', 'subdir/a', 'subdir/newa')

We probably want to check that files are where we say they should be at
this point. Using self.failIfExists() and self.failUnlessExists()
It isn't critical, though.

...

> +    def test_mv_invalid(self):
> +        tree = self.make_branch_and_tree('.')
> +        self.build_tree(['test.txt', 'sub1/'])
> +        tree.add(['test.txt'])
> +
> +        self.run_bzr_error(
> +            ["^bzr: ERROR: destination u'sub1' is not a versioned directory$"],
> +            'rename', 'test.txt', 'sub1')
> +        
> +        self.run_bzr_error(
> +            ["^bzr: ERROR: can't determine destination directory id for u'sub1'$"],
> +            'rename', 'test.txt', 'sub1/hello.txt')

I'm glad to see this being checked, since it shows that we probably
aren't giving the right error. I think we should actually be giving
'sub1' is not a versioned directory.

Also, looking at it, we shouldn't be using %r, we should be using %s,
because u'sub1' is confusing to anyone who isn't a python programmer.

I don't expect you to fix this, but if you could make a TODO out of it,
that would be nice.

> +        
> +        self.run_bzr_error(
> +            ["^bzr: ERROR: destination u'sub1' is not a versioned directory$"],
> +            'move', 'test.txt', 'sub1')

Also, these are testing 'move' and 'rename', not 'mv'. They are aliases
for 'mv', but I don't know if they are stable aliases.

(At one point 'rename' would change the name of a file, but not change
its directory, and 'move' would move a file, but not change its name.)

I probably would just switch to testing just 'mv', and then this last
check is redundant.


> +    
> +    def test_mv_dirs(self):
> +        tree = self.make_branch_and_tree('.')
> +        self.build_tree(['hello.txt', 'sub1/'])
> +        tree.add(['hello.txt', 'sub1'])
> +
> +        self.run_bzr('rename', 'sub1', 'sub2')
> +        self.run_bzr('move', 'hello.txt', 'sub2')

Same thing here, switch to 'mv'. There are a few more places that are
using 'move' instead of 'mv'.

If you want to test 'rename' and 'move' as aliases of 'mv'. That's fine,
but it should be done in a single test function, not randomly throughout
the other tests. (And I do realize that this is just because you are
cleaning up the ugly ugly OldTests.)


> === modified file bzrlib/builtins.py
> --- bzrlib/builtins.py
> +++ bzrlib/builtins.py
> @@ -375,6 +375,9 @@
>      encoding_type = 'replace'
>  
>      def run(self, names_list):
> +        if names_list is None:
> +            names_list = []
> +
>          if len(names_list) < 2:
>              raise BzrCommandError("missing file argument")
>          tree, rel_names = tree_files(names_list)

All of this for this one simple fix. :)

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.2 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFEr9F+JdeBCYSNAAMRAlh1AJ9s2ng3aGijw/gQfPpnNcRpJJaXUgCg1+tz
Lo3/DYbteMCdRhI7RT1qKBw=
=npdX
-----END PGP SIGNATURE-----