[MERGE] enhanced mv command

[Aaron Bentley]

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

Marius Kruger wrote:
> Aaron Bentley wrote:
>     ^^^ I'm dubious about trying to generate correct English plurals for
>     error strings -- we've always just done "File(s) exist".  You don't
>     have
>     to change this, it's just my reaction.
> 
> 
> I was also ok with file(s), but in the end it is more user friendly to
> handle plurals properly.

Once you start down this path, internationalization gets trickier, and
even English plurals aren't entirely regular.  Plus the source is harder
to read.  So as a rule I try to avoid building grammar into my programs.

>     +        This is the new mode introduced
>     +        in version 0.13.
> 
>     More like version 0.14.  I think.  Hey, who's flying this plane, anyway?
> 
> More like version 0.15  :(
> what plane?
> btw is it good to have references this in the code?

I think it's fine.  We already have similar things describing, for
example, the changes in how inventories are handled.


>     ^^^ All this does is insert a trailing space.  Not an improvement :-) 
> 
> 
> very funny.
> I just can't win:  If I set my editor to remove those pesky trailing
> whitespace
> automatically you complain that I remove them. if I don't, you complain
> that I add them.

It is spurious changes that I object to; both adding and removing
whitespace fall into that category, as does changing the execute bit.
(changing the names of the errors comes close, but I wouldn't consider
it *spurious*, per se, just out-of-place in this bundle)

I make a practice of diffing my changes, usually against the mainline,
so that I can catch and undo spurious changes.

> I think after this thing is merged I should start looking at doing a
> patch which:
> 1) removes all the trailing white space once and for all and
> 2) introduce a test which checks that nobody violates this ever again.
> 
> And I might also write a plugin which warns you that you are
> adding trailing white space when you commit (even failing the commit if
> you choose).

The last sounds the most appealing to me.  Spurious whitespace changes
have harmful effects on merging, and I have several outstanding branches
at the moment, so I would object to 1).

> I see that some of the latest commits still introduce new trailing white
> space,
> when is it going to stop?

Well, it takes time to adjust one's habits.

>     +            except OSError, e:
>     +                self._rollback_move(moved)
>     +                raise errors.BzrRenameFailedError(entry.from_rel,
>     entry.to_rel,
>     +                    e[1])
>     +            except errors.BzrError, e:
>     +                self._rollback_move(moved)
>     +                raise
> 
>     ^^^ Is it possible to catch more specific errors here?  BzrError and
>     OSError are very broad.
> 
> (Again I didn't write this, so don't be too hard on me)
> I think its ok, as we just want to rollback and raise it again.
> It is done like that elsewere in the file too.

Really, I don't see why anyone would do this.  Either you want to catch
*all* exceptions and roll back, or you want to catch specific
exceptions, and only roll back on those.  In particular, OSError does
not necessarily indicate that a rename failed.  You would have to wrap
the try/catch around osutils.rename in order to know whether the rename
failed.

> === modified file 'bzrlib/builtins.py'
> -from bzrlib import symbol_versioning
> +from bzrlib import (symbol_versioning,
> +                    osutils,)

I think the current import style is

from bzlib import (
    osutils,
    symbol_versioning,
    )

It's alphabetical order, with each item formatted the same, to minimize
conflicts.  Not a big deal, just for future reference.

The one below is older (and probably my work).

>  from bzrlib.patches import (PatchSyntax, 
>                              PatchConflict, 
>                              MalformedPatchHeader,



> === modified file 'bzrlib/tests/__init__.py'
> --- bzrlib/tests/__init__.py	2007-01-11 17:00:25 +0000
> +++ bzrlib/tests/__init__.py	2007-01-11 17:31:34 +0000
> @@ -665,6 +665,16 @@
>          if not (left is right):
>              raise AssertionError("%r is not %r." % (left, right))
>  
> +    def assertNone(self, obj, msg):
> +        """Fail if obj is not None"""
> +        if (obj is not None):
> +            raise AssertionError(msg)
> +
> +    def assertNotNone(self, obj, msg):
> +        """Fail if obj is None"""
> +        if (obj is None):
> +            raise AssertionError(msg)
> +

If we're going to add assert*None, it would make sense to provide a
default message.  Another option would be to extend assertIs to take a
message, and add assertIsNot.

>      def test_mv_unqualified(self):
>          self.run_bzr_error(['^bzr: ERROR: missing file argument$'], 'mv')
> -        
> +

^^^ spurious whitespace change

Anyhow, I'd rather have this in than out, so +1, and I'll fix it up when
I merge it.

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

iD8DBQFFpuA+0F+nu1YWqI0RAkEYAJ4347SorcW4zMyfNbrIADY/lEQZkgCfStEs
7CjZh+KBCdlbwAb55hV4mY0=
=HCJ0
-----END PGP SIGNATURE-----