On Thu, 13 Oct 2005 kfogel@collab.net wrote:
> lundblad@tigris.org writes:
> > --- trunk/subversion/include/svn_ra.h (original)
> > +++ trunk/subversion/include/svn_ra.h Thu Oct 13 07:10:27 2005
> > @@ -473,6 +473,16 @@
> > apr_hash_t *config,
> > apr_pool_t *pool);
> >
> > +/** Change the root URL of an open @a ra_session to point to a new path in the
> > + * same repository. @a url is the new root URL. Use @a pool for
> > + * temporary allocations.
> > + *
> > + * @since New in 1.4.
> > + */
> > +svn_error_t *svn_ra_reparent (svn_ra_session_t *ra_session,
> > + const char *url,
> > + apr_pool_t *pool);
> > +
>
> What if the schema portion of the new URL differs from the schema of
> the ra_session's previous URL?
>
> Or, more drastically, what happens if the new URL does not even point
> to the same repository? Is an error returned?
>
SVN_ERR_RA_INVALID_URL. I'll document that.
> Oh, I see. Never mind. drr->url2 might have changed in between these
> calls, due to svn_client__repos_locations().
>
> Well, I could erase my question above, but am showing it to you just
> so you know I cared :-).
>
Good to know, I'll propose adding it to your Karma... It indicates that a
comment here explaining this might be in place.
> > --- trunk/subversion/libsvn_ra/ra_loader.c (original)
> > +++ trunk/subversion/libsvn_ra/ra_loader.c Thu Oct 13 07:10:27 2005
> > @@ -319,6 +320,23 @@
> > return svn_ra_open2 (session_p, repos_URL,
> > callbacks2, callback_baton,
> > config, pool);
> > +}
> > +
> > +svn_error_t *svn_ra_reparent (svn_ra_session_t *session,
> > + const char *url,
> > + apr_pool_t *pool)
> > +{
> > + const char *repos_root;
> > +
> > + /* Make sure the new URL is in the same repository, so that the
> > + implementations don't have to do it. */
> > + SVN_ERR (svn_ra_get_repos_root (session, &repos_root, pool));
> > + if (! svn_path_is_ancestor (repos_root, url))
> > + return svn_error_createf (SVN_ERR_RA_ILLEGAL_URL, NULL,
> > + _("'%s' isn't in the same repository as '%s'"),
> > + url, repos_root);
> > +
> > + return session->vtable->reparent (session, url, pool);
> > }
>
> Okay, I think this answers my schema question :-).
>
> Wasn't this same problem coming up in another context recently, in IRC
> or something? Someone was reporting a bug where the cause of the
> problem was clearly that we use dumb string- or path-compares to
> compare URLs, which can result in false negatives due to schema and/or
> username-in-url differences.
>
I don't think it is a problem in this case. Changing schema or username
might cause side effects that we don't want ot handle here. This function
is *only* a way to change the FS path. The reason it takes a complete URL
is for consistency with other functions. And if it didn't, every caller
would just have to fetch the repository root and strip it anyway.
> We really need URL-comparison
functions.
Yes, in other contexts.
> > > --- trunk/subversion/libsvn_ra/ra_loader.h (original)
> > +++ trunk/subversion/libsvn_ra/ra_loader.h Thu Oct 13 07:10:27 2005
> > @@ -54,6 +54,10 @@
> > void *callback_baton,
> > apr_hash_t *config,
> > apr_pool_t *pool);
> > + /* URL is guaranteed to have what get_repos_root() returns as a prefix. */
> > + svn_error_t *(*reparent) (svn_ra_session_t *session,
> > + const char *url,
> > + apr_pool_t *pool);
>
> s/is guaranteed to/must/, I think?
>
I'm documenting that the caler guarantees it. It is an internal function.
> But also, I'm a little confused about this region of the code in
> general. Some of these vtable functions have tiny little stub doc
> strings, some of them have no doc string at all. (Your commit adds
> another of those tiny stub doc strings.)
>
> Aren't these all interfaces? Shouldn't they have formal doc strings?
> Or if the formal doc strings are the public API doc strings in
> svn_ra.h, then shouldn't all information be there, and not divided
> across two locations?
>
They are internal interfaces called by libsvn_ra. I'm following libsvn_fs
in not repeating docstrings here. So, I'm just documenting the difference
between the public API and this internal function.
> > --- trunk/subversion/libsvn_ra_dav/session.c (original)
> > +++ trunk/subversion/libsvn_ra_dav/session.c Thu Oct 13 07:10:27 2005
> > @@ -585,6 +585,24 @@
> > * call and make this halfway sane. */
> >
> >
> > +/* Parse URL into *URI, doing some sanity checking and initializing the port
> > + to a default value if it wasn't specified in URL. */
> > +static svn_error_t *
> > +parse_url(ne_uri *uri, const char *url)
> > +{
> > + if (ne_uri_parse(url, uri)
> > + || uri->host == NULL || uri->path == NULL || uri->scheme == NULL)
> > + {
> > + ne_uri_free(uri);
> > + return svn_error_create(SVN_ERR_RA_ILLEGAL_URL, NULL,
> > + _("Malformed URL for repository"));
> > + }
> > + if (uri->port == 0)
> > + uri->port = ne_uri_defaultport(uri->scheme);
> > +
> > + return SVN_NO_ERROR;
> > +}
>
> Well, if we've got this, then we ought to be able to have real a
> URI-comparison function! :-)
>
Note that this uses Neon stuff, but we have similar stuff in APR. URL
comparison functions are a litle tricky, because it should probably be
both scheme and platform dependent. For example, on Windows, the
repository root part of a file:// URL should ideally not be case
sensitive, but the FS path part should always be. Anyway, since it isn't
related to this commit, I'm not going to flesh that ot any further here...
> > --- trunk/subversion/libsvn_ra_svn/client.c (original)
> > +++ trunk/subversion/libsvn_ra_svn/client.c Thu Oct 13 07:10:27 2005
> > [...]
>
> Peter, apologies -- I ran out of time before I could complete this
> review, so didn't do the ra_svn portion. Not out of any prejudice
> against ra_svn, it just happened to be at the end of the diff.
>
Karl, you made me really angry and disapointed when skipping that part:-)
ghudson took a look; see another branch of this thread.
Thanks for the review,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Oct 17 11:52:21 2005