On Fri, 2010-11-12, Bert Huijben wrote:
> Is it a path in the form '/trunk/some/path' then svn_relpath_X will assert
> on an invalid relative path.
>
> svn_uri_* is currently the only non-deprecated set of path functions that
> allows this deprecated format.
>
> @julianf: This is why svn_uri handles urls and other paths differently.
Bert, thanks.
I think it's pretty important that we at least do something to stop
spreading the inappropriate use of svn_uri_* for paths that start with
'/'.
See my proposal below.
> We currently have:
> * dirent: local paths (architecture specific)
> * relpath: Only paths in the form 'some' or 'some/subdir' are valid. '/qq'
> and 'some/./dir' are not.
> * uri: All other paths.
>
> Uri should actually be '<schema>://<schema-base>/<path>', but when we
> started working on these apis we deprecated svn_path and had only dirent and
> uri functions to replace them.
>
> I think we should:
> * either introduce a svn_fspath_*() set of functions for '/style/paths'.
> * or deprecate the entire '/style/path' notation in our internal handling.
> (We can replace them with relpaths by just removing the initial '/')
Switching to a dedicated API for these '/style/paths' seems to be the
'safer' option. I can see no potential problems with it. The steps to
get there are of the purely mechanical kind, so the implementation and
testing effort is not completely unpredictable. It leads us closer to
being able to change the path style in future, because the correct
subset of svn_uri_* calls would already have been identified.
Changing to the relpath style also looks attractive. It is a less
mechanical change: as well as the svn_uri_* calls there will be other
places where a leading '/' is coded for. After this change, the
distinction between a root-relative path and a path relative to
something else would be maintained purely by human communication (such
as the naming of variables) rather than by the leading '/' on the data.
It would be easier to confuse them, a kind of error that is harder to
debug. That is normal practice and not a problem in itself, just a
slightly greater risk in the conversion effort. And this kind of change
is less easily reversible, if for any reason we were to change our
minds.
In either case, we have to find all the places where we're currently
calling svn_uri_* on repository paths, and change them.
I recommend we change the API now and don't change the path format now.
We can still change the path format later.
> (An temporary third option would be to introduce a svn_fspath__ internal set
> of functions, to keep svn_uri_ clean)
Proposal for switching to a dedicated "fs path" API:
Step 1: Introduce a new API for FS paths, as a thin layer that maps to
existing path APIs. It must assert that the fspath arguments and fspath
return values begin with '/'. Initially it should map directly to
svn_uri_*, so that it can be seen to be a direct replacement.
Then any new code can start using the new API straight away.
Step 2: Switch over all the existing svn_uri_* calls that handle
fs-paths to use the new API, one at a time or in bunches.
Step 3: Re-implement the new API to use svn_relpath_* while stripping
and re-adding the leading '/'. Optionally, write dedicated
implementations in cases where the wrapper approach seems expensive.
For extra confidence, log the result of every call and check that the
log produced by a test suite run is the same before and after the
change.
Step 4: In svn_uri_*, assert that the URI does *not* begin with '/'.
(Should we go further and assert that it begins with a scheme?)
Bikeshed: svn_fspath__* or svn_fs__path_* or something else?
- Julian
Received on 2010-11-12 12:23:26 CET