I've been looking at a delete bug: delete can remove a directory NODES
row and leave rows for children, thus creating an invalid metadata
state. The code is structured as
svn_wc_delete4(path):
various checks on path
for each child of path:
various checks on child
svn_wc_delete4(child)
svn_wc__db_temp_op_delete(path)
notify
cleanup
The recursion happens in svn_wc_delete4 while svn_wc__db_temp_op_delete
is where the database is modifed (non-recursively). I can fix the bug
by modify the checks in svn_wc_delete4 but I think this is the wrong way
to do it. I wrote svn_wc__db_temp_op_delete and the _temp_ part is
simply because I didn't really know how it should work.
Our metadata API should be responsible for maintaining the implied
database invariants. Thus the svn_wc__db_op_delete needs to do the
checks to ensure that children don't become orphans. It either needs to
reject such a change, or it needs to be recursive and remove the
children.
Making the checks in svn_wc__db_op_delete also solves a race. At
present the checks made by svn_wc_delete4 are only protected by a
wclock, putting them in svn_wc__db_op_delete means they can go inside
the same SQLite transaction that modifies the database.
Once svn_wc__db_op_delete is making the checks there is very little
point in svn_wc_delete4 making the same checks, and a good performance
reason for only making the checks once. It's probably more efficient to
move the recursion into svn_wc__db_op_delete as well and doing so also
solves the problem that currently op_depth needs to be rewritten as each
parent gets deleted.
That all seems to be good until we come to notification and workqueues.
svn_wc__db_op_delete can't really do notification as it would be making
callbacks from within an SQLite transaction. So svn_wc_delete4 must
continue to notify, and as it can't reliably identify which paths were
deleted svn_wc__db_op_delete must return a list of affected paths.
Delete doesn't currently use a workqueue. It could, and perhaps it
should, although I'm not sure it's strictly necessary. If it were to
use a workqueue then the function that modifies the metadata,
svn_wc__db_op_delete, should install any wq items (one per path or one
per tree).
So the delete code ends up looking like
svn_wc_delete4(path):
deleted = svn_wc__db_op_delete(path)
for name in deleted:
notify(name)
run workqueue
cleanup
Does that sound sensible?
Finally, while this mail refers to delete there are similar problems in
other functions: revert, copy, etc.
--
Philip
