On Wed, May 4, 2011 at 12:34, Hyrum K Wright <hyrum_at_hyrumwright.org> wrote:
> On Wed, May 4, 2011 at 11:31 AM, Greg Stein <gstein_at_gmail.com> wrote:
>> On Wed, May 4, 2011 at 08:55, <philip_at_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
Received on 2011-05-04 18:46:08 CEST