[MERGE] Skip unnecessary work in BzrBranch._basic_push.

[Andrew Bennetts]

Robert Collins wrote:
> bb:comment
> 
> Some thoughts, not sure if this is all relevant etc.

It is relevant :)

> On Mon, 2008-09-15 at 20:12 +1000, Andrew Bennetts wrote:
[...]
> > +        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.

The tags change is related.  It helps cut down the round trips: if there are no
tags in the local branch (which is at least somewhat common), then there's no
point retrieving the remote tags because we aren't going to change them.  I
guess your comment means this logic deserves a comment in the code :)

Possibly the check for no local tags could be pushed into the
self.tags.merge_to, but that would also require making target.tags a lazy
object (at the moment merely accessing a RemoteBranch's tags attribute fetches
the remote tags, possibly other branch's tags objects do that too).

> *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.

I wasn't aware of that motive, thanks for letting me know.  If we want to check
that the revision really is in the remote repo then an explicit
self.repository.has_revision check would be better I think, but personally I
don't think it's worth it.

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

I don't follow.  This change only affects the case where there are no tags on
the source, and thus no tags are changed (on either the source or the target).
So I don't see how this would allow a previously valid tag to become invalid.

> > === 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 was just blindly following the idiom of bzrlib/branch.py, but I think you're
right that's not much point doing that here.  I'll move the if.

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

Fair enough.  I didn't think it was great name either, and I think with
hindsight why I wanted another review of this code.

I do think it deserves a new module though; there's nowhere else
that tests this particular combination of factors (remote objects + builtin
cmd_* + measuring round trips), and the existing test modules for remote
object/smart server code are already too large.

The module it most closely overlaps with is test_remote, but that is for testing
which RPCs are triggered by Remote* objects, and also how those objects
unmarshal responses to those RPCs.  That's not close enough.

So how about if I rename the module to something like test_remote_branch or
test_remote_push?  As we write more tests in this area we can always rename and
rearrange.

Thanks for the review!

-Andrew.