You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@newton.ch.collab.net> on 2003/04/29 20:19:41 UTC
Re: svn commit: rev 5760 - trunk/subversion/libsvn_ra_dav
sussman@tigris.org writes:
> +/* Shared helper func: given a public URL which may not exist in HEAD,
> + search up parent directories until we can retrieve a *RSRC
> + containing a standard set of "starting" props: {VCC, resourcetype,
> + baseline-relative-path}. Also return *MISSING_PATH, which is the
> + trailing portion of the URL that did not exist. */
> +static svn_error_t * search_for_starting_props(svn_ra_dav_resource_t **rsrc,
> + const char **missing_path,
> + ne_session *sess,
> + const char *url,
> + apr_pool_t *pool)
Is conventional not to bother to document SESS in these sorts of
functions? (That's not meant to sound sarcastic, I just noticed that
a parameter isn't doc'd and thought I'd ask if it was deliberate.)
You don't say whether *MISSING_PATH's storage is shared with URL, or
whether it is copied into POOL. I could see it going either way, just
looking at the interface of the function (unless I really think about
it, in which case I realize the 'const'-ness of URL makes that
difficult, though not impossible :-) ).
Presumably that *RSRC gets allocated in POOL, not in "SESS's pool" or
anything odd like that? Might want to state so explicitly.
> +{
> + svn_error_t *err;
> + apr_size_t len;
> + svn_stringbuf_t *path_s;
> + ne_uri parsed_url;
> + const char *lopped_path = "";
> +
> + /* Split the url into it's component pieces (schema, host, path,
> + etc). We want the path part. */
> + ne_uri_parse (url, &parsed_url);
> +
> + path_s = svn_stringbuf_create (parsed_url.path, pool);
> +
> + /* Try to get the starting_props from the public url. If the
> + resource no longer exists in HEAD, we'll get a failure. That's
> + fine: just keep removing components and trying to get the
> + starting_props from parent directories. */
> +
> + while (! svn_path_is_empty (path_s->data))
> + {
> + err = svn_ra_dav__get_starting_props(rsrc, sess, path_s->data,
> + NULL, pool);
> + if (! err)
> + break; /* found an existing parent! */
> +
> + if (err->apr_err != SVN_ERR_RA_DAV_REQUEST_FAILED)
> + return err; /* found a _real_ error */
> +
> + /* else... lop off the basename and try again. */
> + lopped_path = svn_path_join(svn_path_basename (path_s->data, pool),
> + lopped_path,
> + pool);
> + len = path_s->len;
> + svn_path_remove_component(path_s);
> + if (path_s->len == len)
> + /* whoa, infinite loop, get out. */
> + return svn_error_quick_wrap(err,
> + "The path was not part of a repository");
> +
> + svn_error_clear (err);
> + }
Hmmm. I was about to say something about how much that could be
optimized, but then you and Mike pointed that each iteration of the
loop *contacts a freakin' DAV server*. Given that, saving a few
cycles or bytes here probably doesn't matter so much :-).
> + *missing_path = lopped_path;
> + return SVN_NO_ERROR;
> +}
Doc string might want to promise that *MISSING_PATH isn't touched if
any error is returned (or, if that promise isn't important, then you
don't need the lopped_path variable at all I suppose, you can just use
*missing_path itself).
> +svn_error_t *svn_ra_dav__get_vcc(const char **vcc,
> + ne_session *sess,
> + const char *url,
> + apr_pool_t *pool)
> +{
> + svn_ra_dav_resource_t *rsrc;
> + const char *lopped_path;
> + const svn_string_t *vcc_s;
> +
> + /* ### Someday, possibly look for memory-cached VCC in the RA session. */
> +
> + /* ### Someday, possibly look for disk-cached VCC via get_wcprop callback. */
> +
> + /* Finally, resort to a set of PROPFINDs up parent directories. */
> + SVN_ERR( search_for_starting_props(&rsrc, &lopped_path,
> + sess, url, pool) );
Isn't it kind of odd for the first call in a function to start with a
comment saying "Finally, ..." :-) ?
> +/* Fetch the repository's unique Version-Controlled-Configuration url.
> +
> + Given a Neon session SESS and a URL, set *VCC to the url of the
> + repository's version-controlled-configuration resource.
> + */
> +svn_error_t *svn_ra_dav__get_vcc(const char **vcc,
> + ne_session *sess,
> + const char *url,
> + apr_pool_t *pool);
>
> /* Issue a PROPPATCH request on URL, transmitting PROP_CHANGES (a hash
> of const svn_string_t * values keyed on Subversion user-visible
Same doc comments about POOL here, but I'm beginning to wonder if
there isn't a convention at work that makes them unnecessary (?).
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5760 - trunk/subversion/libsvn_ra_dav
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Ben Collins-Sussman <su...@collab.net> writes:
> There's a general problem here; this function calls a zillion other
> ra_dav functions, *none* of which are documented. I guess I need to
> trace all the way down the call stack and figure out who allocates
> what. :-(
Well -- sure, but should be about 30 seconds w/ tags table?
> > Isn't it kind of odd for the first call in a function to start with a
> > comment saying "Finally, ..." :-) ?
>
> Not if you read the first two comments. :-)
Oh well. I mean, I did, but I guess it's a matter of taste then.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5760 - trunk/subversion/libsvn_ra_dav
Posted by Ben Collins-Sussman <su...@collab.net>.
Karl Fogel <kf...@newton.ch.collab.net> writes:
> sussman@tigris.org writes:
> > +/* Shared helper func: given a public URL which may not exist in HEAD,
> > + search up parent directories until we can retrieve a *RSRC
> > + containing a standard set of "starting" props: {VCC, resourcetype,
> > + baseline-relative-path}. Also return *MISSING_PATH, which is the
> > + trailing portion of the URL that did not exist. */
> > +static svn_error_t * search_for_starting_props(svn_ra_dav_resource_t **rsrc,
> > + const char **missing_path,
> > + ne_session *sess,
> > + const char *url,
> > + apr_pool_t *pool)
>
> Is conventional not to bother to document SESS in these sorts of
> functions? (That's not meant to sound sarcastic, I just noticed that
> a parameter isn't doc'd and thought I'd ask if it was deliberate.)
Nah, just forgot to doc it. I'll fix that.
>
> You don't say whether *MISSING_PATH's storage is shared with URL, or
> whether it is copied into POOL. I could see it going either way, just
> looking at the interface of the function (unless I really think about
> it, in which case I realize the 'const'-ness of URL makes that
> difficult, though not impossible :-) ).
Will fix.
>
> Presumably that *RSRC gets allocated in POOL, not in "SESS's pool" or
> anything odd like that? Might want to state so explicitly.
There's a general problem here; this function calls a zillion other
ra_dav functions, *none* of which are documented. I guess I need to
trace all the way down the call stack and figure out who allocates
what. :-(
>
> > + *missing_path = lopped_path;
> > + return SVN_NO_ERROR;
> > +}
>
> Doc string might want to promise that *MISSING_PATH isn't touched if
> any error is returned (or, if that promise isn't important, then you
> don't need the lopped_path variable at all I suppose, you can just use
> *missing_path itself).
Sure, makes sense.
> > +svn_error_t *svn_ra_dav__get_vcc(const char **vcc,
> > + ne_session *sess,
> > + const char *url,
> > + apr_pool_t *pool)
> > +{
> > + svn_ra_dav_resource_t *rsrc;
> > + const char *lopped_path;
> > + const svn_string_t *vcc_s;
> > +
> > + /* ### Someday, possibly look for memory-cached VCC in the RA session. */
> > +
> > + /* ### Someday, possibly look for disk-cached VCC via get_wcprop callback. */
> > +
> > + /* Finally, resort to a set of PROPFINDs up parent directories. */
> > + SVN_ERR( search_for_starting_props(&rsrc, &lopped_path,
> > + sess, url, pool) );
>
> Isn't it kind of odd for the first call in a function to start with a
> comment saying "Finally, ..." :-) ?
Not if you read the first two comments. :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5760 - trunk/subversion/libsvn_ra_dav
Posted by Michael Wood <mw...@its.uct.ac.za>.
On Tue, Apr 29, 2003 at 03:19:41PM -0500, Karl Fogel wrote:
> sussman@tigris.org writes:
> > +/* Shared helper func: given a public URL which may not exist in HEAD,
> > + search up parent directories until we can retrieve a *RSRC
> > + containing a standard set of "starting" props: {VCC, resourcetype,
> > + baseline-relative-path}. Also return *MISSING_PATH, which is the
> > + trailing portion of the URL that did not exist. */
> > +static svn_error_t * search_for_starting_props(svn_ra_dav_resource_t **rsrc,
> > + const char **missing_path,
> > + ne_session *sess,
> > + const char *url,
> > + apr_pool_t *pool)
>
[snip]
> Presumably that *RSRC gets allocated in POOL, not in "SESS's pool" or
> anything odd like that? Might want to state so explicitly.
[snip]
Cesspool? :) No, I hope not.
--
Michael Wood <mw...@its.uct.ac.za>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org