[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