On Thu, May 5, 2011 at 12:35 AM, Greg Stein <gst...@gmail.com> wrote: > On Wed, May 4, 2011 at 22:49, Hyrum K Wright <hy...@hyrumwright.org> wrote: >> On Wed, May 4, 2011 at 8:46 PM, <gst...@apache.org> wrote: >>> Author: gstein >>> Date: Thu May 5 01:46:31 2011 >>> New Revision: 1099657 >>> >>> URL: http://svn.apache.org/viewvc?rev=1099657&view=rev >>> Log: >>> Combine the changelist modification notification into the operation >>> itself, so that (in the future) we can make guarantees about dropping the >>> temporary table. Add cancellation support, too. >>> >>> Add a missing clear of the iterpool in db_op_delete. >>> >>> Leave markers for future unification. >>> >>> * subversion/libsvn_wc/wc_db.h: >>> (svn_wc__db_op_set_chnagelist): rename a couple parameters (that >>> differed by a single character) for clarity. add notification and >>> cancellation parameters. >>> (svn_wc__db_changelist_list_notify): remove >>> >>> * subversion/libsvn_wc/wc_db.c: >>> (svn_wc__db_op_set_changelist): combine with ... >>> (svn_wc__db_changelist_list_notify): ... this. leave some comments. >>> adjust a bit of pool usage since we have an iterpool that can be used >>> as a better scratch_pool in the early part of the function. early-exit >>> if there is no NOTIFY_FUNC. fix an implicit 64-bit to 32-bit >>> conversion for the ACTION localvar. add cancellation. >>> (svn_wc__db_op_delete): clear the iterpool, and adjust some localvar >>> initialization to after that call. >>> >>> * subversion/libsvn_wc/adm_ops.c: >>> (add_from_disk, changelist_walker): shift the notification directly into >>> the call to db_op_set_changelist. >>> >>> Modified: >>> subversion/trunk/subversion/libsvn_wc/adm_ops.c >>> subversion/trunk/subversion/libsvn_wc/wc_db.c >>> subversion/trunk/subversion/libsvn_wc/wc_db.h >>> >>>... >>> >>> Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c >>> URL: >>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=1099657&r1=1099656&r2=1099657&view=diff >>> ============================================================================== >>> --- subversion/trunk/subversion/libsvn_wc/wc_db.c (original) >>> +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Thu May 5 01:46:31 2011 >>> @@ -3567,6 +3567,10 @@ svn_wc__db_op_set_changelist(svn_wc__db_ >>>... >>> @@ -3594,32 +3601,31 @@ svn_wc__db_op_set_changelist(svn_wc__db_ >>> NOT_IMPLEMENTED(); >>> } >>> >>> - SVN_ERR(svn_wc__db_with_txn(wcroot, local_relpath, with_triggers, &wtb, >>> - scratch_pool)); >>> + wtb.cb_baton = &scb; >>> >>> - SVN_ERR(flush_entries(wcroot, local_abspath, scratch_pool)); >>> + /* ### fix up the code below: if the callback is invokved, then the >>> + ### 'changelist_list' table may exist. We should ensure it gets >>> dropped >>> + ### before we exit this function. */ >>> >>> - return SVN_NO_ERROR; >>> -} >>> + SVN_ERR(svn_wc__db_with_txn(wcroot, local_relpath, with_triggers, &wtb, >>> + iterpool)); >>> + SVN_ERR(flush_entries(wcroot, local_abspath, iterpool)); >>> >>> + /* ### can we unify this notification logic, in some way, with the >>> + ### similar logic in op_delete? ... I think we probably want a >>> + ### notify_callback that represents the inner loop. the statement >>> + ### selection and binding is probably similar (especially if we >>> + ### remove like_arg, as questioned below). the unification could >>> + ### look similar to db_with_txn or the with_triggers stuff. */ >> >> I agree that it would be nice to unify whatever delayed notification >> stuffs we have in wc_db. I think that ideally, we would shove the >> equivalent of svn_wc_notify_t into the database, and then use that to >> populate the svn_wc_notify_t when doing the actual notifications. (I >> know that svn_wc_notify_t is a beast, though, so maybe this is a >> chance to think designing something a bit more intelligent.) >> >> There would be some complexity, though, in serializing svn_wc_notify_t >> to the DB. We could either make the temp table mirror the actual >> struct, which leaves lots of NULL values, or serialize the required >> values with skels. In the latter case, we'd probably need to write >> custom sqlite functions to do the serialization, since it all happens >> within the various triggers. > > I was thinking of the three cases. Not a generalized serialization of > notify_t :-) > > * do some work > * sent notifications > * cleanup > [ do all the above "safely" ] > > Thoughts?
So long as it's extensible to other cases where we might want to use the pattern, I'm fine with the above approach. -Hyrum
