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.
]]]
Paul
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 991396)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -5302,6 +5302,17 @@
{
/* Array of paths that have explicit mergeinfo and/or are switched. */
apr_array_header_t *children_with_mergeinfo;
+
+ /* A hash of MERGE_TARGET_ABSPATH's subdirectories' dirents. Maps
+ const char * absolute working copy paths to dirent hashes as obtained
+ by svn_io_get_dirents3(). Contents are allocated in CB_POOL. */
+ apr_hash_t *subtree_dirents;
+
+ /* A hash to keep track of any subtrees in the merge target which are
+ unexpectedly missing from disk. Maps const char * absolute working
+ copy paths to the same. Contents are allocated in CB_POOL. */
+ apr_hash_t *missing_subtrees;
+
/* Merge source canonical path. */
const char* merge_src_canon_path;
@@ -5322,9 +5333,116 @@
/* Pool from which to allocate new elements of CHILDREN_WITH_MERGEINFO. */
apr_pool_t *pool;
+
+ /* Pool with a lifetime guaranteed over all the get_mergeinfo_walk_cb
+ callbacks. */
+ apr_pool_t *cb_pool;
};
+/* Helper for the svn_wc__node_found_func_t callback get_mergeinfo_walk_cb().
+ Checks for issue #2915 subtrees, i.e. those that the WC thinks are on disk
+ but have been removed due to an OS-level deletion.
+
+ If the supposed working path LOCAL_ABSPATH, of kind KIND, is the root
+ of a missing subtree, then add a (const char *) WC absolute path to
+ (const char *) WC absolute path mapping to MISSING_SUBTREES, where the
+ paths are both a copy of LOCAL_ABSPATH, allocated in RESULT_POOL.
+
+ If LOCAL_ABSPATH is a directory and is not missing from disk, then add
+ a (const char *) WC absolute path to (svn_io_dirent2_t *) dirent mapping
+ to SUBTREE_DIRENTS, again allocated in RESULT_POOL (see
+ svn_io_get_dirents3).
+
+ SCRATCH_POOL is used for temporary allocations.
+
+ Note: Since this is effetively a svn_wc__node_found_func_t callback, it
+ must be called in depth-first order. */
+static svn_error_t *
+record_missing_subtree_roots(const char *local_abspath,
+ svn_node_kind_t kind,
+ apr_hash_t *subtree_dirents,
+ apr_hash_t *missing_subtrees,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ svn_boolean_t missing_subtree_root = FALSE;
+
+ /* Cache the dirents for each directory in SUBTREE_DIRENTS. */
+ if (kind == svn_node_dir)
+ {
+ /* If SUBTREE_DIRENTS is empty LOCAL_ABSPATH is merge target. */
+ if (apr_hash_count(subtree_dirents) == 0
+ || apr_hash_get(subtree_dirents,
+ svn_dirent_dirname(local_abspath,
+ scratch_pool),
+ APR_HASH_KEY_STRING))
+ {
+ apr_hash_t *dirents;
+ svn_error_t *err = svn_io_get_dirents3(&dirents, local_abspath,
+ TRUE, result_pool,
+ scratch_pool);
+ if (err)
+ {
+ if (APR_STATUS_IS_ENOENT(err->apr_err)
+ || SVN__APR_STATUS_IS_ENOTDIR(err->apr_err))
+ {
+ /* We can't get this directory's dirents, it's missing
+ from disk. */
+ svn_error_clear(err);
+ missing_subtree_root = TRUE;
+ }
+ else
+ {
+ return err;
+ }
+ }
+ else
+ {
+ apr_hash_set(subtree_dirents,
+ apr_pstrdup(result_pool, local_abspath),
+ APR_HASH_KEY_STRING, dirents);
+ }
+ }
+ }
+ else /* kind != svn_node_dir */
+ {
+ /* Is this non-directory missing from disk? Check LOCAL_ABSPATH's
+ parent's dirents. */
+ apr_hash_t *parent_dirents = apr_hash_get(subtree_dirents,
+
svn_dirent_dirname(local_abspath,
+
scratch_pool),
+ APR_HASH_KEY_STRING);
+
+ /* If the parent_dirents is NULL, then LOCAL_ABSPATH is the
+ subtree of a missing subtree. Since we only report the roots
+ of missing subtrees there is nothing more to do in that case. */
+ if (parent_dirents)
+ {
+ svn_io_dirent2_t *dirent =
+ apr_hash_get(parent_dirents,
+ svn_dirent_basename(local_abspath, scratch_pool),
+ APR_HASH_KEY_STRING);
+ if (!dirent)
+ missing_subtree_root = TRUE;
+ }
+ else if (kind == svn_node_dir)
+ {
+ missing_subtree_root = FALSE;
+ }
+ }
+
+ if (missing_subtree_root)
+ {
+ const char *path = apr_pstrdup(result_pool, local_abspath);
+
+ apr_hash_set(missing_subtrees, path,
+ APR_HASH_KEY_STRING, path);
+ }
+
+ return SVN_NO_ERROR;
+}
+
/* svn_wc__node_found_func_t callback for get_mergeinfo_paths().
Given LOCAL_ABSPATH and WB, where WB is struct get_mergeinfo_walk_baton *,
@@ -5400,7 +5518,16 @@
&&(kind == svn_node_dir)
&& (strcmp(abs_parent_path,
wb->merge_target_abspath) == 0));
-
+ /* Make sure what the WC thinks is present on disk really is. */
+ if (!absent
+ && !deleted
+ && !obstructed)
+ SVN_ERR(record_missing_subtree_roots(local_abspath, kind,
+ wb->subtree_dirents,
+ wb->missing_subtrees,
+ wb->cb_pool,
+ scratch_pool));
+
/* Store PATHs with explict mergeinfo, which are switched, are missing
children due to a sparse checkout, are scheduled for deletion are absent
from the WC, are first level sub directories relative to merge target if
@@ -5618,7 +5745,7 @@
/* Helper for do_directory_merge()
If HONOR_MERGEINFO is TRUE, then perform a depth first walk of the working
- copy tree rooted at MERGE_CMD_BATON->TARGET_ABSPATH.
+ copy tree rooted at MERGE_CMD_BATON->TARGET_ABSPATH to depth DEPTH.
Create an svn_client__merge_path_t * for any path which meets one or more
of the following criteria:
@@ -5646,6 +5773,9 @@
If HONOR_MERGEINFO is FALSE, then create an svn_client__merge_path_t * only
for MERGE_CMD_BATON->TARGET_ABSPATH (i.e. only criteria 7 is applied).
+ If subtrees within the requested DEPTH are unexpectedly missing disk,
+ then raise SVN_ERR_CLIENT_NOT_READY_TO_MERGE.
+
Store the svn_client__merge_path_t *'s in *CHILDREN_WITH_MERGEINFO in
depth-first order based on the svn_client__merge_path_t *s path member as
sorted by svn_path_compare_paths(). Set the remaining_ranges field of each
@@ -5685,6 +5815,9 @@
struct get_mergeinfo_walk_baton wb = { 0 };
wb.children_with_mergeinfo = children_with_mergeinfo;
+ wb.cb_pool = svn_pool_create(scratch_pool);
+ wb.subtree_dirents = apr_hash_make(wb.cb_pool);
+ wb.missing_subtrees = apr_hash_make(wb.cb_pool);
wb.merge_src_canon_path = merge_src_canon_path;
wb.merge_target_abspath = merge_cmd_baton->target_abspath;
wb.source_root_url = source_root_url;
@@ -5708,6 +5841,37 @@
merge_cmd_baton->ctx->cancel_baton,
scratch_pool));
+ if (apr_hash_count(wb.missing_subtrees))
+ {
+ apr_hash_index_t *hi;
+ apr_pool_t *iterpool = svn_pool_create(scratch_pool);
+ const char *missing_subtree_err_str = NULL;
+
+ for (hi = apr_hash_first(iterpool, wb.missing_subtrees);
+ hi;
+ hi = apr_hash_next(hi))
+ {
+ const char *missing_abspath = svn__apr_hash_index_key(hi);
+
+ missing_subtree_err_str = apr_psprintf(
+ scratch_pool, "%s%s\n",
+ missing_subtree_err_str ? missing_subtree_err_str : "",
+ svn_dirent_local_style(missing_abspath, scratch_pool));
+ }
+
+ if (missing_subtree_err_str)
+ return svn_error_createf(SVN_ERR_CLIENT_NOT_READY_TO_MERGE,
+ NULL,
+ _("Merge tracking not allowed with missing "
+ "subtrees; try restoring these items "
+ "first:\n%s"),
+ missing_subtree_err_str);
+ }
+
+ /* This pool is only needed across all the callbacks to detect
+ missing subtrees. */
+ svn_pool_destroy(wb.cb_pool);
+
/* CHILDREN_WITH_MERGEINFO must be in depth first order, but the node
walk code returns nodes in a non particular order. Also, we may need
to add elements to the array to cover case 3) through 5) from the
Index: subversion/tests/cmdline/merge_tests.py
===================================================================
--- subversion/tests/cmdline/merge_tests.py (revision 991396)
+++ subversion/tests/cmdline/merge_tests.py (working copy)
@@ -15903,6 +15903,87 @@
True, True, new_dir_path)
sbox.simple_commit()
+#----------------------------------------------------------------------
+# Test for issue #2915 'Handle mergeinfo for subtrees missing due to removal
+# by non-svn command'
+def merge_with_os_deleted_subtrees(sbox):
+ "merge tracking fails if target missing subtrees"
+
+ # r1: Create a greek tree.
+ sbox.build()
+ wc_dir = sbox.wc_dir
+
+ # r2 - r6: Copy A to A_COPY and then make some text changes under A.
+ set_up_branch(sbox)
+
+ # Some paths we'll care about
+ A_COPY_path = os.path.join(wc_dir, "A_COPY")
+ C_COPY_path = os.path.join(wc_dir, "A_COPY", "C")
+ psi_COPY_path = os.path.join(wc_dir, "A_COPY", "D", "H", "psi")
+ mu_COPY_path = os.path.join(wc_dir, "A_COPY", "mu")
+ G_COPY_path = os.path.join(wc_dir, "A_COPY", "D", "G")
+
+ # Remove several subtrees from disk.
+ svntest.main.safe_rmtree(C_COPY_path)
+ svntest.main.safe_rmtree(G_COPY_path)
+ os.remove(psi_COPY_path)
+ os.remove(mu_COPY_path)
+
+ # Be sure the regex paths are properly escaped on Windows, see the
+ # note about "The Backslash Plague" in expected_merge_output().
+ if sys.platform == 'win32':
+ re_sep = '\\\\'
+ else:
+ re_sep = os.pathsep
+
+ # Common part of the expected error message for all cases we will test.
+ err_re = "svn: Merge tracking not allowed with missing subtrees; " + \
+ "try restoring these items first:" + \
+ "|(\n)" + \
+ "|(.*apr_err.*\n)" # In case of debug build
+
+ # Case 1: Infinite depth merge into infinite depth WC target.
+ # Every missing subtree under the target should be reported as missing.
+ missing = "|(.*A_COPY" + re_sep + "mu\n)" + \
+ "|(.*A_COPY" + re_sep + "D" + re_sep + "G\n)" + \
+ "|(.*A_COPY" + re_sep + "C\n)" + \
+ "|(.*A_COPY" + re_sep + "D" + re_sep + "H" + re_sep + "psi\n)"
+ exit_code, out, err = svntest.actions.run_and_verify_svn(
+ "Missing subtrees should raise error", [], svntest.verify.AnyOutput,
+ 'merge', sbox.repo_url + '/A', A_COPY_path)
+ svntest.verify.verify_outputs("Merge failed but not in the way expected",
+ err, None, err_re + missing, None,
+ True) # Match *all* lines of stderr
+
+ # Case 2: Immediates depth merge into infinite depth WC target.
+ # Only the two immediate children of the merge target should be reported
+ # as missing.
+ missing = "|(.*A_COPY" + re_sep + "mu\n)" + \
+ "|(.*A_COPY" + re_sep + "C\n)"
+ exit_code, out, err = svntest.actions.run_and_verify_svn(
+ "Missing subtrees should raise error", [], svntest.verify.AnyOutput,
+ 'merge', sbox.repo_url + '/A', A_COPY_path, '--depth=immediates')
+ svntest.verify.verify_outputs("Merge failed but not in the way expected",
+ err, None, err_re + missing, None, True)
+
+ # Case 3: Files depth merge into infinite depth WC target.
+ # Only the single file child of the merge target should be reported
+ # as missing.
+ missing = "|(.*A_COPY" + re_sep + "mu\n)"
+ exit_code, out, err = svntest.actions.run_and_verify_svn(
+ "Missing subtrees should raise error", [], svntest.verify.AnyOutput,
+ 'merge', sbox.repo_url + '/A', A_COPY_path, '--depth=files')
+ svntest.verify.verify_outputs("Merge failed but not in the way expected",
+ err, None, err_re + missing, None, True)
+
+ # Case 4: Empty depth merge into infinite depth WC target.
+ # Only the...oh, wait, the target is present and that is as deep
+ # as the merge goes, so this merge should succeed!
+ svntest.actions.run_and_verify_svn(
+ "Depth empty merge should succeed as long at the target is present",
+ svntest.verify.AnyOutput, [], 'merge', sbox.repo_url + '/A',
+ A_COPY_path, '--depth=empty')
+
########################################################################
# Run the tests
@@ -16093,6 +16174,7 @@
copy_causes_phantom_eol_conflict,
merge_into_locally_added_file,
merge_into_locally_added_directory,
+ merge_with_os_deleted_subtrees,
]
if __name__ == '__main__':
