On Thu, May 5, 2011 at 12:35 AM, Greg Stein <gstein_at_gmail.com> wrote:
> On Wed, May 4, 2011 at 22:49, Hyrum K Wright <hyrum_at_hyrumwright.org> wrote:
>> On Wed, May 4, 2011 at 8:46 PM, <gstein_at_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
Received on 2011-05-05 07:41:17 CEST