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

[Jelmer Vernooij]

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.

[Vincent Ladeuil (vila)]

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

[Jelmer Vernooij (jelmer)]

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

[Vincent Ladeuil (vila)]

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

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2012-01-18 20:27:41 +0000
+++ bzrlib/bzrdir.py	2012-01-24 15:41:25 +0000
@@ -2193,7 +2193,7 @@
     help='Variant of dirstate with support for nested trees. '
          'Introduced in 0.15.',
     branch_format='bzrlib.branch.BzrBranchFormat6',
-    tree_format='bzrlib.workingtree_4.WorkingTreeFormat4',
+    tree_format='bzrlib.workingtree_4.WorkingTreeFormat7',
     experimental=True,
     hidden=True,
     )
@@ -2293,7 +2293,7 @@
         'http://doc.bazaar.canonical.com/latest/developers/development-repo.html '
         'before use.',
     branch_format='bzrlib.branch.BzrBranchFormat7',
-    tree_format='bzrlib.workingtree_4.WorkingTreeFormat6',
+    tree_format='bzrlib.workingtree_4.WorkingTreeFormat7',
     experimental=True,
     hidden=True,
     alias=False, # Restore to being an alias when an actual development subtree format is added
@@ -2309,7 +2309,7 @@
         'http://doc.bazaar.canonical.com/latest/developers/development-repo.html '
         'before use.',
     branch_format='bzrlib.branch.BzrBranchFormat7',
-    tree_format='bzrlib.workingtree_4.WorkingTreeFormat6',
+    tree_format='bzrlib.workingtree_4.WorkingTreeFormat7',
     experimental=True,
     hidden=True,
     alias=False,

=== modified file 'bzrlib/commit.py'
--- bzrlib/commit.py	2011-12-18 12:46:49 +0000
+++ bzrlib/commit.py	2012-01-24 15:41:25 +0000
@@ -347,7 +347,6 @@
         # generation. See bug 347649.
         self.use_record_iter_changes = (
             not self.exclude and 
-            not self.branch.repository._format.supports_tree_reference and
             (self.branch.repository._format.fast_deltas or
              len(self.parents) < 2))
         self.pb = ui.ui_factory.nested_progress_bar()

=== modified file 'bzrlib/repofmt/groupcompress_repo.py'
--- bzrlib/repofmt/groupcompress_repo.py	2011-12-19 19:15:58 +0000
+++ bzrlib/repofmt/groupcompress_repo.py	2012-01-24 15:41:25 +0000
@@ -1362,6 +1362,7 @@
     """A CHK repository that uses the bencode revision serializer."""
 
     repository_class = CHKInventoryRepository
+    supports_tree_reference = True
     supports_external_lookups = True
     supports_chks = True
     _commit_builder_class = PackRootCommitBuilder
@@ -1423,4 +1424,3 @@
                 "group compression and chk inventories")
 
     experimental = True
-    supports_tree_reference = True

=== modified file 'bzrlib/tests/per_pack_repository.py'
--- bzrlib/tests/per_pack_repository.py	2011-08-19 22:34:02 +0000
+++ bzrlib/tests/per_pack_repository.py	2012-01-24 15:41:25 +0000
@@ -884,7 +884,10 @@
             # can only stack on repositories that have compatible internal
             # metadata
             if getattr(repo._format, 'supports_tree_reference', False):
-                matching_format_name = 'pack-0.92-subtree'
+                if repo._format.supports_chks:
+                    matching_format_name = '2a'
+                else:
+                    matching_format_name = 'pack-0.92-subtree'
             else:
                 if repo._format.supports_chks:
                     matching_format_name = '2a'
@@ -915,7 +918,13 @@
         if getattr(repo._format, 'supports_tree_reference', False):
             # can only stack on repositories that have compatible internal
             # metadata
-            matching_format_name = 'pack-0.92-subtree'
+            if repo.supports_rich_root():
+                if repo._format.supports_chks:
+                    matching_format_name = '2a'
+                else:
+                    matching_format_name = 'pack-0.92-subtree'
+            else:
+                matching_format_name = 'dirstate-subtree'
             mismatching_format_name = 'rich-root-pack'
         else:
             if repo.supports_rich_root():

=== modified file 'bzrlib/tests/per_tree/test_path_content_summary.py'
--- bzrlib/tests/per_tree/test_path_content_summary.py	2011-12-16 17:34:46 +0000
+++ bzrlib/tests/per_tree/test_path_content_summary.py	2012-01-24 15:41:25 +0000
@@ -147,7 +147,7 @@
 
     def test_tree_content_summary(self):
         tree = self.make_branch_and_tree('tree')
-        if not tree.branch.repository._format.supports_tree_reference:
+        if not tree.supports_tree_reference():
             raise tests.TestNotApplicable("Tree references not supported.")
         subtree = self.make_branch_and_tree('tree/path')
         tree.add(['path'])

=== modified file 'bzrlib/tests/test_merge.py'
--- bzrlib/tests/test_merge.py	2012-01-05 13:02:31 +0000
+++ bzrlib/tests/test_merge.py	2012-01-24 15:41:25 +0000
@@ -227,9 +227,9 @@
 
     def test_nested_merge(self):
         tree = self.make_branch_and_tree('tree',
-            format='dirstate-with-subtree')
+            format='development-subtree')
         sub_tree = self.make_branch_and_tree('tree/sub-tree',
-            format='dirstate-with-subtree')
+            format='development-subtree')
         sub_tree.set_root_id('sub-tree-root')
         self.build_tree_contents([('tree/sub-tree/file', 'text1')])
         sub_tree.add('file')

=== modified file 'bzrlib/tests/test_smart.py'
--- bzrlib/tests/test_smart.py	2012-01-19 19:18:47 +0000
+++ bzrlib/tests/test_smart.py	2012-01-24 15:41:25 +0000
@@ -2669,7 +2669,7 @@
         base_inv = repository.revision_tree(base_revid).inventory
         inv = repository.revision_tree(revid).inventory
         inv_delta = inv._make_delta(base_inv)
-        serializer = inventory_delta.InventoryDeltaSerializer(True, False)
+        serializer = inventory_delta.InventoryDeltaSerializer(True, True)
         return "".join(serializer.delta_to_lines(base_revid, revid, inv_delta))
 
     def test_single(self):

=== modified file 'bzrlib/workingtree.py'
--- bzrlib/workingtree.py	2012-01-18 17:47:06 +0000
+++ bzrlib/workingtree.py	2012-01-24 15:41:25 +0000
@@ -840,7 +840,7 @@
         raise NotImplementedError(self.subsume)
 
     def _setup_directory_is_tree_reference(self):
-        if self._branch.repository._format.supports_tree_reference:
+        if self.supports_tree_reference():
             self._directory_is_tree_reference = \
                 self._directory_may_be_tree_reference
         else:
@@ -3098,6 +3098,15 @@
         """True if this format supports stored views."""
         return False
 
+    def supports_tree_reference(self):
+        """True if this format supports tree references.
+
+        Note that in order to be able to add tree references the
+        working tree, branch and repository all have to support
+        tree references.
+        """
+        return False
+
     @classmethod
     @symbol_versioning.deprecated_method(
         symbol_versioning.deprecated_in((2, 4, 0)))
@@ -3181,6 +3190,8 @@
     "bzrlib.workingtree_4", "WorkingTreeFormat5")
 format_registry.register_lazy("Bazaar Working Tree Format 6 (bzr 1.14)\n",
     "bzrlib.workingtree_4", "WorkingTreeFormat6")
+format_registry.register_lazy("Bazaar Working Tree Format 7 (bzr 2.6)\n",
+    "bzrlib.workingtree_4", "WorkingTreeFormat7")
 format_registry.register_lazy("Bazaar-NG Working Tree format 3",
     "bzrlib.workingtree_3", "WorkingTreeFormat3")
 format_registry.set_default_key("Bazaar Working Tree Format 6 (bzr 1.14)\n")

=== modified file 'bzrlib/workingtree_4.py'
--- bzrlib/workingtree_4.py	2012-01-06 14:09:04 +0000
+++ bzrlib/workingtree_4.py	2012-01-24 15:41:25 +0000
@@ -201,7 +201,7 @@
             WorkingTree._comparison_data(self, entry, path)
         # it looks like a plain directory, but it's really a reference -- see
         # also kind()
-        if (self._repo_supports_tree_reference and kind == 'directory'
+        if (self.supports_tree_reference() and kind == 'directory'
             and entry is not None and entry.kind == 'tree-reference'):
             kind = 'tree-reference'
         return kind, executable, stat_value
@@ -346,7 +346,7 @@
                     # add this entry to the parent map.
                     parent_ies[(dirname + '/' + name).strip('/')] = inv_entry
                 elif kind == 'tree-reference':
-                    if not self._repo_supports_tree_reference:
+                    if not self.supports_tree_reference():
                         raise errors.UnsupportedOperation(
                             self._generate_inventory,
                             self.branch.repository)
@@ -526,7 +526,7 @@
         return iter(result)
 
     def iter_references(self):
-        if not self._repo_supports_tree_reference:
+        if not self.supports_tree_reference():
             # When the repo doesn't support references, we will have nothing to
             # return
             return
@@ -551,6 +551,7 @@
         entry = self._get_entry(file_id=file_id, path=path)
         state._observed_sha1(entry, sha1, statvalue)
 
+    @needs_read_lock
     def kind(self, file_id):
         """Return the kind of a file.
 
@@ -568,7 +569,7 @@
     def _kind(self, relpath):
         abspath = self.abspath(relpath)
         kind = file_kind(abspath)
-        if (self._repo_supports_tree_reference and kind == 'directory'):
+        if (self.supports_tree_reference() and kind == 'directory'):
             entry = self._get_entry(path=relpath)
             if entry[1] is not None:
                 if entry[1][0][0] == 't':
@@ -1176,7 +1177,8 @@
 
     @needs_read_lock
     def supports_tree_reference(self):
-        return self._repo_supports_tree_reference
+        return (self._format.supports_tree_reference() and
+                self._repo_supports_tree_reference)
 
     def unlock(self):
         """Unlock in format 4 trees needs to write the entire dirstate."""
@@ -1445,6 +1447,19 @@
         return views.PathBasedViews(self)
 
 
+class WorkingTree7(ContentFilteringDirStateWorkingTree):
+    """This is the Format 7 working tree.
+
+    This differs from WorkingTree6 by:
+     - Supporting nested trees.
+
+    This is new in bzr 2.6.
+    """
+
+    def _make_views(self):
+        return views.PathBasedViews(self)
+
+
 class DirStateWorkingTreeFormat(WorkingTreeFormatMetaDir):
 
     missing_parent_conflicts = True
@@ -1597,8 +1612,7 @@
     def _get_matchingbzrdir(self):
         """Overrideable method to get a bzrdir for testing."""
         # please test against something that will let us do tree references
-        return bzrdir.format_registry.make_bzrdir(
-            'dirstate-with-subtree')
+        return bzrdir.format_registry.make_bzrdir('2a')
 
     _matchingbzrdir = property(__get_matchingbzrdir)
 
@@ -1678,6 +1692,39 @@
         return True
 
 
+class WorkingTreeFormat7(DirStateWorkingTreeFormat):
+    """WorkingTree format supporting nested trees.
+
+    This format is still experimental and can change.
+    """
+
+    upgrade_recommended = False
+
+    _tree_class = WorkingTree7
+
+    @classmethod
+    def get_format_string(cls):
+        """See WorkingTreeFormat.get_format_string()."""
+        return "Bazaar Working Tree Format 7 (bzr 2.6)\n"
+
+    def get_format_description(self):
+        """See WorkingTreeFormat.get_format_description()."""
+        return "Working tree format 6"
+
+    def _init_custom_control_files(self, wt):
+        """Subclasses with custom control files should override this method."""
+        wt._transport.put_bytes('views', '', mode=wt.bzrdir._get_file_mode())
+
+    def supports_content_filtering(self):
+        return True
+
+    def supports_views(self):
+        return True
+
+    def supports_tree_reference(self):
+        return True
+
+
 class DirStateRevisionTree(InventoryTree):
     """A revision tree pulling the inventory from a dirstate.
     
@@ -1738,7 +1785,7 @@
         return path_utf8.decode('utf8')
 
     def iter_references(self):
-        if not self._repo_supports_tree_reference:
+        if not self.supports_tree_reference():
             # When the repo doesn't support references, we will have nothing to
             # return
             return iter([])

=== modified file 'doc/en/release-notes/bzr-2.6.txt'
--- doc/en/release-notes/bzr-2.6.txt	2012-01-23 15:07:14 +0000
+++ doc/en/release-notes/bzr-2.6.txt	2012-01-24 15:41:25 +0000
@@ -31,6 +31,11 @@
    the URL scheme when accessing a HTTPS location.
    (Jelmer Vernooij, #125055)
 
+ * The ``2a`` repository format now supports tree references, making
+   it possible to import Git repositories with submodules in their
+   history. It is not yet possible to check out revisions
+   that contain nested trees.  (Jelmer Vernooij, #402814)
+
 Bug Fixes
 *********