Code review comment for lp:~spiv/loggerhead/shared-repo-fix

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.