You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "C. Michael Pilato" <cm...@collab.net> on 2007/11/27 13:52:08 UTC
Re: svn commit: r28053 - in trunk: . subversion/libsvn_client
dlr@tigris.org wrote:
> Author: dlr
> Date: Mon Nov 26 16:17:12 2007
> New Revision: 28053
>
> Log:
> Turn off mergeinfo calculations during 'svn merge' against pre-1.5
> repositories.
AWESOME!
> Modified: trunk/subversion/libsvn_client/merge.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/merge.c?pathrev=28053&r1=28052&r2=28053
> ==============================================================================
> --- trunk/subversion/libsvn_client/merge.c (original)
> +++ trunk/subversion/libsvn_client/merge.c Mon Nov 26 16:17:12 2007
> @@ -43,6 +43,7 @@
> #include "svn_props.h"
> #include "svn_time.h"
> #include "svn_sorts.h"
> +#include "svn_ra.h"
> #include "client.h"
> #include "mergeinfo.h"
> #include <assert.h>
> @@ -170,6 +171,8 @@
> is the same repository as the
> target. Defaults to FALSE if DRY_RUN
> is TRUE.*/
> + svn_boolean_t *mergeinfo_capable; /* Whether the merge source repository
> + is capable of Merge Tracking. */
> svn_boolean_t ignore_ancestry; /* Are we ignoring ancestry (and by
> extension, mergeinfo)? FALSE if
> SOURCES_RELATED is FALSE. */
Wha?! A pointer to a boolean?
> @@ -4299,6 +4305,7 @@
> svn_config_t *cfg;
> const char *diff3_cmd;
> int i;
> + svn_boolean_t mergeinfo_capable = FALSE;
>
> /* If this is a dry-run record-only merge, there's nothing to do. */
> if (record_only && dry_run)
> @@ -4330,6 +4337,7 @@
> merge_cmd_baton.record_only = record_only;
> merge_cmd_baton.ignore_ancestry = ignore_ancestry;
> merge_cmd_baton.same_repos = same_repos;
> + merge_cmd_baton.mergeinfo_capable = NULL;
> merge_cmd_baton.sources_related = sources_related;
> merge_cmd_baton.ctx = ctx;
> merge_cmd_baton.target_missing_child = FALSE;
> @@ -4394,6 +4402,15 @@
> merge_cmd_baton.ra_session1 = ra_session1;
> merge_cmd_baton.ra_session2 = ra_session2;
>
> + /* Populate the portions of the merge context baton that require
> + an RA session to set, but shouldn't be reset for each iteration. */
> + if (merge_cmd_baton.mergeinfo_capable == NULL)
> + {
> + merge_cmd_baton.mergeinfo_capable = &mergeinfo_capable;
> + SVN_ERR(svn_ra_has_capability(ra_session1, &mergeinfo_capable,
> + SVN_RA_CAPABILITY_MERGEINFO, pool));
> + }
> +
> /* If this is a record-only merge and our sources are from the
> same repository as our target, just do the record and move on. */
> if (same_repos && record_only)
I understand what's going on here, but I find this really awkward, trying to
get a ternary state out of a boolean variable. And I'm concerned that it
has a high likelihood of causing programming errors. Because we don't use
pointers-to-boolean very much, programmers might accidentally do something like:
if (merge_b->mergeinfo_capable)
...
Which, of course, ain't the same as:
if (*(merge_b->mergeinfo_capable))
...
May I suggest that we avoid cleverness, just default
merge_cmd_baton.mergeinfo_capable (a straight boolean, now) to TRUE, and use
another boolean checked_capabilities to avoid querying more than once?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r28053 - in trunk: . subversion/libsvn_client
Posted by "Daniel L. Rall" <dl...@finemaltcoding.com>.
On Tue, 27 Nov 2007, C. Michael Pilato wrote:
...
> > + svn_boolean_t *mergeinfo_capable; /* Whether the merge source repository
> > + is capable of Merge Tracking. */
> > svn_boolean_t ignore_ancestry; /* Are we ignoring ancestry (and by
> > extension, mergeinfo)? FALSE if
> > SOURCES_RELATED is FALSE. */
>
> Wha?! A pointer to a boolean?
>
> > @@ -4299,6 +4305,7 @@
> > svn_config_t *cfg;
> > const char *diff3_cmd;
> > int i;
> > + svn_boolean_t mergeinfo_capable = FALSE;
> >
> > /* If this is a dry-run record-only merge, there's nothing to do. */
> > if (record_only && dry_run)
> > @@ -4330,6 +4337,7 @@
> > merge_cmd_baton.record_only = record_only;
> > merge_cmd_baton.ignore_ancestry = ignore_ancestry;
> > merge_cmd_baton.same_repos = same_repos;
> > + merge_cmd_baton.mergeinfo_capable = NULL;
> > merge_cmd_baton.sources_related = sources_related;
> > merge_cmd_baton.ctx = ctx;
> > merge_cmd_baton.target_missing_child = FALSE;
> > @@ -4394,6 +4402,15 @@
> > merge_cmd_baton.ra_session1 = ra_session1;
> > merge_cmd_baton.ra_session2 = ra_session2;
> >
> > + /* Populate the portions of the merge context baton that require
> > + an RA session to set, but shouldn't be reset for each iteration. */
> > + if (merge_cmd_baton.mergeinfo_capable == NULL)
> > + {
> > + merge_cmd_baton.mergeinfo_capable = &mergeinfo_capable;
> > + SVN_ERR(svn_ra_has_capability(ra_session1, &mergeinfo_capable,
> > + SVN_RA_CAPABILITY_MERGEINFO, pool));
> > + }
> > +
> > /* If this is a record-only merge and our sources are from the
> > same repository as our target, just do the record and move on. */
> > if (same_repos && record_only)
>
> I understand what's going on here, but I find this really awkward, trying to
> get a ternary state out of a boolean variable. And I'm concerned that it
> has a high likelihood of causing programming errors. Because we don't use
> pointers-to-boolean very much, programmers might accidentally do something like:
>
> if (merge_b->mergeinfo_capable)
> ...
>
> Which, of course, ain't the same as:
>
> if (*(merge_b->mergeinfo_capable))
> ...
>
> May I suggest that we avoid cleverness, just default
> merge_cmd_baton.mergeinfo_capable (a straight boolean, now) to TRUE, and use
> another boolean checked_capabilities to avoid querying more than once?
Sure, done in r28072. I was already using an additional boolean local to
do_merge() anyways, so your suggestion doesn't even add additional LOCs.
Thanks for the review, Mike!
- Dan