Merge lp:~vila/bzr/388725-progress-bar-config-option into lp:bzr

[Vincent Ladeuil]

Add a config option for the progress bar type.

Add a config option for the progress bar type.

I fixed this bug with a simple grep, splitting tests between those that use
'-Oprogress_bar=[none|text]' and those that still use the env bar and was
pleasantly surprised to see this was so trivial do decide... it demonstrates
(for those that weren't convinced, slightly including myself ;) that there
is indeed a good use case to allow both the env var and the config option.

[John A Meinel (jameinel)]

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

On 9/19/2012 11:16 AM, Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging
> lp:~vila/bzr/388725-progress-bar-config-option into lp:bzr.
>
> Requested reviews: bzr-core (bzr-core) Related bugs: Bug #388725
> in Duplicity: "Report when duplicity is 'done' with restoring a
> file" https://bugs.launchpad.net/duplicity/+bug/388725
>
> For more details, see:
> https://code.launchpad.net/~vila/bzr/388725-progress-bar-config-option/+merge/125114
>
>
>
Add a config option for the progress bar type.
>
> I fixed this bug with a simple grep, splitting tests between those
> that use '-Oprogress_bar=[none|text]' and those that still use the
> env bar and was pleasantly surprised to see this was so trivial do
> decide... it demonstrates (for those that weren't convinced,
> slightly including myself ;) that there is indeed a good use case
> to allow both the env var and the config option.
>

 merge: approve

Looks good.

John
=:->

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

iEYEARECAAYFAlBZcooACgkQJdeBCYSNAAND4ACgiLVx+BUb82iYjIgoNNpbrF5I
9UIAnRG5km6BainfXIyCkz1G1TlVOFhG
=3puI
-----END PGP SIGNATURE-----

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2012-09-18 16:38:02 +0000
+++ bzrlib/config.py	2012-09-19 07:15:22 +0000
@@ -2135,7 +2135,7 @@
 
 class Base64CredentialStore(CredentialStore):
     __doc__ = """Base64 credential store for the authentication.conf file"""
-    
+
     def decode_password(self, credentials):
         """See CredentialStore.decode_password."""
         # GZ 2012-07-28: Will raise binascii.Error if password is not base64,
@@ -2161,7 +2161,8 @@
         for those under repositories.
         """
         if self._config is None:
-            raise errors.BzrError("Cannot set configuration in %s" % self._bzrdir)
+            raise errors.BzrError("Cannot set configuration in %s"
+                                  % self._bzrdir)
         if value is None:
             self._config.set_option('', 'default_stack_on')
         else:
@@ -2473,8 +2474,8 @@
     return float(unicode_str)
 
 
-# Use a an empty dict to initialize an empty configobj avoiding all
-# parsing and encoding checks
+# Use an empty dict to initialize an empty configobj avoiding all parsing and
+# encoding checks
 _list_converter_config = configobj.ConfigObj(
     {}, encoding='utf-8', list_values=True, interpolation=False)
 
@@ -2802,6 +2803,8 @@
 
 Each function takes branch, rev_id as parameters.
 '''))
+option_registry.register_lazy('progress_bar', 'bzrlib.ui.text',
+                              'opt_progress_bar')
 option_registry.register(
     Option('public_branch',
            default=None,

=== modified file 'bzrlib/tests/blackbox/test_debug.py'
--- bzrlib/tests/blackbox/test_debug.py	2011-02-04 22:25:59 +0000
+++ bzrlib/tests/blackbox/test_debug.py	2012-09-19 07:15:22 +0000
@@ -52,9 +52,8 @@
         # 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)
+        out, err = self.run_bzr_subprocess(
+            'branch -Dbytes -Oprogress_bar=text %s/tree target'
+            % (remote_trans.base,))
         self.assertContainsRe(err, 'Branched 1 revision')
         self.assertContainsRe(err, 'Transferred:.*kB')

=== modified file 'bzrlib/tests/blackbox/test_diff.py'
--- bzrlib/tests/blackbox/test_diff.py	2012-01-05 13:02:31 +0000
+++ bzrlib/tests/blackbox/test_diff.py	2012-09-19 07:15:22 +0000
@@ -388,10 +388,10 @@
         # subprocess.py that we had to workaround).
         # However, if 'diff' may not be available
         self.make_example_branch()
-        self.overrideEnv('BZR_PROGRESS_BAR', 'none')
-        out, err = self.run_bzr_subprocess('diff -r 1 --diff-options -ub',
-                                           universal_newlines=True,
-                                           retcode=None)
+        out, err = self.run_bzr_subprocess(
+            'diff -Oprogress_bar=none -r 1 --diff-options -ub',
+            universal_newlines=True,
+            retcode=None)
         if 'Diff is not installed on this machine' in err:
             raise tests.TestSkipped("No external 'diff' is available")
         self.assertEqual('', err)

=== modified file 'bzrlib/tests/blackbox/test_locale.py'
--- bzrlib/tests/blackbox/test_locale.py	2011-05-15 14:35:24 +0000
+++ bzrlib/tests/blackbox/test_locale.py	2012-09-19 07:15:22 +0000
@@ -41,15 +41,21 @@
                     timestamp=1156451297.96, timezone=0)
         self.tree = tree
 
+    def run_log_quiet_long(self, args, env_changes={}):
+        cmd = ['--no-aliases', '--no-plugins', '-Oprogress_bar=none',
+               'log', '-q', '--log-format=long']
+        cmd.extend(args)
+        return self.run_bzr_subprocess(cmd, env_changes=env_changes)
+
     def test_log_C(self):
         self.disable_missing_extensions_warning()
-        # C is not necessarily the default locale, so set both LANG and LC_ALL
-        # explicitly because LC_ALL is preferred on (some?) Linux systems but
-        # only LANG is respected on Windows.
-        out, err = self.run_bzr_subprocess(
-            '--no-aliases --no-plugins log -q --log-format=long tree',
-               env_changes={'LANG': 'C', 'BZR_PROGRESS_BAR':'none',
-                            'LC_ALL': 'C', 'LC_CTYPE':None, 'LANGUAGE':None})
+        out, err = self.run_log_quiet_long(
+            ['tree'],
+            # C is not necessarily the default locale, so set both LANG and
+            # LC_ALL explicitly because LC_ALL is preferred on (some?) Linux
+            # systems but only LANG is respected on Windows.
+            env_changes={'LANG': 'C', 'LC_ALL': 'C', 'LC_CTYPE':None,
+                         'LANGUAGE':None})
         self.assertEqual('', err)
         self.assertEqualDiff("""\
 ------------------------------------------------------------
@@ -62,10 +68,10 @@
 """, out)
 
     def test_log_BOGUS(self):
-        out, err = self.run_bzr_subprocess(
-            '--no-aliases --no-plugins log -q --log-format=long tree',
-               env_changes={'LANG':'BOGUS', 'BZR_PROGRESS_BAR':'none',
-                            'LC_ALL':None, 'LC_CTYPE':None, 'LANGUAGE':None})
+        out, err = self.run_log_quiet_long(
+            ['tree'],
+            env_changes={'LANG':'BOGUS', 'LC_ALL':None, 'LC_CTYPE':None,
+                         'LANGUAGE':None})
         self.assertStartsWith(err, 'bzr: warning: unsupported locale setting')
         self.assertEqualDiff("""\
 ------------------------------------------------------------

=== modified file 'bzrlib/ui/text.py'
--- bzrlib/ui/text.py	2012-04-30 10:44:04 +0000
+++ bzrlib/ui/text.py	2012-09-19 07:15:22 +0000
@@ -31,6 +31,7 @@
 import warnings
 
 from bzrlib import (
+    config,
     debug,
     progress,
     osutils,
@@ -155,8 +156,14 @@
             return index
 
 
+opt_progress_bar = config.Option(
+    'progress_bar', help='Progress bar type.',
+    default_from_env=['BZR_PROGRESS_BAR'], default=None,
+    invalid='error')
+
+
 class TextUIFactory(UIFactory):
-    """A UI factory for Text user interefaces."""
+    """A UI factory for Text user interfaces."""
 
     def __init__(self,
                  stdin=None,
@@ -279,14 +286,14 @@
         # do that.  otherwise, guess based on $TERM and tty presence.
         if self.is_quiet():
             return NullProgressView()
-        elif os.environ.get('BZR_PROGRESS_BAR') == 'text':
-            return TextProgressView(self.stderr)
-        elif os.environ.get('BZR_PROGRESS_BAR') == 'none':
-            return NullProgressView()
-        elif progress._supports_progress(self.stderr):
-            return TextProgressView(self.stderr)
-        else:
-            return NullProgressView()
+        pb_type = config.GlobalStack().get('progress_bar')
+        if pb_type == 'none': # Explicit requirement
+            return NullProgressView()
+        if (pb_type == 'text' # Explicit requirement
+            or progress._supports_progress(self.stderr)): # Guess
+            return TextProgressView(self.stderr)
+        # No explicit requirement and no successful guess
+        return NullProgressView()
 
     def _make_output_stream_explicit(self, encoding, encoding_type):
         if encoding_type == 'exact':

=== modified file 'doc/en/release-notes/bzr-2.6.txt'
--- doc/en/release-notes/bzr-2.6.txt	2012-09-18 16:38:02 +0000
+++ doc/en/release-notes/bzr-2.6.txt	2012-09-19 07:15:22 +0000
@@ -44,6 +44,10 @@
 Bug Fixes
 *********
 
+* Add a ``progress_bar`` configuration option defaulting to
+  ``BZR_PROGRESS_BAR``. This can be set in ``bazaar.conf`` or specified from
+  the command line with ``-Oprogress_bar=text``. (Vincent Ladeuil, #388725)
+
 * Fixed a bug where the entire contents of ``/etc/mailname`` is read in.
   We only want to read in the first line so that comments could be added
   and would be ignored.