Rev 2486: Review cleanups from John, mostly docs in http://sourcefrog.net/bzr/run-hooks

[Martin Pool]

At http://sourcefrog.net/bzr/run-hooks

------------------------------------------------------------
revno: 2486
revision-id: mbp at sourcefrog.net-20070507120314-a2h78bjezemwyl17
parent: mbp at sourcefrog.net-20070504105649-fv0fqgrnq5ojvjdl
committer: Martin Pool <mbp at sourcefrog.net>
branch nick: run-hooks
timestamp: Mon 2007-05-07 22:03:14 +1000
message:
  Review cleanups from John, mostly docs
modified:
  bzrlib/branch.py               branch.py-20050309040759-e4baf4e0d046576e
  bzrlib/remote.py               remote.py-20060720103555-yeeg2x51vn0rbtdp-1
  bzrlib/tests/branch_implementations/test_push.py test_push.py-20070130153159-fhfap8uoifevg30j-1
=== modified file 'bzrlib/branch.py'
--- a/bzrlib/branch.py	2007-05-04 10:56:49 +0000
+++ b/bzrlib/branch.py	2007-05-07 12:03:14 +0000
@@ -1476,7 +1476,7 @@
 
     @needs_write_lock
     def pull(self, source, overwrite=False, stop_revision=None,
-        _hook_master=None, run_hooks=True):
+             _hook_master=None, run_hooks=True):
         """See Branch.pull.
 
         :param _hook_master: Private parameter - set the branch to 
@@ -1526,10 +1526,15 @@
 
     @needs_read_lock
     def push(self, target, overwrite=False, stop_revision=None,
-        _override_hook_source_branch=None):
+             _override_hook_source_branch=None):
         """See Branch.push.
 
         This is the basic concrete implementation of push()
+
+        :param _override_hook_source_branch: If specified, run
+        the hooks passing this Branch as the source, rather than self.  
+        This is for use of RemoteBranch, where push is delegated to the
+        underlying vfs-based Branch. 
         """
         # TODO: Public option to disable running hooks - should be trivial but
         # needs tests.
@@ -1545,15 +1550,10 @@
     def _push_with_bound_branches(self, target, overwrite,
             stop_revision,
             _override_hook_source_branch=None):
-        """Updates branch.push to be bound branch aware
+        """Push from self into target, and into target's master if any.
         
         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.
         """
         def _run_hooks():
             if _override_hook_source_branch:
@@ -1563,11 +1563,10 @@
 
         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.
+            # there is a master branch.
             #
-            # TODO: don't just compare the locations, instead look at the lock
-            # tokens to see if they're the same object -- mbp 20070504
+            # XXX: Why the second check?  Is it even supported for a branch to
+            # be bound to itself? -- mbp 20070507
             master_branch = target.get_master_branch()
             master_branch.lock_write()
             try:
@@ -1687,8 +1686,8 @@
         
     @needs_write_lock
     def pull(self, source, overwrite=False, stop_revision=None,
-        run_hooks=True):
-        """Extends branch.pull to be bound branch aware.
+             run_hooks=True):
+        """Pull from source into self, updating my master if any.
         
         :param run_hooks: Private parameter - if false, this branch
             is being called because it's the master of the primary branch,

=== modified file 'bzrlib/remote.py'
--- a/bzrlib/remote.py	2007-05-04 10:01:17 +0000
+++ b/bzrlib/remote.py	2007-05-07 12:03:14 +0000
@@ -997,9 +997,12 @@
 
     @needs_write_lock
     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
+             **kwargs):
+        # FIXME: This asks the real branch to run the hooks, which means
+        # they're called with the wrong target branch parameter. 
+        # The test suite specifically allows this at present but it should be
+        # fixed.  It should get a _override_hook_target branch,
+        # as push does.  -- mbp 20070405
         self._ensure_real()
         self._real_branch.pull(
             source, overwrite=overwrite, stop_revision=stop_revision,
@@ -1007,8 +1010,6 @@
 
     @needs_read_lock
     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,

=== modified file 'bzrlib/tests/branch_implementations/test_push.py'
--- a/bzrlib/tests/branch_implementations/test_push.py	2007-05-04 10:01:17 +0000
+++ b/bzrlib/tests/branch_implementations/test_push.py	2007-05-07 12:03:14 +0000
@@ -208,12 +208,8 @@
             # remotebranches can't be bound.  Let's instead make a new local
             # 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))
+            local = BzrDir.create_branch_convenience('local2')
+            local.bind(target)
         source = self.make_branch('source')
         Branch.hooks.install_hook('post_push', self.capture_post_push_hook)
         source.push(local)