[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: Julian Foad <julian.foad_at_wandisco.com>
Date: Wed, 10 Nov 2010 11:53:25 +0000

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);

- 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 12:54:08 CET

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