Re: branch tree-conflicts-notify: merge to trunk?

On Sat, 2008-10-25 at 14:15 +0200, Stephen Butler wrote:
> Quoting "Neels J. Hofmeyr" <neels_at_elego.de>:
>
> > Hey Steve! (sbutler)
> >
> > That's so awesome! Somehow you've magically fixed almost everything on the
> > branch! All I had to do was raise and notify a tree-conflict on file
> > addition in a conflicted directory.
>
> Thanks!

That's great. And yes I want it on trunk too. Especially I want the
changes in how we proceed through checking for new tree conflicts,
checking for existing conflicts, and checking for obstructions, as I
think we need to do that more consistently - see below.

I got these failures when I tried the branch a couple of hours ago
(r33896):
FAIL: externals_tests.py 14: old 'PATH URL' format should ignore peg
revisions
FAIL: externals_tests.py 16: place a file external into a directory
external
FAIL: merge_tests.py 103: merge tries to delete a file of different
content
FAIL: stat_tests.py 31: status with tree conflicts
FAIL: tree_conflict_tests.py 9: merge file: modify onto not-file
FAIL: tree_conflict_tests.py 11: merge file: del/rpl/mv onto not-file
FAIL: tree_conflict_tests.py 13: merge dir: modify onto not-dir
FAIL: tree_conflict_tests.py 15: merge dir: del/rpl/mv onto not-dir

I hope it's OK if I do a catch-up merge on your branch. Last Friday (I
think) I tweaked the "check_tree_conflict()" function in update_editor.c
to return the conflict description it raised, and I committed that to
trunk, while you had done it a slightly different way (returning a
boolean flag instead) on the branch. Returning the whole conflict should
allow us more flexibility to choose how much detail we put into the
notification, like I do in the attached patch by sending it to the new
function "notify_new_tree_conflict()".

Also I think check_tree_conflict() may benefit (sooner or later) from
being split into separate "detect" and "raise" functions, with the
conflict description being returned by the former and passed to the
latter.

Yell if that seems wrong.

Now, stages of detection...

We could be clearer on exactly what Subversion should be doing during
"svn update/switch", with respect to conflict detection and skipping of
obstructions.

One of you mentioned before that a note about this would make a good
code comment in update_editor.c, and I agree.

Does this look right?

[[[
/*
 * An "update" or "switch" should include, in this order:
 *
 * 1. if # Incoming change affects an item already in conflict:
 * tree/text/prop change to node beneath tree-conflicted dir, or
 * tree/text/prop change to tree-conflicted dir/file, or
 * tree change to a text/prop-conflicted file/dir, or
 * text/prop change to a text/prop-conflicted file/dir[1],
 * then # Skip this change [2]:
 * do not update the Base nor the Working
 * notify "skipped because already in conflict"
 *
 * 2. if # Incoming change affects an item that's "obstructed":
 * on-disk node kind doesn't match recorded Working node kind,
 * including an absence/presence mis-match
 * then # Skip this change [2]:
 * do not update the Base nor the Working
 * notify "skipped because obstructed"
 *
 * 3. if # Incoming change raises a tree conflict:
 * tree/text/prop change to node beneath tree-scheduled[3] dir, or
 * tree/text/prop change to tree-scheduled[3] dir/file, or
 * text/prop change to tree-scheduled dir/file,
 * then # Skip this change:
 * do not update the Base nor the Working [4]
 * notify "tree conflict"
 *
 * 4. Apply the change:
 * update the Base
 * update the Working, possibly raising text/prop conflicts
 * notify as in v1.5
 *
 *
 * [1] Details of which combinations of property and text changes conflict
 * are out of scope here and defined by v1.5 behaviour.
 *
 * [2] Should skip changes to an entire node, as the base revision number
 * applies to the whole node. Not sure how this affects attempts to
 * handle text and prop changes separately.
 *
 * [3] Not all combinations implied can happen, but that doesn't matter.
 *
 * [4] For now, we skip the update, and require the user to:
 * - Modify the WC to be compatible with the incoming change;
 * - Mark the conflict as resolved;
 * - Repeat the update.
 * Ideally, it would be possible to resolve any conflict without
 * repeating the update. To achieve this, we could store the possible
 * new base(s) at conflict detection time, and update the actual base at
 * the time of resolving.
 */
]]]

I noticed considerable inconsistencies in this, in the current "update"
code, while trying to make "update" skip changes inside a
tree-conflicted directory tree. I haven't worked through trying to
implement the above, I'm just telling you that's somthing I'm looking
at, as I think we need to get this behaviour clearly specified.

Comments?

- Julian

(n.b. the attached "skip-conflicts-6.patch" is for reference, not
particularly for review, and is very little changed from "-5" version of
it.)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org