> -----Original Message----- > From: s...@apache.org [mailto:s...@apache.org] > Sent: donderdag 14 augustus 2014 21:23 > To: comm...@subversion.apache.org > Subject: svn commit: r1618024 - in /subversion/trunk/subversion: > libsvn_client/merge.c libsvn_wc/tree_conflicts.c > > Author: stsp > Date: Thu Aug 14 19:22:30 2014 > New Revision: 1618024
At your request, a review of this change. I found a small issue around replacement detection, see inline. > > URL: http://svn.apache.org/r1618024 > Log: > For tree conflicts flagged during merges, record the proper node kinds > of the the versions of nodes involved in the conflict (the local working > copy node kind, the merge-left node kind, and the merge-right node kind). > > Before this change misleading node kind information was recorded., > For example, nodes which are deleted at the merge-right side of the > incoming > change would produce the following conflict information: > > Tree conflict: local file edit, incoming file delete or move upon merge > Source left: (file) ^/trunk/beta@1 > Source right: (file) ^/branch/beta@3 > > This is misleading since ^/branche/betas does not exist in r3 since it > was deleted in r2. With this change, the recorded node kind is 'none': > > Tree conflict: local file edit, incoming file delete or move upon merge > Source left: (file) ^/trunk/beta@1 > Source right: (none) ^/branch/beta@3 > > * subversion/libsvn_client/merge.c > (make_conflict_versions): Expect two node kinds, for the left and right > side of the merge source, instead of just one. > (merge_dir_baton_t, merge_file_baton_t): Add fields to store node kind > information for tree conflict victims: tree_conflict_local_node_kind, > tree_conflict_merge_left_node_kind, and > tree_conflict_merge_right_node_kind. > (record_tree_conflict): Add two node kind parameters for a total of three > and > rename the existing 'node_kind' parameter to 'local_node_kind'. > Create conflict versions with merge-left/merge-right node kinds. The local > node kind is used as the victim's node kind in the conflict description. > (mark_dir_edited, mark_file_edited): Propagate node kind information > stored > in the dir/file baton to record_tree_conflict(). > (merge_file_opened, merge_dir_opened): Fill in all three node kinds > involved > in the conflict. > (merge_file_changed): Pass merge-left/merge-right node kinds to the > make_conflict_versions() function. For file edits both are svn_node_file. > (merge_file_deleted, merge_dir_deleted): Pass all three node kinds > involved > in the conflict to record_tree_conflict(). > > * subversion/libsvn_wc/tree_conflicts.c > (svn_wc__serialize_conflict): 'none' is a legal node kind for tree > conflicts. > Adjust an assertion accordingly. > > Modified: > subversion/trunk/subversion/libsvn_client/merge.c > subversion/trunk/subversion/libsvn_wc/tree_conflicts.c > > Modified: subversion/trunk/subversion/libsvn_client/merge.c > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/m > erge.c?rev=1618024&r1=1618023&r2=1618024&view=diff > ========================================================== > ==================== > --- subversion/trunk/subversion/libsvn_client/merge.c (original) > +++ subversion/trunk/subversion/libsvn_client/merge.c Thu Aug 14 19:22:30 > 2014 <snip> > @@ -1903,12 +1934,11 @@ merge_file_opened(void **new_file_baton, > && ((pdb && pdb->added) || fb->add_is_replace))) > { > svn_wc_notify_state_t obstr_state; > - svn_node_kind_t kind; > svn_boolean_t is_deleted; > > SVN_ERR(perform_obstruction_check(&obstr_state, &is_deleted, > NULL, > - &kind, NULL, > - merge_b, local_abspath, > + > &fb->tree_conflict_local_node_kind, > + NULL, merge_b, local_abspath, > scratch_pool)); > > if (obstr_state != svn_wc_notify_state_inapplicable) > @@ -1918,7 +1948,8 @@ merge_file_opened(void **new_file_baton, > fb->tree_conflict_reason = CONFLICT_REASON_SKIP; > fb->skip_reason = obstr_state; > } > - else if (kind != svn_node_none && !is_deleted) > + else if (fb->tree_conflict_local_node_kind != svn_node_none > + && !is_deleted) > { > /* Set a tree conflict */ > fb->shadowed = TRUE; > @@ -1997,7 +2028,8 @@ merge_file_changed(const char *relpath, > scratch_pool, scratch_pool)); > > SVN_ERR(make_conflict_versions(&left, &right, local_abspath, > - svn_node_file, &merge_b->merge_source, > merge_b- > >target, > + svn_node_file, svn_node_file, > + &merge_b->merge_source, merge_b->target, > scratch_pool, scratch_pool)); > > /* Do property merge now, if we are not going to perform a text merge */ > @@ -2422,6 +2454,10 @@ merge_file_deleted(const char *relpath, > */ > SVN_ERR(record_tree_conflict(merge_b, local_abspath, fb- > >parent_baton, > svn_node_file, > + svn_node_file, > + fb->add_is_replace > + ? svn_node_file > + : svn_node_none, At this place inside merge_file_deleted() we are still in the delete part of the merge, so fb->add_is_replace is aways FALSE. Only in the add that follows the delete we know that we have a replacement. (That is why the variable is called add_is_replace instead of delete_is_replace) > svn_wc_conflict_action_delete, > svn_wc_conflict_reason_edited, > NULL, TRUE, > @@ -2472,6 +2508,16 @@ merge_dir_opened(void **new_dir_baton, > > *new_dir_baton = db; > > + if (left_source) > + db->tree_conflict_merge_left_node_kind = svn_node_dir; > + else > + db->tree_conflict_merge_left_node_kind = svn_node_none; > + > + if (right_source) > + db->tree_conflict_merge_right_node_kind = svn_node_dir; > + else > + db->tree_conflict_merge_right_node_kind = svn_node_none; > + > if (pdb) > { > db->parent_baton = pdb; > @@ -2488,7 +2534,6 @@ merge_dir_opened(void **new_dir_baton, > else if (left_source != NULL) > { > /* Node is expected to be a directory. */ > - svn_node_kind_t kind; > svn_boolean_t is_deleted; > svn_boolean_t excluded; > svn_depth_t parent_depth; > @@ -2500,9 +2545,9 @@ merge_dir_opened(void **new_dir_baton, > { > svn_wc_notify_state_t obstr_state; > SVN_ERR(perform_obstruction_check(&obstr_state, &is_deleted, > &excluded, > - &kind, &parent_depth, > - merge_b, local_abspath, > - scratch_pool)); > + &db->tree_conflict_local_node_kind, > + &parent_depth, merge_b, > + local_abspath, scratch_pool)); > > if (obstr_state != svn_wc_notify_state_inapplicable) > { > @@ -2537,10 +2582,10 @@ merge_dir_opened(void **new_dir_baton, > } > > if (is_deleted) > - kind = svn_node_none; > + db->tree_conflict_local_node_kind = svn_node_none; > } > > - if (kind == svn_node_none) > + if (db->tree_conflict_local_node_kind == svn_node_none) > { > db->shadowed = TRUE; > > @@ -2576,7 +2621,7 @@ merge_dir_opened(void **new_dir_baton, > return SVN_NO_ERROR; > /* ### /avoid breaking tests */ > } > - else if (kind != svn_node_dir) > + else if (db->tree_conflict_local_node_kind != svn_node_dir) > { > db->shadowed = TRUE; > > @@ -2667,6 +2712,8 @@ merge_dir_opened(void **new_dir_baton, > > /* Update the tree conflict to store that this is a replace */ > SVN_ERR(record_tree_conflict(merge_b, local_abspath, pdb, > + old_tc->local_node_kind, > + svn_node_none, > svn_node_dir, > db->tree_conflict_action, > db->tree_conflict_reason, > @@ -2681,12 +2728,11 @@ merge_dir_opened(void **new_dir_baton, > && ((pdb && pdb->added) || db->add_is_replace))) > { > svn_wc_notify_state_t obstr_state; > - svn_node_kind_t kind; > svn_boolean_t is_deleted; > > SVN_ERR(perform_obstruction_check(&obstr_state, &is_deleted, > NULL, > - &kind, NULL, > - merge_b, local_abspath, > + > &db->tree_conflict_local_node_kind, > + NULL, merge_b, local_abspath, > scratch_pool)); > > /* In this case of adding a directory, we have an exception to the > @@ -2696,7 +2742,8 @@ merge_dir_opened(void **new_dir_baton, > * versioned but unexpectedly missing from disk, or is unversioned > * but obstructed by a node of the wrong kind. */ > if (obstr_state == svn_wc_notify_state_obstructed > - && (is_deleted || kind == svn_node_none)) > + && (is_deleted || > + db->tree_conflict_local_node_kind == svn_node_none)) > { > svn_node_kind_t disk_kind; > > @@ -2717,7 +2764,8 @@ merge_dir_opened(void **new_dir_baton, > db->tree_conflict_reason = CONFLICT_REASON_SKIP; > db->skip_reason = obstr_state; > } > - else if (kind != svn_node_none && !is_deleted) > + else if (db->tree_conflict_local_node_kind != svn_node_none > + && !is_deleted) > { > /* Set a tree conflict */ > db->shadowed = TRUE; > @@ -2796,6 +2844,8 @@ merge_dir_opened(void **new_dir_baton, > { > /* ### Should be atomic with svn_wc_add(4|_from_disk2)() */ > SVN_ERR(record_tree_conflict(merge_b, local_abspath, pdb, > + old_tc->local_node_kind, > + svn_node_none, > svn_node_dir, > db->tree_conflict_action, > db->tree_conflict_reason, > @@ -2864,7 +2914,8 @@ merge_dir_changed(const char *relpath, > svn_wc_notify_state_t prop_state; > > SVN_ERR(make_conflict_versions(&left, &right, local_abspath, > - svn_node_dir, &merge_b->merge_source, > + svn_node_dir, svn_node_dir, > + &merge_b->merge_source, > merge_b->target, > scratch_pool, scratch_pool)); > > @@ -3212,6 +3263,8 @@ merge_dir_deleted(const char *relpath, > */ > SVN_ERR(record_tree_conflict(merge_b, local_abspath, db- > >parent_baton, > svn_node_dir, > + svn_node_dir, > + svn_node_none, This is exactly the same case as discussed above for files. > svn_wc_conflict_action_delete, > svn_wc_conflict_reason_edited, > NULL, TRUE, > > Modified: subversion/trunk/subversion/libsvn_wc/tree_conflicts.c > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/tree > _conflicts.c?rev=1618024&r1=1618023&r2=1618024&view=diff > ========================================================== > ==================== > --- subversion/trunk/subversion/libsvn_wc/tree_conflicts.c (original) > +++ subversion/trunk/subversion/libsvn_wc/tree_conflicts.c Thu Aug 14 > 19:22:30 2014 > @@ -367,7 +367,8 @@ svn_wc__serialize_conflict(svn_skel_t ** > > /* node_kind */ > SVN_ERR_ASSERT(conflict->local_node_kind == svn_node_dir > - || conflict->local_node_kind == svn_node_file); > + || conflict->local_node_kind == svn_node_file > + || conflict->local_node_kind == svn_node_none); > skel_prepend_enum(c_skel, node_kind_map, conflict->local_node_kind, > result_pool); > >
