Re: svn commit: r25233 - in trunk/subversion: libsvn_client tests/cmdline

Nice fix, Paul!

In r25235, I refactored the duplicate code used to determine into a
helper function. Along the way, I realized that if record_only is
set, that we don't actually care about the value of same_repos. It
would be more efficient to either a) not calculate same_repos when
record_only is true, or b) delay calculation until same_repos is
actually needed (sticking any necessary contextual data in the baton).
Is this minor efficiency gain worth the additional complexity from (a)
or (b)? I lean towards "no", but thought I'd mention it anyways..

On Thu, 31 May 2007, pburba@tigris.org wrote:

> Author: pburba
> Date: Thu May 31 09:06:00 2007
> New Revision: 25233
>
> Log:
> Fix issue #2788, Don't update merge info when merging from another repos.
>
> * subversion/libsvn_client/merge.c
> (merge_cmd_baton): Add new field same_repos.
> (do_merge, do_single_file_merge): Don't update WC merge info unless
> same_repos flag is set.
> (svn_client_merge3, svn_client_merge_peg3): Initialize new same_repos field
> in the merge command baton.
>
> * subversion/tests/cmdline/merge_tests.py
> (test_list): Remove XFail from diff_repos_does_not_update_mergeinfo.
>
> Modified:
> trunk/subversion/libsvn_client/merge.c
> trunk/subversion/tests/cmdline/merge_tests.py
>
> Modified: trunk/subversion/libsvn_client/merge.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/merge.c?pathrev=25233&r1=25232&r2=25233
> ==============================================================================
> --- trunk/subversion/libsvn_client/merge.c (original)
> +++ trunk/subversion/libsvn_client/merge.c Thu May 31 09:06:00 2007
...
> if (merge_b->record_only)
> {
> - if (merge_b->dry_run)
> + if (merge_b->dry_run || !merge_b->same_repos)
> {
> return SVN_NO_ERROR;
> }
> @@ -2009,7 +2012,7 @@
>
> if (notify_b.same_urls)
> {
> - if (!merge_b->dry_run)
> + if (!merge_b->dry_run && merge_b->same_repos)
> {
> /* Update the WC merge info here to account for our new
> merges, minus any unresolved conflicts and skips. */
> @@ -2223,7 +2226,7 @@
> merge for the specified range. */
> if (merge_b->record_only)
> {
> - if (merge_b->dry_run)
> + if (merge_b->dry_run || !merge_b->same_repos)
> {
> return SVN_NO_ERROR;
> }
> @@ -2328,7 +2331,7 @@
>
> if (notify_b.same_urls)
> {
> - if (!merge_b->dry_run)
> + if (!merge_b->dry_run && merge_b->same_repos)
> {
> /* Update the WC merge info here to account for our new
> merges, minus any unresolved conflicts and skips. */
> @@ -2555,6 +2558,23 @@
> merge_cmd_baton.add_necessitated_merge = FALSE;
> merge_cmd_baton.dry_run_deletions = (dry_run ? apr_hash_make(pool) : NULL);
> merge_cmd_baton.ctx = ctx;
> +
> + /* Remember if merge source is a different repos from the target so we
> + don't set merge info. */
> + {
> + svn_ra_session_t *ra_session;
> + const char *source_root;
> +
> + /* No need to check URL2 since if it's from a different repository
> + than URL1, then the whole merge will fail anyway. */
> + SVN_ERR(svn_client__open_ra_session_internal(&ra_session, URL1, NULL,
> + NULL, NULL, FALSE,
> + TRUE, ctx, pool));
> + SVN_ERR(svn_ra_get_repos_root(ra_session, &source_root, pool));
> + merge_cmd_baton.same_repos =
> + svn_path_is_ancestor(source_root, entry->repos) ? TRUE : FALSE;
> + }
> +
> merge_cmd_baton.pool = pool;
>
> /* Set up the diff3 command, so various callers don't have to. */
> @@ -2786,6 +2806,21 @@
> merge_cmd_baton.add_necessitated_merge = FALSE;
> merge_cmd_baton.dry_run_deletions = (dry_run ? apr_hash_make(pool) : NULL);
> merge_cmd_baton.ctx = ctx;
> +
> + /* Remember if merge source is a different repos from the target so we
> + don't set merge info. */
> + {
> + svn_ra_session_t *ra_session;
> + const char *source_root;
> +
> + SVN_ERR(svn_client__open_ra_session_internal(&ra_session, URL, NULL,
> + NULL, NULL, FALSE,
> + TRUE, ctx, pool));
> + SVN_ERR(svn_ra_get_repos_root(ra_session, &source_root, pool));
> + merge_cmd_baton.same_repos =
> + svn_path_is_ancestor(source_root, entry->repos) ? TRUE : FALSE;
> + }
> +
...