Re: svn commit: r28213 - in trunk/subversion: libsvn_client libsvn_wc tests/cmdline

On Dec 3, 2007 6:31 PM, Paul Burba <pburba@collab.net> wrote:
>
> > -----Original Message-----
> > From: dglasser@gmail.com [mailto:dglasser@gmail.com] On
> > Behalf Of David Glasser
> > Sent: Monday, December 03, 2007 8:11 PM
> > To: dev@subversion.tigris.org; pburba@tigris.org
> > Cc: svn@subversion.tigris.org
> > Subject: Re: svn commit: r28213 - in trunk/subversion:
> > libsvn_client libsvn_wc tests/cmdline
> >
> > On Dec 3, 2007 12:24 PM, <pburba@tigris.org> wrote:
> > > Author: pburba
> > > Date: Mon Dec 3 12:24:18 2007
> > > New Revision: 28213
> > >
> > > Log:
> > > Finish issue #3029 by getting strict on what values ps
> > svn:mergeinfo permits.
> > >
> >
> >
> > >
> > > Modified: trunk/subversion/libsvn_wc/props.c
> > > URL:
> > >
> > http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_wc/props.c?pa
> > > threv=28213&r1=28212&r2=28213
> > >
> > ======================================================================
> > > ========
> > > --- trunk/subversion/libsvn_wc/props.c (original)
> > > +++ trunk/subversion/libsvn_wc/props.c Mon Dec 3 12:24:18 2007
> > > @@ -2645,6 +2645,46 @@
> > > {
> > > new_value =
> > svn_stringbuf_create_from_string(&boolean_value, pool);
> > > }
> > > + else if (strcmp(propname, SVN_PROP_MERGEINFO) == 0)
> > > + {
> > > + apr_hash_t *mergeinfo;
> > > + SVN_ERR(svn_mergeinfo_parse(&mergeinfo,
> > propval->data, pool));
> > > + /* ### TODO: svn_mergeinfo_parse() conveniently
> > performs almost all
> > > + the checks we require for valid
> > svn:mergeinfo...except one, it
> > > + allows path's mapped to empty revision ranges.
> > In addressing this
> > > + issue,
> > http://subversion.tigris.org/issues/show_bug.cgi?id=3029, we
> > > + want to disallow this practice. But if we move
> > enforcement of this
> > > + to svn_mergeinfo_parse() then anyone who has has
> > mergeinfo created
> > > + by a pre-release version of 1.5 which has paths
> > mapped to empty
> > > + ranges is in for a world of hurt. Why? Can't
> > they simply tweak
> > > + their mergeinfo to follow the newly enforced
> > rules? Sure, they
> > > + could, *if* they could see their
> > mergeinfo...which they won't be
> > > + able to, since svn pl/pg will call
> > svn_mergeinfo_parse() resulting
> > > + in an error.
> >
> > Reeeeally? I could have sworn that
> > svn_wc_canonicalize_svn_prop is called only for propsets, on
> > the new value only.
>
> Dave,
>
> Let me rephrase...
>
> svn_wc_canonicalize_svn_prop() now calls svn_mergeinfo_parse(), not
> because it needs to parse mergeinfo per se, but because
> svn_mergeinfo_parse() will QC the prop val and return a helpful error if
> it violates mergeinfo grammar or a few other select rules (overlapping
> ranges, unordered ranges, and backwards ranges).
>
> The one thing svn_mergeinfo_parse() does *not* check is if paths map to
> empty rev ranges (it considers these valid). This check is done
> separately in svn_wc_canonicalize_svn_prop().
>
> My comment is essentially asking, "Ok, then, why not put the check for
> empty ranges in svn_mergeinfo_parse()?" Answer: Because I didn't want
> *svn_mergeinfo_parse()* to start throwing errors when parsing previously
> valid mergeinfo. Problem is, my main concern, that
> svn_mergeinfo_parse() is called when doing a proplist is, uh, how to put
> this, not true. Why, why, why did I think that!?!

Indeed. In fact, what I did was check to see if proplist/propget did
anything mergeinfo-related, confirmed that they didn't, and promptly
wrote the message you replied to which didn't really have much to do
with the useful check that we both made :)

--dave

-- 
David Glasser | glasser_at_davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org