Merge lp:~doxxx/bzr/271790 into lp:bzr

[Gordon Tyler]

[Gordon Tyler (doxxx)]

Fixed cmd_mv to use trace.note so that it obeys the --quiet option. I didn't add anything to the blackbox tests for mv to verify this, but I can if you want.

[Andrew Bennetts (spiv)]

This looks good to me. I've taken the liberty of adding a blackbox test and NEWS entry for you, as they were so simple:

=== modified file 'NEWS'
--- NEWS 2009-11-16 20:50:41 +0000
+++ NEWS 2009-11-17 01:03:27 +0000
@@ -19,10 +19,13 @@

 Bug Fixes
 *********
+
 * After renaming a file, the dirstate could accidentally reference
   ``source\\path`` rather than ``source/path`` on Windows. This might be a
   source of some dirstate-related failures. (John Arbash Meinel)

+* ``bzr mv --quiet`` really is quiet now. (Gordon Tyler, #271790)
+
 * Lots of bugfixes for the test suite on Windows. We should once again
   have a test suite with no failures on Windows. (Once all the patches
   have landed, remove this line.) (John Arbash Meinel)

=== modified file 'bzrlib/tests/blackbox/test_mv.py'
--- bzrlib/tests/blackbox/test_mv.py 2009-09-17 11:54:41 +0000
+++ bzrlib/tests/blackbox/test_mv.py 2009-11-17 01:07:13 +0000
@@ -487,6 +487,14 @@
         self.assertEqual('bzr: ERROR: --after cannot be specified with'
                          ' --auto.\n', err)

+ def test_mv_quiet(self):
+ tree = self.make_branch_and_tree('.')
+ self.build_tree(['aaa'])
+ tree.add(['aaa'])
+ out, err = self.run_bzr('mv --quiet aaa bbb')
+ self.assertEqual(out, '')
+ self.assertEqual(err, '')
+
     def test_mv_readonly_lightweight_checkout(self):
         branch = self.make_branch('foo')
         branch = bzrlib.branch.Branch.open(self.get_readonly_url('foo'))

I see on IRC that you just sent in a contributor agreement, and this is simple enough that I don't think a second review is needed, so I'll send this to PQM now. Thanks very much!

review: Approve

[Robert Collins (lifeless)]

 review: needsfixing

Doesn't this move the list of files moved to stderr?
 Thats bad for folk capturing output IMO.

It should keep it on stdout.

-Rob

review: Needs Fixing

[Gordon Tyler (doxxx)]

After having discussed it with Robert on IRC and in light of Martin's forthcoming changes to the UIFactory, I should probably revisit this and address it properly in my output-cleanup branch.

[Andrew Bennetts (spiv)]

PQM rejected this. It turns out that it fails the test_non_ascii blackbox tests. If you run "./bzr selftest -s bb.test_non_ascii mv" you'll see the failures.

This appears to be because the output is now emitted to stderr rather than stdout. I think that's an undesireable change in behaviour, stdout feels like the right destination for this output to me. I guess the simplest change would be to continue to use self.outf, but first check is_quiet() (which is already imported from bzrlib.trace).

review: Needs Fixing

[Martin Pool (mbp)]

Yes, I think that's a better approach.

- Martin

On 17/11/2009 12:34 PM, "Andrew Bennetts" <email address hidden>
wrote:

Review: Needs Fixing
PQM rejected this. It turns out that it fails the test_non_ascii blackbox
tests. If you run "./bzr selftest -s bb.test_non_ascii mv" you'll see the
failures.

This appears to be because the output is now emitted to stderr rather than
stdout. I think that's an undesireable change in behaviour, stdout feels
like the right destination for this output to me. I guess the simplest
change would be to continue to use self.outf, but first check is_quiet()
(which is already imported from bzrlib.trace).

-- https://code.edge.launchpad.net/~doxxx/bzr/271790/+merge/14812 Your team
bzr-core is subscribed ...

[Gordon Tyler (doxxx)]

I've made the changes suggested and the bb.test_mv and bb.test_non_ascii mv tests succeed now.

[John A Meinel (jameinel)]

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

Andrew Bennetts wrote:
> Review: Approve
> This looks good to me. I've taken the liberty of adding a blackbox test and NEWS entry for you, as they were so simple:
>

So this changes 'mv' to output what changed to stderr rather than
stdout. Which would break scripts, etc. I don't know of anyone who
depends on 'mv' going to stdout, but I think it should be documented
more clearly.

I *think* it needs an entry at least in Compatibility Breaks.

John
=:->

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

iEYEARECAAYFAksCvHsACgkQJdeBCYSNAAPtBwCgmHLlR5sHaNtw6mFH1/LF0pAY
qPUAn3wQXtIdnObSb735GION57W4Csg8
=lgwg
-----END PGP SIGNATURE-----

[Gordon Tyler (doxxx)]

I don't know if you checked the diffs after my last update but it's really just been reverted back to what it did before, using self.outf.write, but it checks is_quiet first. So it's still going to stdout.

=== modified file 'NEWS'
--- NEWS	2009-11-17 02:34:42 +0000
+++ NEWS	2009-11-17 03:57:10 +0000
@@ -24,8 +24,13 @@
   ``source\\path`` rather than ``source/path`` on Windows. This might be a
   source of some dirstate-related failures. (John Arbash Meinel)
 
+<<<<<<< TREE
 * ``bzr ignore /`` no longer causes an IndexError. (Gorder Tyler, #456036)
 
+=======
+* ``bzr mv --quiet`` really is quiet now.  (Gordon Tyler, #271790)
+
+>>>>>>> MERGE-SOURCE
 * Lots of bugfixes for the test suite on Windows. We should once again
   have a test suite with no failures on Windows. (Once all the patches
   have landed, remove this line.) (John Arbash Meinel)

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-11-12 07:47:19 +0000
+++ bzrlib/builtins.py	2009-11-17 03:57:10 +0000
@@ -849,8 +849,9 @@
             # All entries reference existing inventory items, so fix them up
             # for cicp file-systems.
             rel_names = tree.get_canonical_inventory_paths(rel_names)
-            for pair in tree.move(rel_names[:-1], rel_names[-1], after=after):
-                self.outf.write("%s => %s\n" % pair)
+            for src, dest in tree.move(rel_names[:-1], rel_names[-1], after=after):
+                if not is_quiet():
+                    self.outf.write("%s => %s\n" % (src, dest))
         else:
             if len(names_list) != 2:
                 raise errors.BzrCommandError('to mv multiple files the'
@@ -900,7 +901,8 @@
             dest = osutils.pathjoin(dest_parent, dest_tail)
             mutter("attempting to move %s => %s", src, dest)
             tree.rename_one(src, dest, after=after)
-            self.outf.write("%s => %s\n" % (src, dest))
+            if not is_quiet():
+                self.outf.write("%s => %s\n" % (src, dest))
 
 
 class cmd_pull(Command):

=== modified file 'bzrlib/tests/blackbox/test_mv.py'
--- bzrlib/tests/blackbox/test_mv.py	2009-09-17 11:54:41 +0000
+++ bzrlib/tests/blackbox/test_mv.py	2009-11-17 03:57:10 +0000
@@ -487,6 +487,14 @@
         self.assertEqual('bzr: ERROR: --after cannot be specified with'
                          ' --auto.\n', err)
 
+    def test_mv_quiet(self):
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['aaa'])
+        tree.add(['aaa'])
+        out, err = self.run_bzr('mv --quiet aaa bbb')
+        self.assertEqual(out, '')
+        self.assertEqual(err, '')
+
     def test_mv_readonly_lightweight_checkout(self):
         branch = self.make_branch('foo')
         branch = bzrlib.branch.Branch.open(self.get_readonly_url('foo'))