[MERGE] 4% improvement to commit's common case

[Robert Collins]

On Thu, 2007-09-20 at 16:06 +1000, Ian Clatworthy wrote:
> Rob,
> 
> Some feedback below. If you disagree or I've missed the point, this
> might be closer to tweak than resubmit. But you've typically resubmitted
> on occasions when I've voted tweak before so I'm assuming resubmit is a
> fair call. :-)
> 
> bb:resubmit.
> 

Thanks for the spelling catches.

> > @@ -766,6 +738,8 @@
> >                  local=self.local, reporter=self.reporter)
> >          except errors.PointlessCommit:
> >              pass
> > +        else:
> > +            self.entries_changed = True
> 
> Is this correct? I read this as "branches with nested trees will always
> appear changed". Won't that mean some pointless commits won't get
> reported as such?

You are misreading it I think. nested trees without changes raise
PointlessCommit which is caught by the except clause. I'll check if
there is a corner case here not being handled and get back to you.

> > --- bzrlib/repository.py	2007-09-16 23:48:15 +0000
> > +++ bzrlib/repository.py	2007-09-20 03:43:11 +0000
> > @@ -191,6 +191,7 @@
> >          :param parent_invs: The inventories of the parent revisions of the
> >              commit.
> >          :param tree: The tree that is being committed.
> > +        :return: True if the root entry is versioned properly.
> >          """
> >          if ie.parent_id is not None:
> >              # if ie is not root, add a root automatically.
> > @@ -204,6 +205,7 @@
> >              # serializing out to disk and back in root.revision is always
> >              # _new_revision_id
> >              ie.revision = self._new_revision_id
> > +        return False
> 
> As written, this method always returns False. In the RootCommitBuilder
> subclass, it always returns True. If it truly is an attribute of the
> class, shouldn't we make it so?

It seemed to me it was tied closely to the root handling, but I can make
it an attribute.

>           changer()
> > -        rev2 = tree.commit('')
> > +        tree.lock_write()
> > +        try:
> [snip]
> > +        except:
> > +            builder.abort()
> > +            tree.unlock()
> > +            raise
> > +        else:
> > +            tree.unlock()
> >          tree1, tree2 = self._get_revtrees(tree, [rev1, rev2])
> >          self.assertEqual(rev1, tree1.inventory[name + 'id'].revision)
> >          self.assertEqual(rev2, tree2.inventory[name + 'id'].revision)
> 
> I think this test would be much clearer if the new code become a
> separate method that returned (rev2, list-of-commmit-id-return-codes).
> Please consider that.

That method is going to be unusable from anywhere else because its brain
damaged to this specific case.

I'd like another mini-commit in a test to triangulate before factoring
out. (I did consider it).

-Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070920/446eb7ae/attachment.pgp