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

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