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 <cmpilato@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Received on Tue Nov 27 14:52:28 2007