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. */"? 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. 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. ...". Thanks - Julian