[BUG][0.16 blocker] pull doesn't know about '_run_hooks'

[Martin Pool]

On 4/24/07, John Arbash Meinel <john at arbash-meinel.com> wrote:
> John Arbash Meinel wrote:
> > 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.
> >
> > Thoughts?
> >
> > John
> > =:->
>
> 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