Code review comment for lp:~jelmer/bzr/2a-repo-supports-nested-trees

280 + return "Working tree format 6"

Really ?

Apart from that I don't quite see how your cover letter relates to this
proposal:

> The actual change (a new inventory entry kind 'tree-reference') is small
  and non-controversial.

There is nothing about that in your proposal, are you just referring to the
initial introduction of the tree-reference entry kind bak in the days ?

Ot is the actual change really:

51 + supports_tree_reference = True

and

59 - supports_tree_reference = True

If that's the case, I'm surprised there is no fallouts from activating tree
references support in 2a (line 51) !?! Not a single test failing ?

Also, what's the plan for RepositoryFormat2aSubtree ? Keeping it this way is
likely to get it forgotten, we at least need a bug to track its removal
describing some plan (since it's an experimental one, I think the plan can
be reduced to: delete it, users knew).

> Instead, this marks working tree formats 4-6 as not actually supporting
  nested trees.

So *this* part is uncontroversial and can be landed IMHO.

69 + if repo._format.supports_chks:
70 + matching_format_name = '2a'
71 + else:
72 + matching_format_name = 'pack-0.92-subtree'

81 + if repo.supports_rich_root():
82 + if repo._format.supports_chks:
83 + matching_format_name = '2a'
84 + else:
85 + matching_format_name = 'pack-0.92-subtree'
86 + else:
87 + matching_format_name = 'dirstate-subtree'

Urgh, these tests were already confusing, now they are almost undecipherable
:-(

Can't you abstract the logic here so we get a clearer view of what we are
tested ? Since these tests are about stacking, I'd really prefer them to be
easier to grasp, especially regarding the compatibility between the various
formats. There is also a hint that it's time to address:

        # TODO: Possibly this should be run per-repository-format and raise
        # TestNotApplicable on formats that don't support stacking. -- mbp
        # 20080729

100 + if not tree.supports_tree_reference():

Why is that a method for working trees (or is it wt formats) and an
attribute for repos ?

Highly confusing :-/

> This change is sufficient for bzr-git to allow importing (launchpad bug
  #402814), although not yet sufficient to allow checking out trees with
  submodules. We'd need working tree support for the latter, of course.

Meh, can you elaborate on that ? How does bzr-git uses this change ? By
using wt format 7 ? And how do you allow checking out such trees ? Or is
this proposal incomplete in this respect ?

We add a discussion in Budapest where you mentioned a list of limitations
(which I agreed with) to allow experimenting with subtrees. It would be nice
to formalize them so we a get some understanding on where this proposal
stands on the roadmap ;)

Failing that, I don't know how to review this proposal or if it needs to
be split into several parts that would be easier to understand.