Philip Martin <philip@codematters.co.uk> writes:
> You are correct. It turns out that the server fixes (r9033/r9044)
> already solve the problem for ra_svn, and ra_local was never
> affected. That only leaves ra_dav, how about this
>
> Index: subversion/libsvn_ra_dav/fetch.c
> ===================================================================
> --- subversion/libsvn_ra_dav/fetch.c (revision 9049)
> +++ subversion/libsvn_ra_dav/fetch.c (working copy)
> @@ -1788,7 +1788,7 @@
> svn_stringbuf_set(rb->namestr, name);
>
> parent_dir = &TOP_DIR(rb);
> - subpool = parent_dir->pool;
> + subpool = svn_pool_create (parent_dir->pool);
>
> pathbuf = svn_stringbuf_dup(parent_dir->pathbuf, subpool);
> svn_path_add_component(pathbuf, rb->namestr->data);
> @@ -1797,6 +1797,7 @@
> SVN_INVALID_REVNUM,
> TOP_DIR(rb).baton,
> subpool) );
> + svn_pool_destroy (subpool);
> break;
>
> default:
All of a sudden, I feel weird about this change.
libsvn_ra_dav/fetch.c:start_element() passes different pools to
different editor functions. Sometimes it uses rb->ras->pool,
sometimes it uses parent_dir->pool. And in some case, it creates a
subpool of parent_dir->pool and 'push_dir()'s that subpool onto the
directory stack.
(It's confusing enough that in the current code, 'subpool' is a
karmically inoperative intermediary in the delete_entry case. Why
aren't we just passing parent_dir->pool directly? Well, maybe I just
wanted an excuse to use the phrase "karmically inoperative".)
Anyway, now here we are, about to use a subpool in a totally new way
in this function: it will be passed to an editor function (not in a
loop), then be immediately destroyed. Anyone looking at that would
wonder, what does this caller know about the editor function's
internal implementation that makes a subpool desirable? And *why*
does the caller know it?
In other words, neither subpool-in-caller nor subpool-in-callee is in
harmony with our usual subpool practice, in this case. The problem is
that the "loop" we're in is an XML parsing loop, and there isn't any
way to get the parser to spin off a subpool for each element handler
invocation, and destroy that subpool at the right time. The pools are
all in our batons.
Oh, and even if we *could* get a dedicated subpool passed into the
function, we wouldn't always want to use it, because we need certain
objects created in the function call to live longer than the call...
Okay, that's a lot of thinking out loud. Gist of it is: a comment at
the subpool creation explaining why we're doing things this way might
help reduce the confusion for future visitors.
Your call. I think the underlying issue here is that some pool usage
ugliness is forced on us by the fact that our code is sometimes driven
by third-party libraries.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Mar 16 00:29:23 2004