On Fri, 2009-12-11 at 10:27 +0000, Philip Martin wrote:
> Julian Foad <julianf...@btopenworld.com> writes:
>
> >> > + if (strcmp(svn_fs_base__id_txn_id(old_id), old_txn_id) != 0)
> >> > + {
> >> > + *new_id = old_id;
> >>
> >> Should this dup old_id into result pool?
> >
> > Ah, yes, looks like it should. Thanks. Will check and fix.
>
> Looks like there is a similar problem further on in the same function:
>
> /* Make a deep copy of the child node-rev. */
> SVN_ERR(svn_fs_base__node_rev_dup(&new_child_id, child_entry->id,
> new_txn_id, old_txn_id, trail,
> scratch_pool, scratch_pool));
>
> Should that be pasing result_pool as the second pool?
No, because the only pool-allocated results of the function are the new
child node id (*new_child_id), and that is going to be written to disk
before the function returns.
> /* Make the (new) parent node's rep refer to this new child. */
> SVN_ERR(svn_fs_base__dag_set_entry(parent_dag_node, child_name,
> new_child_id, new_txn_id,
> trail, scratch_pool));
>
> Should that pass result_pool rather than scratch pool?
Ditto. This function saves data into the database, not into the pool.
Any ids stored in the pool are only references into the DB.
In r889581 I have re-jigged the pool handling in these functions, using
TRAIL->pool as the result pool in both functions and passing a separate
scratch_pool. (I hope that makes sense.)
I also added some comments on pool usage and other aspects of the
implementation.
- Julian
