loggerhead

Merge lp:~spiv/loggerhead/shared-repo-fix into lp:loggerhead

  1. shared-repo-fix
  2. Merge into trunk-rich
Proposed by Andrew Bennetts
Status: Merged
Merged at revision: not available
Proposed branch: lp:~spiv/loggerhead/shared-repo-fix
Merge into: lp:loggerhead
Diff against target: 47 lines
1 file modified
loggerhead/apps/transport.py (+7/-2)
To merge this branch: bzr merge lp:~spiv/loggerhead/shared-repo-fix
Reviewer Review Type Date Requested Status
Martin Albisetti Approve
Matt Nordhoff Approve
Review via email: mp+13701@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :

This fix, combined with <https://code.launchpad.net/~spiv/bzr/bzr-http-jailbreak-bug-348308/+merge/13698>, fixes HPSS access for branches in a shared repo. I tested this via "serve-branches path/to/a/shared-repo" and then doing "bzr revno http://localhost:8080/branch-in-that-repo".

Hopefully the code is reasonably self-explanatory. Failing that it's at least simple ;)

Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

I can confirm that this fixes the bug, but I don't actually know the wsgi code, so I'm loath to merge it without input from someone else.

Questions:

1.) Why add loggerhead.path_info? You can just pass 'PATH_INFO' to RelpathSetter, no?

2.) It's interesting that the old code didn't use get_transport_for_thread(), but the new code does. Is it unnecessary, or was the old code broken? (ISTM the change is correct, but I don't want to page threading-related memories into my brain. :P )

review: Approve
Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

Oh, I meant to add:

3.) The new code uses both "wsgi_app" and "app" as variables. I think standardizing on one looks better. I don't care which.

I also meant to say that this _does_ look correct to me, but like I said, my knowledge of "correct" is limited.

Revision history for this message
Andrew Bennetts (spiv) wrote :

Matt Nordhoff wrote:
> Review: Approve
> I can confirm that this fixes the bug, but I don't actually know the wsgi code, so I'm loath to merge it without input from someone else.
>
> Questions:
>
> 1.) Why add loggerhead.path_info? You can just pass 'PATH_INFO' to RelpathSetter, no?

path_info_pop mutates PATH_INFO. So I need to capture it before it is modified.

> 2.) It's interesting that the old code didn't use get_transport_for_thread(),
> but the new code does. Is it unnecessary, or was the old code broken? (ISTM
> the change is correct, but I don't want to page threading-related memories
> into my brain. :P )

The old code was using an existing transport object. My code wants the
transport object corresponding to the root of the server... self.root doesn't
have a transport object, so I do what self.root's implementation does to turn
self.root.base into a transport. I'm not sure exactly what the requirements
here are, but using get_transport_for_thread here seems reasonable on the face
of it and it's consistent with how the previous code was creating transport
objects.

> 3.) The new code uses both "wsgi_app" and "app" as variables. I think standardizing on one looks better. I don't care which.

Fair enough. I don't care either ;)

That was actually a debug dropping, I had a "import pdb; pdb.set_trace()" in
there at some point. Fixed now to simply return the expression, as before.

> I also meant to say that this _does_ look correct to me, but like I said, my knowledge of "correct" is limited.

Hmm, actually, I suspect this breaks UserBranchesFromTransportRoot, because it
isn't setting the new loggerhead.path_info var. Fixed now.

-Andrew.

lp:~spiv/loggerhead/shared-repo-fix updated
391. By Andrew Bennetts

Tweaks prompted by review.

Revision history for this message
Martin Albisetti (beuno) wrote :

Thanks Andrew, can verify it fixes the problem, all tests pass, and I can't see anything else broken.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'loggerhead/apps/transport.py'
2--- loggerhead/apps/transport.py 2009-07-07 23:52:24 +0000
3+++ loggerhead/apps/transport.py 2009-10-21 13:08:09 +0000
4@@ -20,6 +20,7 @@
5
6 from bzrlib import branch, errors, lru_cache, urlutils
7 from bzrlib.config import LocationConfig
8+from bzrlib.smart import request
9 from bzrlib.transport import get_transport
10 from bzrlib.transport.http import wsgi
11
12@@ -82,8 +83,9 @@
13
14 def app_for_bazaar_data(self, relpath):
15 if relpath == '/.bzr/smart':
16- wsgi_app = wsgi.SmartWSGIApp(self.transport)
17- return wsgi.RelpathSetter(wsgi_app, '', 'PATH_INFO')
18+ root_transport = get_transport_for_thread(self.root.base)
19+ wsgi_app = wsgi.SmartWSGIApp(root_transport)
20+ return wsgi.RelpathSetter(wsgi_app, '', 'loggerhead.path_info')
21 else:
22 # TODO: Use something here that uses the transport API
23 # rather than relying on the local filesystem API.
24@@ -134,6 +136,7 @@
25 if base in thread_transports:
26 return thread_transports[base]
27 transport = get_transport(base)
28+ thread_transports[base] = transport
29 return transport
30
31
32@@ -146,6 +149,7 @@
33
34 def __call__(self, environ, start_response):
35 environ['loggerhead.static.url'] = environ['SCRIPT_NAME']
36+ environ['loggerhead.path_info'] = environ['PATH_INFO']
37 if environ['PATH_INFO'].startswith('/static/'):
38 segment = path_info_pop(environ)
39 assert segment == 'static'
40@@ -168,6 +172,7 @@
41
42 def __call__(self, environ, start_response):
43 environ['loggerhead.static.url'] = environ['SCRIPT_NAME']
44+ environ['loggerhead.path_info'] = environ['PATH_INFO']
45 path_info = environ['PATH_INFO']
46 if path_info.startswith('/static/'):
47 segment = path_info_pop(environ)

Subscribers

People subscribed via source and target branches