bzr mv should handle and report failure better

[Spencer E. Chastain]

The Problem
~~~~~~~~~

I was using bzr 1.0 on windows.

I had a branch similar as follows:

branch-dir
- child-dir
- foo.h
- foo.cpp
- bar.h
- bar.cpp
- baz.h
- baz.cpp

So running the following produced:
$ bzr mv foo.h foo.cpp bar.h bar.h baz.h baz.cpp child-dir
bzr: ERROR: Could not move bar.h => child-dir: bar.h is not versioned.

My first guess was that nothing happened and that I needed to modify my command, but when I edited and ran the following:
$ bzr mv foo.h foo.cpp bar.h bar.cpp baz.h baz.cpp child-dir
bzr: ERROR: Could not move foo.h => child-dir: foo.h is not versioned.

So then I ran bzr status to see what was going on:
$ bzr status
renamed:
  bar.h => child-dir/bar.h
  foo.cpp => child-dir/foo.cpp
  foo.h => child-dir/foo.h

so, then I realized what happened - it processed the move of my files up to a point, then quit.

Cognitive Dissonance
~~~~~~~~~~~~~~~~

Due to insufficient reporting, I assumed the command as a whole had failed. However, it makes sense from an implementation perspective that it would work until it couldn't.

From my perspective, I expected bzr mv would inform me that it had done /something/ successfully while other things unsuccessfully.

Since some of my move request was processed, I would expect that it would continue trying to process the rest of my command even if it encountered some errors. Plus, since my "error" was actually a duplicate file name, I wouldn't expect to see an error at all.

What I Expected
~~~~~~~~~~~~

So, if I would have ran the following command with my original branch above, I would expect to see:
$ bzr mv foo.h foo.cpp bar.h bar.h baz.h baz.cpp makebelieve.txt child-dir
renamed:
  bar.h => child-dir/bar.h
  baz.cpp => child-dir/baz.cpp
  baz.h => child-dir/baz.h
  foo.cpp => child-dir/foo.cpp
  foo.h => child-dir/foo.h
failed to rename (does not exist):
  makebelieve.txt
duplicate argument:
  foo.h

[Alexander Belchenko (bialix)]

I agree that bzr should report about already moved things until it encounters error. But I don't think bzr should continue to work if some error encountered. So I'd like to fix reporting problem rather than made all moves independent.

To fix problem with reporting I think we should extent arguments of tree.move() method with one optional argument `report`. This argument could be a list object to accumulate finished moves info. So if error will be raised command mv still will be able to get report and show it in finally block. Something like this:

# move into existing directory
report = []
try:
     tree.move(rel_names[:-1], rel_names[-1], after=after, report=report)
finally:
    for pair in report:
        self.outf.write("%s => %s\n" % pair)

IMO it will be simplier to implementing and testing. But will require additional tests in bzrlib/tests/workingtree_implementations/

Spencer, do you want to rework your patch in this way? If not I'll do it.

[Ian Clatworthy (ian-clatworthy)]

Note: There is additional discussion on this issue on the mailing list.

My initial reaction here was to agree with Alexander. My concern was consistency: it's nice to do things as comprehensively as possible, but if that means being inconsistent with other bzr commands, then the patch would be a local optimisation at the expense of the overall UI. Digging deeper though, I think Spencer's approach is sound and indeed consistent with other parts of Bazaar ...

For example, if I have a branch with 3 files called a, b and c, and run 'bzr remove --keep a d c", I get the following output:

  d is not versioned.
  removed c
  removed a

So remove works as Spencer is suggesting. I'd like to say 'add' works the same way but it arguably has a bug. Consider:

  $ touch a b c
  $ bzr add a d c
  added a
  bzr: ERROR: No such file: u'/home/ian/tmp/play/testmulti/d'
  $ bzr st
  unknown:
    a
    b
    c

So add is reporting partial success when if fact nothing happened. I'll raise a bug for that.

[Martin Pool (mbp)]

The original patch was

http://bundlebuggy.aaronbentley.com/project/bzr/request/%3C64ab70ac0802041811u61752e66l3a04f025336390ae%40mail.gmail.com%3E

[Robert Collins (lifeless)]

To find the thread, look for 177809 in the list archives from early 2008.

The patch foundered over the location of the code and [less so] the manner of the approach.

Just noting this here to help folk coming to work on it.

Personally, I'd be inclined to change bzr to plan the rename, create a tree transform of it, and apply it. That would allow tree transforms rollback etc logic to be used and perhaps shrink some duplicate code.