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

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?

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

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

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

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

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

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

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

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.

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

Cheers,

Jelmer