review needs-info

> === modified file 'lib/canonical/launchpad/scripts/logger.py'
> --- lib/canonical/launchpad/scripts/logger.py	2009-06-25 05:30:52 +0000
> +++ lib/canonical/launchpad/scripts/logger.py	2009-07-22 02:16:56 +0000
> @@ -113,8 +113,8 @@
>      def __init__(self):
>          self.buffer = StringIO()
>  
> -    def message(self, prefix, *stuff, **kw):
> -        self.buffer.write('%s: %s\n' % (prefix, ' '.join(stuff)))
> +    def message(self, prefix, msg, *stuff, **kw):
> +        self.buffer.write('%s: %s\n' % (prefix, msg % stuff))

What is the impact of other code using message?

This to me looks like it'll brake things.


> === modified file 'lib/lp/codehosting/rewrite.py'
> --- lib/lp/codehosting/rewrite.py	2009-07-20 21:32:15 +0000
> +++ lib/lp/codehosting/rewrite.py	2009-07-22 03:24:19 +0000
> @@ -5,38 +5,71 @@
>  """
>  
>  import time
> -import xmlrpclib
>  
>  from bzrlib import urlutils
>  
> -from lp.codehosting.vfs import (
> -    branch_id_to_path, BranchFileSystemClient)
> +from canonical.launchpad.webapp.interfaces import (
> +        IStoreSelector, MAIN_STORE, SLAVE_FLAVOR)
> +from zope.component import getUtility
> +from lp.code.model.branch import Branch
> +from lp.codehosting.vfs import branch_id_to_path
> +
>  from canonical.config import config
> -from lp.code.interfaces.codehosting import (
> -    BRANCH_TRANSPORT, LAUNCHPAD_ANONYMOUS)
> -from canonical.launchpad.xmlrpc import faults
> -from canonical.twistedsupport import extract_result
>  
>  __all__ = ['BranchRewriter']
>  
>  
>  class BranchRewriter:
>  
> -    def __init__(self, logger, proxy):
> +    def __init__(self, logger, _now=None):
>          """
>  
>          :param logger: Logger than messages about what the rewriter is 
doing
>              will be sent to.
>          :param proxy: A blocking proxy for a branchfilesystem endpoint.
>          """
> +        if _now is None:
> +            self._now = time.time
> +        else:
> +            self._now = _now
>          self.logger = logger
> -        self.client = BranchFileSystemClient(proxy, LAUNCHPAD_ANONYMOUS, 
10.0)
> +        self.store = getUtility(IStoreSelector).get(MAIN_STORE, 
SLAVE_FLAVOR)
> +        self._cache = {}
>  
>      def _codebrowse_url(self, path):
>          return urlutils.join(
>              config.codehosting.internal_codebrowse_root,
>              path)
>  
> +    def _getBranchIdAndTrailingPath(self, location):
> +        """Return the branch id and trailing path for 'location'.
> +
> +        In addition this method returns whether the answer can from the 
cache
> +        or from the database.
> +        """
> +        parts = location[1:].split('/')
> +        prefixes = []
> +        for i in range(1, len(parts) + 1):
> +            prefix = '/'.join(parts[:i])
> +            if prefix in self._cache:
> +                branch_id, inserted_time = self._cache[prefix]
> +                if (self._now() < inserted_time +
> +                    config.codehosting.branch_rewrite_cache_lifetime):
> +                    trailing = location[len(prefix) + 1:]
> +                    return branch_id, trailing, "HIT"
> +            prefixes.append(prefix)
> +        result = self.store.find(
> +            Branch,
> +            Branch.unique_name.is_in(prefixes), Branch.private == False)
> +        try:
> +            branch_id, unique_name = result.values(
> +                Branch.id, Branch.unique_name).next()
> +        except StopIteration:
> +            return None, None, "MISS"

How about:

       result = self.store.find(
           (Branch.id, Branch.unique_name),
           Branch.unique_name.is_in(prefixes), Branch.private == False).one()

That way result should be None if not found, and a tuple of id and unique_name 
if it was found.

That'll make this code a little cleaner.

> === modified file 'lib/lp/codehosting/tests/test_rewrite.py'
> --- lib/lp/codehosting/tests/test_rewrite.py	2009-07-17 00:26:05 +0000
> +++ lib/lp/codehosting/tests/test_rewrite.py	2009-07-22 03:23:28 +0000
>  class TestBranchRewriterScript(TestCaseWithFactory):
>  
> -    layer = ZopelessAppServerLayer
> +    layer = DatabaseFunctionalLayer
>  
>      def test_script(self):

I think a comment here explaining why you have '\n'.join(expected) + '\n'
would be *really* helpful.

I think it has todo with how the rewriting works through apache, but a
comment would help.

> -        branch = self.factory.makeAnyBranch()
> -        input = "/%s/.bzr/README\n" % branch.unique_name
> -        expected = (
> -            "file:///var/tmp/bzrsync/%s/.bzr/README\n"
> -            % branch_id_to_path(branch.id))
> -        self.layer.txn.commit()
> +        branches = [
> +            self.factory.makeProductBranch(),
> +            self.factory.makePersonalBranch(),
> +            self.factory.makePackageBranch()]
> +        input = [
> +            "/%s/.bzr/README" % branch.unique_name
> +            for branch in branches] + [
> +            "/%s/changes" % branch.unique_name
> +            for branch in branches]
> +        expected = [
> +            'file:///var/tmp/bzrsync/%s/.bzr/README'
> +            % branch_id_to_path(branch.id)
> +            for branch in branches] + [
> +            'http://localhost:8080/%s/changes' % branch.unique_name
> +            for branch in branches]
> +        transaction.commit()
>          script_file = os.path.join(
>              config.root, 'scripts', 'branch-rewrite.py')
>          proc = subprocess.Popen(
>              [script_file], stdin=subprocess.PIPE, stdout=subprocess.PIPE,
>              stderr=subprocess.PIPE, bufsize=0)
> -        proc.stdin.write(input)
> -        output = proc.stdout.readline()
> +        proc.stdin.write('\n'.join(input) + '\n')

A comment here too about why the extra carriage return.

> +        output = []
> +        for i in range(len(input)):
> +            output.append(proc.stdout.readline())
>          os.kill(proc.pid, signal.SIGINT)
>          err = proc.stderr.read()
>          # The script produces logging output, but not to stderr.
>          self.assertEqual('', err)
> -        self.assertEqual(expected, output)
> +        self.assertEqual('\n'.join(expected) + '\n', ''.join(output))
>  
>  
>  def test_suite():
> 


I look forward to seeing this tested live :)

Thanks
Tim