Update and merge report ‘obstructed; for different reasons… But are within itself pretty consistent, and the documentation of obstructed in svn_wc.h supports both implementations.
(I tried to make this more consistent a few months ago, but found that a lot of code and tests depends on the current implementation).
My change in the specific patch is explained in the thread on users_at_s.a.o. The most important change is that it properly records the tree conflict on the node that is tree conflicted, instead of on descendants of the conflict. (This fixed a related issue, but not the reported issue on the list). I tried not to change the behavior around that. I tried to ask Paul about the history of this behavior and behavior was discussed on the list. But no final solution was found.
The problem is that we mix recording of mergeinfo to keep the intermediate state… and the recording of the final conflict resolving result, by only updating the mergeinfo once… instead of actually doing something to resolve the conflict.
Your patch makes the behavior change specific to add-add tree conflicts… That looks like a much better solution than blindly ignoring all tree conflict types as done in the previous patch.
Sent from Surface
From: Julian Foad
Sent: Wednesday, June 24, 2015 1:22 AM
To: Bert Huijben, dev_at_subversion.apache.org
Bert and Stefan,
The following patch also appears to fix the bug -- all tests pass.
However, see my further comments below.
[[[
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 1687145)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -2791,18 +2791,12 @@ merge_dir_opened(void **new_dir_baton,
SVN_ERR(svn_wc__node_is_added(&added, merge_b->ctx->wc_ctx,
local_abspath, scratch_pool));
db->tree_conflict_reason = added ? svn_wc_conflict_reason_added
:
svn_wc_conflict_reason_obstructed;
-
- if ((merge_b->merge_source.ancestral ||
merge_b->reintegrate_merge)
- && !(pdb && pdb->shadowed))
- {
- store_path(merge_b->skipped_abspaths, local_abspath);
- }
}
}
/* Handle pending conflicts */
SVN_ERR(mark_dir_edited(merge_b, db, local_abspath, scratch_pool));
@@ -5395,20 +5389,12 @@ record_skips_in_mergeinfo(const char *me
merge_b, skipped_abspath,
iterpool));
if (obstruction_state == svn_wc_notify_state_obstructed
|| obstruction_state == svn_wc_notify_state_missing)
continue;
- /* Make sure this is not a tree-conflicted path either. We don't
- * want to add mergeinfo on such nodes since it causes problems down
- * the line (see issue #4582, "reintegrate complains about missing
- * ranges from node unrelated to branch") */
- if (svn_hash_gets(merge_b->tree_conflicted_abspaths,
- skipped_abspath) != NULL)
- continue;
-
/* Add an empty range list for this path.
### TODO: This works fine for a file path skipped because it is
### missing as long as the file's parent directory is present.
### But missing directory paths skipped are not handled yet,
### see issue #2915.
]]]
Rather than adding code to ignore a 'skipped' path if it is also a
tree conflict path, this patch instead removes the code which recorded
an add-vs-add tree conflict victim as 'skipped'. That code block was
added recently (in http://svn.apache.org/r1666690) by Bert. Bert, can
you comment on it?
[[[
------------------------------------------------------------------------
r1666690 | rhuijben | 2015-03-14 15:30:17 +0100 (Sat, 14 Mar 2015) | 26 lines
In the libsvn_client merge code: When reporting and recording that we skip a
node because it is shadowed on an ancestor, properly record that the
ancestor is skipped instead of the descendant itself.
* subversion/libsvn_client/merge.c
(record_skip): Only record a skip, if we really skipped this node, and
not one of its ancestors. To do this handle this add a parent-baton
argument to this function.
(merge_file_changed,
merge_file_added,
merge_file_deleted): Update caller.
(merge_dir_opened): Not only skip the directory add, but also record it
as skipped.
(merge_dir_changed,
merge_dir_added,
merge_dir_deleted,
merge_node_absent): Update caller.
* subversion/tests/cmdline/merge_tree_conflict_tests.py
(merge_obstruction_recording): New function.
(test_list): Add merge_obstruction_recording.
Found by: Pete Harlan <pchpublic88{_AT_}gmail.com>
]]]
However, neither the current code nor the code resulting from this
patch looks entirely correct. Notice the first statement shown in the
patch context:
SVN_ERR(svn_wc__node_is_added(&added,...));
...reason = added ? ...reason_added : ...reason_obstructed;
That looks at the WC schedule to decide whether to call this an
'added' or an 'obstructed' conflict. For a merge, I think if the
directory exists in the working state, then we should report the
reason as 'added' regardless whether the dir was added in a commit on
the checked-out branch or locally added in the WC. And I'm not
entirely sure when we should call it 'obstructed' -- maybe only when
there is a local unversioned object on disk where the WC working state
expects there is nothing?
- Julian
Received on 2015-06-24 02:08:41 CEST