hi, here goes try number 4 see attached zipped bundle, and the total diff for easy reviewing. <div>On 1/3/07, Aaron Bentley <<a href="mailto:aaron.bentley@utoronto.ca" target="_blank" onclick="return top.js.OpenExtLink(window,event,this)"> aaron.bentley@utoronto.ca </a>> wrote: <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> === modified file 'bzrlib/builtins.py' (properties changed) ^^^ I don't think it makes sense to set builtins.py executable.</blockquote><div> I don't know how this happened, I obviously wasn't on the lookout for this sort of thing when committing, but  I've undid all the files that became executable (hope I didn't stuff up again!) : bzrlib/builtins.py bzrlib/tests/blackbox/test_mv.py bzrlib/tests/workingtree_implementations/test_workingtree.py bzrlib/workingtree.py </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> === modified file 'bzrlib/errors.py' +class FilesExist(PathError): +    """Used when reporting that files do exist""" + +    _fmt = "File%(plural)s exist: %(paths_as_string)s%(extra)s" + +    def __init__(self, paths, extra=None): +        # circular import +        from bzrlib.osutils import quotefn +        BzrError.__init__(self) +        self.paths = paths +        self.paths_as_string = ' '.join(paths) +        if extra: +            self.extra = ': ' + str(extra) +        else: +            self.extra = '' +        if len(paths) > 1: +            self.plural = 's' +        else: +            self.plural = '' ^^^ 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.</blockquote><div> I was also ok with file(s), but in the end it is more user friendly to handle plurals properly. John sort of complained about this, and since I think this exception is suited to one or more files, I thought this is the best way to address this. John's complaint: <blockquote style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;" class="gmail_quote"> >          self.run_bzr_error( > -            ["^bzr: ERROR: can't rename: both, old name .* and new name .*" > -             " exist. Use option '--after' to force rename."], <div style="direction: ltr;">> +            ["^bzr: ERROR: File\(s\) exist: .+ .+: can't rename." </div><div style="direction: ltr;">> +             " Use option '--after' to force rename."], </div><div style="direction: ltr;">^- You shouldn't need "File(s)" it should just be Files, since you know there are 2 files. Though it makes me wonder if the right exception is being used. </div></blockquote> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">@@ -147,3 +152,151 @@          self.run_bzr('mv', 'c/b', 'b')          tree = workingtree.WorkingTree.open('.')          self.assertEqual('b-id', tree.path2id('b')) + +    def test_mv_already_moved_file(self): +        """a is in repository, b does not exist. User does +        mv a b; bzr mv a b""" .... Finally, I'm not sure what the point of committing the files is.  'bzr mv' doesn't and shouldn't care whether or not the files have been committed.</blockquote><div>ok I've removed them, I think it was thought that they might be handled differently. the tests pass without them and if you assume they should be handled the the same, the commits are not really needed. </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> - --- bzrlib/tests/workingtree_implementations/test_workingtree.py 2006-09-10 19:04:48 +0000 +++ bzrlib/tests/workingtree_implementations/test_workingtree.py 2007-01-03 02:46:14 +0000 @@ -681,3 +681,50 @@                  tree.add, [u'a\u030a'])          finally:              osutils.normalized_filename = orig +    def test_move_deprecated_wrong_call(self): +        """tree.move has the deprecated parameter 'to_name'. +        It has been replaced by 'to_dir' for consistency. +        Test the new API using wrong parameter""" +        self.build_tree(['a1', 'sub1/']) +        tree = self.make_branch_and_tree ('.') +        tree.add(['a1', 'sub1']) +        tree.commit('initial commit') +        self.assertRaises(TypeError, tree.move, ['a1'], +                          to_this_parameter_does_not_exist='sub1', +                          after=False) ^^^ This is probably superfluous with the following test.  I must admit I thought you were obsoleting (not just deprecating) the parameter, at first. +    def test_move_deprecated_deprecated_call(self): </blockquote><div>(This was done by Steffen Eichenberg, not me. I'm just trying to fix it up to a commitable state remember, I suppose thats makes me responsible.) Is it ok if I leave this, it seems to be there for a good cause: test that we can now and in the future use the named and unnamed api of .move </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> +        This is the new mode introduced +        in version 0.13. More like version 0.14.  I think.  Hey, who's flying this plane, anyway? </blockquote><div>More like version 0.15  :( what plane? btw is it good to have references this in the code? </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">         This returns a list of (from_path, to_path) pairs for each - -        entry that is moved. +         entry, that is moved. ^^^ The original phrasing is correct. As code: [pairs( entry) for entry in moved_entries()] </blockquote><div>I could have sworn one of the previous reviewers requested this, but i could not find it so I must be delusional.  </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> +        # determine which move mode to use. checks also for movability +        rename_entries = self._determine_mv_mode(rename_entries, after) + +        # move pairs.</blockquote><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> ^^^ What does "move pairs" mean? </blockquote><div> You're asking me?  I don't know, so I just removed it. </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">         original_modified = self._inventory_is_modified          try: - -            if len(from_paths): +            if len(from_paths): ^^^ All this does is insert a trailing space.  Not an improvement :-) </blockquote><div> 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. 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). So that this sort of thing is caught earlier. It can maybe also add a command similar to shelve which traverses the changes which have not been committed yet and asks you for every bunch of trailing ws it finds, if you want to remove it and does it for you. This plugin would be usefull for other projects too, like the projects at my dayjob. (where we also have differences of oppinion about trailing whitespace) I also removed all the trailing white space this bundle would have introduced. I see that some of the latest commits still introduce new trailing white space, when is it going to stop? </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">+        for entry in rename_entries: +            try: +                self._move_ entry(entry) +            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.</blockquote><div>(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. </div></div> regards marius. -- white space is overrated