On Mon, 29 Oct 2007, Troy Curtis Jr wrote:
> On 10/29/07, Daniel L. Rall <dlr@finemaltcoding.com> wrote:
...
> > > +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 *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!
Great.
It's actually better to avoid initializing local pointers in C, since you're
otherwise suppressing compiler checking (removing a whole class of "use before
initialization" warnings).
...
> > > + }
> > > + 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.
If the code can't be reasonably factored out into a shared routine, then
perhaps just comments cross-linking the redundant code, indicating that
the "other code" needs updating if either piece of code is changed in the
future.
...
> > > 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?
I'd be fine with that; it's along the lines of our path canonicalization
guidelines. It should probably be documented somewhere, though (e.g.
hacking.html?).
> 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 :).
Personally, I'd rather see this completely implemented for a single
sub-command (and corresponding API), as seems to be your goal, before you
start working on other commands. This also really needs some test cases.
--
Daniel Rall
- application/pgp-signature attachment: stored
Received on Mon Oct 29 18:57:08 2007