Merge lp:~vila/bzr/deprecation-warning-preference into lp:bzr

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

This patch supersedes https://code.edge.launchpad.net/~ted/bzr/deprecation-warning-preference/+merge/15455

It's a patch pilot attempt to land a work started by fullermd far too long ago and re-started by ted more recently.

I tried to capture most of the comments by Ian, Martin and John without spending too much time on it (but still...).

This patch adds:
- specifying a suppress_warnings variable in any configuration file,
- explicitly requiring a config variable as a list,
- a suppres_warning() method to config files,
- checks for format_deprecation at lock time on branch objects (which can then provide
  their config to the repo before locking it),
- checks for format_deprecation at lock on repo objects but relying on bazaar.conf only
  in that case.

I may file a bug or three for the remaining points so please comment if you disagree
with some decision made here but mention if you agree with postponing it (good) or you
think it's required to land (bad :).
The idea was to implement something that we could tweak further
but that will not require some upgrade dance in the future (so mainly: keep the same
variable name, don't force setting it in bazaar.conf so the default behaviour for
new branches is still to raise the warning).

These remaining points are:
- the actual implementation use a single global variable for the whole code base,
  forget about being warned for the second repo accessed (or the 20th for bzrlib users),
  that should be implemented as a repo attribute instead,
- working trees can raise format deprecation warnings too, they should use the new
  facility (I don't think we want to proliferate warning variables gratuitously)
- hpss remote repositories doesn't raise the warning locally (AFAICS but feel free
  to prove me wrong),
- no distinction is made between read and write access (I'm not sure I understand
  the need here),
- there is no config file for repositories were such an option can be specified.

My intent here is to provide a base for future work without addressing some existing problems (some of which have been revealed while writing this patch).

I didn't address John idea about filtering by format as I think we plan to
reduce the number of formats and push people to use the default ones.
The rationale is that: 1) you care or you don't care about being warned, 2) you can set
the option in various ways, the finest grain being the branch. I think that's
good enough.

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/deprecation-warning-preference into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>

...

>
> I didn't address John idea about filtering by format as I think we plan to
> reduce the number of formats and push people to use the default ones.
> The rationale is that: 1) you care or you don't care about being warned, 2) you can set
> the option in various ways, the finest grain being the branch. I think that's
> good enough.
>
>
>

Well, *I* may not care about using --1.9, but I don't want to be using
- --weave anymore. Not to say other people are in that boat.

v- Is the extra 's' for extra suppression ?

+* The ``suppresss_warnings`` configuration option has been introduced and
+ accept the ``format_deprecation`` value to disable the corresponding
+ warning for repositories. It can be set to in either ``bazaar.conf``,
+ ``locations.conf`` or ``branch.conf``.
+ (Ted Gould, Matthew Fuller, Vincent Ladeuil)

You can use

config.LocationConfig(repo.bzrdir.root_transport.base) if you wanted to
support per-repo configuration.

Otherwise:

 review: approve
 merge: approve

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

iEYEARECAAYFAkspC1gACgkQJdeBCYSNAAMPagCgj07xNFxCF94NE0Lv+YtQXagO
+9UAnRLpaeeM3hjUd6OovoZJm2CAcbCF
=Y17e
-----END PGP SIGNATURE-----

review: Approve

[Martin Pool (mbp)]

Skimmed, looks good, thanks!

--
Martin <http://launchpad.net/~mbp/>

[Vincent Ladeuil (vila)]

>>>>> "jam" == John A Meinel <email address hidden> writes:

<snip/>

    jam> Well, *I* may not care about using --1.9, but I don't
    jam> want to be using --weave anymore. Not to say other
    jam> people are in that boat.

What I meant (and forgot to add when writing the cover letter) is
that it's highly likely that people are coherent and use the same
format inside project boundaries so giving a path-based control
over the warning should be pretty close in ease of use to a
format-based control.

But the later is harder to implement and likely harder to use
(and my main point was that we don't want people to get involved
into all of your existing formats if we can avoid it :-)

    jam> v- Is the extra 's' for extra suppression ?

Exactly, I think I made that typo at least 50 times so be
grateful that I left only one as I'm grateful you caught it :D

    jam> +* The ``suppresss_warnings`` configuration option has been introduced and
    jam> + accept the ``format_deprecation`` value to disable the corresponding
    jam> + warning for repositories. It can be set to in either ``bazaar.conf``,
    jam> + ``locations.conf`` or ``branch.conf``.
    jam> + (Ted Gould, Matthew Fuller, Vincent Ladeuil)

    jam> You can use

    jam> config.LocationConfig(repo.bzrdir.root_transport.base) if you wanted to
    jam> support per-repo configuration.

I almost implemented it. I almost mentioned it in the cover letter.

But I had a bad feeling about it, it smells like a workaround. I
don't want to create yet another config file but at the same time
I have a feeling that it may address some other needs.

This needs discussion anyway so I didn't feel bad enough to
implement config.LocationConfig(repo.bzrdir.root_transport.base).

    jam> Otherwise:

    jam> review: approve
    jam> merge: approve

Thanks, you didn't comment on the remaining points so I've filed
bug #497694, comments there welcome.

    Vincent

=== modified file 'NEWS'
--- NEWS	2009-12-15 19:59:00 +0000
+++ NEWS	2009-12-16 10:09:27 +0000
@@ -95,6 +95,7 @@
 * ``bzr commit`` now has a ``--commit-time`` option.
   (Alexander Sack, #459276)
 
+<<<<<<< TREE
 * ``-Dhpss`` now increases logging done when run on the bzr server,
   similarly to how it works on the client. (John Arbash Meinel)
 
@@ -108,6 +109,14 @@
   lengths. 
   (Vincent Ladeuil)
 
+=======
+* The ``suppresss_warnings`` configuration option has been introduced and
+  accept the ``format_deprecation`` value to disable the corresponding
+  warning for repositories. It can be set to in either ``bazaar.conf``,
+  ``locations.conf`` or ``branch.conf``.
+  (Ted Gould, Matthew Fuller, Vincent Ladeuil)
+
+>>>>>>> MERGE-SOURCE
 Bug Fixes
 *********
 

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2009-12-04 22:13:52 +0000
+++ bzrlib/branch.py	2009-12-16 10:09:27 +0000
@@ -2143,6 +2143,7 @@
         # All-in-one needs to always unlock/lock.
         repo_control = getattr(self.repository, 'control_files', None)
         if self.control_files == repo_control or not self.is_locked():
+            self.repository._warn_if_deprecated(self)
             self.repository.lock_write()
             took_lock = True
         else:
@@ -2160,6 +2161,7 @@
         # All-in-one needs to always unlock/lock.
         repo_control = getattr(self.repository, 'control_files', None)
         if self.control_files == repo_control or not self.is_locked():
+            self.repository._warn_if_deprecated(self)
             self.repository.lock_read()
             took_lock = True
         else:

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2009-12-02 22:04:04 +0000
+++ bzrlib/config.py	2009-12-16 10:09:27 +0000
@@ -190,11 +190,23 @@
         """Get a generic option as a boolean - no special process, no default.
 
         :return None if the option doesn't exist or its value can't be
-            interpreted as a boolean. Returns True or False ortherwise.
+            interpreted as a boolean. Returns True or False otherwise.
         """
         s = self._get_user_option(option_name)
         return ui.bool_from_string(s)
 
+    def get_user_option_as_list(self, option_name):
+        """Get a generic option as a list - no special process, no default.
+
+        :return None if the option doesn't exist. Returns the value as a list
+            otherwise.
+        """
+        l = self._get_user_option(option_name)
+        if isinstance(l, (str, unicode)):
+            # A single value, most probably the user forgot the final ','
+            l = [l]
+        return l
+
     def gpg_signing_command(self):
         """What program should be used to sign signatures?"""
         result = self._gpg_signing_command()
@@ -313,6 +325,19 @@
                 path = 'bzr'
             return path
 
+    def suppress_warning(self, warning):
+        """Should the warning be suppressed or emitted.
+
+        :param warning: The name of the warning being tested.
+
+        :returns: True if the warning should be suppressed, False otherwise.
+        """
+        warnings = self.get_user_option_as_list('suppress_warnings')
+        if warnings is None or warning not in warnings:
+            return False
+        else:
+            return True
+
 
 class IniBasedConfig(Config):
     """A configuration policy that draws from ini files."""

=== modified file 'bzrlib/help_topics/en/configuration.txt'
--- bzrlib/help_topics/en/configuration.txt	2009-10-02 09:11:43 +0000
+++ bzrlib/help_topics/en/configuration.txt	2009-12-16 10:09:27 +0000
@@ -428,6 +428,18 @@
 A publically-accessible version of this branch (implying that this version is
 not publically-accessible).  Used (and set) by ``bzr send``.
 
+suppress_warnings
+~~~~~~~~~~~~~~~~~
+
+A list of strings, each string represent a warning that can be emitted by
+bzr. Mentioning a warning in this list tells bzr to not emit it.
+
+Valid values:
+
+* ``format_deprecation``:
+    whether the format deprecation warning is shown on repositories that are
+    using deprecated formats.
+
 
 Branch type specific options
 ----------------------------

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2009-11-11 06:50:40 +0000
+++ bzrlib/remote.py	2009-12-16 10:09:27 +0000
@@ -951,6 +951,11 @@
     def is_write_locked(self):
         return self._lock_mode == 'w'
 
+    def _warn_if_deprecated(self, branch=None):
+        # If we have a real repository, the check will be done there, if we
+        # don't the check will be done remotely.
+        pass
+
     def lock_read(self):
         # wrong eventually - want a local lock cache context
         if not self._lock_mode:

=== modified file 'bzrlib/repofmt/pack_repo.py'
--- bzrlib/repofmt/pack_repo.py	2009-10-29 05:54:49 +0000
+++ bzrlib/repofmt/pack_repo.py	2009-12-16 10:09:27 +0000
@@ -2234,16 +2234,10 @@
         self._reconcile_fixes_text_parents = True
         self._reconcile_backsup_inventory = False
 
-    def _warn_if_deprecated(self):
+    def _warn_if_deprecated(self, branch=None):
         # This class isn't deprecated, but one sub-format is
         if isinstance(self._format, RepositoryFormatKnitPack5RichRootBroken):
-            from bzrlib import repository
-            if repository._deprecation_warning_done:
-                return
-            repository._deprecation_warning_done = True
-            warning("Format %s for %s is deprecated - please use"
-                    " 'bzr upgrade --1.6.1-rich-root'"
-                    % (self._format, self.bzrdir.transport.base))
+            super(KnitPackRepository, self)._warn_if_deprecated(branch)
 
     def _abort_write_group(self):
         self.revisions._index._key_dependencies.clear()

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2009-12-03 05:31:03 +0000
+++ bzrlib/repository.py	2009-12-16 10:09:27 +0000
@@ -24,6 +24,7 @@
     bzrdir,
     check,
     chk_map,
+    config,
     debug,
     errors,
     fetch as _mod_fetch,
@@ -1304,11 +1305,6 @@
         self._reconcile_does_inventory_gc = True
         self._reconcile_fixes_text_parents = False
         self._reconcile_backsup_inventory = True
-        # not right yet - should be more semantically clear ?
-        #
-        # TODO: make sure to construct the right store classes, etc, depending
-        # on whether escaping is required.
-        self._warn_if_deprecated()
         self._write_group = None
         # Additional places to query for data.
         self._fallback_repositories = []
@@ -1389,6 +1385,7 @@
         locked = self.is_locked()
         result = self.control_files.lock_write(token=token)
         if not locked:
+            self._warn_if_deprecated()
             self._note_lock('w')
             for repo in self._fallback_repositories:
                 # Writes don't affect fallback repos
@@ -1400,6 +1397,7 @@
         locked = self.is_locked()
         self.control_files.lock_read()
         if not locked:
+            self._warn_if_deprecated()
             self._note_lock('r')
             for repo in self._fallback_repositories:
                 repo.lock_read()
@@ -2782,13 +2780,22 @@
         result.check(callback_refs)
         return result
 
-    def _warn_if_deprecated(self):
+    def _warn_if_deprecated(self, branch=None):
         global _deprecation_warning_done
         if _deprecation_warning_done:
             return
-        _deprecation_warning_done = True
-        warning("Format %s for %s is deprecated - please use 'bzr upgrade' to get better performance"
-                % (self._format, self.bzrdir.transport.base))
+        try:
+            if branch is None:
+                conf = config.GlobalConfig()
+            else:
+                conf = branch.get_config()
+            if conf.suppress_warning('format_deprecation'):
+                return
+            warning("Format %s for %s is deprecated -"
+                    " please use 'bzr upgrade' to get better performance"
+                    % (self._format, self.bzrdir.transport.base))
+        finally:
+            _deprecation_warning_done = True
 
     def supports_rich_root(self):
         return self._format.rich_root_data

=== modified file 'bzrlib/tests/blackbox/test_exceptions.py'
--- bzrlib/tests/blackbox/test_exceptions.py	2009-08-20 06:25:02 +0000
+++ bzrlib/tests/blackbox/test_exceptions.py	2009-12-16 10:09:27 +0000
@@ -22,8 +22,11 @@
 
 from bzrlib import (
     bzrdir,
+    config,
     errors,
+    osutils,
     repository,
+    tests,
     trace,
     )
 
@@ -45,18 +48,70 @@
         self.assertContainsRe(err, r'Bazaar has encountered an internal error')
 
 
-class TestDeprecationWarning(TestCaseInTempDir):
+class TestDeprecationWarning(tests.TestCaseWithTransport):
+    """The deprecation warning is controlled via a global variable:
+    repository._deprecation_warning_done. As such, it can be emitted only once
+    during a bzr invocation, no matter how many repositories are involved.
+
+    It would be better if it was a repo attribute instead but that's far more
+    work than I want to do right now -- vila 20091215.
+    """
+
+    def setUp(self):
+        super(TestDeprecationWarning, self).setUp()
+        self.disable_deprecation_warning()
+
+    def enable_deprecation_warning(self, repo=None):
+        """repo is not used yet since _deprecation_warning_done is a global"""
+        repository._deprecation_warning_done = False
+
+    def disable_deprecation_warning(self, repo=None):
+        """repo is not used yet since _deprecation_warning_done is a global"""
+        repository._deprecation_warning_done = True
+
+    def make_obsolete_repo(self, path):
+        # We don't want the deprecation raising during the repo creation
+        tree = self.make_branch_and_tree(path, format=bzrdir.BzrDirFormat5())
+        return tree
+
+    def check_warning(self, present):
+        if present:
+            check = self.assertContainsRe
+        else:
+            check = self.assertNotContainsRe
+        check(self._get_log(keep_log_file=True), 'WARNING.*bzr upgrade')
 
     def test_repository_deprecation_warning(self):
         """Old formats give a warning"""
-        # the warning's normally off for testing but we reenable it
-        repository._deprecation_warning_done = False
-        try:
-            os.mkdir('foo')
-            bzrdir.BzrDirFormat5().initialize('foo')
-            out, err = self.run_bzr("status foo")
-            self.assertContainsRe(self._get_log(keep_log_file=True),
-                                  "bzr upgrade")
-        finally:
-            repository._deprecation_warning_done = True
+        self.make_obsolete_repo('foo')
+        self.enable_deprecation_warning()
+        out, err = self.run_bzr('status', working_dir='foo')
+        self.check_warning(True)
+
+    def test_repository_deprecation_warning_suppressed_global(self):
+        """Old formats give a warning"""
+        conf = config.GlobalConfig()
+        conf.set_user_option('suppress_warnings', 'format_deprecation')
+        self.make_obsolete_repo('foo')
+        self.enable_deprecation_warning()
+        out, err = self.run_bzr('status', working_dir='foo')
+        self.check_warning(False)
+
+    def test_repository_deprecation_warning_suppressed_locations(self):
+        """Old formats give a warning"""
+        self.make_obsolete_repo('foo')
+        conf = config.LocationConfig(osutils.pathjoin(self.test_dir, 'foo'))
+        conf.set_user_option('suppress_warnings', 'format_deprecation')
+        self.enable_deprecation_warning()
+        out, err = self.run_bzr('status', working_dir='foo')
+        self.check_warning(False)
+
+    def test_repository_deprecation_warning_suppressed_branch(self):
+        """Old formats give a warning"""
+        tree = self.make_obsolete_repo('foo')
+        conf = tree.branch.get_config()
+        conf.set_user_option('suppress_warnings', 'format_deprecation')
+        self.enable_deprecation_warning()
+        out, err = self.run_bzr('status', working_dir='foo')
+        self.check_warning(False)
 

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2009-10-31 01:43:48 +0000
+++ bzrlib/tests/test_config.py	2009-12-16 10:09:27 +0000
@@ -369,6 +369,14 @@
 
 class TestIniConfig(tests.TestCase):
 
+    def make_config_parser(self, s):
+        conf = config.IniBasedConfig(None)
+        parser = conf._get_parser(file=StringIO(s.encode('utf-8')))
+        return conf, parser
+
+
+class TestIniConfigBuilding(TestIniConfig):
+
     def test_contructs(self):
         my_config = config.IniBasedConfig("nothing")
 
@@ -385,20 +393,53 @@
         parser = my_config._get_parser(file=config_file)
         self.failUnless(my_config._get_parser() is parser)
 
+
+class TestGetUserOptionAs(TestIniConfig):
+
     def test_get_user_option_as_bool(self):
-        config_file = StringIO("""
+        conf, parser = self.make_config_parser("""
 a_true_bool = true
 a_false_bool = 0
 an_invalid_bool = maybe
-a_list = hmm, who knows ? # This interpreted as a list !
-""".encode('utf-8'))
-        my_config = config.IniBasedConfig(None)
-        parser = my_config._get_parser(file=config_file)
-        get_option = my_config.get_user_option_as_bool
-        self.assertEqual(True, get_option('a_true_bool'))
-        self.assertEqual(False, get_option('a_false_bool'))
-        self.assertIs(None, get_option('an_invalid_bool'))
-        self.assertIs(None, get_option('not_defined_in_this_config'))
+a_list = hmm, who knows ? # This is interpreted as a list !
+""")
+        get_bool = conf.get_user_option_as_bool
+        self.assertEqual(True, get_bool('a_true_bool'))
+        self.assertEqual(False, get_bool('a_false_bool'))
+        self.assertIs(None, get_bool('an_invalid_bool'))
+        self.assertIs(None, get_bool('not_defined_in_this_config'))
+
+
+    def test_get_user_option_as_list(self):
+        conf, parser = self.make_config_parser("""
+a_list = a,b,c
+length_1 = 1,
+one_item = x
+""")
+        get_list = conf.get_user_option_as_list
+        self.assertEqual(['a', 'b', 'c'], get_list('a_list'))
+        self.assertEqual(['1'], get_list('length_1'))
+        self.assertEqual('x', conf.get_user_option('one_item'))
+        # automatically cast to list
+        self.assertEqual(['x'], get_list('one_item'))
+
+
+class TestSupressWarning(TestIniConfig):
+
+    def make_warnings_config(self, s):
+        conf, parser = self.make_config_parser(s)
+        return conf.suppress_warning
+
+    def test_suppress_warning_unknown(self):
+        suppress_warning = self.make_warnings_config('')
+        self.assertEqual(False, suppress_warning('unknown_warning'))
+
+    def test_suppress_warning_known(self):
+        suppress_warning = self.make_warnings_config('suppress_warnings=a,b')
+        self.assertEqual(False, suppress_warning('c'))
+        self.assertEqual(True, suppress_warning('a'))
+        self.assertEqual(True, suppress_warning('b'))
+
 
 class TestGetConfig(tests.TestCase):