[MERGE] Create new mutable tree method update_to_one_parent_via_delta for eventual use by commit.

[Ian Clatworthy]

Robert Collins wrote:
> This adds a method that will allow commit to give the working tree a
> delta to update its basis tree to be a different tree. 

bb: tweak

Looks good to me. There are one or two spots that are beyond my
knowledge of how things hang together. I'll flag those below fyi.

BTW, is this NEWS worthy? Perhaps an entry under "Internals" mentioning
the new public APIs would be good?


> +            The children attribute of new_entry is ignored. This is because
> +            apply_inventory_delta preserves children automatically across

You don't want to be referring to a higher level functional call here.
s/apply_inventory_delta/This method/.

> +            alterations to the parent of the children, and cases where the
> +            parent id of a child is changing require the child to be passed in
> +            as a separate change regardless. E.g. in the recursive deletion of
> +            a directory - the directories children must be included in the

directory's children


> +    def update_to_one_parent_via_delta(self, new_revid, delta):
> +        """Update the parents of this tree after a commit.
> +
> +        This gives the tree one parent, with revision id new_revid. The
> +        inventory delta delta is applied ot the current basis tree to generate

You probably don't need "delta delta" there. s/ot/to/.

> +        # if the tree is updated by a pull to the branch, as happens in
> +        # WorkingTree2, when there was no separation between branch and tree,
> +        # then just clear merges, efficiency is not a concern for now as this
> +        # is legacy environments only, and they are slow regardless.
> +        if self.last_revision() == new_revid:
> +            self.set_parent_ids([new_revid])
> +            return

Despite your clear and detailed comment, this test is beyond my
knowledge. I certainly can't say for sure why it applies to the
WorkingTree2 case and if it only applies to that case. (Sorry.) The
logic itself inside the if looks fine FWIW.

> @@ -197,6 +223,8 @@
>          self.build_tree(['dir/', 'dir/child', 'other/'])
>          wt.add(['dir', 'dir/child', 'other'],
>                 ['dir-id', 'child-id', 'other-id'])
> +        # this delta moves dir-id to dir2 and reparents 
> +        # child-id wto a parent of other-id

s/wto/to/.

> +class UpdateToOneParentViaDeltaTests(TestParents):
> +    """Tests for the update_to_one_parent_via_delta call.
> +    
> +    This is intuitively defined as 'apply an inventory delta to the basis and
> +    discard other parents', but for trees that have an inventory that is not
> +    managed as a tree-by-id, the implementation requires roughly duplicated
> +    tests with those for apply_inventory_delta on the main tree.
> +    """

Except as indicated below, these tests look really good to me. The main
comment I have is that you might want to add a test to check changing of
the permission bit is handled. Otherwise, coverage and clarity are
excellent IMO.

> +    def add_new_root(self, new_shape, old_revid, new_revid):
> +        if self.bzrdir_format.repository_format.rich_root_data:
> +            self.add_dir(new_shape, old_revid, 'root-id', None, '')
> +        else:
> +            self.add_dir(new_shape, new_revid, 'root-id', None, '')

Here's the other bit I'm not 100% clear on. The whole rich_root thing is
out of my mind's working-set right now. :-) It all looks ok FWIW but
that doesn't mean I could explain why it's right.

> +        right_revid = 'right-parent'
> +        right_shape = Inventory(root_id=None)
> +        self.add_dir(right_shape, left_revid, 'root-id', None, '')
> +        self.add_link(right_shape, right_revid, 'link-id', 'root-id', 'link',
> +            'left-target')

You might want to make that 'right-target'.

> +    def test_parent_id_changed(self):

s/a entry/an entry/.

> +    def test_name_changed(self):
> +        # test that when the only change to a entry is its name changing

Ditto.

Otherwise, I like it.

A final comment though. Inventory now has a public method called
apply_delta and it doesn't have matching tests at the same level. It is
*well* covered via higher level tests of course. For that reason or
otherwise, you may wish to make it private (_apply_delta) for now. I
don't feel strongly about it either way.

Ian C.