I'm sure I started replying shortly after your reply and I sadly discover my
answer is nowhere to be seen :-(

> On Thu, Jan 26, 2012 at 09:27:24AM -0000, Vincent Ladeuil wrote:
> > Review: Needs Information
> > 280   + return "Working tree format 6"
> >
> > Really ?
> 
> I'm not sure what's so controversial about this?

*6* instead of 7 ? This line is part of WorkingTreeFormat7

> 
> > 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 ?
> Yes.

Ok, with that in mind, I better understand the cover letter ('change' there
wasn't related to the proposal itself which confused me).

> 
> > 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 ?
> Basically, yeah.

;_; so be it.

> 
> > 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).
> I think we should support upgrading from 2a-subtree to 2a and then get
> rid of 2a.

 I assume you meant 'get rid of 2a-subtree'. Ok.

> 
> > > Instead, this marks working tree formats 4-6 as not actually supporting
> >   nested trees.
> >
> > So *this* part is uncontroversial and can be landed IMHO.
> I disagree, doing just that without adding a new working tree format
> means we no longer test the current subtree support. So I think as
> part of disabling nested tree support in wt 4-6 we should also add wt
> 7. Or do you perhaps mean that you would like to see the working tree
> changes split out?

Yup.

> 
> >
> > 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
> I'll have a look.

Cool.

> 
> >
> > 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 :-/
> I agree. That's unfortunately something we've had for ages. I think poolie
> preferred methods so we've slowly been transitioning to that.
> 
> WorkingTreeFormat seems to prefer methods, and RepositoryFormat seems
> to prefer attributes.

Ok.

> 
> > > 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 ?
> No, bzr-git only cares about the repository. We don't really care
> about being able to create working trees at this point. This is the
> first step - having a stable repository format that supports nested
> trees. Having a working tree format that (properly) supports nested trees
> would
> be next after that (and is more complex).

One more reason to split then.

Hmm, at that point I kind of feel that I mostly agree with changes (with the
tweaks mentioned above: s/6/7/ and tests clarified) so I let you decide if
you agree with splitting or not.

I'll make sure to review once you reply in any case.

> 
> There are a lot of git repositories out there that have submodules
> *somewhere* in their history, but no longer at the tip. This change
> allows bzr-git to import those repositories, and allows them to be
> used in daily builds.

Ha, that's clearer. There is still the issue of what will happen if they use
submodules again but let's get rid of the cases we can support first.

> 
> > 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 ;)

This point still stands ;)

Again, sorry for the delay :-(