bzr mv tweaks

[John Arbash Meinel]

Spencer Chastain wrote:
> 
> A few weeks ago I reported a bug (and, really, it's not a bug so much as 
> it is just lack of info to the user) with the mv command where the 
> command will fail and tell you why but hasn't told you what else it has 
> done and what it has yet to do.  If you want better details, check out 
> the bug report here: https://bugs.launchpad.net/bzr/+bug/177809
> 
> This weekend, I started messing around trying to solve this problem for 
> myself.  Seemed straightforward enough of a problem.
> 
> I started off by tweaking the command code itself and worked up the diff 
> included at the end of this email.
> 
> I then started working on fixing up the test code so that my changes 
> would pass the selftest, and it's at this point where I thought I'd get 
> a kosher check on my changes and intended future changes to address 
> passing the tests.
> 
> Essentially, my intent is to leave the format of the output below the 
> same and modify the content with the 'extra' information that comes with 
> the exceptions out of move.  So your error message would look something 
> like:
> 
>     $ bzr mv burg child_dir/
>     No Files Moved!
> 
>     Not Moved:
>             burg : burg is not versioned.

To start with, I don't think we ever use Title Case in our messages. I 
think we have a mix of "all lower case" and "Sentence case."
> 
> 
> I'll have to modify the mv command to handle each different exception 
> that can be raised out of the move function call (no biggie, just 
> tedious for me to figure it out) and then update each test to check the 
> proper output.
> 
> The last thing I'd do would be to create two tests, one that tries to 
> move a file name twice (a not un-common thing to do when you're moving 
> lots of files at one time) and then a test to move a bunch files, some 
> that exist, some that are dups, and some that don't exist or would fail 
> for other reasons.
> 
> The other thing I could do would be to update the move function call to 
> behave as I would like the command to do, but that would mean messing 
> with its return  to be a list of lists - a list of files that moved and 
> a list of files that failed.  I wasn't sure how much of an impact that 
> would be, and figured it was easier (and safer) to leave this kind of 
> fix at the user interface level.
> 
> I guess that's the long of it.  Let me know if I'm on target here.
> 
> Thanks,
> 
> --Spencer

+            if len(moved_files) > 0:
+                self.outf.write("Moved Files:\n")
+                for moved in moved_files:
+                    self.outf.write("\t%s => %s\n" % moved[0])

^- This looks suspect. As "moved" should be a 2-length tuple of (from, 
to), and you are passing in just 'from'.


It looks like the only thing you are using "moved_files" for is to keep 
track if you were requested to move the same file 2 times. I think it 
would be cleaner to just do that with a set() early on. Something like:

to_move = set(rel_names[:-1])
destination = rel_names[-1]

for rel_name in to_move:
   try:
     pair = tree.move([rel_name], destination, after=after)
   except errors.BzrError:
     dne_names.append(rel_name)

The downside to using a set is that you lose the ordering, so if someone 
does:

  bzr mv foo bar target

they may get:
    bar => target/bar
    foo => target/foo

or they might get:
    foo => target/foo
    bar => target/bar

depending on how it gets sorted in the set. So you could do:

to_move_set = set()
to_move = []
for name in rel_names[:-1]:
   if name in to_move_set:
     # Warn about a duplicate???
     continue
   to_move.append(name)
   to_move_set.add(name)

Also, as things that are being moved might be files or directories or 
symlinks, I would tend to just say:

self.outf.write('Moved:\n')

And "Nothing moved" and "Could not move (do not exist):"

John
=:->