pbu...@apache.org wrote:
> 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.
+1 to this solution.
Review comments below.
> * subversion/libsvn_client/merge.c
>
> (get_mergeinfo_walk_baton): Add some new members for tracking sub-
> directories' dirents.
>
> (record_missing_subtree_roots): New function.
>
> (get_mergeinfo_walk_cb): 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_authz_tests.py
>
> (skipped paths get overriding mergeinfo): Don't test the file missing via
> OS-delete scenario, this is now covered in a much clearer manner in the
> new merge_tests.py.merge_with_os_deleted_subtrees test. Also remove not
> about testing a missing directory, that is also covered in the new test.
>
> * subversion/tests/cmdline/merge_tests.py
>
> (merge_into_missing): Use --ignore-ancestry during this test's merge to
> disregard mergeinfo and preserve the original intent of the test.
>
> (skipped_files_get_correct_mergeinfo): Use a shallow WC to rather than an
> OS-level delete to test issue #3440. Remove the second part of this test
> which has no relevance now that merge tracking doesn't tolerate subtrees
> missing via OS-deletion.
>
> (merge_with_os_deleted_subtrees): New test.
>
> (test_list): Add merge_with_os_deleted_subtrees.
>
> * subversion/tests/cmdline/merge_tree_conflict_tests.py
>
> (tree_conflicts_merge_edit_onto_missing,
> tree_conflicts_merge_del_onto_missing): Use --ignore-ancestry during these
> tests' merges to disregard mergeinfo and preserve the original intent of
> the test. Don't bother with the post-merge commit either, since there is
> nothing to commit as there are no mergeinfo changes.
>
> * subversion/tests/cmdline/svntest/actions.py
>
> (deep_trees_run_tests_scheme_for_merge): Add new args allowing caller to
> use --ignore-ancestry during the merge and to skip the post merge commit
> if desired.
[...]
> @@ -5718,6 +5849,37 @@ get_mergeinfo_paths(apr_array_header_t *
> 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);
Iterpool isn't being used effectively here. Normally, we would allocate
the iterator in the outer pool ("hi =
apr_hash_first(scratch_pool, ...)"), and use 'iterpool' for any
temporary allocations within the loop and clear it each time round. In
this loop there aren't any, so it's redundant.
> + 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));
In most realistic cases this way of constructing the string is fine, but
it looks like it's quadratic in time and memory space which might become
a problem when a merge is attempted into a pathological tree with
several thousand individual missing subtree roots.
Off the top of my head I would look at accumulating the string in an
svn_stringbuf, which will expand and occasionally re-alloc as you append
to it rather than re-allocating every time, and which remembers its
length so appending is quick.
> + }
> +
> + if (missing_subtree_err_str)
Minor point: Either this "if" or the "if (apr_hash_count ...)" is
redundant.
> + 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);
> +
[...]
- Julian
