[MERGE] enhanced mv command

[John Arbash Meinel]

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

Steffen Eichenberg wrote:
> Hello,
> 
> first of all, i am very busy at the moment. I will not be able to polish
> the code
> for the deadline today. Sorry for that. I will have more time at the end
> of the week.
> So i guess the code will be incorporated in the next sprint. Will this
> be version 0.14?
> (have to change some text then)

...

>     test_mv_already_moved_files_into_subdir is quite like
>     test_mv_already_moved_file_into_subdir. I'm guessing the latter one is
>     to test the move modes being determined independently per file? Do we
>     really need both?
> 
> 
> aaah, yes and no. If this would have been a real black box test, then
> no. But
> the implementation of bzr mv uses two different methods for the two use
> cases
> and i want both to be tested.
> 
> This has not been my idea. I do not like the differentiation, but the
> original code
> was written that way and i do not want to break any API.


I don't know that we have to test everything at the blackbox layer. We
need at least one test that distinguishes the two modes, but that
doesn't mean we need to do all permutations.

Whitebox testing should be used to test stuff like WorkingTree.move()
and WorkingTree.rename_one(). And then blackbox testing is used to test
stuff above that (cmd_move.run(), for example).

So if you actually factored out cmd_move.run() into a helper function,
it would be easier to test, since you could do it with a whitebox test.
And then the blackbox tests could be quite simple.

> 
> 
>     > +
>     > +    def test_mv_already_moved_file_forcing_after(self):
>     > +        """a is in the repository, b does not exist. User does
>     > +        mv a b; touch a; bzr mv a b"""
>     > +        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')
> 
>     The error message is only clear to me because I've read what is being
>     tested, if I didn't know that, it would be puzzling.
> 
> 
> I seem to be too involved into the functionality to understand what
> exactly is not
> clear. Can you give me a hint, or even better, can you give me a better
> error message ;-)

Maybe:

bzr: ERROR: Unable to rename. Old name 'a' and new name 'b' both exist
in the working tree. If you want to move 'a', please delete 'b' and try
again. If you want to mark 'a' as already moved, use 'bzr mv --after'.

I don't really like to get too wordy with errors like this. But maybe we
have to.


...

> 
>     Is raising a TypeError the right thing to do?
> 
> 
> That is what i have been told to do. I am not quite sure at the moment
> who it was,
> maybe John. I am looking for something more like an IllegalArgumentError
> or a
> MissingArgumentError but there seem to be no standard errors for this
> use case.
> Any suggestions?

I stand by my request for TypeError. Because we are only maintaining the
parameter for compatibility, and if we weren't doing compatibility,
Python would be raising a TypeError.

>>> def func(foo):
...   pass
...
>>> func()
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
TypeError: func() takes exactly 1 argument (0 given)

In general, we avoid raising built-in errors. But since this is a
developer error, I felt it was okay.


> 
> yes, but only with a white box test. I haven't done that, becaus i had
> not the
> time. Because i seem to have missed the deadline for this sprint, i will
> have
> enough time to write a test for this, too.

If you are testing it with a whitebox test, you don't *have* to test it
in a blackbox test. See my earlier comment about what should be tested
where. We don't want to test all permutations of everything. It is an
art, though, not a science. And I'm slowly learning from Robert Collins
about how to draw the lines.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFYdddJdeBCYSNAAMRAne5AJ43FZCcqZwUmTdnOlz/PEdJVpcrSACgzmFD
HZvu4AtweAxa4L9szmHlLX0=
=aW6e
-----END PGP SIGNATURE-----