You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Arwin Arni <ar...@collab.net> on 2013/06/20 14:45:52 UTC
[PATCH] Don't validate properties during deletion.
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
Re: [PATCH] Don't validate properties during deletion.
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
> On Mon, Jun 24, 2013 at 04:32:34PM +0100, Julian Foad wrote:
>> Daniel Shahaf <da...@elego.de>
>>> Daniel Shahaf wrote on Fri, Jun 21, 2013 at 17:28:19 +0300:
>>>> Julian Foad wrote on Fri, Jun 21, 2013 at 15:04:39 +0100:
>>>>> 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...
>>>
>>> Ping? IIRC you had a draft paste somewhere, has it been committed?
>>
>> I pasted a suggestion as a follow-up paste to yours (same pastebin URL, see
>> IRC), and we discussed it. I didn't revise it or commit anything. If you
>> want me to commit something, I can.
>
> Hmmm what happened?
> Has the discussed docstring tweak been committed yet? :)
Daniel asked me to do it and I've just done it. r1496469.
- Julian
Re: [PATCH] Don't validate properties during deletion.
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Jun 24, 2013 at 04:32:34PM +0100, Julian Foad wrote:
> Daniel Shahaf <da...@elego.de>
>
> > To: Julian Foad <ju...@btopenworld.com>
> > Cc: dev@subversion.apache.org
> > Sent: Monday, 24 June 2013, 5:13
> > Subject: Re: [PATCH] Don't validate properties during deletion.
> >
> > Daniel Shahaf wrote on Fri, Jun 21, 2013 at 17:28:19 +0300:
> >> Julian Foad wrote on Fri, Jun 21, 2013 at 15:04:39 +0100:
> >> > 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...
> >
> > Ping? IIRC you had a draft paste somewhere, has it been committed?
>
> I pasted a suggestion as a follow-up paste to yours (same pastebin URL, see IRC), and we discussed it. I didn't revise it or commit anything. If you want me to commit something, I can.
>
> - Julian
Hmmm what happened?
Has the discussed docstring tweak been committed yet? :)
Re: [PATCH] Don't validate properties during deletion.
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Shahaf <da...@elego.de>
> To: Julian Foad <ju...@btopenworld.com>
> Cc: dev@subversion.apache.org
> Sent: Monday, 24 June 2013, 5:13
> Subject: Re: [PATCH] Don't validate properties during deletion.
>
> Daniel Shahaf wrote on Fri, Jun 21, 2013 at 17:28:19 +0300:
>> Julian Foad wrote on Fri, Jun 21, 2013 at 15:04:39 +0100:
>> > 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...
>
> Ping? IIRC you had a draft paste somewhere, has it been committed?
I pasted a suggestion as a follow-up paste to yours (same pastebin URL, see IRC), and we discussed it. I didn't revise it or commit anything. If you want me to commit something, I can.
- Julian
Re: [PATCH] Don't validate properties during deletion.
Posted by Daniel Shahaf <da...@elego.de>.
Daniel Shahaf wrote on Fri, Jun 21, 2013 at 17:28:19 +0300:
> Julian Foad wrote on Fri, Jun 21, 2013 at 15:04:39 +0100:
> > 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...
Ping? IIRC you had a draft paste somewhere, has it been committed?
Re: [PATCH] Don't validate properties during deletion.
Posted by Daniel Shahaf <da...@elego.de>.
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...
Re: [PATCH] Don't validate properties during deletion.
Posted by Julian Foad <ju...@btopenworld.com>.
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
Re: [PATCH] Don't validate properties during deletion.
Posted by Arwin Arni <ar...@collab.net>.
Daniel Shahaf <da...@elego.de> 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
No. I get it now. It's better to check if it's a delete and easy out from within the validation code so all callers of the function will benefit. They don't have to perform the check themselves.
Regards,
Arwin Arni
Re: [PATCH] Don't validate properties during deletion.
Posted by Daniel Shahaf <da...@elego.de>.
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
Re: [PATCH] Don't validate properties during deletion.
Posted by "C. Michael Pilato" <cm...@collab.net>.
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.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: [PATCH] Don't validate properties during deletion.
Posted by Daniel Shahaf <da...@elego.de>.
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
Re: [PATCH] Don't validate properties during deletion.
Posted by Arwin Arni <ar...@collab.net>.
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.
>
>> + }
>>
>> /* Fetch OLD_VALUE for svn_fs_change_rev_prop2(). */
>> if (old_value_p)
Re: [PATCH] Don't validate properties during deletion.
Posted by Daniel Shahaf <da...@apache.org>.
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
> + }
>
> /* Fetch OLD_VALUE for svn_fs_change_rev_prop2(). */
> if (old_value_p)
AW: [PATCH] Don't validate properties during deletion.
Posted by Markus Schaber <m....@codesys.com>.
Hi, Arwin,
Von: Arwin Arni [mailto:arwin@collab.net]
> On 06/20/2013 07:01 PM, Julian Foad wrote:
> > Arwin Arni wrote:
> >> 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.
> > What cases are you talking about, specifically?
>
> Iam talking about svn:entry:committed-date. I came across a repository with
> this non-regular property set on a bunch of revisions. The current code
> disallows the addition (and deletion) of such properties, so figuring out
> when/how that property got there in the first place is something I'm yet to
> look into. I'm guessing it was a very old libsvn_repos that allowed this.
Another culprit might be svnkit (they implement all layers on their own).
Best regards
Markus Schaber
CODESYS(r) a trademark of 3S-Smart Software Solutions GmbH
Inspiring Automation Solutions
3S-Smart Software Solutions GmbH
Dipl.-Inf. Markus Schaber | Product Development Core Technology
Memminger Str. 151 | 87439 Kempten | Germany
Tel. +49-831-54031-979 | Fax +49-831-54031-50
E-Mail: m.schaber@codesys.com | Web: http://www.codesys.com | CODESYS store: http://store.codesys.com
CODESYS forum: http://forum.codesys.com
Managing Directors: Dipl.Inf. Dieter Hess, Dipl.Inf. Manfred Werner | Trade register: Kempten HRB 6186 | Tax ID No.: DE 167014915
Re: [PATCH] Don't validate properties during deletion.
Posted by Arwin Arni <ar...@collab.net>.
On 06/20/2013 07:01 PM, Julian Foad wrote:
> Arwin Arni wrote:
>> 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.
> What cases are you talking about, specifically?
Iam talking about svn:entry:committed-date. I came across a repository
with this non-regular property set on a bunch of revisions. The current
code disallows the addition (and deletion) of such properties, so
figuring out when/how that property got there in the first place is
something I'm yet to look into. I'm guessing it was a very old
libsvn_repos that allowed this.
> Invalid prop names that are rejected by 'svn ps' are not rejected by 'svn propdel':
>
> [[[
> $ svn ps svn:foo v .
> svn: E195011: 'svn:foo' is not a valid svn: property name; re-run with '--force' to set it
> $ svn ps svn:foo v . --force
> property 'svn:foo' set on '.'
> $ svn pd svn:foo .
> property 'svn:foo' deleted from '.'.
> ]]]
>
> Invalid prop values that are rejected by 'svn ps' are not rejected by 'svn propdel':
>
> [[[
> $ svn ps svn:externals foo-bar .
> svn: E195005: Error parsing svn:externals property on 'wc': 'foo-bar'
> $ svn ps svn:externals foo-bar . --force
> svn: E195005: Error parsing svn:externals property on 'wc': 'foo-bar'
> $ svn pl -v
> [...nothing shown...]
>
> [...edit the repository by hand to create an invalid prop value...]
>
> $ svn pl -vR
> Properties on '.':
> svn:externals
> foo-bar
> $ svn pd svn:externals .
> property 'svn:externals' deleted from '.'.
> ]]]
>
> at least for the cases I tried, using an svn 1.8.0 release candidate.
>
>> 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.
> Agreed. We discussed this before, at least in terms of the command-line client, and that was the conclusion we agreed on. The principle should apply consistently, so +1 to fix any APIs or wherever you're seeing the problem, to do the same.
>
> - Julian
Re: [PATCH] Don't validate properties during deletion.
Posted by Julian Foad <ju...@btopenworld.com>.
I (Julian Foad) wrote:
> What cases are you talking about, specifically?
[...]
> Agreed. We discussed this before, at least in terms of the command-line client,
> and that was the conclusion we agreed on. The principle should apply
> consistently, so +1 to fix any APIs or wherever you're seeing the problem,
> to do the same.
Huh, never mind. Somehow I didn't notice before that you had attached a patch, and that patch makes clear that the problem you found is in the FS layer.
- Julian
Re: [PATCH] Don't validate properties during deletion.
Posted by Julian Foad <ju...@btopenworld.com>.
Arwin Arni wrote:
> 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.
What cases are you talking about, specifically?
Invalid prop names that are rejected by 'svn ps' are not rejected by 'svn propdel':
[[[
$ svn ps svn:foo v .
svn: E195011: 'svn:foo' is not a valid svn: property name; re-run with '--force' to set it
$ svn ps svn:foo v . --force
property 'svn:foo' set on '.'
$ svn pd svn:foo .
property 'svn:foo' deleted from '.'.
]]]
Invalid prop values that are rejected by 'svn ps' are not rejected by 'svn propdel':
[[[
$ svn ps svn:externals foo-bar .
svn: E195005: Error parsing svn:externals property on 'wc': 'foo-bar'
$ svn ps svn:externals foo-bar . --force
svn: E195005: Error parsing svn:externals property on 'wc': 'foo-bar'
$ svn pl -v
[...nothing shown...]
[...edit the repository by hand to create an invalid prop value...]
$ svn pl -vR
Properties on '.':
svn:externals
foo-bar
$ svn pd svn:externals .
property 'svn:externals' deleted from '.'.
]]]
at least for the cases I tried, using an svn 1.8.0 release candidate.
> 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.
Agreed. We discussed this before, at least in terms of the command-line client, and that was the conclusion we agreed on. The principle should apply consistently, so +1 to fix any APIs or wherever you're seeing the problem, to do the same.
- Julian
Re: [PATCH] Don't validate properties during deletion.
Posted by Arwin Arni <ar...@collab.net>.
On 06/20/2013 06:30 PM, C. Michael Pilato wrote:
> Perhaps the better change is to make svn_repos__validate_prop() early-out
> (with success, of course) on a NULL value and adjust the rest of the code
> therein accordingly. I can see the benefit in flagging the *addition* (or
> modification) of a non-regular property (which is the first test that
> function performs), but there's need to block the ability to cleanup after
> such an errant change has already occurred. And we don't have any
> properties which are required to be present. So, again, I say there's no
> need to validate any type of property deletion.
Hi,
I'm not entirely sure what you want me to do. My current patch prevents
verification entirely during a delete. I don't see any merit in stepping
inside svn_repos__validate_prop. Also, did you mean to say "there's *no*
need to block the ability to cleanup after such an errant change has
already occurred" ?
Regards,
Arwin Arni
Re: [PATCH] Don't validate properties during deletion.
Posted by "C. Michael Pilato" <cm...@collab.net>.
Perhaps the better change is to make svn_repos__validate_prop() early-out
(with success, of course) on a NULL value and adjust the rest of the code
therein accordingly. I can see the benefit in flagging the *addition* (or
modification) of a non-regular property (which is the first test that
function performs), but there's need to block the ability to cleanup after
such an errant change has already occurred. And we don't have any
properties which are required to be present. So, again, I say there's no
need to validate any type of property deletion.