On Nov 15, 2007 5:38 PM, Julian Foad <julianfoad@btopenworld.com> wrote:
> 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.
>
>
Certainly not, I am rather known for verbosity myself...perhaps it also shows
up in my code? :)
> > * 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.
It's a feature! The reasoning was that in the my intial discussions for this
people wanted this functions in the API interfaces and not just the
command-line client. Someone did say that changes to the API behavior are
more permanent/harder to back out than with the commandline and so we should
support it there first. So it is with an eye to the future that I put them at
this level. Plus, API clients can tap into these functions themselves like
svn while they wait for the svn_client_* functions to support it.
>
> 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.
I thought these functions would be valid as there are many other functions
that operate on arrays and you will find in the more complete patch I am about
to submit that those are the functions that get used in all but a couple of
cases. If anything the array functions should be THE functions...except for
those few cases. And even then they could be re-written to use the array
functions.
>
> 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.
>
Ah, that would be nice wouldn't it? This is exactly where I wanted to stick
it, everything calls this function. Problem solved. However, like David said
I can't put svn_wc_* functions in there because of circular library
dependencies. And I can't impose requiring a repository root url from the
client for *possibly* relative urls.
>
>
> 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.
>
Yeah I like that wording a bit better (minus the command-line client
reference).
> > + * 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.
>
I put it in there because the externals support was there and that's where I
copied and pasted it from. I just thought, "Why not?" And in fact really the
only use that I could think of was referring to a different repository. It's
easy to support because it's just a url shortcut. What do other people think?
>
> > + * 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.
>
I thought someone would comment on that. I used the NULL relative_url
implementation on ALMOST all of my subcommand modifications, but thought it
made sense to support both. Maybe not, what do other people think? It seemed
sort of GNU libc-ish so I thought it's be ok.
> > + */
> > +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.
>
Is six too many? I suppose a couple of them are simply convenience functions,
other people have comments?
> Thanks.
>
> - Julian
>
Troy
--
Beware of spyware. If you can, use the Firefox browser. - USA Today
Download now at http://getfirefox.com
Registered Linux User #354814 ( http://counter.li.org/)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Nov 16 06:00:24 2007