Julian, thank you for the feedback. Pl. find my comments attached.
On Fri, 2005-04-01 at 18:11, Julian Foad wrote:
> 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.
>
Correct. Thats why I didnt say "fixed". I guess you are asking me to be
explicit in saying that the issue is not fixed... I will do that too.
> > * 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."
>
Okay.
> > * 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."
>
I agree. Will change.
> > * 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.
>
I didnt want to lose this test case. But then again, as you say, I could
commit it with the UI change. Would do that.
> > ]]]
> >
> >
> >
> > ------------------------------------------------------------------------
> >
> > 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:
>
makes perfect sense. Thanks.
> 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?
>
Yes. You are correct. I forgot to cross check with your earlier points.
Sorry.
> > + _("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)?
>
oops.... no. have to fix this.
> > + 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 Sat Apr 2 20:55:12 2005