Merge into bzr.dev : 2a-repo-supports-nested-trees : Code : Bazaar

Status:

Rejected

Rejected by:

Jelmer Vernooij on 2012-04-10

Proposed branch:

lp://staging/~jelmer/bzr/2a-repo-supports-nested-trees

Merge into:

lp://staging/bzr

Diff against target:

322 lines (+91/-20)

10 files modified

bzrlib/bzrdir.py (+3/-3)
bzrlib/commit.py (+0/-1)
bzrlib/repofmt/groupcompress_repo.py (+1/-1)
bzrlib/tests/per_pack_repository.py (+11/-2)
bzrlib/tests/per_tree/test_path_content_summary.py (+1/-1)
bzrlib/tests/test_merge.py (+2/-2)
bzrlib/tests/test_smart.py (+1/-1)
bzrlib/workingtree.py (+12/-1)
bzrlib/workingtree_4.py (+55/-8)
doc/en/release-notes/bzr-2.6.txt (+5/-0)

To merge this branch:

bzr merge lp://staging/~jelmer/bzr/2a-repo-supports-nested-trees

High

Triaged

Link a bug report

Reviewer	Review Type	Date Requested	Status
Vincent Ladeuil		2012-01-24	Needs Information on 2012-01-26
Review via email: mp+89919@code.staging.launchpad.net

Description of the change

Enable support for nested trees in the '2a' repository format.

Support for storing 'tree-reference' inventory entries has always
been present in the '2a' repository format, just never been officially
supported by the format. The actual change (a new inventory entry kind
'tree-reference') is small and non-controversial.

Instead, this marks working tree formats 4-6 as not actually supporting
nested trees. Previously it would claim to support nested trees
depending on whether the associated repository supported nested trees.
In practice the current wt implementation doesn't properly support nested trees:

* There is no cheap way to iterate over all nested trees, so every
   working tree that supports nested trees (even if there are none) pays
   the penalty of scanning the entire working tree for nested trees
   every time it is accessed.
* There are various bugs around the current implementation that
   make it impossible to use it with nested trees. Just checking out
   a containing tree shows the kind of the nested trees of having changed
   from nested tree to regular directory

This adds an experimental 'Working Tree Format 7' which does claim to support
nested trees and is used by development-subtree. We can extend this to
do whatever is necessary to properly handle tree references and then
potentially merge it back into the other working tree formats with a
feature flag.

This change is sufficient for bzr-git to allow importing
git repositories with submodules somewhere in their history (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.

Revision history for this message

Vincent Ladeuil (vila) wrote on 2012-01-26:

#

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.

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.

review: Needs Information

Revision history for this message

Jelmer Vernooij (jelmer) wrote on 2012-01-26:

#

Download full text (4.2 KiB)

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

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

Revision history for this message

Vincent Ladeuil (vila) wrote on 2012-02-15:

#

Download full text (5.2 KiB)

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

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 :-(

Bazaar

Merge lp://staging/~jelmer/bzr/2a-repo-supports-nested-trees into lp://staging/bzr

Commit message

Description of the change

Preview Diff

Subscribers