[merge] change commit to use update_to_one_parent_via_delta; other fixes

Martin Pool mbp at sourcefrog.net
Mon Oct 15 06:49:56 BST 2007


> >                  self._update_builder_with_changes()
> > +                self._report_and_accumulate_deletes()
> >                  self._check_pointless()
>
> Now that we're calculating a delta, we can use it in _check_pointless().
> That can be a separate patch of course but adding a TODO to this one
> mentioning that would be good.

It's actually pretty cheap already, as it just checks the size of the
inventories and the memory of whether we added or deleted anything.
It could be cleaned up possible by combining with the delta but I
don't think it's urgent.

>
> >          # Check and warn about old CommitBuilders
> >          if not self.builder.record_root_entry:
> > -            symbol_versioning.warn('CommitBuilders should support recording'
> > -                ' the root entry as of bzr 0.10.', DeprecationWarning,
> > -                stacklevel=1)
> > -            self.builder.new_inventory.add(self.basis_inv.root.copy())
> > +            raise AssertionError('CommitBuilders should support recording'
> > +                ' the root entry as of bzr 0.10.')
>
> As best I can tell, this test at this level is actually completely
> redundant as it's done anyway (inside finish_inventory) at the level below.
> I recall taking it out of one of my branches and all tests still passed.
> There may be good reasons for leaving it there?

Done.

>
>
> > +                # XXX: It looks like this is only hit when _check_root decided
> > +                # to set a new revision on the root.  We seem to be overriding
> > +                # ie.revision being set at this point to mean either its an
> > +                # unversioned root, or that it's an unchanged file.
> > +                #
>
> I actually find the code easier to follow without this comment.

Yes, you're right, that was mostly me thinking out loud.

>
> >                  # repositories that do not version the root set the root's
> >                  # revision to the new commit even when no change occurs, and
> >                  # this masks when a change may have occurred against the basis,
> >                  # so calculate if one happened.
> > -                if ie.file_id not in basis_inv:
> > +                if ie.file_id in basis_inv:
> > +                    delta = (basis_inv.id2path(ie.file_id), path,
> > +                        ie.file_id, ie)
>
> Hmm. This *feels* wrong to me: always returning a delta even when the
> old path and the new path are the same. In Robert's old code, he
> explicitly tested for that case and didn't bother with a delta when
> encountered. Given this change is the essence of this fix, I think
> we need a clear comment immediately before we set the delta,
> explaining that we still need a delta because ie has an updated
> entry (including an updated rev-no).

I think it's right because we can still update entries that haven't moved.

> I think "return None, ..." is clear enough here. (The separate setting
> of delta to None looks like a hangover from refactoring the earlier code.)

Done.

> > +        # XXX: This assumes its being run against a repository that does
> > +        # update the root revision on every commit.  Newer ones that use
> > +        # RootCommitBuilder won't update it on each commit.
>
> Minor spelling grammar notes:

Done

I measured the performance again: it's about 6% slower than without
update_etc, which is precisely accounted for by the extra time to do
_get_inventory and apply_delta.  So for the moment I'm not enabling
that change but would like to merge in the rest.

This update eliminates an unnecessary iteration of the inventory when
handling deletions, which cuts about 15% off the commit time in a
50kfile tree after a deletion has occurred.



-- 
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20071015-commit-basis-updates.diff
Type: text/x-diff
Size: 20318 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071015/b8bd1a17/attachment-0001.bin 


More information about the bazaar mailing list