You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Hyrum K Wright <hy...@hyrumwright.org> on 2011/05/05 04:49:03 UTC
Re: svn commit: r1099657 - in /subversion/trunk/subversion/libsvn_wc:
adm_ops.c wc_db.c wc_db.h
On Wed, May 4, 2011 at 8:46 PM, <gs...@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.
> ...
-Hyrum
Re: svn commit: r1099657 - in /subversion/trunk/subversion/libsvn_wc:
adm_ops.c wc_db.c wc_db.h
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Thu, May 5, 2011 at 12:35 AM, Greg Stein <gs...@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, <gs...@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
Re: svn commit: r1099657 - in /subversion/trunk/subversion/libsvn_wc:
adm_ops.c wc_db.c wc_db.h
Posted by Greg Stein <gs...@gmail.com>.
On Wed, May 4, 2011 at 22:49, Hyrum K Wright <hy...@hyrumwright.org> wrote:
> On Wed, May 4, 2011 at 8:46 PM, <gs...@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?
Cheers,
-g