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

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 ;)

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 )

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.

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.

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