RemoteBranch._pull doesn't know about _run_hooks

[Martin Pool]

_push and _pull also run their master branch's _push and _pull methods, passing across the arguments. The problem is that some implementations of these methods take _run_hooks and _master_branch parameters, and some do not.

John writes:

I'm trying to sort out the code in question, but I just did:

$ cd jam-integration
$ bzr pull ../bzr.dev

and got (and a traceback):

bzr: ERROR: exceptions.TypeError: pull() got an unexpected keyword
argument '_run_hooks'

I think the problem is that I'm using a checkout of a "bzr+ssh://"
branch, and the "branch-hooks" updates weren't taught to the new smart
server code.

It seems to be that the base implementation does not have "_run_hooks"
as a parameter, and neither does "RemoteBranch".

Further, "BzrBranch" also has a "_hook_master" variable. I don't know
how this should be passed around, since BzrBranch5 does *not* have that
parameter.

I think the failure is that Branch.pull() seems to assume that the
master is of the same format as current, so we expected that we could
update the local with a private variable and pass it around.

However, we can't, because master may be a completely different branch
(it could be an SVN branch, for example).

The *easy* fix is to just add "_run_hooks" on the RemoteBranch.pull(),
but that doesn't seem like the correct fix.

>
> One other thing I wanted to mention...
>
> The reason I feel this is complicated, was I tried to think about how to
> write a test for this.
>
> And basically it means creating a local (heavy) checkout of an hpss
> branch, and then trying to do local_branch.pull(another_branch). Since
> that will trigger the master.pull() bug.
>
> But I don't think we want to create that situation for testing every
> function on Branch. Especially since it should also fail for SVNBranch,
> which means we would need to test every branch format with a master
> branch of the other format.
>
> Which says to me that Branch.pull() is not using the correct api to
> access master_branch.pull(). It should either:
>
> 1) Promote _run_hooks to being a public parameter, and thus have
> interface tests for it in branch_implementations/*
>
> 2) Check that the master branch supports the flag, either by catching
> the error (ugly ugly ugly), or by having a "supports pull hooks" flag.
> And supporting the hook means you support disabling it through _run_hooks().
>
> I don't know how to test this one, though. Maybe a
> branch_implementations/* test which checks for "supports pull hooks" and
> then verifies that all branches which have the support flag set also
> support '_run_bzr'.

I think you're right about the cause and also that the solution is not
obvious.

But I think it has to be (1) - that we say there is a public parameter
to pull that lets you disable the hooks, and also one that tells it
what the master branch is. It's a bit ugly that the master branch
needs to be specially mentioned here. I think that makes it plausible
to test - for each type of branch, just call with this disabled and
check we get the appropriate thing.

I would ask at this point whether the hooks really really need to get
the master location and whether it needs to be in the result - after
all if you want it the can just call get_master_branch. Robert added
this, maybe he has an opinion.

[Martin Pool (mbp)]

Fix pending review.