On Tue, 2008-09-16 at 17:21 -0700, pburba_at_tigris.org wrote:
> Author: pburba
> Date: Tue Sep 16 17:21:53 2008
> New Revision: 33112
>
> Log:
> Fix a bug and greatly improve merge performance in a common use case.
>
> Quite a while back glasser pointed out that the code to filter
> self-referential mergeinfo was a performance hog - see
> http://svn.haxx.se/dev/archive-2008-02/0206.shtml. More recently arfrever
> found a merge bug where this filtering code removed valid mergeinfo - see
> http://subversion.tigris.org/servlets/ReadMsg?list=dev&&msgNo=142849.
> This change implements glasser's suggestion, which also fixes the problem
> arfrever noted.
>
> Suggested by: glasser
>
> * subversion/libsvn_client/merge.c
> (split_mergeinfo_on_revision): New.
> (filter_self_referential_mergeinfo): Where possible use
> svn_client__get_history_as_mergeinfo() one time per incoming mergeinfo
> merge source rather than svn_client__repos_locations() for each discrete
> range in each merge source's rangelist.
>
> * subversion/tests/cmdline/merge_tests.py
> (natural_history_filtering): Remove comment about XFail.
> (test_list): Remove XFail from natural_history_filtering.
[...]
> + /* So far we've only been manipulating rangelists, now we
> + actually create *YOUNGER_MERGEINFO and then remove the older
> + ranges from *MERGEINFO */
> + if (!(*younger_mergeinfo))
> + *younger_mergeinfo = apr_hash_make(pool);
> + apr_hash_set(*younger_mergeinfo,
> + (const char *)merge_source_path,
No functional harm, but that's a redundant cast.
> + APR_HASH_KEY_STRING, younger_rangelist);
> + SVN_ERR(svn_mergeinfo_remove(mergeinfo, *younger_mergeinfo,
> + *mergeinfo, pool));
> + break; /* ...out of for (i = 0; i < rangelist->nelts; i++) */
> + }
[...]
> + /* Check if PATH_at_TARGET_ENTRY->REVISION exists at
> + RANGE->START on the same line of history. */
> + err = svn_client__repos_locations(&start_url,
> + &start_revision,
> + NULL,
> + NULL,
> + merge_b->ra_session2,
> + target_url,
> + &peg_rev,
> + &rev1_opt,
> + &rev2_opt,
> + merge_b->ctx,
> + pool);
> + if (err)
> {
> - /* PATH_at_TARGET_ENTRY->REVISION didn't exist at
> - RANGE->START or is unrelated to the resource
> - PATH_at_RANGE->START. Either way we don't
> - filter. */
> - svn_error_clear(err);
> - err = NULL;
> - APR_ARRAY_PUSH(adjusted_rangelist,
> - svn_merge_range_t *) = range;
> - }
> + if (err->apr_err == SVN_ERR_CLIENT_UNRELATED_RESOURCES
> + || err->apr_err == SVN_ERR_FS_NOT_FOUND
> + || err->apr_err == SVN_ERR_FS_NO_SUCH_REVISION)
The addition of SVN_ERR_FS_NO_SUCH_REVISION here should be supported by
documenting that svn_client__repos_locations() does in fact return this
error code. (I assume it was found to do so in practice, but I cannot
see where it comes from.)
Note also that in r33296, cmpilato removed a bit of redundant code that
was introduced in this function, that looked like:
[[[
const char *merge_source_root_url;
SVN_ERR(svn_ra_get_repos_root2(merge_b->ra_session2,
&merge_source_root_url, pool));
]]]
This caused GCC to issue a warning because the same variable is declared
and initialised again at an inner scope, but is otherwise harmless.
r33296 is not proposed for back-port.
Everything else looks fine.
I have voted +1 on back-porting to 1.5.x. (Haven't time tonight to
review the test as well.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-10-06 23:59:34 CEST