On Thu, Sep 2, 2010 at 5:52 PM, Julian Foad <[email protected]> wrote: > [email protected] 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 <[email protected]> wrote: > [email protected] 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
