Troy Curtis Jr wrote:
> [[[
> Initial support for repository root relative URLs in the svn command line
> client. This includes all the functions for detecting and resolving relative
> URLs and full support of relative urls in the merge subcommand of the svn
> client.
Hi Troy. A good start. In fact, in your enthusiasm, you've written more code
than necessary - see below. Don't be put off by my rather long comments.
> * subversion/include/svn_path.h,
> subversion/libsvn_subr/path.c
> (svn_path_is_relative_url): New function.
> (svn_path_array_has_relative_url): New function.
> (svn_path_resolve_relative_url): New function.
> (svn_path_resolve_relative_url_array): New function.
>
> * subversion/include/svn_wc.h,
> subversion/libsvn_wc/util.c
> (svn_wc_get_absolute_url): New function.
> (svn_wc_get_absolute_url_array): New function.
I think you've defined too many public functions at low levels. Let's examine
what concepts we have at what levels.
Command-line client "svn":
Command-line syntax "^/..." and its meaning of "relative to repository root
of the current working directory". The recognition of this syntax must be very
close to the user input layer of the program: for instance, it should come
before canonicalising the path (which removes leading "./" elements among other
things). Nothing below the command-line client needs to know this syntax even
exists.
Relative Externals:
Syntax including "^/...", and its meaning of "relative to repository root of
the directory it appears in".
Global / low level utility:
Handling of pure URLs, absolute and relative. Some functions already
available, like svn_path_join("http://svn.net", "repos").
Note that the command-line syntax is the same as the relative externals syntax
only for convenience, not fundamentally. Think: if we were forced to change the
command-line syntax, say for Windows compatibility, would we be forced to
change the Externals syntax to match? No, we wouldn't. That means the client
code should use command-line-specific or externals-specific APIs as appropriate
to what it's doing, though the implementations of those functions can have any
common code factored out.
Basically you need the following functions to start with, private to the
command-line client:
* Check whether a given argument is in relative-URL syntax.
* Convert an argument in relative-URL syntax to an absolute URL relative to
the current working directory.
Then you can add your array versions of the functions too, if you wish, for
convenience of the existing callers. When they were proposed as public APIs, I
would have objected to the addition of "array" versions on the grounds of too
much code and tests and overlap of functionality, and too specific to what the
existing callers want, but it's OK if they're private.
It looks to me like all of this functionality belongs inside
svn_opt_args_to_target_array2(). Any reason why not? Does anything outside that
need to know? That's the appropriate level, where raw user input is converted
to standard forms.
For the record, here are the comments I'd make on the main function if it's to
be in a public API:
> Index: subversion/include/svn_path.h
> ===================================================================
[...]
> +/**
> + * Convert the possibly relative url in @a relative_url to an absolute
> + * url using @a repos_root_url and stick it in @a absolute_url. If the
> + * @a relative_url starts with the characters '^/', they are replaced by
> + * the repository root url, otherwise the @a relative_url is returned.
Is that strictly accurate: wouldn't that end up missing a slash between the two
parts? Better not to go into that detail here. Maybe something like:
* If @a relative_url is in the command-line client's relative URL syntax,
* set @a *absolute_url to the absolute URL it represents relative
* to @a repos_root_url, else set @a *absolute_url to @a relative_url.
> + * Backpaths ("../") within the URL are also supported.
Within relative_url, I assume? I'm not sure what you really want here. If
relative_url can back-track past repos_root_url, it means referring to a
different repository which is not something I think we should be supporting at
first. If it can't, then what's the point of allowing it at all?
Maybe you were thinking of sharing this with Relative Externals, which would
explain that, but don't. Would this be OK:
* @a relative_url need not be in canonical format. It may contain ".."
* elements but no such element may refer to a path above its root.
> + * If @a relative_url is NULL, then the current value of the dereferenced
> + * @a absolute_url is used as the relative URL.
No, too many behaviour variations - see how it doubled the size of some of your
tests. Choose only one method or the other of passing in the relative URL, and
let the caller deal with it.
> + */
> +svn_error_t *
> +svn_path_resolve_relative_url(const char **absolute_url,
> + const char *relative_url,
> + const char *repos_root_url,
> + apr_pool_t *pool);
> +
[...]
I've not reviewed the implementation, but if you can revise your patch to put
this all inside svn_opt_args_to_target_array2() or at least to have a smaller
set of API functions private to svn_cl, it will look in much better shape.
Thanks.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Nov 16 00:38:42 2007