[MERGE] Refactor commit to prepare for population by tree walking

[Ian Clatworthy]

After most of the last two weeks on release managing 0.18 and some
broader QA, back to commit performance at last ...

Thanks to Aaron for the review. Updated bundled attached. I've removed
populate_from_tree because it's not used yet. There is one UI change:
commit now reports progress in terms of a directory count x, not an
entry count x/y. That's because we want to get away from needing to
display the total, and directory count updating is frequent enough in
terms of feedback.

Otherwise, this change is 98% refactoring of the commit code to make it
more readable and put logical blocks together. Don't be fooled by the
large amount of diff: except as explained in this email, I've taken
great care *not* to make logic changes of any kind in this particular
patch.

I'm aware that moving stuff into methods can hurt performance but it
isn't in this context and it helps me progressively move the code to a
better place. We are not getting a performance improvement (or loss)
from these changes but I feel that it's still a useful step forward
along that path.

The other 2% of changes are:

1. Changing how the nested_tree is obtained as recommended by
   Aaron below
2. Some changes to memorytree.py to make it work with iter_changes.

I'm happy to submit the memorytree.py changes later if people feel
strongly about it. Those changes are independent of the others but I've
included them here as they are all part of the commit refactoring in
preparation for moving to a better iteration algorithm.

Some particular comments not yet covered in other emails ...

Aaron Bentley wrote:

>> +    def _commit_nested_tree(self, path):
>> +        "Commit a nested tree."
>> +        sub_tree = WorkingTree.open(self.work_tree.abspath(path))
> 
> ^^^ the recommended way of doing this is self.get_nested_tree(self,
> file_id, path)

Fixed.

>> +    def _record_entry(self, path, file_id, specific_files, kind, name,
>> +                      parent_id, definitely_changed, existing_ie=None):
> 
> ^^^ I think this should be building up an inventory delta.  That's meant
> to be the new way of manipulating tree contents.  Basically, we
> shouldn't be touching Tree.inventory at all.

Agreed. That's coming.

>> +    def _report_change(self, ie, path):
>> +        """Report a change to the user.
>> +
>> +        The change that has occurred is described relative to the basis
>> +        inventory.
>> +        """
>> +        if (self.basis_inv.has_id(ie.file_id)):
>> +            basis_ie = self.basis_inv[ie.file_id]
>> +        else:
>> +            basis_ie = None
>> +        change = ie.describe_change(basis_ie, ie)
>> +        if change in (InventoryEntry.RENAMED,
>> +            InventoryEntry.MODIFIED_AND_RENAMED):
>> +            old_path = self.basis_inv.id2path(ie.file_id)
>> +            self.reporter.renamed(change, old_path, path)
>> +        else:
>> +            self.reporter.snapshot_change(change, path)
> 
> I think it would be nice to reuse delta._ChangeReporter here.
> Consistency FTW.  Also not using inventories FTW.

Right now, it's just the existing code put into a method. There is a bit
of work to do in terms of commit reporting rationalisation around 3
levels (quiet, normal and verbose) and that all needs to tie in IMO. In
fact, for all but the verbose level, this method can be skipped
altogether. In terms of priorities, I want to focus on getting the API
refactoring and performance work delivered. I think the message level
stuff and implementation should come after that. Having said that, it's
likely the move to a new iterator will drive some changes in this
routine sooner rather than later perhaps.

>> +    def _emit_progress_set_stage(self, name, entries_title=None):
>>          """Set the progress stage and emit an update to the progress bar."""
> 
> ^^^ does this need to be emitting progress *and* setting stage?

I've renamed this method (as you encouraged in a follow-up email) and a
sister method to make them clearer.

In summary, there's limited user value in this change. I still believe
this is a useful refactoring though for the trunk code base, let alone
for me.

Ian C.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: commit-prepare-populate-alternatives-2.patch
Type: text/x-patch
Size: 91720 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070713/4e255a26/attachment-0001.bin