> -----Original Message-----
> From: stsp_at_apache.org [mailto:stsp_at_apache.org]
> Sent: donderdag 14 augustus 2014 21:23
> To: commits_at_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_at_1
> Source right: (file) ^/branch/beta_at_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_at_1
> Source right: (none) ^/branch/beta_at_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);
>
>
Received on 2014-08-18 14:34:24 CEST