record_iter_changes and subtrees

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John Arbash Meinel wrote:
> Aaron Bentley wrote:
>> Robert Collins wrote:
>>> So, iter_changes and subtrees.
>>> currently recursive='down'
>> recursive='down' in what context?
> 
>> Aaron
> 
> 
> Robert is trying to get "bzr commit" to work when you have subtrees
> enabled. I believe the internal "commit.py" code uses "recursive='down'"
> to indicate that while you are committing the top-level tree, you also
> want to commit all of the child trees.

Right.

> And the problem Robert is having is that 'iter_changes()' isn't being
> recursive, or at least not helpfully so.
> 
> As I see it the commit code itself needs to be recursive.

I prefer that the recursion be limited.  Doing commits of subtrees in
the middle of committing the parent tree is messy because of how much
state you have.

My original code for MutableTree.commit was:

    @needs_write_lock
    def commit(self, message=None, revprops=None, recursive='down', *args,
               **kwargs):
        if recursive == 'down':
            for tree in self.iter_nested_trees():
                try:
                    tree.commit(message, revprops, recursive, *args,
**kwargs)
                except errors.PointlessCommit:
                    pass
        # avoid circular imports
        from bzrlib import commit
        if revprops is None:
            revprops = {}
        if not 'branch-nick' in revprops:
            revprops['branch-nick'] = self.branch.nick
        # args for wt.commit start at message from the Commit.commit method,
        # but with branch a kwarg now, passing in args as is results in the
        #message being used for the branch
        args = (DEPRECATED_PARAMETER, message, ) + args
        committed_id = commit.Commit().commit(working_tree=self,
            revprops=revprops, *args, **kwargs)
        return committed_id

> So when you do
> commit at the top level, it starts scanning directories for changes. If
> it finds a tree-reference, it then needs to "recurse" and start a
> 'commit' in that tree. Then the sub-tree can decide whether it has
> changed or not, if it has, it needs to commit and generate a new
> revision_id, so that the parent tree can see the new revision_id, and
> see that it needs to record a new inventory entry for the subtree.
> 
> I think the current layering issue is that it is 'iter_changes' which is
> walking the directories, and it doesn't know that a 'commit' is going
> on, in order to say anything to the 'tree-reference' it finds.
> 
> I believe Aaron's original plan was that 'bzr commit' in a tree with
> subtrees would use a completely different "Tree" object (some sort of
> combined tree).

No, I didn't think that CompositeTree was suitable for commit.

> Another possibility comes back to having an index of tree-references. In
> which case "commit" then first calls "iter_references()" and pushes a
> "commit" into each of those, which then return with the revision_id they
> want to be, and only after all children have been processed does it
> start commit in the parent tree.

As above, that is my preferred approach, yes.

> Note that the commit in the child trees should be "pending", until the
> top level commit finishes. (A 2-phase commit sort of action.)

I think that's an additional requirement for foreign branches that may
be unworkable.  Jelmer?

> Anyway, if we did the 'commit(iter_references())' before commit in the
> top-level tree starts, then it should be able to notice that the
> revision_id of the child trees has been updated.

it == iter_changes?  Yes.

> Or alternatively, just a flag to iter_changes that tells it to always
> return 'tree-reference' (treating them as always modified until further
> inspection.)
> 
> That may not even need a flag. Consider the "bzr diff" case. We want to
> recurse into the child directories, because we don't know whether
> anything has changed there or not (yet).

The way that iter_changes is continually being tweaked to report things
other than changes is frustrating to me.

> Again, I believe the current design is to recurse either before- or
> after-the-fact

Correct.

> (using iter_references), but it seems nicer to have it
> done *during* another operation.

Disagree.  That's messy, messy.  Complicates error handling, complicates
debugging...

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAknKReoACgkQ0F+nu1YWqI0u/ACfVF+MD1rMNmYCx3Ww6c43v+Cg
Yw0AnRuJ1+XzyPUDgGG1+fFjO03LGCgk
=Np8p
-----END PGP SIGNATURE-----