Arwin Arni wrote on Fri, Jun 21, 2013 at 12:07:00 +0530:
> On 06/20/2013 06:46 PM, Daniel Shahaf wrote:
>> On Thu, Jun 20, 2013 at 06:15:52PM +0530, Arwin Arni wrote:
>>> Hi,
>>>
>>> Properties are validated regardless of the action on the property.
>>> This poses a problem when it comes to very old repositories that
>>> contain "invalid" properties committed by very old clients that
>>> didn't perform these validations. My contention is that we need to
>>> check the validity of the property only during addition and
>>> modification. Validation during deletion prevents the user from
>>> removing what he recognizes as an invalid property.
>>>
>>> Regards,
>>> Arwin Arni
>>> * subversion/libsvn_repos/fs-wrap.c
>>> (svn_repos_fs_change_rev_prop4): Don't validate properties during
>>> deletion.
>>>
>>> Patch by Arwin Arni <arwin{_AT_}collab.net>
>>> Index: subversion/libsvn_repos/fs-wrap.c
>>> ===================================================================
>>> --- subversion/libsvn_repos/fs-wrap.c (revision 1494892)
>>> +++ subversion/libsvn_repos/fs-wrap.c (working copy)
>>> @@ -331,7 +331,12 @@
>>> char action;
>>> apr_hash_t *hooks_env;
>>> - SVN_ERR(svn_repos__validate_prop(name, new_value, pool));
>>> + /* We should validate properties only for additions and
>>> + modifications. NEW_VALUE exists in both these cases. */
>>> + if (new_value)
>>> + {
>>> + SVN_ERR(svn_repos__validate_prop(name, new_value, pool));
>> Shouldn't svn_repos__validate_prop accept NULL values as valid?
>>
>> There is (at present) no value of NAME for which NULL is not a valid value.
>>
>> Daniel
>
> The idea is to not even bother with verification if new_value is NULL,
> as this means it is a property deletion.
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
