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

[Ian Clatworthy]

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.

> +   * Committing a change which is not a merge and does not change the number of
> +     files in the tree is faster by utilising the data about whether files are
> +     changed to determine if a the tree is unchanged rather than recalculating
                                ^^^^^
Typo here.

> +        # note that deletes have occured

In the comment, occurred has 2 r's.

> @@ -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?

> --- 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?

As it stands, the setting of self._versioned_root smells bad to me even
though it's probably correct as best I can determine. I say that because
it's *only* set inside an if branch and yet potentially "always" read in
the expression evaluated as the return result. Looking closer, the read
is in fact guarded but via a different condition to the if setting the
value. To cut a long story short, I hope I know this code well and it
took me a few looks to confirm it was all ok. If _versioned_root was a
class attribute, the complexity would be reduced IMO.

          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.

Ian C.