[MERGE][#109143] Support bzr+ssh://host/~/path

[Martin Pool]

2009/3/19 Andrew Bennetts <andrew.bennetts at canonical.com>:
> I pretty surprised.  I thought the largest concern would be that with this
> patch /~/ would stop users accessing files outside their home directory!
> But no-one apart from me has even mentioned it.

It's reasonable to ask why

  bzr branch bzr+ssh://host/repos/foo bzr+ssh://host/~/repo/foo

would need to open two connections.  (I don't know if it gets by with
just one at the moment, but it could.)

> Only bzr-over-ssh has a default working directory provided by the server,
> and it's only because the client gets some say in how the server is started
> that it can take advantage of that (as this patch does).  Once the server is
> started then all the client can do is issue stateless requests, so there's
> no possibility to change a working directory (which would need to affect
> future requests to be meaningful).

So you seem to see it as being that the presence of the ~ near the
start of the URL means, "open a connection that uses the ssh server's
default working directory".  Whereas I see it as being part of the
path.  For everything else, the path is sent as part of the request to
the server, which then interprets it through chroot or whatever.

> For bzr-over-tcp and bzr-over-http the client has no direct say in what
> directories are being served.  The client gets whatever the server is
> configured to give.  That's no different to e.g. plain HTTP.

So suppose we later get a bug asking why ~user doesn't work in
bzr-over-tcp.  We'd then need to change the server to do expanduser
when it maps the paths its given into local urls.  That seems
redundant with doing it at the SSH level.

>> > Oh, I think I see what you have in mind.  Presumably the server internally
>> > generates file:///~mbp/foo when the client asks for ~mbp/foo via the smart
>> > protocol (there's a ChrootTransport between the client and the remote's
>> > LocalTransport, but it doesn't matter to this discussion), so you're
>> > proposing that the LocalTransport should expand file:///~mbp to
>> > file:///home/mbp on the server.
>>
>> Yes, that's what I meant.  This would even be somewhat useful in file urls.
>
> Yeah.  But it might make round-tripping paths through file URLs reliably
> difficult.  And I'd also worry about the potential cost of calling
> os.path.expanduser for every single LocalTransport operation, or the cost of
> the extra complexity we'd need to avoid that overhead.

This is spurious, it can be looked up when the top-level transport is created.

> As far as solving at the correct layer goes, if the server wants control
> over how e.g. ~mbp is interpreted, then we have a whole VFS abstraction in
> bzrlib for that already.  That is, servers can override cmd_serve to use a
> different backing transport than LocalTransport, and that transport can do
> anything they like.  Rewrite paths to be more convenient, expand ~user dirs,
> impose access restrictions...  At some point I'd like to write an example
> plugin demonstrating that approach.  AIUI bazaar.launchpad.net's bzr+ssh
> server works this way.

I don't think overriding cmd_serve is really the best way to do that.
If you want to change the way VFS things are rewritten, you should be
able to do that just by working in the Transport registry.

>> > I suppose that's an option, although changing how we handle file:/// URLs
>> > makes me nervous.  We'd also lose the neat correspondence with sftp:// URLs,
>> > which AIUI don't expand ~mbp, just ~.
>>
>> But we're already going to have a difference between ssh and sftp
>> URLs, which one would otherwise expect to be more similar.  Anyhow it
>> certainly seems possible that an sftp server would handle ~mbp.
>
> Launchpad's sftp server already does... by having all sftp sessions start in
> virtual /, and putting ~user directories in that /.  ;)

That's consistent with what I'm asking for, I think: the paths go over
the wire uninterpreted and the server expands them within itself.

-- 
Martin <http://launchpad.net/~mbp/>