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

Re: svn commit: r997026 - /subversion/trunk/subversion/mod_dav_svn/repos.c

From: C. Michael Pilato <cmpilato_at_collab.net>
Date: Tue, 09 Nov 2010 15:32:26 -0500

On 11/09/2010 03:00 PM, Stefan Sperling wrote:
> Hi Mike,
>
> I'm reviewing this for 1.6.14. I have two questions (see below).
>
>> * subversion/mod_dav_svn/repos.c
>> (do_walk): Replace a decade-old TODO about checking authorization on
>> a directory's children with, you know, code that checks
>> authorization on a directory's children.
>>
>> Modified:
>> subversion/trunk/subversion/mod_dav_svn/repos.c
>>
>> Modified: subversion/trunk/subversion/mod_dav_svn/repos.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/repos.c?rev=997026&r1=997025&r2=997026&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/mod_dav_svn/repos.c (original)
>> +++ subversion/trunk/subversion/mod_dav_svn/repos.c Tue Sep 14 18:08:59 2010

[...]

>> @@ -4046,7 +4050,16 @@ do_walk(walker_ctx_t *ctx, int depth)
>> /* authorize access to this resource, if applicable */
>> if (params->walk_type & DAV_WALKTYPE_AUTH)
>> {
>> - /* ### how/what to do? */
>> + const char *repos_relpath =
>> + apr_pstrcat(iterpool,
>> + apr_pstrmemdup(iterpool,
>> + ctx->repos_path->data,
>> + ctx->repos_path->len),
>> + key, NULL);
>
> Here, key is const void*. Cast it const char* for clarity?

In the past, we've frowned on all unnecessary casts. I was merely adhering
to a decade's worth of Subversion code tradition. :-)

> Are we guaranteed that repos_path->data always ends with a slash,
> or that key always starts with one?
> Why not use svn_{dirent,uri,relpath}_join() or similar?

"key" is guaranteed to be a single path component (relative).

But outside the diff context is this code:

  if (ctx->repos_path->data[ctx->repos_path->len - 1] != '/')
    svn_stringbuf_appendcstr(ctx->repos_path, "/");

ctx->repos_path is a stringbuf that's constantly being telescoped and
de-telescoped. It's 'data' component cannot be used independently of its
'len' component (if you want the expected results, at least).

-- 
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Received on 2010-11-09 21:33:10 CET

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.