Stefan Sperling wrote:
>>> - svn_boolean_t is_replace = FALSE;
>>> + svn_boolean_t notified = FALSE;
>>> deleted_path_notify_t *dpn = apr_hash_get(eb->deleted_paths, b->wcpath,
>>> APR_HASH_KEY_STRING);
>>> if (dpn)
>>> @@ -904,26 +904,24 @@ close_file(void *file_baton,
>>> svn_wc_notify_action_t new_action;
>>> if (dpn->action == svn_wc_notify_update_delete
>>> && action == svn_wc_notify_update_add)
>>> - {
>>> - is_replace = TRUE;
>>> - new_action = svn_wc_notify_update_replace;
>>> - }
>>> + new_action = svn_wc_notify_update_replace;
>>> else
>>> new_action = dpn->action;
>>>
>>> if (action != svn_wc_notify_tree_conflict)
>>> {
>>> - notify = svn_wc_create_notify(b->wcpath, new_action, pool);
>>> + notify = svn_wc_create_notify(b->wcpath, new_action, pool);
>>> notify->kind = dpn->kind;
>>> notify->content_state = notify->prop_state = dpn->state;
>>> notify->lock_state = svn_wc_notify_lock_state_inapplicable;
>>> (*eb->notify_func)(eb->notify_baton, notify, pool);
>>> + notified = TRUE;
>>> }
>>> apr_hash_set(eb->deleted_paths, b->wcpath,
>>> APR_HASH_KEY_STRING, NULL);
>>> }
>>>
>>> - if (!is_replace)
>>> + if (!notified)
>>> {
>>> notify = svn_wc_create_notify(b->wcpath, action, pool);
>>> notify->kind = svn_node_file;
>
> Can't we change this code to call eb->notify_func exactly once,
> and sort out just the parameters in the various code paths?
> Just like I did in add_directory()?
Ah, that's where I saw this before!
> I think that's much simpler to follow and less bug prone.
I actually didn't think so. I considered that, but instead of just one flag,
we have to carry all the parameters passed through all the code. Also note
the value
notify->lock_state = svn_wc_notify_lock_state_inapplicable;
is only set in the first notification. Bla bla rant, that said:
I took another look at the "action" thing, which looks kind of stupid, and
now I technically agree. But I also found a serious problem with your way:
it could overwrite an
action = svn_wc_notify_tree_conflict
in case of a replace. I'm not sure why we haven't noticed yet, but your way
should actually break TC notifications during with-dir-replaces.
I'll check it out. A patch for close_file() is testing now, and I'll try to
hit the case where I think your way should break.
~Neels
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2390761
Received on 2009-09-03 19:37:10 CEST