Branko Čibej <brane@xbc.nu> writes:
> I don't see anything wrong with this patch. The idea is definitely sound.
> Needs another pair of eyes, but on the whole I recommend we commit it.
Just gave it another pair of eyes, and basically agree.
I have only a few concerns below. Actually, the same concern three
times. With the changes below, I'd say go ahead and commit it. (I
assume it already passes "make check" for you, of course -- I didn't
build the patch myself, just looked at it.)
Thanks for the patch, Yoshiki!
-Karl
> Index: ./subversion/clients/cmdline/trace-commit.c
> ===================================================================
> --- ./subversion/clients/cmdline/trace-commit.c
> +++ ./subversion/clients/cmdline/trace-commit.c
>
> [...]
>
> static svn_error_t *
> +decrement_dir_ref_count (struct dir_baton *db)
> +{
> + if (db == NULL)
> + return SVN_NO_ERROR;
> +
> + db->ref_count--;
> +
> + /* Check to see if *any* child batons still depend on this
> + directory's pool. */
> + if (db->ref_count == 0)
> + {
> + struct dir_baton *dbparent = db->parent_dir_baton;
> +
> + /* Destroy all memory used by this baton, including the baton
> + itself! */
> + svn_pool_destroy (db->subpool);
> +
> + /* Tell your parent that you're gone. */
> + SVN_ERR (decrement_dir_ref_count (dbparent));
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> open_root (void *edit_baton, svn_revnum_t base_revision, void **root_baton)
> {
> + apr_pool_t *subpool;
> struct edit_baton *eb = edit_baton;
> - struct dir_baton *rb = apr_pcalloc (eb->pool, sizeof (*rb));
> + struct dir_baton *rb;
>
> + subpool = svn_pool_create (eb->pool);;
> + rb = apr_pcalloc (eb->pool, sizeof (*rb));
`rb' should be created in `subpool', not `eb->pool', right? That way
it fulfills the claim made by the comment in the middle of
decrement_dir_ref_count().
(Or one could change the comment in decrement_dir_ref_count(), I
suppose, but it seems cleaner to allocate the batons themselves in
their own subpools.)
> @@ -102,16 +134,21 @@
> svn_revnum_t copyfrom_revision,
> void **child_baton)
> {
> + apr_pool_t *subpool;
> struct dir_baton *parent_d = parent_baton;
> - struct dir_baton *child_d
> - = apr_pcalloc (parent_d->edit_baton->pool, sizeof (*child_d));
> + struct dir_baton *child_d;
>
> + subpool = svn_pool_create (parent_d->subpool);
> + child_d = apr_pcalloc (parent_d->edit_baton->pool, sizeof (*child_d));
Same thing regarding `child_d'.
> @@ -126,15 +163,20 @@
> svn_revnum_t base_revision,
> void **child_baton)
> {
> + apr_pool_t *subpool;
> struct dir_baton *parent_d = parent_baton;
> - struct dir_baton *child_d
> - = apr_pcalloc (parent_d->edit_baton->pool, sizeof (*child_d));
> + struct dir_baton *child_d;
>
> + subpool = svn_pool_create (parent_d->subpool);
> + child_d = apr_pcalloc (parent_d->edit_baton->pool, sizeof (*child_d));
Same.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:36:59 2006