On Tue, Aug 31, 2010 at 8:27 PM, Paul Burba <ptbu...@gmail.com> wrote: > On Thu, Aug 26, 2010 at 8:07 AM, Julian Foad <julian.f...@wandisco.com> wrote: >> On Wed, 2010-08-25, Paul Burba wrote: >>> On Tue, Aug 24, 2010 at 7:25 AM, Julian Foad <julian.f...@wandisco.com> >>> wrote: >> [...] >>> Agreed, we simply can't be sure what the user's intention was...I'm >>> beginning to be swayed to the idea that restoring the missing subtrees >>> may not be the ideal approach... >> >> [...] >>> > The pattern that's emerging from my thoughts is: throw an error if >>> > logically we need to access the missing working version of the node. If >>> > we don't need to access it, just let it be. Never "restore" it unless >>> > the user specifically requests so and says what kind of restoration is >>> > required. >>> > >>> > For these reasons I think "merge" should also throw an error if it needs >>> > to merge changes into a missing node. (If instead it needs to delete >>> > it, then it has the options I mentioned for "svn delete".) >>> > >>> > But I suspect part of the reason why "restore" seems an attractive >>> > option is because Subversion isn't very friendly when "merge" stops with >>> > an error. We don't provide any way to resume the same merge, >>> >>> Quite right about the unfriendliness. Resuming the merge is really a >>> hit-or-miss proposition, depending on how much of the merge was done >>> successfully prior to encountering the missing subtree. If it >>> encountered early, before the application of any diffs, then things >>> are ok, but after that things get ugly fast, particularly if the merge >>> target originally had any local mods. >>> >>> It's worth noting again there there are *two* error-out approaches: >>> >>> i) Throw an error when a subtree the editor needs is found to be >>> missing. This is what you are talking about here. >>> >>> ii) Identify any missing subtrees at the *start* of the merge and >>> throw an error before any editor drives are done. Basically we >>> disallow merges with missing subtrees due to OS-level deletes. >> >> You're right, I hadn't thought of erroring out before starting the >> merge, and that is a much, much better option than erroring out in the >> middle. >> >>> > and we >>> > don't make it particularly easy to roll back the merge (although that's >>> > getting better now that "revert" is, I hope, going to remove nodes that >>> > it created by copying), and we don't have any way at all to roll back a >>> > merge performed in a non-clean WC. So we're trying to avoid erroring >>> > out. >>> > >>> > Long term, those difficult problems are are the problems we should be >>> > looking to solve. Short term, I suppose it's useful to avoid erroring >>> > out as much as we can. >>> >>> Equally bad is the case with trunk right now, where we simply ignore >>> missing directories, even if the merge would affect them - see >>> http://subversion.tigris.org/issues/show_bug.cgi?id=2915#desc4 for an >>> example. >>> >>> > If that's the important issue, and we recognize >>> > it as such, then I could support the idea of "merge" doing this >>> > "restore" while other commands don't. >>> >>> Given what you've said here and thinking anew about merge and missing >>> subtrees, I think the best approach might simply be what we currently >>> do with files: Skip the missing subtree and record non-inheritable >>> mergeinfo so the missing subtree is handled correctly (i.e. the >>> mergeinfo reflects the fact that the merge never touched the missing >>> subtree). >>> >>> This has a few things going for it: >>> >>> A) It's consistent with 1.5-1.6's treatment of missing files. >>> >>> B) As Daniel hinted at in >>> http://svn.haxx.se/dev/archive-2010-08/0189.shtml, it's consistent >>> with how merge tracking treats every other type of "missing" subtree >>> (e.g. shallow WCs, switches, paths missing due to authz restrictions). >>> >>> C) It makes no assumptions about what the user intended, it just deals >>> with the fact that the subtrees are missing. >>> >>> D) It allows the merge to complete, no errors mid-merge. >>> >>> E) It allows the missing subtrees to be restored via update (either >>> before or after the merge is committed) and for the merge to be >>> repeated. The repeat merge will notice that the merge never touched >>> the subtrees and will drive the editor such that only those subtrees >>> have the merge repeated. >>> >>> Any thoughts to this approach? >> >> Another plus: >> >> F) It means the merge algorithm has one less case that can choke it, >> which is better than relying on a check to have been done beforehand. >> It makes the subroutines easier to re-use (callable by GUIs etc.). And >> it could be extremely difficult to make sure that such an external check >> exactly matches the merge code in all the corner cases. >> >> The only minus I can think of: In many ways we would serve our users >> better by simply not allowing them to get into complex situations and >> instead just disallowing such a merge. >> >> But the many advantages seem to outweigh that, and your suggestion >> sounds close to perfect. >> >> If you can do that without much additional complexity, +1. > > I really thought that would be the case, as most of the logic is > already in place to handle other types of missing subtrees. But after > hacking on this entirely too long and finding new bugs at every turn, > I had some serious second thoughts, mainly along the lines of, "Is all > this complexity worth it?" > > I think "no" and have come to see the wisdom of something you said earlier: > > "You're right, I hadn't thought of erroring out before starting the > merge, and that is a much, much better option than erroring out in the > middle." > > This is relatively simple to do as the attached patch demonstrates. > If there are no objections I will go in this direction instead. > > Note that there are a few test failures I still need to fix: > > FAIL: merge_tests.py 16: merge into missing must not break working copy > FAIL: merge_tests.py 104: skipped files get correct mergeinfo set > FAIL: merge_tree_conflict_tests.py 21: tree conflicts: tree missing, leaf del > FAIL: merge_tree_conflict_tests.py 20: tree conflicts: tree missing, leaf > edit > FAIL: merge_authz_tests.py 1: skipped paths get overriding mergeinfo > > These all fail because they expect the old behavior of OS-deleted > paths to be skipped and OS-deleted directories to create > tree-conflicts. With the patch in place, if we try to merge to a > target which is missing subtrees due to OS-level deletions, then we > get an error like this: > > ### Oops, move rather than svn move: > > C:\SVN\src-branch-1.6.x-WCNG>move > subversion\tests\cmdline\merge_authz_tests.py > subversion\tests\cmdline\merge_auth_tests.py > 1 file(s) moved. > > ### Let's merge: > > C:\SVN\src-branch-1.6.x-WCNG>svn merge ^^/subversion/trunk -c879766 > ..\..\..\subversion\svn\util.c:902: (apr_err=195016) > ..\..\..\subversion\libsvn_client\merge.c:10548: (apr_err=195016) > ..\..\..\subversion\libsvn_client\merge.c:10504: (apr_err=195016) > ..\..\..\subversion\libsvn_client\merge.c:10504: (apr_err=195016) > ..\..\..\subversion\libsvn_client\merge.c:10476: (apr_err=195016) > ..\..\..\subversion\libsvn_client\merge.c:8611: (apr_err=195016) > ..\..\..\subversion\libsvn_client\merge.c:8096: (apr_err=195016) > ..\..\..\subversion\libsvn_client\merge.c:5851: (apr_err=195016) > svn: Merge tracking not allowed with missing subtrees; try restoring > these items first: > C:\SVN\src-branch-1.6.x-WCNG\subversion\tests\cmdline\merge_authz_tests.py > > ### Oh, I see where I screwed up: > > C:\SVN\src-branch-1.6.x-WCNG>svn st > ? subversion\tests\cmdline\merge_auth_tests.py > ! subversion\tests\cmdline\merge_authz_tests.py > > Note: In the interest of sanity, if there are many OS-deleted subtrees > in a merge target, the error lists only the *roots* of the missing > subtrees. > > [[[ > Fix issue #2915 'Handle mergeinfo for subtrees missing due to removal by > non-svn command'. > > With this change, if you attempt a merge-tracking aware merge to a WC > which is missing subtrees due to an OS-level deletion, then an error is > raised before any editor drives begin. The error message describes the > root of each missing path. > > * subversion/libsvn_client/merge.c > > (get_mergeinfo_walk_baton): Add some new members for tracking sub- > directories' dirents. > > (get_mergeinfo_walk_cb): > > (record_missing_subtree_roots): New function. > > (record_missing_subtree_roots): Use new function to flag missing subtree > roots. > > (get_mergeinfo_paths): Raise a SVN_ERR_CLIENT_NOT_READY_TO_MERGE error if > any unexpectedly missing subtrees are found. > > * subversion/tests/cmdline/merge_tests.py > > (merge_with_os_deleted_subtrees): New test, > > (test_list): Add merge_with_os_deleted_subtrees. > > ]]]
Committed this change with a few minor code changes and the failing tests fixed: http://svn.apache.org/viewvc?view=revision&revision=992042 Paul
