On Wed, Sep 23, 2009 at 11:59:07PM +0100, Julian Foad wrote:
> On Wed, 2009-09-23 at 20:25 +0100, Stefan Sperling wrote:
> > On Wed, Sep 23, 2009 at 07:23:52PM +0100, Stefan Sperling wrote:
> > > However, the local schedule *does* matter if the item is locally deleted.
> > > After all, a user could want the merge to add another file on
> > > top of a locally deleted file, causing it to be scheduled replace.
>
> That's true, but the code path we are looking at is inside "case
> svn_node_file" in a switch(node kind on disk). Therefore we know that
> the file is NOT properly locally deleted. In "case svn_node_file", if it
> is schedule-delete then there is an obstruction on disk so the WC state
> is inconsistent and the call to obstructed_or_missing() earlier in the
> function should have caused it to be skipped.
>
> Therefore there is no need to look for schedule-delete in this case.
Julian,
thanks for helping me with this. After having gotten some sleep,
the whole function fits into my brain again.
I have adapted the code again with your feedback in mind.
I also believe I found the cause of remaining "attempt to add tree
conflict that already exists" problem reports while doing so.
tree_conflict_on_add() has a bug which causes it to add conflicts
on top of existing conflicts in some situations. See the diff below
which I'm now running through make check.
Any more clever insights you can provide are appreciated :)
Stefan
[[[
* subversion/libsvn_client/merge.c
(tree_conflict_on_add): Do not ever try to add new tree conflicts
before removing existing ones. The old logic would try to do this
in some cases since it did not consider the "existing_conflict != NULL"
case in isolation.
(merge_file_added): If there are no obstructions or missing nodes,
but there is no node on disk, only add the new file if no tree
conflict has been flagged at this path yet. Otherwise, branch out
to tree_conflict_on_add().
If there are no obstructions or missing nodes, and a file is already
present on disk, flag a tree conflict as before, but do not consider
the existing file's schedule as an indicator for the tree conflict
reason. Rather, just flag the conflict with reason "added", based
on the assumption that an add must have happened in the history of
the branch.
]]]
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 39557)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -582,15 +582,20 @@ tree_conflict_on_add(merge_cmd_baton_t *merge_b,
victim_abspath, merge_b->pool,
merge_b->pool));
- /* If this change is an add, and there is already a tree conflict raised
- * by a previous incoming change that attempted to delete the item (whether
- * in this same merge operation or not), then don't try to add a second tree
- * conflict. Instead, just change the existing conflict to note that the
- * incoming change is replacement. */
- if (existing_conflict != NULL
- && existing_conflict->action == svn_wc_conflict_action_delete
- && conflict->action == svn_wc_conflict_action_add)
+ if (existing_conflict == NULL)
{
+ /* There is no existing tree conflict so it is safe to add one. */
+ return svn_error_return(svn_wc__add_tree_conflict(merge_b->ctx->wc_ctx,
+ conflict, merge_b->pool));
+ }
+ else if (existing_conflict->action == svn_wc_conflict_action_delete &&
+ conflict->action == svn_wc_conflict_action_add)
+ {
+ /* There is already a tree conflict raised by a previous incoming
+ * change that attempted to delete the item (whether in this same
+ * merge operation or not). Change the existing conflict to note
+ * that the incoming change is replacement. */
+
/* Remove the existing tree-conflict so we can add a new one.*/
SVN_ERR(svn_wc__del_tree_conflict(merge_b->ctx->wc_ctx,
victim_abspath, merge_b->pool));
@@ -603,10 +608,14 @@ tree_conflict_on_add(merge_cmd_baton_t *merge_b,
conflict->src_left_version = svn_wc_conflict_version_dup(
existing_conflict->src_left_version,
merge_b->pool);
+
+ return svn_error_return(svn_wc__add_tree_conflict(merge_b->ctx->wc_ctx,
+ conflict,
+ merge_b->pool));
}
- return svn_error_return(
- svn_wc__add_tree_conflict(merge_b->ctx->wc_ctx, conflict, merge_b->pool));
+ /* In any other cases, we don't touch the existing conflict. */
+ return SVN_NO_ERROR;
}
/* Set *HONOR_MERGEINFO and *RECORD_MERGEINFO (if non-NULL) based on the
@@ -1589,6 +1598,7 @@ merge_file_added(svn_wc_adm_access_t *adm_access,
const char *copyfrom_url = NULL;
svn_revnum_t copyfrom_rev = SVN_INVALID_REVNUM;
svn_stream_t *new_base_contents;
+ svn_wc_conflict_version_t *existing_conflict;
/* If this is a merge from the same repository as our working copy,
we handle adds as add-with-history. Otherwise, we'll use a pure
@@ -1612,20 +1622,40 @@ merge_file_added(svn_wc_adm_access_t *adm_access,
SVN_ERR(svn_stream_open_readonly(&new_base_contents, yours,
subpool, subpool));
- /* Since 'mine' doesn't exist, and this is
- 'merge_file_added', I hope it's safe to assume that
- 'older' is empty, and 'yours' is the full file. Merely
- copying 'yours' to 'mine', isn't enough; we need to get
- the whole text-base and props installed too, just as if
- we had called 'svn cp wc wc'. */
- /* ### would be nice to have cancel/notify funcs to pass */
- SVN_ERR(svn_wc_add_repos_file3(
- mine, adm_access,
- new_base_contents, NULL, new_props, NULL,
- copyfrom_url, copyfrom_rev,
- NULL, NULL, NULL, NULL, subpool));
+ SVN_ERR(svn_wc__get_tree_conflict(&existing_conflict,
+ merge_b->ctx->wc_ctx,
+ mine_abspath, merge_b->pool,
+ merge_b->pool));
+ if (existing_conflict)
+ {
+ /* Possibly collapse the existing conflict into a 'replace'
+ * tree conflict. The conflict reason is 'added' because
+ * the now-deleted tree conflict victim must have been
+ * added in the history of the merge target. */
+ SVN_ERR(tree_conflict_on_add(merge_b, mine_abspath,
+ svn_node_file,
+ svn_wc_conflict_action_add,
+ svn_wc_conflict_reason_added));
+ if (tree_conflicted)
+ *tree_conflicted = TRUE;
+ }
+ else
+ {
+ /* Since 'mine' doesn't exist, and this is
+ 'merge_file_added', I hope it's safe to assume that
+ 'older' is empty, and 'yours' is the full file. Merely
+ copying 'yours' to 'mine', isn't enough; we need to get
+ the whole text-base and props installed too, just as if
+ we had called 'svn cp wc wc'. */
+ /* ### would be nice to have cancel/notify funcs to pass */
+ SVN_ERR(svn_wc_add_repos_file3(
+ mine, adm_access,
+ new_base_contents, NULL, new_props, NULL,
+ copyfrom_url, copyfrom_rev,
+ NULL, NULL, NULL, NULL, subpool));
- /* ### delete 'yours' ? */
+ /* ### delete 'yours' ? */
+ }
}
if (content_state)
*content_state = svn_wc_notify_state_changed;
@@ -1667,42 +1697,15 @@ merge_file_added(svn_wc_adm_access_t *adm_access,
}
else
{
- const svn_wc_entry_t *entry;
- svn_wc_conflict_reason_t reason;
- const char *abs_mine;
+ /* The file add the merge wants to carry out is obstructed by
+ * a versioned file. This file must have been added in the
+ * history of the merge target, hence we flag a tree conflict
+ * with reason 'added'. */
+ SVN_ERR(tree_conflict_on_add(
+ merge_b, mine_abspath, svn_node_file,
+ svn_wc_conflict_action_add,
+ svn_wc_conflict_reason_added));
- /* Figure out what state the file which is already present
- * is in, and set the conflict reason accordingly. */
- SVN_ERR(svn_dirent_get_absolute(&abs_mine, mine, subpool));
- SVN_ERR(svn_wc__get_entry_versioned(&entry,
- merge_b->ctx->wc_ctx,
- abs_mine, kind, FALSE,
- FALSE, subpool, subpool));
- switch (entry->schedule)
- {
- case svn_wc_schedule_normal:
- reason = svn_wc_conflict_reason_obstructed;
- break;
- case svn_wc_schedule_add:
- reason = svn_wc_conflict_reason_added;
- break;
- case svn_wc_schedule_delete:
- reason = svn_wc_conflict_reason_deleted;
- break;
- case svn_wc_schedule_replace:
- reason = svn_wc_conflict_reason_replaced;
- break;
- }
-
- /* The file add the merge wants to carry out is obstructed by
- * a versioned file, so the file the merge wants to add is a
- * tree conflict victim. See notes about obstructions in
- * notes/tree-conflicts/detection.txt.
- */
- SVN_ERR(tree_conflict_on_add(merge_b, mine_abspath,
- svn_node_file,
- svn_wc_conflict_action_add,
- reason));
if (tree_conflicted)
*tree_conflicted = TRUE;
}
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2399261
Received on 2009-09-24 14:13:58 CEST