Julian Foad wrote on Fri, Jun 21, 2013 at 15:04:39 +0100:
> Daniel Shahaf wrote:
>
> > C. Michael Pilato wrote on Fri, Jun 21, 2013 at 09:00:46 -0400:
> >> On 06/21/2013 06:43 AM, Daniel Shahaf wrote:
> >> > I still think the logic better belongs inside
> > svn_repos__validate_prop().
> >> > (Not the least because it has three other callers which also need to
> >> > accept NULL values.)
> >> >
> >> > --- subversion/libsvn_repos/fs-wrap.c (revision 1495373)
> >> > +++ subversion/libsvn_repos/fs-wrap.c (working copy)
> >> > @@ -172,6 +172,10 @@ svn_repos__validate_prop(const char *name,
> >> > {
> >> > svn_prop_kind_t kind = svn_property_kind2(name);
> >> >
> >> > + /* No property is mandatory. */
> >> > + if (value == NULL)
> >> > + return SVN_NO_ERROR;
> >> > +
> >> > /* Disallow setting non-regular properties. */
> >> > if (kind != svn_prop_regular_kind)
> >> > return svn_error_createf
> >>
> >> +1. This is precisely what I was suggesting as well.
> >
> > Committed and nominated for backport. Arwin, if you think that's not
> > a good solution, we can still amend it in subsequent revisions. Thanks
> > for the patch.
> >
> > Daniel
>
> Daniel, some comments on your patch.
>
> "/* No property is mandatory. */" doesn't make sense to me as a comment. How
> about "/* Allow deleting any property, even an invalid property. */"?
>
Added with tweaks in
r1495446.
> The code immediately after the chunk shown above continues like so:
>
> /* Validate "svn:" properties. */
> if (svn_prop_is_svn_prop(name) && value != NULL)
> {
> ...
> }
> return SVN_NO_ERROR;
>
> The "value != NULL" there can be removed now.
>
I'd rather not. The block dereferences 'value', so it's clearer (and
more robust against future refactorings) to leave the validation in.
The optimizer will remove it.
> Please update the doc string, which currently begins "Validate that property
> @a name is valid for use in a Subversion repository; return @c
> SVN_ERR_REPOS_BAD_ARGS if it isn't. ...".
Discussing this on IRC...
