[MERGE] Graph.find_distance_to_null

Ian Clatworthy ian.clatworthy at internode.on.net
Thu May 29 07:10:23 BST 2008


John Arbash Meinel wrote:
> This patch has 3 performance improvements applied.

Nice. Some minor stuff below to consider ...

bb:tweak

> +                stop_revno = other_revno
> +
>              # whats the current last revision, before we fetch [and change it
>              # possibly]

It's outside your change but a small comment fix: s/whats/what's/.

> +            # We assume that during 'pull' the local repository is closer than
> +            # the remote one.

That comment is useful. I think it's worth putting a similar one in the
matching place in the push code.

> +            if not parents: # An empty list, or None is still a ghost

I'd reword that as simply "An empty list or None is a ghost."

> +                if next in known_revnos:
> +                    assert known_revnos[next] == next_revno

Our policy is not to use asserts in our code now. I think it can go.
If you want to keep it, turn it into an if+AssertionError please.

> -    def update_revisions(self, other, stop_revision=None, overwrite=False):
> +    def update_revisions(self, other, stop_revision=None, overwrite=False, graph=None):

Line length > 80. (PEP-8)

> +    def test_ignores_older(self):

I'd prefer a longer name like 'test_ignores_older_unless_overwrite'.

> +    def test_target_parallel_to_known_limits(self):
> +        # Even though the known revision isn't part of the other ancestry, the
> +        # eventually converge

You probably mean 'they eventually' instead of 'the eventually'.

Ian C.



More information about the bazaar mailing list