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

Re: svn commit: r36203 - trunk/subversion/libsvn_ra_serf

From: Ivan Zhakov <ivan_at_visualsvn.com>
Date: Sat, 28 Feb 2009 12:09:47 +0300

On Sat, Feb 28, 2009 at 2:34 AM, Bert Huijben <bert_at_vmoo.com> wrote:
>> >       ctx->resource_url = svn_uri_canonicalize(uri.path, ctx->pool);
>> > +
>> > +      if (ctx->resource_url == uri.path)
>> > +        ctx->resource_url = apr_pstrdup(ctx->pool, ctx-
>> >resource_url);
>> Hi Bert,
>> Sorry but I don't like this fix. It looks like premature optimization
>> which is always bad thing.
>> We should follow contract of svn_uri_canonicalize function that could
>> theoretically return pointer to other than first character in provided
>> buffer.
>> So it's much better  just add apr_pstrdup() in all cases from point of
>> readability and preventing memory errors.
> Feel free to fix this yourself after you spend a few days profiling. (You can
> also fix the about a dozen of other locations in our code that use the same
> construct). We try to do most memory consuming operations in scratch
> pools and this context pool is certainly not short lived.
> I'd personally suggest fixing the documentation of the svn_(uri/dirent)_canonicalize
> function to say that it will always copy or return a static value (for ""). The current
> canonicalization rules (including case fixing) make it expensive not to copy while
> canonicalizing.
> The optimization of our code should be in reducing these expensive canonicalization
> calls and just assuming everything passed is canonical, not in doing the operation
> twice to reduce a copy.
Hi Bert, it seems you slightly misunderstand me. I've just noticed
that code you wrote does not follow svn_uri_canonicalize() contract.
After your commit r36204 everything is great. And now you can remove
code to apr_pstrdup() since result will be guaranteed to be allocated
in ctx->pool or statically allocated :
      if (ctx->resource_url == uri.path)
        ctx->resource_url = apr_pstrdup(ctx->pool, ctx-resource_url);

Btw Greg pointed that uri should be canonicalized when passed to RA layer.

Ivan Zhakov
VisualSVN Team
Received on 2009-02-28 10:10:03 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.