Code review comment for lp:~wallyworld/launchpad/codehost-error-trackback-leak

Hi Ian,

A couple of comments but nothing major. We agreed on IRC that you'd
request a review from Thumper since I don't have enough domain knowledge
to judge whether your fix is the best way to do things.

> === modified file 'lib/canonical/launchpad/xmlrpc/faults.py'
> --- lib/canonical/launchpad/xmlrpc/faults.py 2010-07-02 12:10:21 +0000
> +++ lib/canonical/launchpad/xmlrpc/faults.py 2010-11-27 05:11:16 +0000
> @@ -461,3 +462,15 @@
>
> def __init__(self, openid_identifier):
> LaunchpadFault.__init__(self, openid_identifier=openid_identifier)
> +
> +
> +class OopsOccurred(LaunchpadFault):
> + """An oops has occurred performing the requested operation."""
> +
> + error_code = 380
> + msg_template = ('An unexpected error has occurred while %(server_op)s. '
> + 'Please report a Launchpad bug and quote: %(oopsid)s.')

Multi-line strings should start on the line after the declaration and by
indented by 4 spaces:

    msg_template = (
        'An unexpected error has occurred while %(server_op)s. '
        'Please report a Launchpad bug and quote: %(oopsid)s.')

> === modified file 'lib/lp/codehosting/vfs/branchfs.py'
> --- lib/lp/codehosting/vfs/branchfs.py 2010-09-22 18:32:22 +0000
> +++ lib/lp/codehosting/vfs/branchfs.py 2010-11-27 05:11:16 +0000
> @@ -141,7 +146,7 @@
> # directories that Bazaar creates as part of regular operation. We support
> # only two numbered backups to avoid indefinite space usage.
> ALLOWED_DIRECTORIES = ('.bzr', '.bzr.backup', 'backup.bzr', 'backup.bzr.~1~',
> - 'backup.bzr.~2~')
> + 'backup.bzr.~2~')

A nice tidy-up here would be to have each element in the tuple on its
own line, as we do with lists and dicts. The closing paren should go on
a separate line and the last element should be followed by a comma.

> FORBIDDEN_DIRECTORY_ERROR = (
> "Cannot create '%s'. Only Bazaar branches are allowed.")
>
> @@ -500,10 +505,12 @@
> # Launchpad branch.
> deferred = AsyncVirtualTransport._getUnderylingTransportAndPath(
> self, relpath)
> +
> def maybe_make_branch_in_db(failure):
> # Looks like we are trying to make a branch.
> failure.trap(NoSuchFile)
> return self.server.createBranch(self._abspath(relpath))
> +
> def real_mkdir((transport, path)):
> return getattr(transport, 'mkdir')(path, mode)
>
> @@ -687,10 +694,42 @@
> data['id'], stacked_on_url, last_revision,
> control_string, branch_string, repository_string)
>
> - # It gets really confusing if we raise an exception from this method
> - # (the branch remains locked, but this isn't obvious to the client) so
> - # just log the error, which will result in an OOPS being logged.
> - return deferred.addCallback(got_path_info).addErrback(log.err)
> + def handle_error(failure=None, **kw):
> + # It gets really confusing if we raise an exception from this
> + # method (the branch remains locked, but this isn't obvious to
> + # the client). We could just log the failure using Twisted's
> + # log.err but this results in text containing traceback
> + # information etc being written to stderr. Since stderr is
> + # displayed to the user, if for example they arrive at this point
> + # via the smart server, we want to ensure that the message is
> + # sanitised. So what we will do is raise an oops and ask the user
> + # to log a bug with the oops information.
> + # See bugs 674305 and 675517 for details.
> +
> + request = errorlog.ScriptRequest([
> + ('source', virtual_url_fragment),
> + ('error-explanation', failure.getErrorMessage())])
> + self.unexpectedError(failure, request)
> + fault = faults.OopsOccurred("updating a Launchpad branch",
> + request.oopsid)

Argument lists for multi-line method calls always start on a new line:

            fault = faults.OopsOccurred(
                "updating a Launchpad branch", request.oopsid)