[MERGE] Add mechanism for pre_change_branch_tip hook to cleanly reject a change.

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
| Hi,
|
| This patch adds a TipChangeRejected error for pre_change_branch_tip
hooks to
| raise.  The message given to TipChangeRejected will be reported
directly to the
| user, even over the smart server.  This is important for a hook that
wants to
| explain to the user *why* a change is rejected (e.g. "foo.xml is not
valid XML:
| syntax error on line 1234").
|
| This patch is relative to my last patch for a smart server hooks
integration
| test, but doesn't really depend on it.
|
| -Andrew.
|
|

@@ -1522,11 +1524,18 @@
~         check_not_reserved_id = _mod_revision.check_not_reserved_id
~         for rev_id in rev_history:
~             check_not_reserved_id(rev_id)
+        old_revno, old_revid = self.last_revision_info()
+        if len(rev_history) == 0:
+            revid = _mod_revision.NULL_REVISION
+        else:
+            revid = rev_history[-1]
+        self._run_pre_change_branch_tip_hooks(len(rev_history), revid)

^- Why did you chose to move it into set_revision_history() ? Just in
case something else called it directly?

Should we have a test for that?

+
+
+class HookFailed(BzrError):
+
+    _fmt = ("Hook '%(hook_name)s' during %(hook_stage)s failed:\n"
+            "%(traceback_text)s%(exc_value)s")
+
+    def __init__(self, hook_stage, hook_name, exc_info):
+        import traceback
+        self.hook_stage = hook_stage
+        self.hook_name = hook_name
+        self.exc_info = exc_info
+        self.exc_type = exc_info[0]
+        self.exc_value = exc_info[1]
+        self.exc_tb = exc_info[2]
+        self.traceback_text = ''.join(traceback.format_tb(self.exc_tb))
+
+
+class TipChangeRejected(BzrError):
+
+    _fmt = "%(msg)s"
+
+    def __init__(self, msg):
+        self.msg = msg
+

^- A docstring for when these are used, and what they represent would be
nice.



+    def test_tip_change_rejected(self):
+        transport = MemoryTransport()
+        transport.mkdir('branch')
+        transport = transport.clone('branch')
+        # A response of 'NoSuchRevision' is translated into an exception.
+        client = FakeClient(transport.base)
+        # lock_write
+        client.add_success_response('ok', 'branch token', 'repo token')
+        # set_last_revision
+        rejection_msg_unicode = u'rejection message\N{INTERROBANG}'
+        rejection_msg_utf8 = rejection_msg_unicode.encode('utf8')
+        client.add_error_response('TipChangeRejected', rejection_msg_utf8)
+        # unlock
+        client.add_success_response('ok')
+
+        bzrdir = RemoteBzrDir(transport, _client=False)
+        repo = RemoteRepository(bzrdir, None, _client=client)
+        branch = RemoteBranch(bzrdir, repo, _client=client)
+        branch._ensure_real = lambda: None
+        branch.lock_write()
+        client._calls = []
+
+        err = self.assertRaises(
+            errors.TipChangeRejected, branch.set_revision_history,
['rev-id'])
+        self.assertIsInstance(err.msg, unicode)
+        self.assertEqual(rejection_msg_unicode, err.msg)
+        branch.unlock()
+

^- you should really be using try/finally or addCleanup rather than a
bare unlock.

Also, shouldn't you be asserting the last revision hasn't changed?



You should also have direct tests in test_errors() for the formatting of
these exceptions.

And I wonder if we would want to add:

_fmt = 'Tip change rejected: %(msg)s'

Have you tried it in 'reality' so that we know what it looks like when
it fails?


I really like the functionality, but it seems to need a bit more polish.

BB:resubmit.

John
=:->



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkh/ywwACgkQJdeBCYSNAAPJ1ACgmMjB01EkKPWO9LOcIVmvjxuH
pZIAnjjSOpMFv59Lv6zuVD7YxeE7fn+z
=b6AV
-----END PGP SIGNATURE-----