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

[Jelmer Vernooij]

Add basic support for feature flags.

Add basic support for feature flags to the 2.1 release series.

This does the bare minimum to:
 * ignore optional feature flags
 * raise an appropriate error (non-confusing) for any other feature flags

[Vincent Ladeuil (vila)]

Looks mostly fine :)

I'm unclear about how this diverges from implementation in 2.5 though, can you elaborate ?

This seems to special-case for branch, why is that ? Repo and wt relies on bzrdir but not branch ? Is this also true for 2.2 and up ?

Also, the following tests fails:

=== modified file 'bzrlib/tests/test_bzrdir.py'
--- bzrlib/tests/test_bzrdir.py 2012-03-15 14:54:05 +0000
+++ bzrlib/tests/test_bzrdir.py 2012-03-16 08:27:13 +0000
@@ -1386,3 +1386,11 @@
         self.assertRaises(errors.ParseFormatError,
             bzrdir.extract_format_string, "Bazaar-NG branch, format 0.0.4\n"
                                           "requiredfoo\n")
+
+ def test_with_empty_file(self):
+ self.assertRaises(errors.ParseFormatError,
+ bzrdir.extract_format_string, "")
+
+ def test_with_corrupted_file(self):
+ self.assertRaises(errors.ParseFormatError,
+ bzrdir.extract_format_string, "Bazaar-NG branch")

Again, I'm not sure if this diverges from the *actual* behavior but since the format file is modified when a feature is added/removed, I'd like us to be paranoid about *new* potential failures. I.e. if the actual implementation does not raise ParseFormatError for these tests, fine, otherwise, please consider adding them anf fixing the fallouts ;)

Finally, I don't quite understand why you 4 proposals instead of just this one. If they are all the same, no need for 4 reviews, once this one is approved, just merge up to trunk.

review: Needs Information

[Vincent Ladeuil (vila)]

I failed to mention: I've dreamed for a looong time of being able to review some code by just adding failing tests... raising the S/N ratio to a factual question.

Thanks for making a proposal focused and well tested allowing my dream to come true ;-)

[Vincent Ladeuil (vila)]

One more thing: how did you test this ?

For the review purpose I tried './bzr selftest -s bt.test_bzrdir.ExtractFormatStringTests' which failed horribly with the python/subunit/testtools I use on precise. I then switch to your bzr-2.4 proposal (noticing no difference from a causal reading) and things were fine there.

Still, not being able to run the tests for a supported release makes it harder to release or review (not unrelated to the ML thread about selftest ;). So please enlight me :) Some lucid chroot ?

[Jelmer Vernooij (jelmer)]

> I'm unclear about how this diverges from implementation in 2.5 though, can you
> elaborate ?
It doesn't actually allow users to register features or use them. It just makes bzr 2.1 ignore optional features and raise an appropriate error when non-optional features are encountered. This is why the branch is so simple.

> This seems to special-case for branch, why is that ? Repo and wt relies on
> bzrdir but not branch ? Is this also true for 2.2 and up ?
I'm not sure I follow; It doesn't special case branch, updating branch is a single line change just like working tree, bzrdir and repository. Can you elaborate perhaps?

>
> Also, the following tests fails:
>
> === modified file 'bzrlib/tests/test_bzrdir.py'
> --- bzrlib/tests/test_bzrdir.py 2012-03-15 14:54:05 +0000
> +++ bzrlib/tests/test_bzrdir.py 2012-03-16 08:27:13 +0000
> @@ -1386,3 +1386,11 @@
> self.assertRaises(errors.ParseFormatError,
> bzrdir.extract_format_string, "Bazaar-NG branch, format 0.0.4\n"
> "requiredfoo\n")
> +
> + def test_with_empty_file(self):
> + self.assertRaises(errors.ParseFormatError,
> + bzrdir.extract_format_string, "")
> +
> + def test_with_corrupted_file(self):
> + self.assertRaises(errors.ParseFormatError,
> + bzrdir.extract_format_string, "Bazaar-NG branch")
>
> Again, I'm not sure if this diverges from the *actual* behavior but since the
> format file is modified when a feature is added/removed, I'd like us to be
> paranoid about *new* potential failures. I.e. if the actual implementation
> does not raise ParseFormatError for these tests, fine, otherwise, please
> consider adding them anf fixing the fallouts ;)
The actual implementation doesn't have extract_format_string, since it doesn't apply there (it has a more complex parsing function). I'll look into the failures.

> Finally, I don't quite understand why you 4 proposals instead of just this
> one. If they are all the same, no need for 4 reviews, once this one is
> approved, just merge up to trunk.
They aren't the same, there are differences to the format infrastructure for each branch that would otherwise cause conflicts.

[Vincent Ladeuil (vila)]

> > I'm unclear about how this diverges from implementation in 2.5 though, can
> you
> > elaborate ?
> It doesn't actually allow users to register features or use them. It just
> makes bzr 2.1 ignore optional features and raise an appropriate error when
> non-optional features are encountered. This is why the branch is so simple.

Right. I was slightly confused that I couldn't find extract_format_string() in trunk.

>
> > This seems to special-case for branch, why is that ? Repo and wt relies on
> > bzrdir but not branch ? Is this also true for 2.2 and up ?
> I'm not sure I follow; It doesn't special case branch, updating branch is a
> single line change just like working tree, bzrdir and repository. Can you
> elaborate perhaps?

Hmm, yeah, gimme a minute to turn sober....

...

ha, indeed, I missed the single line change in wt and repo, please forgive me ;)

> The actual implementation doesn't have extract_format_string, since it doesn't
> apply there (it has a more complex parsing function).

Ok, so you will remove extract_format_string when merging up then. Ok, I was hoping we could keep it and add the needed features instead to keep a single place to maintain, but no big deal as long as we can keep the tests ?

> I'll look into the
> failures.

Cool.

>
> > Finally, I don't quite understand why you 4 proposals instead of just this
> > one. If they are all the same, no need for 4 reviews, once this one is
> > approved, just merge up to trunk.
> They aren't the same, there are differences to the format infrastructure for
> each branch that would otherwise cause conflicts.

:-/ Let's hope we don't need to fix bugs there then. Maintaining the same feature in 5 (or 6 ?) different forms *for* a dvcs tool sounds like a good way to become insane ;)

[Jelmer Vernooij (jelmer)]

> > The actual implementation doesn't have extract_format_string, since it
> doesn't
> > apply there (it has a more complex parsing function).
> Ok, so you will remove extract_format_string when merging up then. Ok, I was
> hoping we could keep it and add the needed features instead to keep a single
> place to maintain, but no big deal as long as we can keep the tests ?
Yep.

> > I'll look into the
> > failures.
> Cool.
Fixed now.

> > > Finally, I don't quite understand why you 4 proposals instead of just this
> > > one. If they are all the same, no need for 4 reviews, once this one is
> > > approved, just merge up to trunk.
> > They aren't the same, there are differences to the format infrastructure for
> > each branch that would otherwise cause conflicts.
> :-/ Let's hope we don't need to fix bugs there then. Maintaining the same
> feature in 5 (or 6 ?) different forms *for* a dvcs tool sounds like a good way
> to become insane ;
It's not radically different, but simply upmerging triggers conflicts.

I've tested this in a lucid chroot, as 2.1's testsuite indeed doesn't run on precise.

Cheers,

Jelmer

[Vincent Ladeuil (vila)]

> > > The actual implementation doesn't have extract_format_string, since it
> > doesn't
> > > apply there (it has a more complex parsing function).
> > Ok, so you will remove extract_format_string when merging up then. Ok, I was
> > hoping we could keep it and add the needed features instead to keep a single
> > place to maintain, but no big deal as long as we can keep the tests ?
> Yep.

Great.

>
> > > I'll look into the
> > > failures.
> > Cool.
> Fixed now.

Thanks.

>
> > > > Finally, I don't quite understand why you 4 proposals instead of just
> this
> > > > one. If they are all the same, no need for 4 reviews, once this one is
> > > > approved, just merge up to trunk.
> > > They aren't the same, there are differences to the format infrastructure
> for
> > > each branch that would otherwise cause conflicts.
> > :-/ Let's hope we don't need to fix bugs there then. Maintaining the same
> > feature in 5 (or 6 ?) different forms *for* a dvcs tool sounds like a good
> way
> > to become insane ;
> It's not radically different, but simply upmerging triggers conflicts.
>

:-/

> I've tested this in a lucid chroot, as 2.1's testsuite indeed doesn't run on
> precise.
>

Ok.

review: Approve

[Vincent Ladeuil (vila)]

Unless you encounter issues or want more feedback for your other related proposals, just merge up the tests and land them too.

[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 'NEWS'
--- NEWS	2011-08-19 12:49:28 +0000
+++ NEWS	2012-03-18 19:06:24 +0000
@@ -47,6 +47,10 @@
 Improvements
 ************
 
+ * When opening formats with feature flags, optional features are
+   ignored and an improved error is printed for non-optional features.
+   (Jelmer Vernooij)
+
 Documentation
 *************
 

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2010-02-17 17:11:16 +0000
+++ bzrlib/branch.py	2012-03-18 19:06:24 +0000
@@ -1438,7 +1438,8 @@
         """Return the format for the branch object in a_bzrdir."""
         try:
             transport = a_bzrdir.get_branch_transport(None)
-            format_string = transport.get_bytes("format")
+            format_string = bzrdir.extract_format_string(
+                transport.get_bytes("format"))
             return klass._formats[format_string]
         except errors.NoSuchFile:
             raise errors.NotBranchError(path=transport.base, bzrdir=a_bzrdir)

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2010-11-26 18:10:01 +0000
+++ bzrlib/bzrdir.py	2012-03-18 19:06:24 +0000
@@ -87,6 +87,32 @@
     )
 
 
+def extract_format_string(text):
+    """Read a format string from a file.
+
+    The first line is returned. The other lines can contain
+    optional features. An exception is raised when a
+    required feature is present.
+    """
+    lines = text.splitlines(True)
+    try:
+        firstline = lines.pop(0)
+    except IndexError:
+        raise errors.UnknownFormatError(format=text, kind='')
+    for lineno, line in enumerate(lines):
+        try:
+            (necessity, feature) = line.split(" ", 1)
+        except ValueError:
+            raise errors.ParseFormatError(lineno=lineno+2,
+                line=line, text=text)
+        else:
+            if necessity == "optional":
+                mutter("Ignoring optional feature %s", feature)
+            else:
+                raise errors.MissingFeature(feature)
+    return firstline
+
+
 class BzrDir(object):
     """A .bzr control diretory.
 
@@ -1829,6 +1855,8 @@
         except errors.NoSuchFile:
             raise errors.NotBranchError(path=transport.base)
 
+        format_string = extract_format_string(format_string)
+
         try:
             return klass._formats[format_string]
         except KeyError:

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2010-09-10 05:13:57 +0000
+++ bzrlib/errors.py	2012-03-18 19:06:24 +0000
@@ -3136,3 +3136,23 @@
 
     def __init__(self, path):
         self.path = path
+
+
+class MissingFeature(BzrError):
+
+    _fmt = ("Missing feature %(feature)s not provided by this "
+            "version of Bazaar or any plugin.")
+
+    def __init__(self, feature):
+        self.feature = feature
+
+
+class ParseFormatError(BzrError):
+
+    _fmt = "Parse error on line %(lineno)d of format name: %(line)s"
+
+    def __init__(self, lineno, line, text):
+        BzrError.__init__(self)
+        self.lineno = lineno
+        self.line = line
+        self.text = text

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2010-11-30 20:42:42 +0000
+++ bzrlib/repository.py	2012-03-18 19:06:24 +0000
@@ -3107,6 +3107,7 @@
         try:
             transport = a_bzrdir.get_repository_transport(None)
             format_string = transport.get_bytes("format")
+            format_string = bzrdir.extract_format_string(format_string)
             return format_registry.get(format_string)
         except errors.NoSuchFile:
             raise errors.NoRepositoryPresent(a_bzrdir)

=== modified file 'bzrlib/tests/test_bzrdir.py'
--- bzrlib/tests/test_bzrdir.py	2010-02-17 17:11:16 +0000
+++ bzrlib/tests/test_bzrdir.py	2012-03-18 19:06:24 +0000
@@ -1333,3 +1333,25 @@
         url = transport.base
         err = self.assertRaises(errors.BzrError, bzrdir.BzrDir.open, url)
         self.assertEqual('fail', err._preformatted_string)
+
+
+class ExtractFormatStringTests(TestCase):
+
+    def test_normal(self):
+        self.assertEquals("Bazaar-NG branch, format 0.0.4\n",
+            bzrdir.extract_format_string("Bazaar-NG branch, format 0.0.4\n"))
+
+    def test_with_optional_feature(self):
+        self.assertEquals("Bazaar-NG branch, format 0.0.4\n",
+            bzrdir.extract_format_string("Bazaar-NG branch, format 0.0.4\n"
+                                         "optional feature foo\n"))
+
+    def test_with_required_feature(self):
+        self.assertRaises(errors.MissingFeature,
+            bzrdir.extract_format_string, "Bazaar-NG branch, format 0.0.4\n"
+                                          "required feature foo\n")
+
+    def test_with_invalid_line(self):
+        self.assertRaises(errors.ParseFormatError,
+            bzrdir.extract_format_string, "Bazaar-NG branch, format 0.0.4\n"
+                                          "requiredfoo\n")

=== modified file 'bzrlib/workingtree.py'
--- bzrlib/workingtree.py	2010-01-21 17:54:58 +0000
+++ bzrlib/workingtree.py	2012-03-18 19:06:24 +0000
@@ -2799,6 +2799,7 @@
         try:
             transport = a_bzrdir.get_workingtree_transport(None)
             format_string = transport.get_bytes("format")
+            format_string = bzrdir.extract_format_string(format_string)
             return klass._formats[format_string]
         except errors.NoSuchFile:
             raise errors.NoWorkingTree(base=transport.base)