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

[John Arbash Meinel]

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.



      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.


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


+    def _push_with_bound_branches(self, target, overwrite,
+            stop_revision,
+            _override_hook_source_branch=None):
+        """Updates branch.push to be bound branch aware
+
+        This is on the base BzrBranch class even though it doesn't 
support
+        bound branches because the *target* might be bound.
+
+        :param run_hooks_cb: Callback taking the Result object which 
should run
+            any applicable hooks.  It can make adjustments to the 
Result object
+            first.  May be a do-nothing function to disable running 
hooks.
+            Called with all branches locked, including the master if 
any.
+        """

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


+        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?

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()".


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


      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.

...

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


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



For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3Ce01316480705040357g1fb8ca0ek242fc8dac494bb8b%40mail.gmail.com%3E