Re: r32158: svn_repos_mergeinfo_changed() - meaning vs. representation

[Julian Foad <ju...@btopenworld.com>]

On Mon, 2008-07-21 at 10:25 -0400, C. Michael Pilato wrote:
> Julian Foad wrote:
> > A question about mergeinfo representation versus mergeinfo canonical
> > meaning after taking account of inheritance etc. (Bear in mind that I'm
> > not well aquainted with this merging stuff.)
> > 
> > The new function svn_repos_mergeinfo_changed() looks like a useful
> > helper for things that want to know in what way mergeinfo changed in a
> > particular revision.
> > 
> >> /* Set @a *deleted_mergeinfo_catalog and @a *added_mergeinfo_catalog
> >>  * to mergeinfo catalogs describing how mergeinfo values on paths
> >>  * (which are the keys of those catalogs) were changed via the commit
> >>  * of @a rev in @a repos.  Allocate returned data from @a pool.
> >>  *
> >>  * Every path whose mergeinfo was created or modified in @a rev in
> >>  * such a way that it differs from its value in @a rev - 1 is
> >>  * represented in both returned catalog hashes, and only those paths
> >>  * are represented therein.
> >>  *
> >>  * @since New in 1.6.
> >>  */
> >> svn_error_t *
> >> svn_repos_mergeinfo_changed(svn_mergeinfo_catalog_t *deleted_mergeinfo_catalog,
> >>                             svn_mergeinfo_catalog_t *added_mergeinfo_catalog,
> >>                             svn_repos_t *repos,
> >>                             svn_revnum_t rev,
> >>                             apr_pool_t *pool);
> > 
> >>From the fact that it reports its results in "svn_mergeinfo_catalog_t"
> > which has separate representations for "no mergeinfo" and "empty
> > mergeinfo", it looks like it reports changes to the mergeinfo
> > representation, so, for example, if somebody "cleans up" their mergeinfo
> > properties by performing some elision of mergeinfo that would be
> > inherited, or by removing inoperative revision ranges, that would be
> > reported as a change.
> > 
> > Have I understood it correctly?
> 
> That's correct.  Just as svn_fs_get_mergeinfo()
> 
> > There's nothing wrong with that as such, but I'm wondering:
> > 
> >   * Do we have the complementary support function(s) for determining
> > whether the canonical meaning of the mergeinfo has changed?
> 
> No.
> 
> >   * Where are we drawing the architectural line between APIs that deal
> > with the representation and APIs that deal with the canonical meaning?
> >
> >   * Do we need to make that distinction clearer in the doc strings,
> > perhaps using terminology like "mergeinfo properties" versus "canonical
> > mergeinfo"?
> 
> Per this line of questioning, I'm wondering if you'd have been happier if 
> I'd named the function svn_repos_fs_props_changed() and had it accept any 
> property name and return a hash of path->propvals.  :-)

Mmm... Separating the basic data-gathering algorithm from any
mergeinfo-specific processing is architecturally good. I hadn't thought
of that here but I've just been thinking the analogous thing about some
libsvn_wc APIs, so +1.

> > Also, does this new function need "authz_read" functionality like the
> > v1.5 function svn_repos_fs_get_mergeinfo() has, or need to document its
> > behaviour in this regard?
> 
> Ah, yes, it probably does if it's going to be public.
> 
> Many of these concerns appeared the minute I made the function public from 
> its previous internal-helper state.  The easiest fix for them all is to 
> de-publicize it again and revisit when/if third-party software grows a 
> tangible need for the functionality.  Shall I go that route?

Seems like it might be the most effective thing to do here.

- Julian



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org