On Wed, Nov 10, 2010 at 5:53 AM, Julian Foad <julian.foad_at_wandisco.com> wrote:
> C. Michael Pilato wrote:
>> On 11/09/2010 03:00 PM, Stefan Sperling wrote:
>> >> @@ -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. :-)
>
> Avoid casts. An old traditional style is
>
> void *key;
> const char *propname /* or whatever the key represents */;
>
> apr_hash_this(hi, &key, ...);
> propname = key;
>
> and a neater, more recent option is
>
> const char *propname = svn_apr_hash_index_key(hi);
Minor nit: it's actually svn__apr_hash_index_key(hi) (note the double
underscore).
-Hyrum
>
>
> - Julian
>
>
>> > 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).
>>
>
>
>
Received on 2010-11-10 13:58:55 CET