You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Rall <dl...@collab.net> on 2007/05/31 17:36:09 UTC
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;
> + }
> +
...
Discussion of the fix for avoiding updating merge info when merging from another repos (issue #2788/r25233)
Posted by Daniel Rall <dl...@collab.net>.
On Fri, 01 Jun 2007, Paul Burba wrote:
> > -----Original Message-----
> > From: Daniel Rall [mailto:dlr@collab.net]
...
> > Along the way, I realized that if
> > record_only is set, that we don't actually care about the
> > value of same_repos.
>
> I don't think I follow, are you saying a merge without --record-only
> from a different repos won't update mergeinfo, but with --record-only it
> will?
You didn't follow because what I wrote didn't make any sense.
Substitute "--dry-run" for "--record-only" (as I intended to write),
and it does.
> > 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..
...
RE: svn commit: r25233 - in trunk/subversion: libsvn_client tests/cmdline
Posted by Paul Burba <pb...@collab.net>.
> -----Original Message-----
> From: Daniel Rall [mailto:dlr@collab.net]
> Sent: Thursday, May 31, 2007 1:36 PM
> To: dev@subversion.tigris.org
> Cc: pburba@tigris.org
> Subject: 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.
Thanks Dan, my code duplication detector was busted :-\
> Along the way, I realized that if
> record_only is set, that we don't actually care about the
> value of same_repos.
I don't think I follow, are you saying a merge without --record-only
from a different repos won't update mergeinfo, but with --record-only it
will?
> 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;
> > + }
> > +
> ...
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org