launchpad-reviewers team mailing list archive

[Jelmer Vernooij <jelmer.vernooij@xxxxxxxxxxxxx>]

Jelmer Vernooij has proposed merging lp:~jelmer/launchpad/append_revisions_only-oops into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #798956 in Launchpad itself: "  AppendRevisionsOnlyViolation: Operation denied because it would change the main history, which is not permitted by the append_revisions_only setting on branch"
  https://bugs.launchpad.net/launchpad/+bug/798956

For more details, see:
https://code.launchpad.net/~jelmer/launchpad/append_revisions_only-oops/+merge/113053

Don't create OOPSes when AppendRevisionsOnlyViolation is raised in the
codehosting service.

These are user errors (if 'append_revisions_only' is set to True), and
should not be of concern to launchpad developers. The bazaar smart server
already forwards them to the user properly.

== Tests ==
./bin/test lp.codehosting.tests.test_bzrutils

== QA ==
1. Create a branch on Launchpad and push some code to it
2. Verify ``bzr uncommit lp:yourbranch`` works and does the right thing
3. Run ``bzr config -d lp:yourbranch append_revisions_only=True``
4. Verify ``bzr uncommit lp:yourbranch`` prints a AppendRevisionsOnlyViolation error
5. Verify that no oopses have been created
-- 
https://code.launchpad.net/~jelmer/launchpad/append_revisions_only-oops/+merge/113053
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jelmer/launchpad/append_revisions_only-oops into lp:launchpad.

=== modified file 'lib/lp/codehosting/bzrutils.py'
--- lib/lp/codehosting/bzrutils.py	2012-06-14 05:18:22 +0000
+++ lib/lp/codehosting/bzrutils.py	2012-07-02 14:23:24 +0000
@@ -33,6 +33,7 @@
     trace,
     )
 from bzrlib.errors import (
+    AppendRevisionsOnlyViolation,
     NotStacked,
     UnstackableBranchFormat,
     UnstackableRepositoryFormat,
@@ -56,6 +57,14 @@
     ScriptRequest,
     )
 
+# Exception classes which are not converted into OOPSes
+NOT_OOPS_EXCEPTIONS = (AppendRevisionsOnlyViolation,)
+
+def should_log_oops(exc):
+    """Return true if exc should trigger an OOPS.
+    """
+    return not issubclass(exc, NOT_OOPS_EXCEPTIONS)
+
 
 def is_branch_stackable(bzr_branch):
     """Return True if the bzr_branch is able to be stacked."""
@@ -163,7 +172,8 @@
     """Make a hook for logging OOPSes."""
 
     def log_oops():
-        error_utility.raising(sys.exc_info(), request)
+        if should_log_oops(sys.exc_info()[0]):
+            error_utility.raising(sys.exc_info(), request)
     return log_oops
 
 
@@ -199,6 +209,7 @@
     request = BazaarOopsRequest(user_id)
     hook = make_oops_logging_exception_hook(error_utility, request)
     add_exception_logging_hook(hook)
+    return hook
 
 
 class HttpAsLocalTransport(LocalTransport):

=== modified file 'lib/lp/codehosting/tests/test_bzrutils.py'
--- lib/lp/codehosting/tests/test_bzrutils.py	2011-12-15 10:09:59 +0000
+++ lib/lp/codehosting/tests/test_bzrutils.py	2012-07-02 14:23:24 +0000
@@ -14,11 +14,11 @@
     )
 from bzrlib.branch import Branch
 from bzrlib.bzrdir import format_registry
+from bzrlib.errors import AppendRevisionsOnlyViolation
 from bzrlib.remote import RemoteBranch
 from bzrlib.tests import (
     multiply_tests,
     test_server,
-    TestCase,
     TestCaseWithTransport,
     TestLoader,
     TestNotApplicable,
@@ -33,10 +33,12 @@
     DenyingServer,
     get_branch_stacked_on_url,
     get_vfs_format_classes,
+    install_oops_handler,
     is_branch_stackable,
     remove_exception_logging_hook,
     )
 from lp.codehosting.tests.helpers import TestResultWrapper
+from lp.testing import TestCase
 
 
 class TestGetBranchStackedOnURL(TestCaseWithControlDir):
@@ -171,6 +173,17 @@
         self.logException(exception)
         self.assertEqual([(RuntimeError, exception)], exceptions)
 
+    def test_doesnt_call_hook_for_non_important_exception(self):
+        # Some exceptions are exempt from OOPSes.
+        exceptions = []
+
+        self.assertEqual(0, len(self.oopses))
+        hook = install_oops_handler(1000)
+        self.addCleanup(remove_exception_logging_hook, hook)
+        exception = AppendRevisionsOnlyViolation("foo")
+        self.logException(exception)
+        self.assertEqual(0, len(self.oopses))
+
     def test_doesnt_call_hook_when_removed(self):
         # remove_exception_logging_hook removes the hook function, ensuring
         # it's not called when Bazaar logs an exception.