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?
> --- trunk/subversion/libsvn_client/diff.c (original)
> +++ trunk/subversion/libsvn_client/diff.c Thu Oct 13 07:10:27 2005
> @@ -1535,6 +1535,9 @@
>
> /* Target based on url2 */
> const char *target2;
> +
> + /* RA session pointing at anchor1. */
> + svn_ra_session_t *ra_session;
> };
>
> /** Helper function: prepare a repos repos diff. Fills DRR
> @@ -1545,12 +1548,9 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> - svn_ra_session_t *ra_session1, *ra_session2;
> + svn_ra_session_t *ra_session;
> svn_node_kind_t kind1, kind2;
>
> - /* For RA sessions */
> - apr_pool_t *temppool = svn_pool_create (pool);
> -
> /* Figure out URL1 and URL2. */
> SVN_ERR (convert_to_url (&drr->url1, params->path1, pool));
> SVN_ERR (convert_to_url (&drr->url2, params->path2, pool));
> @@ -1565,6 +1565,10 @@
> if (drr->url2 != params->path2)
> drr->base_path = params->path2;
>
> + SVN_ERR (svn_client__open_ra_session_internal (&ra_session, drr->url2,
> + NULL, NULL, NULL, FALSE,
> + TRUE, ctx, pool));
> +
> /* If we are performing a pegged diff, we need to find out what our
> actual URLs will be. */
> if (params->peg_revision->kind != svn_opt_revision_unspecified)
> @@ -1573,6 +1577,7 @@
>
> SVN_ERR (svn_client__repos_locations (&drr->url1, &start_ignore,
> &drr->url2, &end_ignore,
> + ra_session,
> params->path2,
> params->peg_revision,
> params->revision1,
> @@ -1580,40 +1585,35 @@
> ctx, pool));
> }
>
> - /* Open temporary RA sessions to each URL. */
> - SVN_ERR (svn_client__open_ra_session_internal (&ra_session1, drr->url1,
> - NULL, NULL, NULL, FALSE,
> - TRUE, ctx, temppool));
> - SVN_ERR (svn_client__open_ra_session_internal (&ra_session2, drr->url2,
> - NULL, NULL, NULL, FALSE,
> - TRUE, ctx, temppool));
> -
> - /* Resolve named revisions to real numbers. */
> + /* Resolve revision and get path kind for the second target. */
> + SVN_ERR (svn_ra_reparent (ra_session, drr->url2, pool));
When we open the ra_session, we do so on drr->url2. Then we pass the
ra_session to svn_client__repos_locations(), which does not reparent
the ra_session. Then the next thing we do is reparent on drr->url2.
Does this have any effect? Wasn't ra_session already parented on
drr->url2...
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 :-).
> --- 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.
We really need URL-comparison functions.
> --- 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?
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?
> --- 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! :-)
> --- 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.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Oct 13 19:26:31 2005