You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David Glasser <gl...@davidglasser.net> on 2007/12/04 01:10:47 UTC
Re: svn commit: r28213 - in trunk/subversion: libsvn_client libsvn_wc tests/cmdline
On Dec 3, 2007 12:24 PM, <pb...@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?pathrev=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. (Note that it's not
actually called at commit time unless it's a propedit-on-URL, so it is
possible by working around the WC layer to commit invalid props
anyway...)
--dave
--
David Glasser | glasser@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
Re: svn commit: r28213 - in trunk/subversion: libsvn_client libsvn_wc tests/cmdline
Posted by Daniel Rall <dl...@collab.net>.
On Mon, 03 Dec 2007, Paul Burba wrote:
...
> Anyway, my comment had very little to do with
> svn_wc_canonicalize_svn_prop(). And FWIW, after reading Dan's thoughts
> here, http://svn.haxx.se/dev/archive-2007-12/0100.shtml, I was planning
> to move the empty range check into svn_mergeinfo_parse() and remove this
> comment.
>
> Geez, I hope this all makes some sense(?)
Absolutely, Paul. :-)
- Dan
Re: svn commit: r28213 - in trunk/subversion: libsvn_client libsvn_wc tests/cmdline
Posted by David Glasser <gl...@davidglasser.net>.
On Dec 3, 2007 6:31 PM, Paul Burba <pb...@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, <pb...@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@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
Re: svn commit: r28213 - in trunk/subversion: libsvn_client libsvn_wc tests/cmdline
Posted by Paul Burba <pb...@collab.net>.
> -----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, <pb...@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!?!
Anyway, my comment had very little to do with
svn_wc_canonicalize_svn_prop(). And FWIW, after reading Dan's thoughts
here, http://svn.haxx.se/dev/archive-2007-12/0100.shtml, I was planning
to move the empty range check into svn_mergeinfo_parse() and remove this
comment.
Geez, I hope this all makes some sense(?)
Paul
> (Note that it's not actually called at
> commit time unless it's a propedit-on-URL, so it is possible
> by working around the WC layer to commit invalid props
> anyway...)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org