[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: rev 5760 - trunk/subversion/libsvn_ra_dav

From: Ben Collins-Sussman <sussman_at_collab.net>
Date: 2003-04-29 23:28:47 CEST

Karl Fogel <kfogel@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
Received on Tue Apr 29 23:30:22 2003

This is an archived mail posted to the Subversion Dev mailing list.