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

[Jelmer Vernooij]

Add support for feature flags in bzr formats.

Add support for feature flags.

For a background on feature flags, see doc/developers/feature-flags.txt.

This adds a common base class ("BzrFormat") for all classes that deal with bzr 'format' files. This includes all bzrdir implementations, and branch/repository/workingtree formats that can live in a .bzr meta dir. This base class handles serialization of the contents of the format file, and allows the setting of feature flags.

There currently is no way to set feature flags from the Repository/WorkingTree/Branch API (including locking, etc) I'm leaving that for another branch as this one is already big enough.

There is a very simple example of a plugin using this in lp:~jelmer/bzr-search/feature-flags.

[Vincent Ladeuil (vila)]

Summary (spoiler alert): I have only minor concerns about the format. Most
of my remarks are more about interoperability with older bzr versions and
even how newer bzr versions will interact around the feature flags. So
basically, let's make sure we have a good format first. So I'm voting
NeedsInfo but nothing really wrong caught my eye.

Re-reading doc/developers/feature-flags.txt, a couple of typos:

   modified doc/developers/feature-flags.txt

=== modified file 'doc/developers/feature-flags.txt'
--- doc/developers/feature-flags.txt 2011-12-07 17:45:57 +0000
+++ doc/developers/feature-flags.txt 2011-12-14 14:56:20 +0000
@@ -95,7 +95,7 @@
 -----------

 Class methods will be added to ``BzrFormat`` to allow registering
-and deregistering the presence of particular features. This class is inherited
+and unregistering the presence of particular features. This class is inherited
 by ``BzrBranchFormat``, ``VersionedFileRepositoryFormat`` and
 ``InventoryWorkingTreeFormat``.

@@ -104,7 +104,7 @@

 Upon opening, BzrFormat will be responsible for checking that the
 required features are present. lock_write will raise an exception
-when there is an un unsupported mandatory feature required for write access.
+when there is an unsupported mandatory feature required for write access.

 Methods will also be added to BzrFormat to allow plugins, etc,
 to check whether a feature is present and adding new features:

Derigester is valid but we use unregister everywhere else ;)

Now, about the format:

  Bazaar repository format 2a (needs bzr 1.16 or later)
  optional feature git
  optional feature search
  optional feature tiplog
  required feature nested-trees

I'd rather use an existing one than a new one so, how about:

  Bazaar repository format 2a (needs bzr 1.16 or later)
  [features]
  optional = git,search,tiplog
  required = nested-trees

or

  Bazaar repository format 2a (needs bzr 1.16 or later)
  [features]
  git = optional
  search = optional
  tiplog = optional
  nested_trees = required # _ not - if that matters...

Even '[features]' is only needed if we plan to support something else than
features or leave open this possibility.

Note that it doesn't have to support all the fancy features of ConfigObj,
especially quoting (not even comments...).

But more importantly, I think a needed feature of this format is that old
bzr clients can safely ignore anything but the first line, but also get a
way to check that the file itself has not been corrupted.

This probably means some sort of final marker, a checksum or a robust
parser. Note that the later means we won't change the format of this
file. This is probably ok as we can still create new formats... but it's a
bit weird to introduce a limitation when trying to make the format open to
additions.

Your proposed implementation in bzr-search looks simple, that's great.

128 + Feature flags are supported as of bzr 2.5. Setting feature flags on formats
129 + will render them inaccessible to older versions of bzr.

Hmm. I'm not sure I correctly understand the implications here.

What do we need to backport to older (su...

review: Needs Information

[Jelmer Vernooij (jelmer)]

Am 14/12/11 17:23, schrieb Vincent Ladeuil:
> Review: Needs Information
>
> Summary (spoiler alert): I have only minor concerns about the format. Most
> of my remarks are more about interoperability with older bzr versions and
> even how newer bzr versions will interact around the feature flags. So
> basically, let's make sure we have a good format first. So I'm voting
> NeedsInfo but nothing really wrong caught my eye.
Cool. Thanks for taking the time to review this, much appreciated!
>
>
> Re-reading doc/developers/feature-flags.txt, a couple of typos:
>
> modified doc/developers/feature-flags.txt
>
>
> === modified file 'doc/developers/feature-flags.txt'
> --- doc/developers/feature-flags.txt 2011-12-07 17:45:57 +0000
> +++ doc/developers/feature-flags.txt 2011-12-14 14:56:20 +0000
> @@ -95,7 +95,7 @@
> -----------
>
> Class methods will be added to ``BzrFormat`` to allow registering
> -and deregistering the presence of particular features. This class is inherited
> +and unregistering the presence of particular features. This class is inherited
> by ``BzrBranchFormat``, ``VersionedFileRepositoryFormat`` and
> ``InventoryWorkingTreeFormat``.
>
> @@ -104,7 +104,7 @@
>
> Upon opening, BzrFormat will be responsible for checking that the
> required features are present. lock_write will raise an exception
> -when there is an un unsupported mandatory feature required for write access.
> +when there is an unsupported mandatory feature required for write access.
>
> Methods will also be added to BzrFormat to allow plugins, etc,
> to check whether a feature is present and adding new features:
>
>
> Derigester is valid but we use unregister everywhere else ;)
Thanks, fixed.

> Now, about the format:
>
> Bazaar repository format 2a (needs bzr 1.16 or later)
> optional feature git
> optional feature search
> optional feature tiplog
> required feature nested-trees
>
> I'd rather use an existing one than a new one so, how about:
>
> Bazaar repository format 2a (needs bzr 1.16 or later)
> [features]
> optional = git,search,tiplog
> required = nested-trees
>
> or
>
> Bazaar repository format 2a (needs bzr 1.16 or later)
> [features]
> git = optional
> search = optional
> tiplog = optional
> nested_trees = required # _ not - if that matters...
>
> Even '[features]' is only needed if we plan to support something else than
> features or leave open this possibility.
>
> Note that it doesn't have to support all the fancy features of ConfigObj,
> especially quoting (not even comments...).
I'm not sure what using an ini-like format really gains us: the file
still isn't a standard ini-style file, because it has a custom first
line. It also makes parsing (and thus, backporting) much more complex.
One of the advantages of having one feature per line is that we can
allow whitespace in the feature information in the future, or allow
features to add parameters.

>
> But more importantly, I think a needed feature of this format is that old
> bzr clients can safely ignore anything but the first line, but also get a
> way to check that the file i...

[Vincent Ladeuil (vila)]

> > Bazaar repository format 2a (needs bzr 1.16 or later)
> > [features]
> > git = optional
> > search = optional
> > tiplog = optional
> > nested_trees = required # _ not - if that matters...
> >
> > Even '[features]' is only needed if we plan to support something else than
> > features or leave open this possibility.
> >
> > Note that it doesn't have to support all the fancy features of ConfigObj,
> > especially quoting (not even comments...).

> I'm not sure what using an ini-like format really gains us: the file
> still isn't a standard ini-style file, because it has a custom first
> line.

That's trivial to skip.

> It also makes parsing (and thus, backporting) much more complex.

Quite the contrary, with the first line skipped we can use ConfigObj for
that. We should also get line numbers in error messages for ~free.

> One of the advantages of having one feature per line is that we can
> allow whitespace in the feature information in the future, or allow
> features to add parameters.

git = optional, version, pumpernickel

works fine too.

>
> >
> > But more importantly, I think a needed feature of this format is that old
> > bzr clients can safely ignore anything but the first line, but also get a
> > way to check that the file itself has not been corrupted.
>
> > This probably means some sort of final marker, a checksum or a robust
> > parser. Note that the later means we won't change the format of this
> > file. This is probably ok as we can still create new formats... but it's a
> > bit weird to introduce a limitation when trying to make the format open to
> > additions.
> They should be able to ignore any lines starting with "optional ", but
> should refuse to open formats when there are lines present that start
> with something else. This is so features can be mandatory, and we can
> prevent older bzr clients from opening objects that they can corrupt if
> they don't have a particular feature.

I think this is the core issue.

As long as features are optional the tree/branch/repo can still
inter-operate with users that doesn't have the bzr/plugin implementing the
feature.

As soon as a feature is mandatory, we are in the same situation as a format
bump.

From there, I'm very very tempted to say that mandatory features are not
that interesting since they raise almost the same issues as a format bump.

What about starting with just optional features and explore the fallouts ?
What are the requirements for plugins ? How do they install such a feature,
how to they produce the data they need when discovering a new
tree/branch/repo ? Is such data automatically propogated, always re-created
on demand or do the plugins need to set hooks for fetch ?

If we can implement features that are really optional and don't require any
upgrade for interop... they are extremely compelling !

>
> Adding support for ignoring all lines starting with "optional" in older
> versions of bzr should be a fairly small change.

Yup, but the goal is still the same: ignore anything that is not mandatory
while still making sure we have a valid content.

>
> We should always take a write lock before updating this file, and
> atomically replacing it....

[Vincent Ladeuil (vila)]

> > I'm not sure what using an ini-like format really gains us: the file
> > still isn't a standard ini-style file, because it has a custom first
> > line.
>
> That's trivial to skip.

Hear, hear ! It's always trivial when somebody else does the work ;-D

So I gave it a try:

lp:~bzr/bzr/feature-flags which still pass the whole test suite with
--no-plugins (hung
bzrlib.tests.test_bzrdir.TestHTTPRedirections_pycurl.test_loop tracked down
to bzr-svn but I considered it irrelevant at this point).

Found that you used 'optional', 'required' and (more suprisingly)
'necessity'.

Overall, the experiment revealed that your design is sound and your tests
were quite cool to play around with a new implementation (and caught my noob
errors including forgetting to support a simple format file without any
support ;) !

>
> > It also makes parsing (and thus, backporting) much more complex.
>
> Quite the contrary, with the first line skipped we can use ConfigObj for
> that. We should also get line numbers in error messages for ~free.

But not that part (didn't try).

Running the whole test suite, it seems that
bzrlib.tests.test_workingtree.TestDefaultFormat.test_from_string needs a fix
even in your branch:

--- bzrlib/tests/test_workingtree.py 2011-12-11 03:49:52 +0000
+++ bzrlib/tests/test_workingtree.py 2011-12-16 12:34:48 +0000
@@ -146,10 +146,6 @@
         t.put_bytes('format', self.get_format_string())
         return 'A tree'

- @classmethod
- def from_string(cls, format_string):
- return cls()
-
     def is_supported(self):
         return False

Further feedback: setting a feature by appending a line to the format file
is brittle, better to add support at the BzrFormat level, but AIUI that's
out of scope for now. So only tests needs to do that which is fine so far.

[Jelmer Vernooij (jelmer)]

Am 16/12/11 11:55, schrieb Vincent Ladeuil:
>>> Bazaar repository format 2a (needs bzr 1.16 or later)
>>> [features]
>>> git = optional
>>> search = optional
>>> tiplog = optional
>>> nested_trees = required # _ not - if that matters...
>>>
>>> Even '[features]' is only needed if we plan to support something else than
>>> features or leave open this possibility.
>>>
>>> Note that it doesn't have to support all the fancy features of ConfigObj,
>>> especially quoting (not even comments...).
>> I'm not sure what using an ini-like format really gains us: the file
>> still isn't a standard ini-style file, because it has a custom first
>> line.
> That's trivial to skip.
Sure, but it also reduces the amount of usefulness of it being an
ini-like file.
>
>> It also makes parsing (and thus, backporting) much more complex.
> Quite the contrary, with the first line skipped we can use ConfigObj for
> that. We should also get line numbers in error messages for ~free.
The current parser is per-line, counting lines shouldn't be an issue. :-)
>
>> One of the advantages of having one feature per line is that we can
>> allow whitespace in the feature information in the future, or allow
>> features to add parameters.
> git = optional, version, pumpernickel
>
> works fine too.
Sure, but what's the advantage of using an ini-like file in that case?

I'd much prefer a simple to parse format over a more complex one. It
should also have less object overhead, though I'm not sure how relevant
that is. And it would make backporting support for ignoring "optional "
lines to older formats a lot easier.

The situation would be different if we wanted users to be able to edit
this file, but they really shouldn't.

>>> But more importantly, I think a needed feature of this format is that old
>>> bzr clients can safely ignore anything but the first line, but also get a
>>> way to check that the file itself has not been corrupted.
>>> This probably means some sort of final marker, a checksum or a robust
>>> parser. Note that the later means we won't change the format of this
>>> file. This is probably ok as we can still create new formats... but it's a
>>> bit weird to introduce a limitation when trying to make the format open to
>>> additions.
>> They should be able to ignore any lines starting with "optional ", but
>> should refuse to open formats when there are lines present that start
>> with something else. This is so features can be mandatory, and we can
>> prevent older bzr clients from opening objects that they can corrupt if
>> they don't have a particular feature.
> I think this is the core issue.
>
> As long as features are optional the tree/branch/repo can still
> inter-operate with users that doesn't have the bzr/plugin implementing the
> feature.
>
> As soon as a feature is mandatory, we are in the same situation as a format
> bump.
>
> >From there, I'm very very tempted to say that mandatory features are not
> that interesting since they raise almost the same issues as a format bump.
>
> What about starting with just optional features and explore the fallouts ?
> What are the requirements for plugins ? How do they install such a feature,
> how to they ...

[Jelmer Vernooij (jelmer)]

Am 16/12/11 13:43, schrieb Vincent Ladeuil:
>>> I'm not sure what using an ini-like format really gains us: the file
>>> still isn't a standard ini-style file, because it has a custom first
>>> line.
>> That's trivial to skip.
> Hear, hear ! It's always trivial when somebody else does the work ;-D
:-) Thanks again for thoroughly reviewing this. I appreciate it,
especially as this involves format changes that will affect us long from
now.
>
> So I gave it a try:
>
> lp:~bzr/bzr/feature-flags which still pass the whole test suite with
> --no-plugins (hung
> bzrlib.tests.test_bzrdir.TestHTTPRedirections_pycurl.test_loop tracked down
> to bzr-svn but I considered it irrelevant at this point).
Urgh - I should have a look at that.
>
> Found that you used 'optional', 'required' and (more suprisingly)
> 'necessity'.
Hmm, I guess using "necessity" is a bit confusing as that's the class
for things like "optional" and "required". Should I update
>
> Overall, the experiment revealed that your design is sound and your tests
> were quite cool to play around with a new implementation (and caught my noob
> errors including forgetting to support a simple format file without any
> support ;) !
>
>>> It also makes parsing (and thus, backporting) much more complex.
>> Quite the contrary, with the first line skipped we can use ConfigObj for
>> that. We should also get line numbers in error messages for ~free.
> But not that part (didn't try).
>
> Running the whole test suite, it seems that
> bzrlib.tests.test_workingtree.TestDefaultFormat.test_from_string needs a fix
> even in your branch:
>
> --- bzrlib/tests/test_workingtree.py 2011-12-11 03:49:52 +0000
> +++ bzrlib/tests/test_workingtree.py 2011-12-16 12:34:48 +0000
> @@ -146,10 +146,6 @@
> t.put_bytes('format', self.get_format_string())
> return 'A tree'
>
> - @classmethod
> - def from_string(cls, format_string):
> - return cls()
> -
> def is_supported(self):
> return False
Thanks!
>
> Further feedback: setting a feature by appending a line to the format file
> is brittle, better to add support at the BzrFormat level, but AIUI that's
> out of scope for now. So only tests needs to do that which is fine so far.
Yeah, I'd rather leave that for another branch since this one is already
big enough. I agree we need a dedicated API for it, and we shouldn't
encourage anybody to write to that file themselves.

Cheers,

Jelmer

[Vincent Ladeuil (vila)]

> > That's trivial to skip.

> Sure, but it also reduces the amount of usefulness of it being an ini-like
  file.

The point is not to make it an ini-like file but to address (end marker,
checksum or robust parser).

This is a robust parser.

It means that's the only thing to be backported to older supported bzr
releases to allow them to ignore the optional feature flags. They won't even
try to use the parsed content, they will ignore it, as long as it can be
parsed.

This means that corruptions will be detected.

> > that. We should also get line numbers in error messages for ~free.

> The current parser is per-line, counting lines shouldn't be an issue. :-)

I was talking about the new one, not yours ;)

> Sure, but what's the advantage of using an ini-like file in that case?

It means changes to the parser doesn't need to be backported and more
importantly deployed. We backport once, tell people to upgrade once and then
we can still change the data we need without changing the parser and more
importantly without requiring people to upgrade bzr more than once. And I
mean upgrading code not data, very important distinction.

> I'd much prefer a simple to parse format over a more complex one.

The point is that we can't be sure that the format is complete for now.

> It should also have less object overhead, though I'm not sure how relevant
> that is.

I fon't thnk it's relevant since older bzr will just ignore it.

> And it would make backporting support for ignoring "optional "
> lines to older formats a lot easier.

My point is that whatever data we need (present and future) can safely be
represented in a format for which the backport can be done now and once.

> The situation would be different if we wanted users to be able to edit
> this file, but they really shouldn't.

I din't even consider that, users should not edit format files, this is a
private bzr area only :)

> Of course, it would be nice if all features could be optional, but that
> isn't always the case - sometimes adding a feature renders a repository
> inaccessible to older versions of bzr.

That's the definition of a format bump. I think that allowing feature to
trigger a format bump is a Bad Idea.

> We should allow feature flags to represent that - by allowing features to
> be required. I want bzr 2.5 to be able to give a proper error if we add
> support for nested trees in the future, and start adding "required feature
> ..." lines.

What would that gain us ?

I fear that allowing required features will mean allowing (and therefore
supporting) *two* upgrade stories instead of focusing on a single one.

> The fact that plugins can add "required" features to a
> bzrdir/branch/repository/workingtree doesn't mean they actually have to do
> so. Where possible, they should make features optional.

I don't know yet how to articulate that clearly but my feeling is that it
would be better to allow required features only via format bumps (since
that's what they are really).

> We won't nag users into upgrading, and it would only affect users that
> actually use the features rather than everybody. If we can add support for
> ignoring "optional " lines to older release series o...

[Jelmer Vernooij (jelmer)]

Am 19/12/11 11:08, schrieb Vincent Ladeuil:
>>> That's trivial to skip.
>> Sure, but it also reduces the amount of usefulness of it being an ini-like
> file.
>
> The point is not to make it an ini-like file but to address (end marker,
> checksum or robust parser).
>
> This is a robust parser.
>
> It means that's the only thing to be backported to older supported bzr
> releases to allow them to ignore the optional feature flags. They won't even
> try to use the parsed content, they will ignore it, as long as it can be
> parsed.
>
> This means that corruptions will be detected.
I really don't see how this will protect against corruption - if we care
about that we should be using a checksum. As far as I can tell it only
protects against corruption that happens to render the file imparsable
to an ini-parser - there are lots of different kinds of corruption it
doesn't help against. The same is true for the current format.

>> Sure, but what's the advantage of using an ini-like file in that case?
> It means changes to the parser doesn't need to be backported and more
> importantly deployed. We backport once, tell people to upgrade once and then
> we can still change the data we need without changing the parser and more
> importantly without requiring people to upgrade bzr more than once. And I
> mean upgrading code not data, very important distinction.
It also means that we can't add anything else to the format file that
clients may not ignore.

The nice thing about the current format is that if we wanted to add
something else than feature flags, we could also allow optional and
required things for that.

Bzr branch format v1
optional feature search
required feature bar
optional index bar

will still work.

>
>> I'd much prefer a simple to parse format over a more complex one.
> The point is that we can't be sure that the format is complete for now.
Right, but that doesn't mean anything else in the file should optional.
It should be possible to add more to this file in the future and *still*
make it so the current version of bzr will refuse to open it. If we
parse the rest of the file as ini file then that means that we can't add
any mandatory stuff later.
>
>> It should also have less object overhead, though I'm not sure how relevant
>> that is.
> I fon't thnk it's relevant since older bzr will just ignore it.
They shouldn't simply ignore it - that would make it impossible to add
mandatory features.

>> Of course, it would be nice if all features could be optional, but that
>> isn't always the case - sometimes adding a feature renders a repository
>> inaccessible to older versions of bzr.
> That's the definition of a format bump. I think that allowing feature to
> trigger a format bump is a Bad Idea.
But that's exactly the main point of features. The goal is to allow
people to enable a particular feature without requiring a full upgrade,
being able to turn that feature off later or adding combinations of
features. It's a lot more granular than full format bumps.

Currently, we have full formats that combine a bunch of different
features. If we add support for nested trees and introduce a new format,
then repositories created with that new ...

[Vincent Ladeuil (vila)]

> Am 19/12/11 11:08, schrieb Vincent Ladeuil:
> >>> That's trivial to skip.
> >> Sure, but it also reduces the amount of usefulness of it being an ini-like
> > file.
> >
> > The point is not to make it an ini-like file but to address (end marker,
> > checksum or robust parser).
> >
> > This is a robust parser.
> >
> > It means that's the only thing to be backported to older supported bzr
> > releases to allow them to ignore the optional feature flags. They won't even
> > try to use the parsed content, they will ignore it, as long as it can be
> > parsed.
> >
> > This means that corruptions will be detected.

> I really don't see how this will protect against corruption - if we care
> about that we should be using a checksum. As far as I can tell it only
> protects against corruption that happens to render the file imparsable to
> an ini-parser - there are lots of different kinds of corruption it doesn't
> help against.

I'm not aiming perfection but robustness, the point is not to detect all
corruptions but to not break if the corruption occurs in ignored data.

> The same is true for the current format.

Only for the data you've already specified. Each addition requires
backporting to previous releases and then deployment.

>
>
> >> Sure, but what's the advantage of using an ini-like file in that case?
> > It means changes to the parser doesn't need to be backported and more
> > importantly deployed. We backport once, tell people to upgrade once and then
> > we can still change the data we need without changing the parser and more
> > importantly without requiring people to upgrade bzr more than once. And I
> > mean upgrading code not data, very important distinction.

> It also means that we can't add anything else to the format file that
> clients may not ignore.

But I don't want to add data that clients may not ignore, that's the
definition of a format bump !

>
> The nice thing about the current format is that if we wanted to add
> something else than feature flags, we could also allow optional and
> required things for that.

>
> Bzr branch format v1
> optional feature search
> required feature bar
> optional index bar
>
> will still work.

You can add that and far more with an ini-like format.

The disagreement here is not about the format it's about whether we want to
turn supported formats into unsupported ones.

> >
> >> I'd much prefer a simple to parse format over a more complex one.
> > The point is that we can't be sure that the format is complete for now.

> Right, but that doesn't mean anything else in the file should optional.
> It should be possible to add more to this file in the future and *still*
> make it so the current version of bzr will refuse to open it. If we
> parse the rest of the file as ini file then that means that we can't add
> any mandatory stuff later.

That's exactly what I want. It's an issue separate from the format itself,
I'm *against* adding data in the format file if it triggers a format bump.

> > I fear that allowing required features will mean allowing (and therefore
> > supporting) *two* upgrade stories instead of focusing on a single one.

> Features aren't separate of formats, they are...

[Vincent Ladeuil (vila)]

Thanks for your patience on IRC, I think the result was worth it.

review: Approve

[Vincent Ladeuil (vila)]

Thanks for your patience on IRC, I think the result was worth it.

Voting on the right proposal ;)

review: Approve

[Jelmer Vernooij (jelmer)]

[10:49] <jelmer> vila: let me know if you'd like to discus feature flags some more
[10:50] <jelmer> I've updated the docs
[10:51] <vila> I'll look into it, I was reviewing switch-possible-transports and there are still 2 test failures :-/
[10:51] <vila> comment posted
[10:52] <jelmer> vila: switch-possible-transports has two prerequisites, one of which isn't approved either.. so there are other branches I'm more interested in tbh :)
[10:53] <vila> yeah, well...
[11:01] <jelmer> vila: that's not to say I don't appreciate the review..
[11:05] <vila> so, if the feature name is global, _present_features should contain all features provided by the installed code base (ie. bzr and plugins) right ?
[11:06] <jelmer> yep
[11:06] <vila> hmm, I was about to ask for some help associated with the feature but that's useless for features that are not "installed"
[11:07] <vila> (thinking out aloud)
[11:08] <vila> lock_write raises for write acces, but what about read access ?
[11:09] <vila> if the format talks about feature, why does each line says feature ? Didn't we settle to put all features on the same line for each kind of necessity ? And consider all unknown necessities as required ?
[11:10] <jelmer> vila: All unknown necessities are considered required by the existing code
[11:10] <vila> (given that when a plugin provides a feature it's present even if the necessity s unknown)
[11:10] <jelmer> vila: I don't think we discussed the layout
[11:10] <vila> ha, misunderstanding then :)
[11:11] <jelmer> vila: I agree there are in concept two lists at the moment - the optional and the required features
[11:11] <vila> and optional additional lists for unknown necessities
[11:12] <vila> the core issues are still the same whatever format we chose :)
[11:12] <vila> I'm trying to agree with you on the simplest one that allows the most robust parser :)
[11:13] <vila> we're close to a simple scanner and scanners are more robust than parsers
[11:13] <jelmer> what's the difference?
[11:14] <vila> wow, many
[11:14] <vila> roughly a scanner doesn't need to take a context into account nor handle error recovery
[11:14] <vila> it sees a stream of text and split it into tokens
[11:15] <vila> a parser sees a stream of tokens and recognizes more complex constructs
[11:15] <jelmer> vila: right, I don't see how we can really avoid a parser?
[11:15] <jelmer> we'll need one in either case, even if it's very simple
[11:16] <vila> right, scanners are often called parsers even where there is no grammar to parse
[11:16] <jelmer> I think there is some really basic grammar here :)
[11:17] <vila> no, only tokens separated by spaces
[11:17] <vila> and newlines
[11:17] <jelmer> grammar exists over those tokens
[11:18] <vila> well, if you insist... but it's so trivial it doesn't need a parser
[11:18] <jelmer> we only allow "optional feature foo", not "foo bar bla"
[11:18] <vila> anyway, forget about the distinction
[11:18] <jelmer> sure
[11:18] <vila> I'd prefer: optional foo, bar
[11:18] <vila> required biz, bob
[11:19] <vila> 'feature' is just noise here
[11:19] <jelmer> vila: I've considered that as well, but it's less extendable in the future
[11:19] <jelmer> it doesn't allow adding ex...

[Jelmer Vernooij (jelmer)]

... for those reading the MP at home.

Thanks for your patience too Vincent. :)

[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-21 20:43:29 +0000
+++ bzrlib/branch.py	2011-12-22 17:33:44 +0000
@@ -53,6 +53,7 @@
 import bzrlib.bzrdir
 
 from bzrlib import (
+    bzrdir,
     controldir,
     )
 from bzrlib.decorators import (
@@ -1587,7 +1588,7 @@
 
     Formats provide three things:
      * An initialization routine,
-     * a format string,
+     * a format description
      * an open routine.
 
     Formats are placed in an dict by their format string for reference
@@ -1999,13 +2000,13 @@
             self.revision_id)
 
 
-class BranchFormatMetadir(bzrdir.BzrDirMetaComponentFormat, BranchFormat):
+class BranchFormatMetadir(bzrdir.BzrFormat, BranchFormat):
     """Base class for branch formats that live in meta directories.
     """
 
     def __init__(self):
         BranchFormat.__init__(self)
-        bzrdir.BzrDirMetaComponentFormat.__init__(self)
+        bzrdir.BzrFormat.__init__(self)
 
     @classmethod
     def find_format(klass, controldir, name=None):
@@ -2049,7 +2050,7 @@
         control_files.create_lock()
         control_files.lock_write()
         try:
-            utf8_files += [('format', self.get_format_string())]
+            utf8_files += [('format', self.as_string())]
             for (filename, content) in utf8_files:
                 branch_transport.put_bytes(
                     filename, content,
@@ -2097,6 +2098,14 @@
     def supports_leaving_lock(self):
         return True
 
+    def check_support_status(self, allow_unsupported, recommend_upgrade=True,
+            basedir=None):
+        BranchFormat.check_support_status(self,
+            allow_unsupported=allow_unsupported, recommend_upgrade=recommend_upgrade,
+            basedir=basedir)
+        bzrdir.BzrFormat.check_support_status(self, allow_unsupported=allow_unsupported,
+            recommend_upgrade=recommend_upgrade, basedir=basedir)
+
 
 class BzrBranchFormat5(BranchFormatMetadir):
     """Bzr branch format 5.
@@ -2304,7 +2313,7 @@
         branch_transport = a_bzrdir.get_branch_transport(self, name=name)
         branch_transport.put_bytes('location',
             target_branch.user_url)
-        branch_transport.put_bytes('format', self.get_format_string())
+        branch_transport.put_bytes('format', self.as_string())
         branch = self.open(
             a_bzrdir, name, _found=True,
             possible_transports=[target_branch.bzrdir.root_transport])
@@ -3218,7 +3227,7 @@
 
         # Copying done; now update target format
         new_branch._transport.put_bytes('format',
-            format.get_format_string(),
+            format.as_string(),
             mode=new_branch.bzrdir._get_file_mode())
 
         # Clean up old files
@@ -3237,7 +3246,7 @@
         format = BzrBranchFormat7()
         branch._set_config_location('stacked_on_location', '')
         # update target format
-        branch._transport.put_bytes('format', format.get_format_string())
+        branch._transport.put_bytes('format', format.as_string())
 
 
 class Converter7to8(object):
@@ -3247,7 +3256,7 @@
         format = BzrBranchFormat8()
         branch._transport.put_bytes('references', '')
         # update target format
-        branch._transport.put_bytes('format', format.get_format_string())
+        branch._transport.put_bytes('format', format.as_string())
 
 
 class InterBranch(InterObject):

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2011-12-19 13:23:58 +0000
+++ bzrlib/bzrdir.py	2011-12-22 17:33:44 +0000
@@ -1102,7 +1102,7 @@
         return self.transport.clone(path)
 
 
-class BzrDirMetaComponentFormat(controldir.ControlComponentFormat):
+class BzrFormat(object):
     """Base class for all formats of things living in metadirs.
 
     This class manages the format string that is stored in the 'format'
@@ -1113,36 +1113,105 @@
     (i.e. different from .bzr/branch-format) derive from this class,
     as well as the relevant base class for their kind
     (BranchFormat, WorkingTreeFormat, RepositoryFormat).
+
+    Each format is identified by a "format" or "branch-format" file with a
+    single line containing the base format name and then an optional list of
+    feature flags.
+
+    Feature flags are supported as of bzr 2.5. Setting feature flags on formats
+    will render them inaccessible to older versions of bzr.
+
+    :ivar features: Dictionary mapping feature names to their necessity
     """
 
+    _present_features = set()
+
+    def __init__(self):
+        self.features = {}
+
+    @classmethod
+    def register_feature(cls, name):
+        """Register a feature as being present.
+
+        :param name: Name of the feature
+        """
+        if " " in name:
+            raise ValueError("spaces are not allowed in feature names")
+        if name in cls._present_features:
+            raise errors.FeatureAlreadyRegistered(name)
+        cls._present_features.add(name)
+
+    @classmethod
+    def unregister_feature(cls, name):
+        """Unregister a feature."""
+        cls._present_features.remove(name)
+
+    def check_support_status(self, allow_unsupported, recommend_upgrade=True,
+            basedir=None):
+        for name, necessity in self.features.iteritems():
+            if name in self._present_features:
+                continue
+            if necessity == "optional":
+                mutter("ignoring optional missing feature %s", name)
+                continue
+            elif necessity == "required":
+                raise errors.MissingFeature(name)
+            else:
+                mutter("treating unknown necessity as require for %s",
+                       name)
+                raise errors.MissingFeature(name)
+
     @classmethod
     def get_format_string(cls):
         """Return the ASCII format string that identifies this format."""
         raise NotImplementedError(cls.get_format_string)
 
     @classmethod
-    def from_string(cls, format_string):
-        if format_string != cls.get_format_string():
-            raise ValueError("Invalid format header %r" % format_string)
-        return cls()
+    def from_string(cls, text):
+        format_string = cls.get_format_string()
+        if not text.startswith(format_string):
+            raise AssertionError("Invalid format header %r for %r" % (text, cls))
+        lines = text[len(format_string):].splitlines()
+        ret = cls()
+        for lineno, line in enumerate(lines):
+            try:
+                (necessity, feature) = line.split(" ", 1)
+            except ValueError:
+                raise errors.ParseFormatError(format=cls, lineno=lineno+2,
+                    line=line, text=text)
+            ret.features[feature] = necessity
+        return ret
+
+    def as_string(self):
+        """Return the string representation of this format.
+        """
+        lines = [self.get_format_string()]
+        lines.extend([("%s %s\n" % (item[1], item[0])) for item in
+            self.features.iteritems()])
+        return "".join(lines)
 
     @classmethod
     def _find_format(klass, registry, kind, format_string):
         try:
-            cls = registry.get(format_string)
+            first_line = format_string[:format_string.index("\n")+1]
+        except ValueError:
+            first_line = format_string
+        try:
+            cls = registry.get(first_line)
         except KeyError:
-            raise errors.UnknownFormatError(format=format_string, kind=kind)
-        return cls
+            raise errors.UnknownFormatError(format=first_line, kind=kind)
+        return cls.from_string(format_string)
 
     def network_name(self):
         """A simple byte string uniquely identifying this format for RPC calls.
 
         Metadir branch formats use their format string.
         """
-        return self.get_format_string()
+        return self.as_string()
 
     def __eq__(self, other):
-        return (self.__class__ is other.__class__)
+        return (self.__class__ is other.__class__ and
+                self.features == other.features)
 
 
 class BzrProber(controldir.Prober):
@@ -1169,9 +1238,13 @@
         except errors.NoSuchFile:
             raise errors.NotBranchError(path=transport.base)
         try:
-            cls = klass.formats.get(format_string)
+            first_line = format_string[:format_string.index("\n")+1]
+        except ValueError:
+            first_line = format_string
+        try:
+            cls = klass.formats.get(first_line)
         except KeyError:
-            raise errors.UnknownFormatError(format=format_string, kind='bzrdir')
+            raise errors.UnknownFormatError(format=first_line, kind='bzrdir')
         return cls.from_string(format_string)
 
     @classmethod
@@ -1221,7 +1294,7 @@
         return set([RemoteBzrDirFormat()])
 
 
-class BzrDirFormat(controldir.ControlDirFormat):
+class BzrDirFormat(BzrFormat, controldir.ControlDirFormat):
     """ControlDirFormat base class for .bzr/ directories.
 
     Formats are placed in a dict by their format string for reference
@@ -1238,11 +1311,6 @@
     # _lock_class must be set in subclasses to the lock type, typ.
     # TransportLock or LockDir
 
-    @classmethod
-    def get_format_string(cls):
-        """Return the ASCII format string that identifies this format."""
-        raise NotImplementedError(cls.get_format_string)
-
     def initialize_on_transport(self, transport):
         """Initialize a new bzrdir in the base directory of a Transport."""
         try:
@@ -1433,11 +1501,20 @@
             compatible with whatever sub formats are supported by self.
         :return: None.
         """
+        other_format.features = dict(self.features)
 
     def supports_transport(self, transport):
         # bzr formats can be opened over all known transports
         return True
 
+    def check_support_status(self, allow_unsupported, recommend_upgrade=True,
+            basedir=None):
+        controldir.ControlDirFormat.check_support_status(self,
+            allow_unsupported=allow_unsupported, recommend_upgrade=recommend_upgrade,
+            basedir=basedir)
+        BzrFormat.check_support_status(self, allow_unsupported=allow_unsupported,
+            recommend_upgrade=recommend_upgrade, basedir=basedir)
+
 
 class BzrDirMetaFormat1(BzrDirFormat):
     """Bzr meta control format 1
@@ -1459,6 +1536,7 @@
     colocated_branches = False
 
     def __init__(self):
+        BzrDirFormat.__init__(self)
         self._workingtree_format = None
         self._branch_format = None
         self._repository_format = None
@@ -1470,6 +1548,8 @@
             return False
         if other.workingtree_format != self.workingtree_format:
             return False
+        if other.features != self.features:
+            return False
         return True
 
     def __ne__(self, other):
@@ -1601,15 +1681,6 @@
         """See BzrDirFormat.get_format_description()."""
         return "Meta directory format 1"
 
-    @classmethod
-    def from_string(cls, format_string):
-        if format_string != cls.get_format_string():
-            raise ValueError("Invalid format string %r" % format_string)
-        return cls()
-
-    def network_name(self):
-        return self.get_format_string()
-
     def _open(self, transport):
         """See BzrDirFormat._open."""
         # Create a new format instance because otherwise initialisation of new
@@ -1644,6 +1715,7 @@
             compatible with whatever sub formats are supported by self.
         :return: None.
         """
+        super(BzrDirMetaFormat1, self)._supply_sub_formats_to(other_format)
         if getattr(self, '_repository_format', None) is not None:
             other_format.repository_format = self.repository_format
         if self._branch_format is not None:
@@ -1919,7 +1991,7 @@
         :return: A repository, is_new_flag (True if the repository was
             created).
         """
-        raise NotImplemented(RepositoryAcquisitionPolicy.acquire_repository)
+        raise NotImplementedError(RepositoryAcquisitionPolicy.acquire_repository)
 
 
 class CreateRepository(RepositoryAcquisitionPolicy):

=== modified file 'bzrlib/controldir.py'
--- bzrlib/controldir.py	2011-12-19 13:23:58 +0000
+++ bzrlib/controldir.py	2011-12-22 17:33:44 +0000
@@ -842,7 +842,7 @@
 
 
 class ControlComponentFormat(object):
-    """A component that can live inside of a .bzr meta directory."""
+    """A component that can live inside of a control directory."""
 
     upgrade_recommended = False
 

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2011-12-19 13:23:58 +0000
+++ bzrlib/errors.py	2011-12-22 17:33:44 +0000
@@ -763,6 +763,18 @@
         self.bzrdir = bzrdir_format
 
 
+class ParseFormatError(BzrError):
+
+    _fmt = "Parse error on line %(lineno)d of %(format)s format: %(line)s"
+
+    def __init__(self, format, lineno, line, text):
+        BzrError.__init__(self)
+        self.format = format
+        self.lineno = lineno
+        self.line = line
+        self.text = text
+
+
 class IncompatibleRepositories(BzrError):
     """Report an error that two repositories are not compatible.
 
@@ -3242,6 +3254,15 @@
         self.format = format
 
 
+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 PatchSyntax(BzrError):
     """Base class for patch syntax errors."""
 
@@ -3291,3 +3312,11 @@
         self.line_no = line_no
         self.orig_line = orig_line.rstrip('\n')
         self.patch_line = patch_line.rstrip('\n')
+
+
+class FeatureAlreadyRegistered(BzrError):
+
+    _fmt = 'The feature %(feature)s has already been registered.'
+
+    def __init__(self, feature):
+        self.feature = feature

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2011-12-19 11:49:56 +0000
+++ bzrlib/repository.py	2011-12-22 17:33:44 +0000
@@ -1497,7 +1497,7 @@
             hook(params)
 
 
-class RepositoryFormatMetaDir(bzrdir.BzrDirMetaComponentFormat, RepositoryFormat):
+class RepositoryFormatMetaDir(bzrdir.BzrFormat, RepositoryFormat):
     """Common base class for the new repositories using the metadir layout."""
 
     rich_root_data = False
@@ -1514,7 +1514,7 @@
 
     def __init__(self):
         RepositoryFormat.__init__(self)
-        bzrdir.BzrDirMetaComponentFormat.__init__(self)
+        bzrdir.BzrFormat.__init__(self)
 
     def _create_control_files(self, a_bzrdir):
         """Create the required files and the initial control_files object."""
@@ -1559,6 +1559,14 @@
             raise errors.NoRepositoryPresent(a_bzrdir)
         return klass._find_format(format_registry, 'repository', format_string)
 
+    def check_support_status(self, allow_unsupported, recommend_upgrade=True,
+            basedir=None):
+        RepositoryFormat.check_support_status(self,
+            allow_unsupported=allow_unsupported, recommend_upgrade=recommend_upgrade,
+            basedir=basedir)
+        bzrdir.BzrFormat.check_support_status(self, allow_unsupported=allow_unsupported,
+            recommend_upgrade=recommend_upgrade, basedir=basedir)
+
 
 # formats which have no format string are not discoverable or independently
 # creatable on disk, so are not registered in format_registry.  They're

=== modified file 'bzrlib/tests/test_branch.py'
--- bzrlib/tests/test_branch.py	2011-12-20 14:04:21 +0000
+++ bzrlib/tests/test_branch.py	2011-12-22 17:33:44 +0000
@@ -201,7 +201,7 @@
         self.assertIsInstance(
             SampleBranchFormat.from_string("Sample branch format."),
             SampleBranchFormat)
-        self.assertRaises(ValueError,
+        self.assertRaises(AssertionError,
             SampleBranchFormat.from_string, "Different branch format.")
 
     def test_find_format_not_branch(self):
@@ -217,6 +217,15 @@
                           _mod_branch.BranchFormatMetadir.find_format,
                           dir)
 
+    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")
+        found_format = _mod_branch.BranchFormatMetadir.find_format(tree.bzrdir)
+        self.assertIsInstance(found_format, _mod_branch.BranchFormatMetadir)
+        self.assertEquals(found_format.features.get("name"), "optional")
+
     def test_register_unregister_format(self):
         # Test the deprecated format registration functions
         format = SampleBranchFormat()
@@ -718,4 +727,3 @@
         f = StringIO()
         r.report(f)
         self.assertEqual("No revisions or tags to pull.\n", f.getvalue())
-

=== modified file 'bzrlib/tests/test_bzrdir.py'
--- bzrlib/tests/test_bzrdir.py	2011-12-07 14:03:01 +0000
+++ bzrlib/tests/test_bzrdir.py	2011-12-22 17:33:44 +0000
@@ -1025,6 +1025,17 @@
         self.assertNotEqual(otherdir2, mydir)
         self.assertFalse(otherdir2 == mydir)
 
+    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")
+        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"))
+
     def test_needs_conversion_different_working_tree(self):
         # meta1dirs need an conversion if any element is not the default.
         new_format = bzrdir.format_registry.make_bzrdir('dirstate')
@@ -1435,3 +1446,112 @@
         self.assertRaises(errors.BzrError, converter.convert, tree.bzrdir,
             None)
 
+
+class SampleBzrFormat(bzrdir.BzrFormat):
+
+    @classmethod
+    def get_format_string(cls):
+        return "First line\n"
+
+
+class TestBzrFormat(TestCase):
+    """Tests for BzrFormat."""
+
+    def test_as_string(self):
+        format = SampleBzrFormat()
+        format.features = {"foo": "required"}
+        self.assertEquals(format.as_string(),
+            "First line\n"
+            "required foo\n")
+        format.features["another"] = "optional"
+        self.assertEquals(format.as_string(),
+            "First line\n"
+            "required foo\n"
+            "optional another\n")
+
+    def test_network_name(self):
+        # The network string should include the feature info
+        format = SampleBzrFormat()
+        format.features = {"foo": "required"}
+        self.assertEquals(
+            "First line\nrequired foo\n",
+            format.network_name())
+
+    def test_from_string_no_features(self):
+        # No features
+        format = SampleBzrFormat.from_string(
+            "First line\n")
+        self.assertEquals({}, format.features)
+
+    def test_from_string_with_feature(self):
+        # Proper feature
+        format = SampleBzrFormat.from_string(
+            "First line\nrequired foo\n")
+        self.assertEquals("required", format.features.get("foo"))
+
+    def test_from_string_format_string_mismatch(self):
+        # The first line has to match the format string
+        self.assertRaises(AssertionError, SampleBzrFormat.from_string,
+            "Second line\nrequired foo\n")
+
+    def test_from_string_missing_space(self):
+        # At least one space is required in the feature lines
+        self.assertRaises(errors.ParseFormatError, SampleBzrFormat.from_string,
+            "First line\nfoo\n")
+
+    def test_from_string_with_spaces(self):
+        # Feature with spaces (in case we add stuff like this in the future)
+        format = SampleBzrFormat.from_string(
+            "First line\nrequired foo with spaces\n")
+        self.assertEquals("required", format.features.get("foo with spaces"))
+
+    def test_eq(self):
+        format1 = SampleBzrFormat()
+        format1.features = {"nested-trees": "optional"}
+        format2 = SampleBzrFormat()
+        format2.features = {"nested-trees": "optional"}
+        self.assertEquals(format1, format1)
+        self.assertEquals(format1, format2)
+        format3 = SampleBzrFormat()
+        self.assertNotEquals(format1, format3)
+
+    def test_check_support_status_optional(self):
+        # Optional, so silently ignore
+        format = SampleBzrFormat()
+        format.features = {"nested-trees": "optional"}
+        format.check_support_status(True)
+        self.addCleanup(SampleBzrFormat.unregister_feature, "nested-trees")
+        SampleBzrFormat.register_feature("nested-trees")
+        format.check_support_status(True)
+
+    def test_check_support_status_required(self):
+        # Optional, so trigger an exception
+        format = SampleBzrFormat()
+        format.features = {"nested-trees": "required"}
+        self.assertRaises(errors.MissingFeature, format.check_support_status,
+            True)
+        self.addCleanup(SampleBzrFormat.unregister_feature, "nested-trees")
+        SampleBzrFormat.register_feature("nested-trees")
+        format.check_support_status(True)
+
+    def test_check_support_status_unknown(self):
+        # treat unknown necessity as required
+        format = SampleBzrFormat()
+        format.features = {"nested-trees": "unknown"}
+        self.assertRaises(errors.MissingFeature, format.check_support_status,
+            True)
+        self.addCleanup(SampleBzrFormat.unregister_feature, "nested-trees")
+        SampleBzrFormat.register_feature("nested-trees")
+        format.check_support_status(True)
+
+    def test_feature_already_registered(self):
+        # a feature can only be registered once
+        self.addCleanup(SampleBzrFormat.unregister_feature, "nested-trees")
+        SampleBzrFormat.register_feature("nested-trees")
+        self.assertRaises(errors.FeatureAlreadyRegistered,
+            SampleBzrFormat.register_feature, "nested-trees")
+
+    def test_feature_with_space(self):
+        # spaces are not allowed in feature names
+        self.assertRaises(ValueError, SampleBzrFormat.register_feature,
+            "nested trees")

=== modified file 'bzrlib/tests/test_repository.py'
--- bzrlib/tests/test_repository.py	2011-12-14 14:09:47 +0000
+++ bzrlib/tests/test_repository.py	2011-12-22 17:33:44 +0000
@@ -153,7 +153,7 @@
             SampleRepositoryFormat.from_string(
                 "Sample .bzr repository format."),
             SampleRepositoryFormat)
-        self.assertRaises(ValueError,
+        self.assertRaises(AssertionError,
             SampleRepositoryFormat.from_string,
                 "Different .bzr repository format.")
 
@@ -164,6 +164,21 @@
                           repository.RepositoryFormatMetaDir.find_format,
                           dir)
 
+    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")
+        found_format = repository.RepositoryFormatMetaDir.find_format(tree.bzrdir)
+        self.assertIsInstance(found_format, repository.RepositoryFormatMetaDir)
+        self.assertEquals(found_format.features.get("name"), "necessity")
+        self.assertRaises(errors.MissingFeature, found_format.check_support_status,
+            True)
+        self.addCleanup(repository.RepositoryFormatMetaDir.unregister_feature,
+            "name")
+        repository.RepositoryFormatMetaDir.register_feature("name")
+        found_format.check_support_status(True)
+
     def test_register_unregister_format(self):
         # Test deprecated format registration functions
         format = SampleRepositoryFormat()
@@ -1703,3 +1718,25 @@
         lazy = repository._LazyListJoin(['a'], ['b'])
         self.assertEqual("bzrlib.repository._LazyListJoin((['a'], ['b']))",
                          repr(lazy))
+
+
+class TestFeatures(tests.TestCaseWithTransport):
+
+    def test_open_with_present_feature(self):
+        self.addCleanup(
+            repository.RepositoryFormatMetaDir.unregister_feature,
+            "makes-cheese-sandwich")
+        repository.RepositoryFormatMetaDir.register_feature(
+            "makes-cheese-sandwich")
+        repo = self.make_repository('.')
+        repo.lock_write()
+        repo._format.features["makes-cheese-sandwich"] = "required"
+        repo._format.check_support_status(False)
+        repo.unlock()
+
+    def test_open_with_missing_required_feature(self):
+        repo = self.make_repository('.')
+        repo.lock_write()
+        repo._format.features["makes-cheese-sandwich"] = "required"
+        self.assertRaises(errors.MissingFeature,
+            repo._format.check_support_status, False)

=== modified file 'bzrlib/tests/test_workingtree.py'
--- bzrlib/tests/test_workingtree.py	2011-12-11 02:43:03 +0000
+++ bzrlib/tests/test_workingtree.py	2011-12-22 17:33:44 +0000
@@ -83,7 +83,7 @@
         self.assertIsInstance(
             SampleTreeFormat.from_string("Sample tree format."),
             SampleTreeFormat)
-        self.assertRaises(ValueError,
+        self.assertRaises(AssertionError,
             SampleTreeFormat.from_string, "Different format string.")
 
     def test_get_set_default_format_by_key(self):
@@ -243,6 +243,21 @@
             self.applyDeprecated(symbol_versioning.deprecated_in((2, 4, 0)),
                 workingtree.WorkingTreeFormat.get_formats))
 
+    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")
+        found_format = workingtree.WorkingTreeFormatMetaDir.find_format(
+            tree.bzrdir)
+        self.assertIsInstance(found_format, workingtree.WorkingTreeFormat)
+        self.assertEquals(found_format.features.get("name"), "necessity")
+        self.assertRaises(errors.MissingFeature, found_format.check_support_status,
+            True)
+        self.addCleanup(workingtree.WorkingTreeFormatMetaDir.unregister_feature,
+            "name")
+        workingtree.WorkingTreeFormatMetaDir.register_feature("name")
+        found_format.check_support_status(True)
+
 
 class TestWorkingTreeIterEntriesByDir_wSubtrees(TestCaseWithTransport):
 

=== modified file 'bzrlib/workingtree.py'
--- bzrlib/workingtree.py	2011-12-19 17:39:35 +0000
+++ bzrlib/workingtree.py	2011-12-22 17:33:44 +0000
@@ -47,7 +47,6 @@
 
 from bzrlib import (
     branch,
-    bzrdir,
     conflicts as _mod_conflicts,
     controldir,
     errors,
@@ -72,9 +71,11 @@
 
 # Explicitly import bzrlib.bzrdir so that the BzrProber
 # is guaranteed to be registered.
-import bzrlib.bzrdir
+from bzrlib import (
+    bzrdir,
+    symbol_versioning,
+    )
 
-from bzrlib import symbol_versioning
 from bzrlib.decorators import needs_read_lock, needs_write_lock
 from bzrlib.i18n import gettext
 from bzrlib.lock import LogicalLockResult
@@ -3133,12 +3134,12 @@
         return self._matchingbzrdir
 
 
-class WorkingTreeFormatMetaDir(bzrdir.BzrDirMetaComponentFormat, WorkingTreeFormat):
+class WorkingTreeFormatMetaDir(bzrdir.BzrFormat, WorkingTreeFormat):
     """Base class for working trees that live in bzr meta directories."""
 
     def __init__(self):
         WorkingTreeFormat.__init__(self)
-        bzrdir.BzrDirMetaComponentFormat.__init__(self)
+        bzrdir.BzrFormat.__init__(self)
 
     @classmethod
     def find_format_string(klass, controldir):
@@ -3156,6 +3157,14 @@
         return klass._find_format(format_registry, 'working tree',
                 format_string)
 
+    def check_support_status(self, allow_unsupported, recommend_upgrade=True,
+            basedir=None):
+        WorkingTreeFormat.check_support_status(self,
+            allow_unsupported=allow_unsupported, recommend_upgrade=recommend_upgrade,
+            basedir=basedir)
+        bzrdir.BzrFormat.check_support_status(self, allow_unsupported=allow_unsupported,
+            recommend_upgrade=recommend_upgrade, basedir=basedir)
+
 
 format_registry.register_lazy("Bazaar Working Tree Format 4 (bzr 0.15)\n",
     "bzrlib.workingtree_4", "WorkingTreeFormat4")

=== modified file 'bzrlib/workingtree_3.py'
--- bzrlib/workingtree_3.py	2011-12-19 13:23:58 +0000
+++ bzrlib/workingtree_3.py	2011-12-22 17:33:44 +0000
@@ -195,7 +195,7 @@
         control_files = self._open_control_files(a_bzrdir)
         control_files.create_lock()
         control_files.lock_write()
-        transport.put_bytes('format', self.get_format_string(),
+        transport.put_bytes('format', self.as_string(),
             mode=a_bzrdir._get_file_mode())
         if from_branch is not None:
             branch = from_branch

=== modified file 'bzrlib/workingtree_4.py'
--- bzrlib/workingtree_4.py	2011-12-19 17:39:35 +0000
+++ bzrlib/workingtree_4.py	2011-12-22 17:33:44 +0000
@@ -1478,7 +1478,7 @@
         control_files = self._open_control_files(a_bzrdir)
         control_files.create_lock()
         control_files.lock_write()
-        transport.put_bytes('format', self.get_format_string(),
+        transport.put_bytes('format', self.as_string(),
             mode=a_bzrdir._get_file_mode())
         if from_branch is not None:
             branch = from_branch
@@ -2260,7 +2260,7 @@
     def update_format(self, tree):
         """Change the format marker."""
         tree._transport.put_bytes('format',
-            self.target_format.get_format_string(),
+            self.target_format.as_string(),
             mode=tree.bzrdir._get_file_mode())
 
 
@@ -2283,7 +2283,7 @@
     def update_format(self, tree):
         """Change the format marker."""
         tree._transport.put_bytes('format',
-            self.target_format.get_format_string(),
+            self.target_format.as_string(),
             mode=tree.bzrdir._get_file_mode())
 
 
@@ -2312,5 +2312,5 @@
     def update_format(self, tree):
         """Change the format marker."""
         tree._transport.put_bytes('format',
-            self.target_format.get_format_string(),
+            self.target_format.as_string(),
             mode=tree.bzrdir._get_file_mode())

=== modified file 'doc/developers/feature-flags.txt'
--- doc/developers/feature-flags.txt	2011-10-24 19:21:00 +0000
+++ doc/developers/feature-flags.txt	2011-12-22 17:33:44 +0000
@@ -51,8 +51,8 @@
 Feature necessity
 -----------------
 
-The initial implementation will feature the following set of possible
-settings for feature "necessity". Any format necessity that can't
+The initial implementation will feature the following two possible
+settings for feature ``necessity``. Any format necessity that can't
 be understood should be interpreted as "required", and an appropriate
 warning printed.
 
@@ -62,8 +62,16 @@
       annotate cache)
  - required: read and write access is only possible if the feature
       is supported. Useful for things like nested trees.
- - write-required: read access is possible if the feature is not supported,
+
+In the future, we might add more values for necessity. Older
+versions of bzr treat unknown necessities as "required". Some likely
+candidates for new necessities that might be added in the future:
+
+ - read-optional: read access is possible if the feature is not supported,
       but write access requires it
+ - client-read-optional: directly writing to the object requires
+      the feature, but reading or writing through an intermediary (such as
+      a HPSS server) doesn't.
 
 Format changes
 --------------
@@ -94,23 +102,32 @@
 API Changes
 -----------
 
-Class methods will be added to ``BzrDirComponentFormat`` to allow registering
-and deregistering the presence of particular features. This class is inherited
-by ``BzrBranchFormat``, ``VersionedFileRepositoryFormat`` and
-``InventoryWorkingTreeFormat``.
-
- * BzrDirComponentFormat.register_feature(name)
- * BzrDirComponentFormat.unregister_feature(name)
-
-Upon opening, BzrDirComponentFormat will be responsible for checking that the
+Class methods will be added to ``BzrFormat`` to allow registering
+and unregistering the presence of particular features.
+
+ * BzrFormat.register_feature(name)
+ * BzrFormat.unregister_feature(name)
+
+The namespace for features is global. It is assumed
+that the plugin that provides the feature X provides that feature
+in all objects that it is relevant for. For example, if a plugin
+provides the ``nested-trees`` feature, it is assumed to support
+that in both working trees and repositories. If this is not the case,
+it should use a different feature name for the working tree support
+and the repository support.
+
+BzrFormat is inherited by ``BranchFormatMetaDir``, ``BzrDirFormat``,
+``RepositoryFormatMetaDir`` and ``WorkingTreeFormatMetaDir``.
+
+Upon opening, BzrFormat will be responsible for checking that the
 required features are present.  lock_write will raise an exception
-when there is an un unsupported mandatory feature required for write access.
+when there is an unsupported mandatory feature required for write access.
 
-Methods will also be added to BzrDirComponentFormat to allow plugins, etc,
+Methods will also be added to BzrFormat to allow plugins, etc,
 to check whether a feature is present and adding new features:
 
- * BzrDirComponentFormat.set_feature(name, necessity)
- * BzrDirComponentFormat.get_feature(name) -> necessity
+ * BzrFormat.features.set(name, necessity)
+ * BzrFormat.features.get(name) -> necessity
 
 See also
 --------

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-12-21 21:31:03 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-12-22 17:33:44 +0000
@@ -365,6 +365,12 @@
 * The registry of merge types has been moved to ``merge`` from ``option`` but
   ``merge.get_merge_type_registry`` remains as an accessor. (Martin Packman)
 
+* All bzr control directories, branch formats, repository formats and
+  working tree formats now support feature flags, which are
+  serialized in their respective format files. See
+  ``doc/developers/feature-flags.txt`` for details.
+  (Jelmer Vernooij)
+
 Testing
 *******