Tim Penhey wrote:
> Review: Needs Information
>  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?

It means that logger.info('%f', 0.1) doesn't break.

> This to me looks like it'll brake things.

I don't think it will.  ec2test will confirm :)

>> === 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.

Ah, yes, thanks for the tip.

>> === 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.

I've tried to make this make more sense -- have a look at the attached diff.

>> -        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.

Subsumed by the above effort, I hope.

>> +        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 :)

So do I!

Cheers,
mwh