You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2011/05/04 18:31:38 UTC
Re: svn commit: r1099436 - in /subversion/trunk/subversion/libsvn_wc:
adm_ops.c update_editor.c wc_db.c wc_db.h
On Wed, May 4, 2011 at 08:55, <ph...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_wc/wc_db.h Wed May 4 12:55:03 2011
> @@ -1215,7 +1215,10 @@ svn_wc__db_op_delete(svn_wc__db_t *db,
> /* Make delete notifications for all paths in the delete list created
> * by deleting LOCAL_ABSPATH.
> *
> - * This function drops the delete list.
> + * This function drops the delete list. NOTIFY_FUNC may be NULL in
> + * which case the table is dropped without any notification.
> + *
> + * ### Perhaps this should be part of svn_wc__db_op_delete?
I say "yes". If the actual deletion produces an error, then we still
want the table dropped before returning that error. Tho... it may be
that the sqlite transaction rollback takes the table with it(?). ...
in any case, these two functions need to be called as a pair, and that
is a bad pattern to enforce on the caller.
Cheers,
-g
Re: svn commit: r1099436 - in /subversion/trunk/subversion/libsvn_wc:
adm_ops.c update_editor.c wc_db.c wc_db.h
Posted by Greg Stein <gs...@gmail.com>.
On Wed, May 4, 2011 at 12:34, Hyrum K Wright <hy...@hyrumwright.org> wrote:
> On Wed, May 4, 2011 at 11:31 AM, Greg Stein <gs...@gmail.com> wrote:
>> On Wed, May 4, 2011 at 08:55, <ph...@apache.org> wrote:
>>>...
>>> +++ subversion/trunk/subversion/libsvn_wc/wc_db.h Wed May 4 12:55:03 2011
>>> @@ -1215,7 +1215,10 @@ svn_wc__db_op_delete(svn_wc__db_t *db,
>>> /* Make delete notifications for all paths in the delete list created
>>> * by deleting LOCAL_ABSPATH.
>>> *
>>> - * This function drops the delete list.
>>> + * This function drops the delete list. NOTIFY_FUNC may be NULL in
>>> + * which case the table is dropped without any notification.
>>> + *
>>> + * ### Perhaps this should be part of svn_wc__db_op_delete?
>>
>> I say "yes". If the actual deletion produces an error, then we still
>> want the table dropped before returning that error. Tho... it may be
>> that the sqlite transaction rollback takes the table with it(?). ...
>> in any case, these two functions need to be called as a pair, and that
>> is a bad pattern to enforce on the caller.
>
> I'll also note that we have a few other places where we do this, too.
> My spidey sense has been telling me we need to make the action and
> notification bits a single wc_db function call, but I haven't yet
> acted on it.
Likewise. These separate notification functions have been bugging me,
too. It introduces a coupling between wc_db and its callers -- those
callers need to do "something more" when they call into wc_db. If they
don't... then things start to go wrong.
I'll look into fixing these.
Cheers,
-g
Re: svn commit: r1099436 - in /subversion/trunk/subversion/libsvn_wc:
adm_ops.c update_editor.c wc_db.c wc_db.h
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Wed, May 4, 2011 at 11:31 AM, Greg Stein <gs...@gmail.com> wrote:
> On Wed, May 4, 2011 at 08:55, <ph...@apache.org> wrote:
>>...
>> +++ subversion/trunk/subversion/libsvn_wc/wc_db.h Wed May 4 12:55:03 2011
>> @@ -1215,7 +1215,10 @@ svn_wc__db_op_delete(svn_wc__db_t *db,
>> /* Make delete notifications for all paths in the delete list created
>> * by deleting LOCAL_ABSPATH.
>> *
>> - * This function drops the delete list.
>> + * This function drops the delete list. NOTIFY_FUNC may be NULL in
>> + * which case the table is dropped without any notification.
>> + *
>> + * ### Perhaps this should be part of svn_wc__db_op_delete?
>
> I say "yes". If the actual deletion produces an error, then we still
> want the table dropped before returning that error. Tho... it may be
> that the sqlite transaction rollback takes the table with it(?). ...
> in any case, these two functions need to be called as a pair, and that
> is a bad pattern to enforce on the caller.
I'll also note that we have a few other places where we do this, too.
My spidey sense has been telling me we need to make the action and
notification bits a single wc_db function call, but I haven't yet
acted on it.
-Hyrum