Madan U Sreenivasan wrote:
> [[[
> Fixed Issue #2220: propdel returns success on deleting a non-existent prop.
>
> * subversion/libsvn_wc/props.c
> (svn_wc_prop_set2): Modified with check to see
> if property already exists.
This contravenes the API. The doc string says "If @a force is true, do no
validity checking." However, this patch causes a deletion to fail (even with
"force") where before it would have succeeded (in the sense of ensuring the
property is not present, regardless whether it was before).
My feeling is that this function should fail to delete a non-existent property
without "force", but succeed with "force". I would accept a patch that makes
svn_wc_prop_set2 fail only if "force" is set, if it also makes the "propdel"
command pass force=true so that the command-line interface behaviour is
unchanged. That would not by itself solve issue #2220, but it would be a step
closer.
The current behaviour is useful in usage like "svn propdel myprop --recursive
*" where not all files have the property. Do others think it would be
acceptable to change the behaviour of "svn propdel" so that it fails
unconditionally if the property does not exist? (I vote -1.)
"svn propdel" doesn't accept "--force". What about adding the "--force" option
and requiring that to get the old behaviour? (I vote +0.)
>
> * subversion/tests/clients/cmdline/prop_tests.py
> (remove_nonexistent_prop): Added function
> to test removal of non-existent property.
Also:
(test_list): Add the new test.
> ]]]
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/libsvn_wc/props.c
> ===================================================================
> --- subversion/libsvn_wc/props.c (revision 13553)
> +++ subversion/libsvn_wc/props.c (working copy)
> @@ -1188,6 +1188,13 @@
> property into it. */
> if (new_value)
> value = svn_string_create_from_buf (new_value, pool);
> + /* value == NULL means the operation is propdel
> + * if so, check if such a property already exists.
> + * If not, return an error
> + */
> + if (!value && (apr_hash_get (prophash, name, APR_HASH_KEY_STRING)) == NULL)
> + return svn_error_createf (SVN_ERR_BAD_PROP_KIND, NULL,
SVN_ERR_PROPERTY_NOT_FOUND would be more appropriate.
> + _("Property '%s' does not exist"), name);
> apr_hash_set (prophash, name, APR_HASH_KEY_STRING, value);
>
> /* Open the propfile for writing. */
> Index: subversion/tests/clients/cmdline/prop_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/prop_tests.py (revision 13553)
> +++ subversion/tests/clients/cmdline/prop_tests.py (working copy)
> @@ -1123,6 +1123,25 @@
> verify_output([ prop1 + ' : ' + propval1, prop2 + ' : ' + propval2,
> 'Properties on ' ], output, errput)
>
> +#----------------------------------------------------------------------
> +# remove non existent property
That line is not needed because...
> +def remove_nonexistent_prop(sbox):
> + "remove a property that does not exist"
... the doc string is here.
> +
> + # Bootstrap
> + sbox.build()
> + wc_dir = sbox.wc_dir
> +
> + # delete a non-existent property from a file
> + iota_path = os.path.join(wc_dir, 'iota')
> + exp_stderr = "svn: Property 'thispropdoesntexist' does not exist"
Checking for this exact error message seems unnecessarily strict, but it's safe.
> + output, errput = svntest.main.run_svn(1, 'propdel',
> + 'thispropdoesntexist',
> + iota_path)
> + if(errput[1].find(exp_stderr) == -1):
When no error is given, as before this fix, then the test fails with
"'errput[1]': list index out of range" which is a bit ugly but probably OK.
> + print 'Error: expected stderr: ', exp_stderr
> + print ' actual stderr : ', errput[1]
> + raise svntest.Failure
>
> ########################################################################
> # Run the tests
> @@ -1148,6 +1167,7 @@
> binary_props,
> recursive_base_wc_ops,
> url_props_ops,
> + remove_nonexistent_prop,
> ]
>
> if __name__ == '__main__':
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Mar 23 00:58:22 2005