Merge lp:~jelmer/bzr/update-feature-flags into lp:bzr

[Jelmer Vernooij]

Add {Repository,Branch,WorkingTree,BzrDir}.update_feature_flags methods.

Add update_feature_flags methods to the various bzr objects (BzrDir, BzrBranch, RepositoryMetaDir, InventoryWorkingTree).

This allows us to actually use feature flags.

[Vincent Ladeuil (vila)]

> This allows us to actually use feature flags.

The code seems fine, but there is no doc to explain the sentence above :)

How should this be used ? Via hooks ? Directly ? Some example or at least
some guidance sounds necessary at this point.

95 + ('branch-format', self.as_string()),
104 + self.target_format.as_string())
113 + self.target_format.as_string())

Strictly speaking this should have been part of your previous proposal,
right ? I'm mostly concerned about whether this needs to be backported and
why no tests caught them (I suspect it's because no test activate features
but it's slightly worrying that other occurrences could have been
forgotten).

review: Needs Fixing

[Jelmer Vernooij (jelmer)]

On 12/23/2011 08:18 AM, Vincent Ladeuil wrote:
> Review: Needs Fixing
>
>> This allows us to actually use feature flags.
>
> The code seems fine, but there is no doc to explain the sentence above :)
>
> How should this be used ? Via hooks ? Directly ? Some example or at least
> some guidance sounds necessary at this point.
That's intentional because there is no right way. :-)

Plugins would call these methods when they want to enable features on a
repository/branch/tree/dir. When they do that is up to them. In e.g.
bzr-search I imagine it will set the feature flag when the user runs
"bzr index" for the first time. In bzr-git it would automatically set it
when the user fetches into the repository with bzr-git, etc.

Where should I be documenting this?
> 95 + ('branch-format', self.as_string()),
> 104 + self.target_format.as_string())
> 113 + self.target_format.as_string())
>
> Strictly speaking this should have been part of your previous proposal,
> right ? I'm mostly concerned about whether this needs to be backported and
> why no tests caught them (I suspect it's because no test activate features
> but it's slightly worrying that other occurrences could have been
> forgotten).
This is for upgrades dealing with features properly. I'll pull it out
into a searate branch with tests.

Cheers,

Jelmer

[Vincent Ladeuil (vila)]

> > How should this be used ? Via hooks ? Directly ? Some example or at least
> > some guidance sounds necessary at this point.

> That's intentional because there is no right way. :-)

>
> Plugins would call these methods when they want to enable features on a
> repository/branch/tree/dir. When they do that is up to them. In e.g.
> bzr-search I imagine it will set the feature flag when the user runs
> "bzr index" for the first time. In bzr-git it would automatically set it
> when the user fetches into the repository with bzr-git, etc.

Right, thanks for the clarification. Indeed, these examples make it quite clearer how they should be used.

>
> Where should I be documenting this?

Right into doc/developers/feature-flags.txt :)

> > 95 + ('branch-format', self.as_string()),
> > 104 + self.target_format.as_string())
> > 113 + self.target_format.as_string())
> >
> > Strictly speaking this should have been part of your previous proposal,
> > right ? I'm mostly concerned about whether this needs to be backported and
> > why no tests caught them (I suspect it's because no test activate features
> > but it's slightly worrying that other occurrences could have been
> > forgotten).

> This is for upgrades dealing with features properly.

Ha ok.

> I'll pull it out into a searate branch with tests.

Cool.

With some doc added based on your answer above, I'm fine with this landing, up to you to ask for a review about it or from someone else.

review: Approve

[Jelmer Vernooij (jelmer)]

sent to pqm by email

[Jelmer Vernooij (jelmer)]

sent to pqm by email

[Jelmer Vernooij (jelmer)]

sent to pqm by email

[Jelmer Vernooij (jelmer)]

sent to pqm by email

[Jelmer Vernooij (jelmer)]

sent to pqm by email

[Jelmer Vernooij (jelmer)]

sent to pqm by email

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2011-12-22 18:52:58 +0000
+++ bzrlib/branch.py	2011-12-30 15:29:25 +0000
@@ -2721,6 +2721,16 @@
         self._transport.put_bytes('last-revision', out_string,
             mode=self.bzrdir._get_file_mode())
 
+    @needs_write_lock
+    def update_feature_flags(self, updated_flags):
+        """Update the feature flags for this branch.
+
+        :param updated_flags: Dictionary mapping feature names to necessities
+            A necessity can be None to indicate the feature should be removed
+        """
+        self._format._update_feature_flags(updated_flags)
+        self.control_transport.put_bytes('format', self._format.as_string())
+
 
 class FullHistoryBzrBranch(BzrBranch):
     """Bzr branch which contains the full revision history."""

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2011-12-22 15:33:16 +0000
+++ bzrlib/bzrdir.py	2011-12-30 15:29:25 +0000
@@ -786,6 +786,19 @@
     def __repr__(self):
         return "<%s at %r>" % (self.__class__.__name__, self.user_url)
 
+    def update_feature_flags(self, updated_flags):
+        """Update the features required by this bzrdir.
+
+        :param updated_flags: Dictionary mapping feature names to necessities
+            A necessity can be None to indicate the feature should be removed
+        """
+        self.control_files.lock_write()
+        try:
+            self._format._update_feature_flags(updated_flags)
+            self.transport.put_bytes('branch-format', self._format.as_string())
+        finally:
+            self.control_files.unlock()
+
 
 class BzrDirMeta1(BzrDir):
     """A .bzr meta version 1 control object.
@@ -796,6 +809,11 @@
     present within a BzrDir.
     """
 
+    def __init__(self, _transport, _format):
+        super(BzrDirMeta1, self).__init__(_transport, _format)
+        self.control_files = lockable_files.LockableFiles(self.control_transport,
+            self._format._lock_file_name, self._format._lock_class)
+
     def can_convert_format(self):
         """See BzrDir.can_convert_format()."""
         return True
@@ -993,11 +1011,6 @@
     BzrDirMeta1.
     """
 
-    def __init__(self, _transport, _format):
-        super(BzrDirMeta1Colo, self).__init__(_transport, _format)
-        self.control_files = lockable_files.LockableFiles(self.control_transport,
-            self._format._lock_file_name, self._format._lock_class)
-
     def _get_branch_path(self, name):
         """Obtain the branch path to use.
 
@@ -1213,6 +1226,20 @@
         return (self.__class__ is other.__class__ and
                 self.features == other.features)
 
+    def _update_feature_flags(self, updated_flags):
+        """Update the feature flags in this format.
+
+        :param updated_flags: Updated feature flags
+        """
+        for name, necessity in updated_flags.iteritems():
+            if necessity is None:
+                try:
+                    del self.features[name]
+                except KeyError:
+                    pass
+            else:
+                self.features[name] = necessity
+
 
 class BzrProber(controldir.Prober):
     """Prober for formats that use a .bzr/ control directory."""
@@ -1451,7 +1478,7 @@
                        "This is a Bazaar control directory.\n"
                        "Do not change any files in this directory.\n"
                        "See http://bazaar.canonical.com/ for more information about Bazaar.\n"),
-                      ('branch-format', self.get_format_string()),
+                      ('branch-format', self.as_string()),
                       ]
         # NB: no need to escape relative paths that are url safe.
         control_files = lockable_files.LockableFiles(bzrdir_transport,
@@ -1866,7 +1893,7 @@
     def convert(self, to_convert, pb):
         """See Converter.convert()."""
         to_convert.transport.put_bytes('branch-format',
-            self.target_format.get_format_string())
+            self.target_format.as_string())
         return BzrDir.open_from_transport(to_convert.root_transport)
 
 
@@ -1892,7 +1919,7 @@
         finally:
             to_convert.control_files.unlock()
         to_convert.transport.put_bytes('branch-format',
-            self.target_format.get_format_string())
+            self.target_format.as_string())
         return BzrDir.open_from_transport(to_convert.root_transport)
 
 

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2011-12-19 19:15:58 +0000
+++ bzrlib/repository.py	2011-12-30 15:29:25 +0000
@@ -1293,6 +1293,16 @@
         """Returns the policy for making working trees on new branches."""
         return not self._transport.has('no-working-trees')
 
+    @needs_write_lock
+    def update_feature_flags(self, updated_flags):
+        """Update the feature flags for this branch.
+
+        :param updated_flags: Dictionary mapping feature names to necessities
+            A necessity can be None to indicate the feature should be removed
+        """
+        self._format._update_feature_flags(updated_flags)
+        self.control_transport.put_bytes('format', self._format.as_string())
+
 
 class RepositoryFormatRegistry(controldir.ControlComponentFormatRegistry):
     """Repository format registry."""

=== modified file 'bzrlib/tests/blackbox/test_annotate.py'
--- bzrlib/tests/blackbox/test_annotate.py	2011-12-14 21:57:37 +0000
+++ bzrlib/tests/blackbox/test_annotate.py	2011-12-30 15:29:25 +0000
@@ -326,7 +326,7 @@
         # being too low. If rpc_count increases, more network roundtrips have
         # become necessary for this use case. Please do not adjust this number
         # upwards without agreement from bzr's network support maintainers.
-        self.assertLength(15, self.hpss_calls)
+        self.assertLength(16, self.hpss_calls)
         self.assertLength(1, self.hpss_connections)
         self.expectFailure("annotate accesses inventories, which require VFS access",
             self.assertThat, self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py	2011-12-30 14:19:53 +0000
+++ bzrlib/tests/blackbox/test_commit.py	2011-12-30 15:29:25 +0000
@@ -889,7 +889,7 @@
         # being too low. If rpc_count increases, more network roundtrips have
         # become necessary for this use case. Please do not adjust this number
         # upwards without agreement from bzr's network support maintainers.
-        self.assertLength(213, self.hpss_calls)
+        self.assertLength(214, self.hpss_calls)
         self.assertLength(2, self.hpss_connections)
         self.expectFailure("commit still uses VFS calls",
             self.assertThat, self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/test_branch.py'
--- bzrlib/tests/test_branch.py	2011-12-22 18:52:58 +0000
+++ bzrlib/tests/test_branch.py	2011-12-30 15:29:25 +0000
@@ -219,12 +219,13 @@
 
     def test_find_format_with_features(self):
         tree = self.make_branch_and_tree('.', format='2a')
-        tree.branch.control_transport.put_bytes('format',
-            tree.branch._format.get_format_string() +
-            "optional name\n")
+        tree.branch.update_feature_flags({"name": "optional"})
         found_format = _mod_branch.BranchFormatMetadir.find_format(tree.bzrdir)
         self.assertIsInstance(found_format, _mod_branch.BranchFormatMetadir)
         self.assertEquals(found_format.features.get("name"), "optional")
+        tree.branch.update_feature_flags({"name": None})
+        branch = _mod_branch.Branch.open('.')
+        self.assertEquals(branch._format.features, {})
 
     def test_register_unregister_format(self):
         # Test the deprecated format registration functions

=== modified file 'bzrlib/tests/test_bzrdir.py'
--- bzrlib/tests/test_bzrdir.py	2011-12-22 15:33:16 +0000
+++ bzrlib/tests/test_bzrdir.py	2011-12-30 15:29:25 +0000
@@ -1027,14 +1027,15 @@
 
     def test_with_features(self):
         tree = self.make_branch_and_tree('tree', format='2a')
-        tree.bzrdir.control_transport.put_bytes(
-            'branch-format',
-            tree.bzrdir._format.get_format_string() + "required bar\n")
+        tree.bzrdir.update_feature_flags({"bar": "required"})
         self.assertRaises(errors.MissingFeature, bzrdir.BzrDir.open, 'tree')
         bzrdir.BzrDirMetaFormat1.register_feature('bar')
         self.addCleanup(bzrdir.BzrDirMetaFormat1.unregister_feature, 'bar')
         dir = bzrdir.BzrDir.open('tree')
         self.assertEquals("required", dir._format.features.get("bar"))
+        tree.bzrdir.update_feature_flags({"bar": None, "nonexistant": None})
+        dir = bzrdir.BzrDir.open('tree')
+        self.assertEquals({}, dir._format.features)
 
     def test_needs_conversion_different_working_tree(self):
         # meta1dirs need an conversion if any element is not the default.

=== modified file 'bzrlib/tests/test_remote.py'
--- bzrlib/tests/test_remote.py	2011-12-15 12:59:56 +0000
+++ bzrlib/tests/test_remote.py	2011-12-30 15:29:25 +0000
@@ -933,6 +933,7 @@
         # name.
         client.add_success_response_with_body(
             "Bazaar-NG meta directory, format 1\n", 'ok')
+        client.add_success_response('stat', '0', '65535')
         client.add_success_response_with_body(
             reference_format.get_format_string(), 'ok')
         # PackRepository wants to do a stat
@@ -947,6 +948,7 @@
              ('call', 'BzrDir.find_repositoryV2', ('quack/',)),
              ('call', 'BzrDir.find_repository', ('quack/',)),
              ('call_expecting_body', 'get', ('/quack/.bzr/branch-format',)),
+             ('call', 'stat', ('/quack/.bzr',)),
              ('call_expecting_body', 'get', ('/quack/.bzr/repository/format',)),
              ('call', 'stat', ('/quack/.bzr/repository',)),
              ],
@@ -966,6 +968,7 @@
         # name.
         client.add_success_response_with_body(
             "Bazaar-NG meta directory, format 1\n", 'ok')
+        client.add_success_response('stat', '0', '65535')
         client.add_success_response_with_body(
             reference_format.get_format_string(), 'ok')
         # PackRepository wants to do a stat
@@ -979,6 +982,7 @@
             [('call', 'BzrDir.find_repositoryV3', ('quack/',)),
              ('call', 'BzrDir.find_repositoryV2', ('quack/',)),
              ('call_expecting_body', 'get', ('/quack/.bzr/branch-format',)),
+             ('call', 'stat', ('/quack/.bzr',)),
              ('call_expecting_body', 'get', ('/quack/.bzr/repository/format',)),
              ('call', 'stat', ('/quack/.bzr/repository',)),
              ],
@@ -1270,7 +1274,7 @@
         verb = 'Branch.set_parent_location'
         self.disable_verb(verb)
         branch.set_parent('http://foo/')
-        self.assertLength(12, self.hpss_calls)
+        self.assertLength(13, self.hpss_calls)
 
 
 class TestBranchGetTagsBytes(RemoteBranchTestCase):
@@ -1998,7 +2002,7 @@
         self.addCleanup(branch.unlock)
         self.reset_smart_call_log()
         branch._get_config().set_option('value', 'name')
-        self.assertLength(10, self.hpss_calls)
+        self.assertLength(11, self.hpss_calls)
         self.assertEqual('value', branch._get_config().get_option('name'))
 
     def test_backwards_compat_set_option_with_dict(self):
@@ -2012,7 +2016,7 @@
         config = branch._get_config()
         value_dict = {'ascii': 'a', u'unicode \N{WATCH}': u'\N{INTERROBANG}'}
         config.set_option(value_dict, 'name')
-        self.assertLength(10, self.hpss_calls)
+        self.assertLength(11, self.hpss_calls)
         self.assertEqual(value_dict, branch._get_config().get_option('name'))
 
 
@@ -2138,7 +2142,7 @@
         self.reset_smart_call_log()
         self.assertEquals((0, ),
             branch.revision_id_to_dotted_revno('null:'))
-        self.assertLength(7, self.hpss_calls)
+        self.assertLength(8, self.hpss_calls)
 
 
 class TestBzrDirGetSetConfig(RemoteBzrDirTestCase):
@@ -2160,7 +2164,7 @@
         self.reset_smart_call_log()
         config = bzrdir.get_config()
         config.set_default_stack_on('/')
-        self.assertLength(3, self.hpss_calls)
+        self.assertLength(4, self.hpss_calls)
 
     def test_backwards_compat_get_option(self):
         self.setup_smart_server_with_call_log()
@@ -2170,7 +2174,7 @@
         self.reset_smart_call_log()
         self.assertEqual(None,
             bzrdir._get_config().get_option('default_stack_on'))
-        self.assertLength(3, self.hpss_calls)
+        self.assertLength(4, self.hpss_calls)
 
 
 class TestTransportIsReadonly(tests.TestCase):

=== modified file 'bzrlib/tests/test_repository.py'
--- bzrlib/tests/test_repository.py	2011-12-22 16:21:25 +0000
+++ bzrlib/tests/test_repository.py	2011-12-30 15:29:25 +0000
@@ -166,9 +166,7 @@
 
     def test_find_format_with_features(self):
         tree = self.make_branch_and_tree('.', format='2a')
-        tree.branch.repository.control_transport.put_bytes('format',
-            tree.branch.repository._format.get_format_string() +
-            "necessity name\n")
+        tree.branch.repository.update_feature_flags({"name": "necessity"})
         found_format = repository.RepositoryFormatMetaDir.find_format(tree.bzrdir)
         self.assertIsInstance(found_format, repository.RepositoryFormatMetaDir)
         self.assertEquals(found_format.features.get("name"), "necessity")

=== modified file 'bzrlib/tests/test_workingtree.py'
--- bzrlib/tests/test_workingtree.py	2011-12-22 17:30:20 +0000
+++ bzrlib/tests/test_workingtree.py	2011-12-30 15:29:25 +0000
@@ -245,8 +245,7 @@
 
     def test_find_format_with_features(self):
         tree = self.make_branch_and_tree('.', format='2a')
-        tree.control_transport.put_bytes('format',
-            tree._format.get_format_string() + "necessity name\n")
+        tree.update_feature_flags({"name": "necessity"})
         found_format = workingtree.WorkingTreeFormatMetaDir.find_format(
             tree.bzrdir)
         self.assertIsInstance(found_format, workingtree.WorkingTreeFormat)

=== modified file 'bzrlib/workingtree.py'
--- bzrlib/workingtree.py	2011-12-19 19:15:58 +0000
+++ bzrlib/workingtree.py	2011-12-30 15:29:25 +0000
@@ -2977,6 +2977,16 @@
                 if dir[2] == _directory:
                     pending.append(dir)
 
+    @needs_write_lock
+    def update_feature_flags(self, updated_flags):
+        """Update the feature flags for this branch.
+
+        :param updated_flags: Dictionary mapping feature names to necessities
+            A necessity can be None to indicate the feature should be removed
+        """
+        self._format._update_feature_flags(updated_flags)
+        self.control_transport.put_bytes('format', self._format.as_string())
+
 
 class WorkingTreeFormatRegistry(controldir.ControlComponentFormatRegistry):
     """Registry for working tree formats."""

=== modified file 'doc/developers/feature-flags.txt'
--- doc/developers/feature-flags.txt	2011-12-22 14:25:34 +0000
+++ doc/developers/feature-flags.txt	2011-12-30 15:29:25 +0000
@@ -129,6 +129,16 @@
  * BzrFormat.features.set(name, necessity)
  * BzrFormat.features.get(name) -> necessity
 
+Enabling features
+-----------------
+
+Features are enabled through the ``update_feature_flags`` method on
+``Repository``, ``Branch``, ``WorkingTree`` and ``BzrDir``.
+
+These methods are called by whatever needs to enable features.
+When they do that is up to them - e.g. bzr-search would enable its
+feature when ``bzr index`` is first run.
+
 See also
 --------