On Tue, Aug 31, 2010 at 8:27 PM, Paul Burba <ptburba_at_gmail.com> wrote:
> On Thu, Aug 26, 2010 at 8:07 AM, Julian Foad <julian.foad_at_wandisco.com> wrote:
>> On Wed, 2010-08-25, Paul Burba wrote:
>>> On Tue, Aug 24, 2010 at 7:25 AM, Julian Foad <julian.foad_at_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
Received on 2010-09-02 20:11:52 CEST