Ben Reser wrote on Sun, Mar 31, 2013 at 00:24:56 -0700:
> On Fri, Mar 29, 2013 at 1:17 PM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
> > Please note especially the section labeled "Reviewed But Need Further
> > Attention". In it, I call out some of the stuff about which I couldn't come
> > to clear and obvious conclusion/solution/etc. Please take a moment to
> > review that section, and to follow up with an appropriate action on any API
> > items which may have been the result of your own work or involve an area of
> > the codebase with which you are suitably familiar.
>
> This isn't in your reviewed but need further attention bucket but I'd
> put it there:
>
> svn_repos_authz_read2 takes "repos relative url" but such paths are
> not canonical. In the client library we don't accept these and our
> command line client converts these into absolute canonical URLs before
> passing them into the client library. When I originally set out to
> write this code I didn't really consider this. After I wrote it I
> refactored it a couple of times since only handling OS filesystem
> paths and URLs means you end up converting a repos relative path to a
> URL only to split it back up again since to open the repo you need to
> do so.
>
> However, not doing it this way means this function ends up being weird
> in that it expects path and groups_path to be canonical most of the
> time but not when it's a repos relative URL.
>
> The alternative is to take the approach we took with the client
> library and force the servers to convert to a absolute URL.
>
> While this is less efficient, after wrestling with this idea I think
> it ends up being a better API.
>
> What do other people think? It wouldn't take long to change this, but
> if we don't like it later it's going to be a pain to change due to
> compatibility guarantees.
How about forbidding PATH to be a repos-relative URL, but permitting it
to be a relpath, that is then interpreted relative to the repos root.
(I would say permit it to be an fspath, which is by definition absolute,
but fspaths aren't part of the public API.)
The conversion is trivial, just +2 on the argument, but it avoids
"non-canonical" and "^/ URLs" in the repos API.
Received on 2013-03-31 14:59:13 CEST