[MERGE] Skip unnecessary work in BzrBranch._basic_push.

[Robert Collins]

bb:comment

Some thoughts, not sure if this is all relevant etc.

On Mon, 2008-09-15 at 20:12 +1000, Andrew Bennetts wrote:
> === modified file 'bzrlib/branch.py'
> --- bzrlib/branch.py    2008-08-28 18:57:59 +0000
> +++ bzrlib/branch.py    2008-09-12 07:37:47 +0000
> @@ -1802,13 +1802,14 @@
>          result.source_branch = self
>          result.target_branch = target
>          result.old_revno, result.old_revid =
> target.last_revision_info()
> -
> -        # We assume that during 'push' this repository is closer than
> -        # the target.
> -        graph = self.repository.get_graph(target.repository)
> -        target.update_revisions(self, stop_revision,
> overwrite=overwrite,
> -                                graph=graph)
> -        result.tag_conflicts = self.tags.merge_to(target.tags,
> overwrite)
> +        if result.old_revid != self.last_revision():
> +            # We assume that during 'push' this repository is closer
> than
> +            # the target.
> +            graph = self.repository.get_graph(target.repository)
> +            target.update_revisions(self, stop_revision,
> overwrite=overwrite,
> +                                    graph=graph)
> +        if self.tags.supports_tags() and self.tags.get_tag_dict():
> +            result.tag_conflicts = self.tags.merge_to(target.tags,
> overwrite)
>          result.new_revno, result.new_revid =
> target.last_revision_info()
>          return result

The above tags change looks unrelated? Anyhow, the guard and check for
last_revision_id looks like something best pushed down to the
update_revisions call.

*also* Note that there is/was a case for having the repository always
check that the revision can be pushed. In particular checking the
revision is in the repository is arguably useful, OTOH perhaps we should
assume the branch state is always valid.

A bigger related problem which will probably cause this to be
undone/changed further is that tag revisions are not checked for, and
they should be.

> === modified file 'bzrlib/smart/client.py'
> --- bzrlib/smart/client.py      2008-06-17 04:22:26 +0000
> +++ bzrlib/smart/client.py      2008-09-15 10:02:00 +0000
>  class _SmartClient(object):
> @@ -50,8 +53,15 @@
>              encoder.call(method, *args)
>          return response_handler
>  
> +    def _run_call_hooks(self, method, args, body, readv_body):
> +        params = CallHookParams(method, args, body, readv_body)
> +        for hook in _SmartClient.hooks['call']:
> +            hook(params)
> +            
>      def _call_and_read_response(self, method, args, body=None,
> readv_body=None,
>              expect_response_body=True):
> +        if _SmartClient.hooks['call']:
> +            self._run_call_hooks(method, args, body, readv_body)

Why not delegate the if check here to _run_call_hooks? There should be
nearly-no marshalling overhead, and remote method latency means python
bytecode overhead is not really a factor on a per-rpc basis.

I don't think the effort test module is a good place to put this test;
its clearly either a Branch test, or a Remote test to me.

-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/20080916/106081df/attachment.pgp