[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r992042 - in /subversion/trunk/subversion: libsvn_client/merge.c tests/cmdline/merge_authz_tests.py tests/cmdline/merge_tests.py tests/cmdline/merge_tree_conflict_tests.py tests/cmdline/svntest/actions.py

From: Paul Burba <ptburba_at_gmail.com>
Date: Fri, 3 Sep 2010 10:49:59 -0400

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

This is an archived mail posted to the Subversion Dev mailing list.