On Thu, Jan 22, 2009 at 11:20 AM, Stephen Butler <sbutler_at_elego.de> wrote:
> Quoting Paul Burba <ptburba_at_gmail.com>:
>
>> While reviewing issue #3209 I went through all the XFailing tests in
>> checkout_tests.py, update_tests.py, switch_tests.py, and
>> merge_tests.py looking for tests that fail because their expectations
>> were not adjusted to account for new tree conflict behavior.
>>
>> Ignoring tests that fail for reasons unrelated to TCs, those that are
>> failing because they test unimplemented TC behavior, those that are
>> actively being worked on by others, and those I was able to fix, there
>> are six tests remaining that started failing with the advent of tree
>> conflict handling. These fall into four groups:
>
> Hi Paul,
>
> thanks for this report! I wrote some comments and todo-items inline
> below.
>
> Regards,
> Steve
Steve,
I'm often mocked for my overly-long e-mails, so thanks for wading
through...comments below.
>> =======
>> GROUP 1
>> =======
>>
>> checkout_tests.py 13 'co handles obstructing paths scheduled for add'
>> update_tests.py 34 'update handles obstructing paths scheduled for add'
>> switch_tests.py 21 'forced switch detects tree conflicts'
>>
>> These three tests all cover behavior added in r22257:
>>
>> Enable up/sw/co to tolerate obstructions scheduled
>> for addition without history.
>>
>> It's no longer an error when an update, switch, or
>> checkout attempts to add a path that is already
>> scheduled for addition *without* history in the WC.
>>
>> In the case of dirs, the directory is tolerated and
>> reported as 'E'xisting.
>>
>> For files, if the obstruction is identical textually
>> to the addition from the repos it is reported as
>> 'E'xisting, if it differs it's treated as a conflict.
>>
>> Property merges for both files and dirs are reported
>> normally, i.e. ' ', 'C', 'G'.
>>
>> Obstructions scheduled for addition *with* history
>> still result in an error.
>>
>> See these three threads for more on this change:
>>
>> http://svn.haxx.se/dev/archive-2006-08/0336.shtml
>> http://svn.haxx.se/dev/archive-2006-09/0362.shtml
>> http://svn.haxx.se/dev/archive-2006-10/0708.shtml
>>
>> All these tests start with two identical working copies, WC1 and WC2.
>> A set of files and directories are added to the repository via WC1.
>> Then the test adds the same set of paths *without* history to WC2.
>> These local adds in WC2 fall into 3 categories:
>>
>> A) Files textually identical to the files added to WC1.
>>
>> Previously we expected these obstructions to be
>> tolerated, reported as 'E'xisting, and leave no trace
>> after the checkout. Now however this produces a tree
>> conflict of the 'local add, incoming add upon update'
>> type.
>>
>> B) Files which differ textually from the files added
>> to WC1.
>>
>> Prior to TC we expected these to be conflicts and they
>> still are, but they are tree conflicts of the 'local
>> add, incoming add upon update' type instead.
>>
>> C) Added directories that have no properties, the same
>> properties, or conflicting properties with the the
>> directories added to WC1.
>>
>> Prior to TC we expected all of these to be tolerated and
>> reported as 'E'xisting, though their properties might mer'G'e
>> or 'C'onflict. That is still the case.
>>
>> The desired behavior in cases A) & B) need to be resolved. Do we want
>> to support the behavior introduced in r22257 that tolerates
>> obstructing adds with history or remove it? The behavior in case C)
>> should follow the decision on A) and B), files and directories are
>> treated consistently I think.
>
> To see this from the user's point of view, let's refer to
> notes/tree-conflicts/resolution.txt. In section "up/sw: Add onto
> Add", there are 3 sample use cases. The local and incoming changes...
>
> 1. are the same patch (local change can be reverted, or merged).
> 2. are different, but share a name (local change can be renamed).
> 3. are similar, and should be merged.
>
> 1 and 3 are safe for reporting the existing paths and merging any
> differences.
>
> 2 is a bit tricky to sort out, if the merge has already been
> attempted by the update.
You're quite right; assuming we return to the pre-TC behavior of
tolerating the existing file add-without-history, then we'll get a
traditional text conflict and the user will have to resort to some
non-obvious trickery to keep both the incoming file and their
completely unrelated local addition. But that is also true if '2' is
treated as a tree conflict right? So I'm not sure they are any worse
off either way. Though *maybe* it is more obvious what the problem is
if we flag a tree conflict. Though maybe not, if the files really
have nothing to do with each other, then the text conflict is quite
likely to be of this variety...
<<<<<<< .mine
*The entire text of my locally added file*
=======
*The entire text of the incoming added file*
>>>>>>> .rNEWREVISION
...which seems pretty obvious too.
> Maybe the user should update back to the
> old BASE revision, move the files, and add them again? I'll have to
> try it myself.
Or, resolve --accept mine-full, OS copy, revert, add, commit (RORAC?):
# Oops, nu.r2 landed on my local add-without-history nu.mine,
# but these two file have nothing to do with each other, I
# want to keep both.
>svn st merge_tests-111.other
? merge_tests-111.other\A\C\nu.r0
? merge_tests-111.other\A\C\nu.r2
? merge_tests-111.other\A\C\nu.mine
C merge_tests-111.other\A\C\nu
# Keep nu.mine
>svn resolve --accept mine-full merge_tests-111.other\A\C\nu
Resolved conflicted state of 'merge_tests-111.other\A\C\nu'
>svn st merge_tests-111.other
M merge_tests-111.other\A\C\nu
# Kludgey copy outside of Subversion.
>copy merge_tests-111.other\A\C\nu merge_tests-111.other\A\C\new_nu
1 file(s) copied.
# Now, keep the incoming nu.
>svn revert merge_tests-111.other\A\C\nu
Reverted 'merge_tests-111.other\A\C\nu'
# And lastly add and commit "my" nu.
>svn add merge_tests-111.other\A\C\new_nu
A merge_tests-111.other\A\C\new_nu
>svn ci -m "New nus" merge_tests-111.other
Adding merge_tests-111.other\A\C\new_nu
Transmitting file data .
Committed revision 3.
Yup, that is capital 'U' Ugly, but again, if this is treated as a tree
conflict we are left in quite a similar situation.
> Assuming that situation 2 isn't too tough for the user,
FWIW, I searched through the users mailing list, back to the release
of 1.5.0, looking for reports of use case '2', but didn't find any.
> I'd prefer to
> tolerate the local-add-without-history (i.e., not raise a tree
> conflict). To do this, I'll make the add_file() callback act like
> add_directory(), checking for the local-add before checking for a tree
> conflict.
+1
>> There is also a regression in these tests. Taking checkout_tests.py
>> 13 for example, after the above cases are covered, the test does a
>> URL-to-URL copy of A/D/G to A/D/M and then does a WC-to-WC copy of
>> A/D/H
>> to A/D/M, leaving us with (I'm simplifying here, the test does some other
>> stuff
>> as well):
>>
>> trunk>svn st checkout_tests-13 -u
>> * checkout_tests-13\A\D\M\pi
>> * checkout_tests-13\A\D\M\rho
>> * checkout_tests-13\A\D\M\tau
>> A + * - checkout_tests-13\A\D\M
>> * 1 checkout_tests-13\A\D
>> Status against revision: 3
>>
>> Then when we try to update (the test does a checkout but the effect is
>> the same)
>> we get this error:
>>
>> trunk>svn up checkout_tests-13
>> ..\..\..\subversion\libsvn_wc\adm_files.c:672: (apr_err=155000)
>> svn: Revision 3 doesn't match existing revision 1 in
>> 'checkout_tests-13/A/D/M'
>
> You're right, the update editor is being too picky here. Calling
> svn_wc_ensure_adm3() is overkill anyway, it includes code that creates
> the admin dir if it doesn't exist. All we want to do here is a
> read-only sanity check. I'll change it to compare only the incoming
> and existing URLs. If they don't match, there will be an error.
Great!
>> =======
>> GROUP 2
>> =======
>>
>> update_tests.py 31 'forced up fails with some types of obstructions'
>>
>> This test fails when we a (--forced) update tries to add a directory
>> when a versioned directory of the same name already exists.
>> Admittedly this is quite a contrived situation: The test adds a
>> directory A/C/I in r2, updates A/C to r1, then checks out %URL%/A/C/I
>> directly to A/C/I in the WC. This leaves us with a situation where
>> A/C thinks A/C/I is some unversioned item:
>>
>> >svn st update_tests-31.backup\A\C -v
>> 1 1 jrandom update_tests-31.backup\A\C
>> ? update_tests-31.backup\A\C\I
>>
>> >svn st update_tests-31.backup\A\C\I -v
>> 2 2 jrandom update_tests-31.backup\A\C\I
>>
>> Prior to TC we expected this to fail with an error:
>>
>> svn: Failed to add directory 'update_tests-31.backup\A\C\I':
>> a versioned directory of the same name already exists
>>
>> This is now a tree conflict:
>>
>> >svn up --force update_tests-31.backup
>> C update_tests-31.backup\A\C\I
>> At revision 2.
>> Summary of conflicts:
>> Tree conflicts: 1
>>
>> >svn st update_tests-31.backup
>> ? C update_tests-31.backup\A\C\I
>> > local add, incoming add upon update
>>
>> It strikes me that *both* of these behaviors are probably wrong.
>> Shouldn't the update simply tolerate A/C/I's presence and simply bring
>> A/C up to r2? Though if A/C/I pointed to a different location then
>> there should be an error, but what?
>
> I agree, it would be nice to subsume the existing A/C/I working copy
> when checking out a new working copy in the parent dir, and simply to
> bring A/C/I up to date, raising new conflicts within it as necessary.
>
> If the URLs don't match, I'll raise an error, because it's the same
> situation as an existing added-without-history item (see end of "GROUP
> 1", above).
+1
>> FWIW this doesn't seem a very common use case. The old behavior was
>> added by me back with the r20945 'takeover' functionality, and IIRC it
>> was simply because this was a possibility and the code had to do
>> *something* in this case.
>>
>>
>> =======
>> GROUP 3
>> =======
>>
>> update_tests.py 50 'tree conflicts on update 2.3'
>>
>> This tests that updates of tree conflicted paths report the TCs as
>> skipped, but what exactly does 2.3 refer to?
>
> To no other document. The names 2.1 and 2.2 are arbitrary, too.
> Descriptive docstrings might help.
>
> tree conflicts 2.1 leaf edit, tree del on update
> tree conflicts 2.2 leaf del, tree del on update
> tree conflicts 2.3 skip TCs on 2nd update
Ok, just wanted to be sure I wasn't missing some secret TC docs
printed on the back of the Constitution :-)
http://www.theonion.com/content/node/39384
>> http://svn.collab.net/repos/svn/trunk/notes/tree-conflicts/use-cases.txt
>> has use case 2, but there are no subsections. This appears to be an
>> test for issue #3329,
>> http://subversion.tigris.org/issues/show_bug.cgi?id=3329#desc2, and as
>> I noted there, many of the cases this test covers now work and report
>> skips:
>>
>> a) Directory tree conflict victim is updated
>>
>> b) File tree conflict victim is updated
>>
>> c) Directory within a tree conflict victim is updated
>>
>> d) Fle within a tree conflict victim is updated
>> (file doesn't exist in the repos)
>>
>> What doesn't work is:
>>
>> e) A file within a tree conflict victim is updated
>> (file doesn't exist in the repos but shows as
>> locally added)
>>
>> f) Directory update target is not tree conflicted
>> but has tree conflicted file child.
>>
>> g) Directory update target is not tree conflicted
>> but has tree conflicted dir child.
>>
>> In all three of these cases the update prints no skip notifications,
>> the 'At revision N.' is the only output. The status of the WC is the
>> same in each case, so no harm is done outside of the missing skip
>> notifications.
>>
>> So, is this simply a test for issue #3329 or is there anything else to
>> consider?
>
> Yes, it's simply a regression test for #3329 "Update throws error when
> skipping some tree conflicts".
Is anyone working on e), f), and g)? I'm glad to take a look if not.
>> =======
>> GROUP 4
>> =======
>>
>> merge_tests.py 20 'merge into missing must not break working copy'
>>
>> In r31326 (on the tree-conflicts branch) this test was changed to
>> expect a tree conflict when merging into a directory that has a
>> missing subtree (to be clear, this is missing in the '!' removed by a
>> non-svn command sense of missing). The current (and pre-TC?) behavior
>> was to report the missing subtrees as skipped.
>>
>> Why is a tree conflict expected here? We report the files as skipped
>> during the merge and they show as missing in status. You can't even
>> commit the change without updating first to get the missing items
>> back. Yes, at that point you could commit and most likely you won't
>> have what you want in the repos, but you must willfully ignore a lot
>> of warnings that something is not quite right.
>>
>> This case mentioned in the 'SCENARIO PLAYGROUND' in
>>
>> http://svn.collab.net/repos/svn/trunk/notes/tree-conflicts/scratch-pad.txt,
>> but there is no conclusion:
>>
>> 'svn merge' pulls file modification atop missing file
>>
>> NOW: "Skipped missing target" message.
>>
>> NEW:
>>
>> There is also some mention of this in
>>
>> http://svn.collab.net/repos/svn/trunk/notes/tree-conflicts/design-overview.txt
>>
>> * Pre-existing obstructions (status "~") or missing items ("!") should be
>> detected first, before attempting to apply the change, and abort the
>> operation.
>>
>> What is the latest thinking on this scenario?
I know, people lose energy by the end of my mails, did you have any
thoughts on this last item?
Paul
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1043893
Received on 2009-01-23 01:11:40 CET