[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r39096 - trunk/subversion/libsvn_client

From: Stefan Sperling <stsp_at_elego.de>
Date: Thu, 3 Sep 2009 11:47:51 +0100

On Thu, Sep 03, 2009 at 04:06:56AM +0200, Neels J Hofmeyr wrote:
> Do you think we should add this one to the r38000 group? It fixes
> [[[
> C A/mu
> A A/mu
> ]]]
> to
> [[[
> C A/mu
> ]]]
> with a file-with-file replacement onto a delete during merge.
>
> Also see merge_tests.py 135, there still is some erratic status in the end
> there, which is related to TC during replace. Maybe we also want this fixed
> and added to r38000? ... when is 1.6.6 due?
>
> (There also is the next test, nr. 136 -- incoming replace vs. local delete
> during merge don't conflict when the delete is already committed --, but I
> think that's definitely detached enough to be treated separately)
>
> ~Neels
>
> Neels Janosch Hofmeyr wrote:
> > Author: neels
> > Date: Wed Sep 2 18:49:31 2009
> > New Revision: 39096
> >
> > Log:
> > * subversion/libsvn_client/repos_diff.c
> > (close_file): Fix duplicate notification for tree-conflict with incoming
> > replace (file replaced with file), possibly others. This case is visible
> > in merge_tests.py merge_replace_causes_tree_conflict2 (135) on file
> > A/mu during the final merge: there was an additional "A" notification
> > for A/mu. (Test still XFails for various status imperfections.)
> >
> > Modified:
> > trunk/subversion/libsvn_client/repos_diff.c
> >
> > Modified: trunk/subversion/libsvn_client/repos_diff.c
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/repos_diff.c?pathrev=39096&r1=39095&r2=39096
> > ==============================================================================
> > --- trunk/subversion/libsvn_client/repos_diff.c Wed Sep 2 18:41:03 2009 (r39095)
> > +++ trunk/subversion/libsvn_client/repos_diff.c Wed Sep 2 18:49:31 2009 (r39096)
> > @@ -896,7 +896,7 @@ close_file(void *file_baton,
> > if (eb->notify_func)
> > {
> > svn_wc_notify_t *notify;
> > - 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()?

I think that's much simpler to follow and less bug prone.
Because if someone comes along later and adds extra conditions to
if statements protecting calls to eb->notify_func we can run into
the same problem again.

Stefan

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2390595
Received on 2009-09-03 14:16:01 CEST

This is an archived mail posted to the Subversion Dev mailing list.