On Thu, Sep 2, 2010 at 5:52 PM, Julian Foad <julian.foad_at_wandisco.com> wrote:
> pburba_at_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);
That's what I get for writing some quick-and-dirty code for testing
purposes and letting it morph in my mind to "complete" status while I
work on the "real" problem. Fixed in
http://svn.apache.org/viewvc?view=revision&revision=992314.
> 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.
Ditto.
>> + }
>> +
>> + if (missing_subtree_err_str)
>
> Minor point: Either this "if" or the "if (apr_hash_count ...)" is
> redundant.
Gone.
> - Julian
On Fri, Sep 3, 2010 at 7:43 AM, Philip Martin
<philip.martin_at_wandisco.com> wrote:
> pburba_at_apache.org writes:
>
>> Author: pburba
>> Date: Thu Sep 2 18:10:01 2010
>> New Revision: 992042
>
>> @@ -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);
>
> ../src/subversion/libsvn_client/merge.c: In function ‘get_mergeinfo_paths’:
> ../src/subversion/libsvn_client/merge.c:5855: warning: declaration of ‘iterpool’ shadows a previous local
> ../src/subversion/libsvn_client/merge.c:5822: warning: shadowed declaration is here
>
> Is the second iterpool deliberate?
That was a mistake, VS2008 doesn't catch shadowed variables; need to
do the old fashioned way. Fixed in r992314.
> Philip
Thanks for the review gents.
Paul
Received on 2010-09-03 16:50:53 CEST