[MERGE] Skip unnecessary work in BzrBranch._basic_push.

[Robert Collins]

On Tue, 2008-09-16 at 15:35 +1000, Andrew Bennetts wrote:
> 
> > 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 :)

No tags locally might mean 'we should delete the remote tags' for a
versioned-tag implementation (which is what bzr-svn has). So I think it
needs a comment and consideration :).

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

Recalling deeper, this was with ghosts too, we'd fetch ghosts by doing a
push. So i think this change is fine, but we should support the raw
cmd_fetch a little better.

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

If tag X is not reachable from the tip, push can push a tag that isn't
reconstructable.

e.g.

bzr init
bzr commit --tag foo -m bar --unchanged
bzr uncommit
bzr commit -m fixed-bar --unchanged
bzr push ../new-branch

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

I don't think a large test module is a particular problem. That said,
what we've done to address that in the past is to make a package e.g.
bzrlib.tests.remote with modules like bzrlib.tests.remote.test_branch,
bzrlib.tests.remote.test_repository, etc.

What I'd like to see is that these 'effort' tests are coupled to the
code performing the effort, either the remote tests, or the cmd tests
for the command being assessed, etc. If I remember this one accurately
its testing a branch method, so perhaps a branch method test is most
relevant. But if its a RemoteBranch method thats being tested, they
should be there. What this is actually testing is that the code invoked
by push only triggers a limited number of remote methods.

So, where does this lead me....

the test belongs in bzrlib/tests/per_branch/test_push.py. Because:
 - different implementations of branch might do 'push' differently

I think we need to talk about the tags more, so this is still
bb:comment.

-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/74081d76/attachment.pgp