You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Neels Janosch Hofmeyr <ne...@elego.de> on 2009/09/03 02:06:56 UTC
Re: svn commit: r39096 - trunk/subversion/libsvn_client
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;
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageId=2390495
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2390499
Re: svn commit: r39096 - trunk/subversion/libsvn_client
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Sep 03, 2009 at 07:33:59PM +0200, Neels Janosch Hofmeyr wrote:
> Stefan Sperling wrote:
> > 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;
I think that's fairly bogus because svn/notify.c never checks this
field for any value other than svn_wc_notify_lock_state_unlocked,
and svn_wc_create_notify sets it to svn_wc_notify_lock_state_unknown
anyway. We can just remove that line.
> 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.
Oh, yes, looks like that could be a problem. Good catch.
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2390777
Re: svn commit: r39096 - trunk/subversion/libsvn_client
Posted by Neels Janosch Hofmeyr <ne...@elego.de>.
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
Re: svn commit: r39096 - trunk/subversion/libsvn_client
Posted by Stefan Sperling <st...@elego.de>.
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