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

Reply via email to