Madan US wrote:
> [[[
> Issue #2220: propdel returns success on deleting a non-existent prop.
This is work towards issue 2220, but does not resolve the whole issue.
> * subversion/libsvn_wc/props.c
> (svn_wc_prop_set2): Modified with check to see
> if property already exists. Check done only if
> skip_checks is not TRUE.
OK, though I would describe the change as "Don't allow deletion of a
non-existent property unless 'skip_checks' is true."
> * subversion/clients/cmdline/propdel-cmd.c
> (svn_cl__propdel): Modified to always pass TRUE
> for the skip_checks parameter of svn_client_propset2().
Reason: to preserve the current UI behaviour. I suggest: "Pass TRUE for the
skip_checks parameter of svn_client_propset2() to preserve the current UI
behaviour."
> * subversion/tests/clients/cmdline/prop_tests.py
> (remove_nonexistent_prop): Added function
> to test for error on removal of non-existent property.
>
> * subversion/tests/clients/cmdline/prop_tests.py
> (test_list): Added function with Skip().
The test is related to a UI change which we have not yet agreed to make. I
suggest you make the API change and the UI change in two separate patches.
> ]]]
>
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/libsvn_wc/props.c
> ===================================================================
> --- subversion/libsvn_wc/props.c (revision 13823)
> +++ subversion/libsvn_wc/props.c (working copy)
> @@ -1213,6 +1213,16 @@
> 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 - do all this only
> + * if skip_checks is not TRUE
> + */
> + if (skip_checks != TRUE
Please don't compare boolean values with FALSE or TRUE explicitly. These are
all equivalent, so use the first (simplest) one:
if (! skip_checks)
if (skip_checks == FALSE)
if (skip_checks != TRUE)
if (! (skip_checks == TRUE))
if ((skip_checks != TRUE) == TRUE)
if ((skip_checks == TRUE) == FALSE)
if (((skip_checks != TRUE) == TRUE) == TRUE)
...
> + && (value == NULL)
I don't mind pointers being compared explicitly with NULL, though "if (!
value)" is just as good.
> + && (apr_hash_get (prophash, name, APR_HASH_KEY_STRING)) == NULL)
> + return svn_error_createf (SVN_ERR_BAD_PROP_KIND, NULL,
As I said when you posted your first version, SVN_ERR_PROPERTY_NOT_FOUND would
be more appropriate. Would it not?
> + _("Property '%s' does not exist"), name);
> apr_hash_set (prophash, name, APR_HASH_KEY_STRING, value);
>
> /* Open the propfile for writing. */
> Index: subversion/clients/cmdline/propdel-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/propdel-cmd.c (revision 13823)
> +++ subversion/clients/cmdline/propdel-cmd.c (working copy)
> @@ -119,7 +119,7 @@
> opt_state->force doesn't apply to this command anyway. */
> SVN_CL__TRY (svn_client_propset2 (pname_utf8, NULL, target,
> opt_state->recursive,
> - FALSE, ctx, subpool),
> + TRUE, ctx, subpool),
> success);
>
> if (success && (! opt_state->quiet))
> Index: subversion/tests/clients/cmdline/prop_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/prop_tests.py (revision 13823)
> +++ subversion/tests/clients/cmdline/prop_tests.py (working copy)
> @@ -1112,6 +1112,24 @@
> verify_output([ prop1 + ' : ' + propval1, prop2 + ' : ' + propval2,
> 'Properties on ' ], output, errput)
>
> +#----------------------------------------------------------------------
> +def remove_nonexistent_prop(sbox):
> + "remove a property that does not exist"
> +
> + # 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"
> + output, errput = svntest.main.run_svn(1, 'propdel',
> + 'thispropdoesntexist',
> + iota_path)
> + if(errput[1].find(exp_stderr) == -1):
Does the line you are checking appear at index 1 in both maintainer-mode and
user-mode builds (i.e. with and without the extra error messages)?
> + print 'Error: expected stderr: ', exp_stderr
> + print ' actual stderr : ', errput[1]
> + raise svntest.Failure
>
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Apr 1 14:43:06 2005