[merge][0.16][#111968] error in _run_hooks when pulling or pushing in a branch bound to a remote branch

[Martin Pool]

On 5/5/07, John Arbash Meinel <john at arbash-meinel.com> wrote:
> John Arbash Meinel has voted +1 (conditional).
> Status is now: Conditionally approved
> Comment:
> Overall, it looks really good. You have some cruft from comments and
> documentation, but otherwise merge it.

Yes, I do.  Thanks very much for reviewing this so promptly.

>       def pull(self, source, overwrite=False, stop_revision=None,
> -        _hook_master=None, _run_hooks=True):
> +        _hook_master=None, run_hooks=True):
>
> I realize we have a bit of mixed indenting, but I really prefer:
>       def pull(self, source, overwrite=False, stop_revision=None,
>                _hook_master=None, run_hooks=True):
>
> At least to me it makes it clearer what the parameters are. Though I
> realize we should always have a docstring to break between the
> parameters and the actual code.

So just to be clear (since my mailer at least shows things in a
proportionally spaced font), the rule is that parameters should be
indented to line up with the inside of the opening parenthesis?
That's OK with me.  Sometimes this doesn't work if the opening
parenthesis is already far to the right, and in that case I would say
that wrapping with a double indent is probably ok:

a = some_long_function_call_that_stretches_over_here(param,
        param, param, ...)

I think this is cleaner than having a backslash before the parenthesis
or suchlike.

>
>
>       def push(self, target, overwrite=False, stop_revision=None,
> -        _hook_master=None, _run_hooks=True):
> +        _override_hook_source_branch=None):
>           """See Branch.push.
> -
> -        :param _hook_master: Private parameter - set the branch to
> -            be supplied as the master to push hooks.
> -        :param _run_hooks: Private parameter - allow disabling of
> -            hooks, used when pushing to a master branch.
> +
> +        This is the basic concrete implementation of push()
> +        """
>
> You removed the documentation for other parameters, but you didn't *add*
> documentation for '_override_hook_source_branch'.

OK


> Incorrect documentation. You don't have a "run_hooks_cb". Probably an
> old intermediate form of your refactoring.

Yes.

> +        bound_location = target.get_bound_location()
> +        if bound_location and target.base != bound_location:
> +            # there is a master branch and we're not pushing to it, so
> we need
> +            # to update the master.
> +            #
> +            # TODO: don't just compare the locations, instead look at
> the lock
> +            # tokens to see if they're the same object -- mbp 20070504
>
> Why do you need to check if a target is bound to itself?

The condition was already there in the code I refactored; I just tried
to explain it.

-        bound_location = target.get_bound_location()
-        master_branch = None
-        if bound_location and target.base != bound_location:

But I think I misunderstood the condition, and I'm not sure if it
should be there at all.  What I thought it was trying to do is this:
normally if the target is bound we need to update its master too.
However, if we're pushing from the master into the bound branch then
we don't need to.  I wonder if this code was incorrectly adapted from
pull?

A branch bound to itself does sound like a strange situation.  I might
try removing that condition and see what happens.

> At the very least I think the TODO should be closer to what it is
> commenting about. To me it looks like you are commenting about the
> "master_branch = target.get_master_branch()".

OK
>
>
>       @needs_write_lock
> -    def pull(self, source, overwrite=False, stop_revision=None):
> +    def pull(self, source, overwrite=False, stop_revision=None,
> +            **kwargs):
> +        # FIXME: This lets the real branch run the hooks, so they'll be
> called
> +        # with the wrong branch parameters -- mbp 20070405
>
> The indenting of '**kwargs' needs 1 more character.
>
> And your comment sounds like it is intentional, rather than being
> accidental. I think you want to say:
>
> # FIXME: This asks the real branch to run the hooks, which causes them
> to
> # be called with the wrong branch parameters -- mbp 20070405
>
> It sounds like you just need to pass another _override_branch_source
> parameter. Like you did with push. Can you add that as a TODO, and
> reference 'push()' as an implementation that can be cribbed from? Maybe
> also add a bug for it.

Yes.

>
>
>       def push(self, target, overwrite=False, stop_revision=None):
> +        # FIXME: This lets the real branch run the hooks, so they'll be
> called
> +        # with the wrong branch parameters -- mbp 20070405
>           self._ensure_real()
>           return self._real_branch.push(
> -            target, overwrite=overwrite, stop_revision=stop_revision)
> +            target, overwrite=overwrite, stop_revision=stop_revision,
> +            _override_hook_source_branch=self)
>
> You are passing "_override_hook_source_branch", so I'm pretty sure you
> can remove the FIXME.

Correct.

>
> ...
>
> +        # XXX: It looks like there are some orders for generating tests
> that
> +        # fail as of 20070504 - maybe because of import order
> dependencies.
> +        # So unfortunately this will rarely intermittently fail at the
> moment.
> +        # -- mbp 20070504
>
> Didn't you fix that? I don't really like having an XXX: that we can't
> prove, but if you feel it is helpful for people encountering the bug,
> I'm okay with leaving it in.

I fixed at least one of them but I suspect there might be more.  The
thing to do is to run selftest --randomize repeatedly until it fails,
then fix the failure - or conclude after many runs that there are no
more such bugs.

I dislike having tests with random components, but we do need to test
the option and I suppose this is reasonably confined, since it doesn't
actually run the tests.

>
>
> +            # branch of the default type, which does allow binding.
> +            # See https://bugs.launchpad.net/bzr/+bug/112020
> +            if 1:
> +                local = BzrDir.create_branch_convenience('local2')
> +                local.bind(target)
> +            else:
> +                raise TestSkipped("Can't bind %s to %s" %
> +                    (local, target))
>
> At the very least, I would expect "if True:", but I think it is better
> to just remove the whole if/else. and just always create the local
> branch.

Right.

Depending on my weekend I'll make an 0.16 release with these changes
either today or Monday.
-- 
Martin