Quoting Julian Foad <julianfoad_at_btopenworld.com>:
> 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
I'll throw in a couple of svn resolved command to get rid of the
inadvertently-created tree conflicts (of the add:add variety).
> FAIL: merge_tests.py 103: merge tries to delete a file of different
> content
For me test 103 fails intermittently. When it does fail, the expected
stdout and actual stdout appear to match anyway. Rather strange.
EXPECTED STDOUT:
Skipped 'A/D/G2/tau'
--- Merging r2 into 'A/D/G2':
C A/D/G2
Summary of conflicts:
Tree conflicts: 1
Skipped paths: 1
ACTUAL STDOUT:
Skipped 'A/D/G2/tau'
--- Merging r2 into 'A/D/G2':
C A/D/G2
Summary of conflicts:
Text conflicts: 1
Skipped paths: 1
EXCEPTION: SVNLineUnequal
Traceback [...]
> FAIL: stat_tests.py 31: status with tree conflicts
Expected status
> 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
In all 4 cases, commit of a dir containing a text-conflicted file
produced no output, not even an error message. Not sure how we broke
this.
Aside from sorting out these tests, we also need to make some new
tests for detection of existing conflicts, following the general
scheme that you outline below. Oops, guess I just volunteered. :-o
>
> 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()".
Yes, I suppose returning the whole conflict is the long-term solution.
I'd like to merge the branch to trunk ASAP, though. Can we do the
boolean->conflict type change in a separate commit to trunk?
>
> 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.
Yes, the tree conflict notification should be encapsulated in a simple
function.
Currently the tree conflict notifications are mixed in with other
notifications, leading to output such as 'D C'. But very soon we'll
eliminate that mixing, by skipping updates to the victims. Then we
can notify the user of a tree conflict immediately instead of waiting
for dir_close or file_close (in some cases).
>
> 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:
a.)
> * tree/text/prop change to node beneath tree-conflicted dir,
> or
b.)
> * tree/text/prop change to tree-conflicted dir/file, or
c.)
> * tree change to a text/prop-conflicted file/dir, or
d.)
> * 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"
I hadn't thought about sub-items 1c or 1d above. Good catch.
So IIUC we'll print one line of output upon detecting an existing conflict
(1b, 1c, 1d above), or creating fresh tree conflict (#3 below).
I think we should be completely silent for the rest of a conflicted
tree (1a), to avoid burying the user in lots of "Skipped" messages.
Too much output would distract the user from the initial message,
which describes where the tree conflict is. After resolving the tree
conflict, the user can check status or resume update/switch, and find
any other conflicts in the tree.
> *
> * 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.
But if we can't add a tree due to a tree conflict, where would we stash it
in the working copy? Anyway, that's a performance enhancement for future
consideration.
> */
> ]]]
All in all this looks good to me.
>
>
> 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.
Yes the current update code works only for fresh tree conflicts. It's
definitely incomplete.
Here's my basic implementation plan:
The following update callbacks need to check for existing conflicts:
open_directory
open_file
add_directory
add_file
delete_entry
absent_file
absent_directory
In each callback, we'll make the following checks/calls.
(eb->inside_a_tree_conflict)
True if we're walking a tree-conflicted tree. Works even if the
target and its parent dir do not exist (e.g., we've skipped adding
the parent dir due to the tree conflict). Skip target silently if
true.
svn_wc_conflicted_p2()
Check for existing conflicts. Requires a versioned parent dir for
a tree conflict. Requires a versioned parent dir and versioned
target for text or prop conflicts. Skip target with notification
if conflicts are found.
check_tree_conflict(&tree_conflict, eb, ...)
Returns a fresh tree conflict if the target is a victim. Requires
a versioned parent dir. If a conflict is found, record it
persistently.
notify_tree_conflict(tree_conflict, eb, ...)
If there's a tree conflict and an eb->notify_func, notify the user.
Regards,
Steve
--
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-10-27 20:37:13 CET