Merge lp:~jameinel/bzr/2.3.1-track-bytes into lp:bzr/2.3

[John A Meinel]

Fix bug #713258, restore logging of bytes transferred for a given command.

This restores the logging of bytes transferred for a given command, and adds a test that it is shown when using '-Dbytes'.

I'm not positive that 'test_debug' is the best place for it, but it seemed reasonable to test that a -D flag has an effect.

I suppose the only other bit I *could* test, is that it doesn't report 0 bytes, but that is a bit tricky, since you could search for [^0]kB, but then if it was exactly 10kB the test would fail again. (Right now it is 4kB in my testing.)

I'm proposing this for bzr/2.3 and then we'll merge up to bzr core from there.

[Martin Pool (mbp)]

Thanks for fixing that, and for adding a test.

I am a bit reluctant to make the test use a subprocess though. It may not show up much for this particular test, but if in general we start running tests in subprocesses things will eventually get very slow, especially on Windows. I thought we already had, or could easily add, an option to run things with a more realistic ui factory.

It seems like the null progress view could easily at least record the activity, even if it doesn't display it.

So 'needs fixing' but I'm open to negotiation.

review: Needs Fixing

[John A Meinel (jameinel)]

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

On 2/6/2011 11:58 PM, Martin Pool wrote:
> Review: Needs Fixing
> Thanks for fixing that, and for adding a test.
>
> I am a bit reluctant to make the test use a subprocess though. It may not show up much for this particular test, but if in general we start running tests in subprocesses things will eventually get very slow, especially on Windows. I thought we already had, or could easily add, an option to run things with a more realistic ui factory.
>
> It seems like the null progress view could easily at least record the activity, even if it doesn't display it.
>
> So 'needs fixing' but I'm open to negotiation.

The existing code tracks the transfer, but isn't logging it. And the
specific bug is that it is also not getting displayed (though that is
because it ends up at 0 bytes, which is intentionally suppressed, so
stuff like local 'bzr st' doesn't spit out Transferred 0kB...)

I agree things could be better, but

a) In a very real sense, this was an integration failure.
b) Tests actually use TestUIFactory, not TextUIFactory or
SilentUIFactory, so we really wouldn't be testing what we think we are.
I could make TestUIFactory do what I want, but it would pretty much be
unrelated to the fix.

I had thought about doing a minimal 'spawn' version for 2.3, and then
doing more invasive changes for bzr.dev, but I honestly couldn't come up
with a better way to test it.

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

iEYEARECAAYFAk1QZ0AACgkQJdeBCYSNAAMjLwCg0LwXaVNpy2eqcLlZYR0BzJSS
6PYAnREbHzczYjLhyp/poWgTncSZjRR1
=C5H7
-----END PGP SIGNATURE-----

[Martin Pool (mbp)]

On 8 February 2011 08:42, John Arbash Meinel <email address hidden> wrote:
> a) In a very real sense, this was an integration failure.

True; this is close to being something where we want to measure the
behavior as the process exits.

  vote approve

review: Approve

[John A Meinel (jameinel)]

sent to pqm by email

=== modified file 'bzrlib/commands.py'
--- bzrlib/commands.py	2010-10-29 17:07:42 +0000
+++ bzrlib/commands.py	2011-02-04 22:37:53 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005-2010 Canonical Ltd
+# Copyright (C) 2005-2011 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -691,7 +691,10 @@
             return self.run(**all_cmd_args)
         finally:
             # reset it, so that other commands run in the same process won't
-            # inherit state
+            # inherit state. Before we reset it, log any activity, so that it
+            # gets properly tracked.
+            ui.ui_factory.log_transport_activity(
+                display=('bytes' in debug.debug_flags))
             trace.set_verbosity_level(0)
 
     def _setup_run(self):
@@ -1202,8 +1205,6 @@
     argv = _specified_or_unicode_argv(argv)
     _register_builtin_commands()
     ret = run_bzr_catch_errors(argv)
-    bzrlib.ui.ui_factory.log_transport_activity(
-        display=('bytes' in debug.debug_flags))
     trace.mutter("return code %d", ret)
     return ret
 

=== modified file 'bzrlib/tests/blackbox/test_debug.py'
--- bzrlib/tests/blackbox/test_debug.py	2010-02-17 17:11:16 +0000
+++ bzrlib/tests/blackbox/test_debug.py	2011-02-04 22:37:53 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006, 2007, 2009, 2010 Canonical Ltd
+# Copyright (C) 2006, 2007, 2009, 2010, 2011 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -21,10 +21,10 @@
 import sys
 import time
 
-from bzrlib.tests import TestCaseInTempDir
-
-
-class TestDebugOption(TestCaseInTempDir):
+from bzrlib import debug, tests
+
+
+class TestDebugOption(tests.TestCaseInTempDir):
 
     def test_dash_derror(self):
         """With -Derror, tracebacks are shown even for user errors"""
@@ -39,3 +39,22 @@
         # With -Dlock, locking and unlocking is recorded into the log
         self.run_bzr("-Dlock init foo")
         self.assertContainsRe(self.get_log(), "lock_write")
+
+
+class TestDebugBytes(tests.TestCaseWithTransport):
+
+    def test_bytes_reports_activity(self):
+        tree = self.make_branch_and_tree('tree')
+        self.build_tree(['tree/one'])
+        tree.add('one')
+        rev_id = tree.commit('first')
+        remote_trans = self.make_smart_server('.')
+        # I would like to avoid run_bzr_subprocess here, but we need it to be
+        # connected to a real TextUIFactory. The NullProgressView always
+        # ignores transport activity.
+        env = {'BZR_PROGRESS_BAR': 'text'}
+        out, err = self.run_bzr_subprocess('branch -Dbytes %s/tree target'
+                                           % (remote_trans.base,),
+                                           env_changes=env)
+        self.assertContainsRe(err, 'Branched 1 revision')
+        self.assertContainsRe(err, 'Transferred:.*kB')

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2011-02-04 14:10:48 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2011-02-04 22:37:53 +0000
@@ -32,6 +32,10 @@
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.
 
+* Restore proper logging of bytes transferred. We accidentally reset the
+  counter when commands finished before we logged the total transferred.
+  (John Arbash Meinel, #713258)
+
 Documentation
 *************