Re: RFC: How should Subversion handle OS-deleted paths?

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.

]]]

Paul