[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r28053 - in trunk: . subversion/libsvn_client

From: C. Michael Pilato <cmpilato_at_collab.net>
Date: 2007-11-27 14:52:08 CET

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

This is an archived mail posted to the Subversion Dev mailing list.