On Wed, Sep 23, 2009 at 06:37:30PM +0100, Julian Foad wrote:
> I still have some concerns about this back-port branch.
>
> I have just gone through the branch and made a diff that represents the
> whole state of the branch, and I have started to go through it and in so
> doing I have written a log message based purely on what code changes I
> see. I want to compare this with the original log messages of the
> individual revisions, and see what light that throws on it, but I
> haven't done so yet and it's end of day for me now.
>
> Please look at the "###" concerns in the attached log message, repeated
> here:
>
> * subversion/libsvn_wc/update_editor.c
> (schedule_existing_item_for_re_add): Teach it do plain adds (without
> history) as well as copies, controlled by a new "modify_copyfrom"
> parameter.
> (do_entry_deletion): If we raise a tree conflict because the node is locally
> replaced, re-schedule the node for addition (without history) rather than
> trying to delete it.
> ### Without history sounds wrong. A replacement can be with or without history,
> ### and the schedule-add should correspondingly be with or without history.
Agreed. Should be possible to fix.
> * subversion/libsvn_client/merge.c
> (make_tree_conflict): New function, factored out of tree_conflict().
> (tree_conflict): Factor out a chunk into make_tree_conflict().
> (tree_conflict_on_add): New function, like tree_conflict() but can combine
> an existing action=delete conflict with a new action=add conflict to make
> an action=replace conflict.
> ### Better doc-string needed.
> [But I'm not fussing about that.]
>
> (merge_file_added): Use tree_conflict_on_add() instead of tree_conflict() so
> an incoming file replacement results in a single tree conflict instead of
> an (abortive) attempt to add two tree conflicts on the same victim. If it
> conflicts with an existing file, determine the "reason" (added, deleted or
> obstructed) from the node's local scheduling.
> ### This schedule => reason sounds wrong. Expected state is no node present.
> ### Actual state is there is a file present. Therefore reason for conflict is
> ### "obstructed" (or we could describe it as "added").
>
> The point here ^^ is that this is a merge, so the target is a branch,
> and the state of stuff in the branch is dependent mainly on the branch
> history, and only incidentally on the local WC scheduling state as well.
> I'm not convinced that this will in any situation generate a wrong
> result, however, so it might be just-about-acceptable.
I made this change with good intentions but you are right that it is not
entirely correct. As you point out, the local scheduling state is not
the actual conflict reason. The actual conflict reason is buried in the
history of the merge target.
After all, someone had to put the thing in the merge target in place.
Let's think this through again:
When saying "obstructed" we are basically saying "there is
something here and I don't know what it's doing there, I did not
expect it". This is OK for unversioned items.
But for versioned items we can assume that they were "added" if the
local schedule is normal (which the current code wrongly flags a
"obstructed"). For items which are locally added in the merge target,
we can also say "added".
I guess we can ignore a replace which happened either in the history
of the merge target or locally in the WC, and just say "added" in those
cases, too. If an item is locally replaced, it must have replaced
something that was there before it, and this something must have been
added after the merge target was created.
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.
We could thus consider a locally deleted node to satisfy the expected
state of "node not present".
Do you agree with the above? Below is a patch that makes this change,
I've already kicked off make check on it. There might be some test
fallout but I think the change is correct and we should adjust any
failing tests to deal with this.
Thanks so much for reviewing this!
Stefan
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 39543)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -1667,7 +1667,6 @@ 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;
/* Figure out what state the file which is already present
@@ -1677,33 +1676,17 @@ merge_file_added(svn_wc_adm_access_t *adm_access,
merge_b->ctx->wc_ctx,
abs_mine, kind, FALSE,
FALSE, subpool, subpool));
- switch (entry->schedule)
+ if (entry->schedule != svn_wc_schedule_delete)
{
- 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;
+ /* Something was added locally in the merge target,
+ * or in the merge target's history. */
+ 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;
}
-
- /* 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;
}
break;
}
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2399010
Received on 2009-09-23 20:24:33 CEST