On 10/29/07, Daniel L. Rall <dlr@finemaltcoding.com> wrote:
> > Index: subversion/include/svn_path.h
> > ===================================================================
> > --- subversion/include/svn_path.h (revision 27293)
> > +++ subversion/include/svn_path.h (working copy)
> > @@ -264,6 +264,14 @@
> > const char *relative,
> > apr_pool_t *pool);
> >
> > +/**
>
> This needs to doc that it can be called on relative URLs, and that in
> that case it returns @a relative_url.
>
> > + */
> > +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);
> > +
> > /** Return the path part of the canonicalized @a path in @a
> > * *pdirectory, and the file part in @a *pfile. If @a path is a
> > * directory, set @a *pdirectory to @a path, and @a *pfile to the
> > @@ -456,6 +464,12 @@
> > /** Return true iff @a path looks like a valid absolute URL. */
> > svn_boolean_t svn_path_is_url(const char *path);
> >
> > +/**
> > + * Return true iff @a url is a relative URL. Specifically that it starts
>
> I prefer "Return whether ..."
>
> > + * with the characters "^/"
> > + */
> > +svn_boolean_t svn_path_is_relative_url(const char *url);
> ...
> > Index: subversion/include/svn_wc.h
> > ===================================================================
> > --- subversion/include/svn_wc.h (revision 27293)
> > +++ subversion/include/svn_wc.h (working copy)
> ...
> +/**
>
> Needs doc.
>
> > + */
> > +svn_error_t *
> > +svn_wc_get_absolute_url(const char **absolute_url,
> > + const char *relative_url,
> > + const char *wc_path,
> > + apr_pool_t *pool);
> ...
> > Index: subversion/libsvn_wc/util.c
> > ===================================================================
> > --- subversion/libsvn_wc/util.c (revision 27293)
> > +++ subversion/libsvn_wc/util.c (working copy)
> > @@ -289,3 +289,43 @@
> >
> > return SVN_NO_ERROR;
> > }
> > +
> > +svn_error_t *
> > +svn_wc_get_absolute_url(const char **absolute_url,
> > + const char *relative_url,
> > + const char *wc_path,
> > + apr_pool_t *pool)
> > +{
> > + const char *canonicalized_path;
>
> You can lose this variable.
>
> > + const char *repos_root;
> > + const svn_wc_entry_t *entry;
> > + svn_wc_adm_access_t *adm_access = NULL;
> > + svn_error_t *err = SVN_NO_ERROR;
>
> You don't need to set err to NULL, since you set it below when calling
> entry_versioned() (to NULL, if no error).
>
Of course there is one school of thought that every pointer should be
initialized to NULL, which is exactly what I typically do in my
non-Subversion code. :) But I took this out anyway!
> > +
> > + canonicalized_path = svn_path_canonicalize(wc_path, pool);
>
> Subversion APIs typically assuem that their inputs are already canonicalized.
>
Ah! So you are right. Gone!
> > +
> > + SVN_ERR(svn_wc_adm_probe_open3(&adm_access, NULL, canonicalized_path,
> > + FALSE, 0, NULL, NULL, pool));
> > +
> > + if ((err = svn_wc__entry_versioned(&entry, canonicalized_path, adm_access,
> > + FALSE, pool)))
> > + goto cleanup;
> > +
> > + repos_root = entry->repos;
> > +
> > + if ((err = svn_path_resolve_relative_url(absolute_url, relative_url,
> > + repos_root, pool)))
> > + goto cleanup;
> > +
> > +cleanup:
> > + if (adm_access)
> > + {
> > + svn_error_t *err2 = svn_wc_adm_close(adm_access);
> > + if(! err)
> ^
> Space-before paren for flow-control.
>
> > + err = err2;
> > + else
> > + svn_error_clear(err2);
> > + }
> > +
> > + return err;
> > +}
> > Index: subversion/libsvn_subr/path.c
> > ===================================================================
> > --- subversion/libsvn_subr/path.c (revision 27293)
> > +++ subversion/libsvn_subr/path.c (working copy)
> ...
> > +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)
> > +{
> > + apr_status_t status;
> > + const char *canonicalized_url;
>
> You can lose this variable.
>
> > + apr_array_header_t *base_components;
> > + apr_array_header_t *relative_components;
> > + apr_uri_t parsed_uri;
> > + int i;
> >
> > + canonicalized_url = svn_path_canonicalize(relative_url, pool);
>
> Subversion APIs typically assuem that their inputs are already canonicalized.
>
> > +
> > + if(svn_path_is_url(canonicalized_url))
> ^
> More of these throughout.
>
> > + {
> > + *absolute_url = canonicalized_url;
> > + return SVN_NO_ERROR;
> > + }
> > +
> > + if(0 == strncmp("^/", relative_url, 2))
> > + {
> > + status = apr_uri_parse(pool, repos_root_url, &parsed_uri);
> > + if (status)
> > + return svn_error_createf(SVN_ERR_BAD_URL, 0,
> > + _("Illegal repository root URL '%s'."),
>
> Drop the period from the error message.
>
Oops, copy-n-paste from resolve_relative_external_url(). :)
> > + repos_root_url);
> > +
> > + base_components = svn_path_decompose(parsed_uri.path,
> > + pool);
> > + relative_components = svn_path_decompose(canonicalized_url + 2,
> > + pool);
> > + }
> > +
> > + for (i = 0; i < relative_components->nelts; ++i)
> > + {
> > + const char *component = APR_ARRAY_IDX(relative_components,
> > + i,
> > + const char *);
> > + if (0 == strcmp("..", component))
> > + {
> > + /* Constructing the final absolute URL together with
> > + * apr_uri_unparse() requires that the path be absolute,
> > + * so only pop a component if the component being popped
> > + * is not the component for the root directory. */
> > + if (base_components->nelts > 1)
> > + apr_array_pop(base_components);
>
> Level of indentation is off.
>
> > + }
> > + else
> > + APR_ARRAY_PUSH(base_components, const char *) = component;
> > + }
> > +
> > + parsed_uri.path = (char *)svn_path_compose(base_components,
> > + pool);
> > + parsed_uri.query = NULL;
> > + parsed_uri.fragment = NULL;
> > +
> > + *absolute_url = apr_uri_unparse(pool, &parsed_uri, 0);
>
> Is this the bit of code that should be shared with blair's externals work?
>
This issue here is that I can't support things exactly like blair's
work. He supports "//" and "../" along with "^/" in the externals
URL. There isn't a way to disambiguate that second case between a URL
and path in th command-line client.
> > +
> > + return SVN_NO_ERROR;
> > +}
> > +
> > +
> ...
> > Index: subversion/libsvn_client/info.c
> > ===================================================================
> > --- subversion/libsvn_client/info.c (revision 27293)
> > +++ subversion/libsvn_client/info.c (working copy)
> > @@ -356,7 +356,15 @@
> > svn_info_t *info;
> > svn_error_t *err;
> >
> > - if ((revision == NULL
> > + if (svn_path_is_relative_url(path_or_url))
> > + {
> > + const char *abs_url;
> > + SVN_ERR(svn_wc_get_absolute_url(&abs_url, path_or_url, ".", pool));
> > +
> > + /* Replace the relative URL with an absolute one. */
> > + path_or_url = abs_url;
> > + }
> > + else if ((revision == NULL
> > || revision->kind == svn_opt_revision_unspecified)
> > && (peg_revision == NULL
> > || peg_revision->kind == svn_opt_revision_unspecified))
>
> This API should document that it accepts relative URLs.
Is this something I should do? I am working on implementing this on
every sub-command that accepts URLs (though not every function that
accepts URLs). It isn't as though each one is already explicitly
documented as accepting svn://, http://, svn+ssh://. I was thinking
of the relative URL syntax as just another way to specify a URL in
subversion, but maybe this is wrong since I am only implementing them
in the client and command-line APIs?
> --
>
> Daniel Rall
>
>
Thanks so much for your comments and time! It won't be long now
before I have a larger patch for you to pick through :).
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 Mon Oct 29 12:41:28 2007