Philip Martin wrote on Wed, 16 Jun 2010 at 11:17 -0000: > danie...@apache.org writes: > > > ============================================================================== > > --- subversion/trunk/subversion/include/svn_fs.h (original) > > +++ subversion/trunk/subversion/include/svn_fs.h Wed Jun 16 06:05:17 2010 > > @@ -1894,6 +1894,10 @@ svn_fs_revision_proplist(apr_hash_t **ta > > * - @a fs is a filesystem, and @a rev is the revision in that filesystem > > * whose property should change. > > * - @a name is the name of the property to change. > > + * - if @a old_value_p is not @c NULL, then @a *old_value_p is the > > expected old > > + * value of the property, and changing the value will fail with error > > + * #SVN_ERR_BAD_PROPERTY_VALUE if the present value of the property is > > not @a > > + * *old_value_p. > > * - @a value is the new value of the property, or zero if the property > > should > > How are you going to check adds? During an add the old value is NULL > but must still be checked. >
During an add, old_value_p!=NULL and *old_value_p==NULL, and the code checks that present_value==NULL as well. The case old_value_p==NULL means "unspecified" (i.e., just make the change regardless of what's there now). > > > Author: danielsh > > Date: Wed Jun 16 06:05:17 2010 > > New Revision: 955136 > > > > URL: http://svn.apache.org/viewvc?rev=955136&view=rev > > Log: > > Revv the FS change_rev_prop() interface towards more atomicity. > > > > Suggested by: philip > > > > > > * subversion/include/svn_fs.h > > (svn_fs_change_rev_prop2): New, takes OLD_VALUE_P parameter. > > (svn_fs_change_rev_prop): Deprecate. > > I don't think the old interface should be deprecated. > Why? The new interface has all the functionality of the old interface; one can do s/svn_fs_change_rev_prop(*args)/svn_fs_change_rev_prop2(args, old_value_p=NULL)/g with no change in functionality. > > svn_fs_base__set_rev_prop(svn_fs_t *fs, > > svn_revnum_t rev, > > const char *name, > > + const svn_string_t **old_value_p, > > const svn_string_t *value, > > trail_t *trail, > > apr_pool_t *pool) > > @@ -259,6 +260,23 @@ svn_fs_base__set_rev_prop(svn_fs_t *fs, > > txn->proplist = apr_hash_make(pool); > > > > /* Set the property. */ > > + if (old_value_p) > > + { > > That's not going to work for adds because it will skip the validation. > See above; for adds, old_value_p!=NULL and wanted_value==NULL. > > + const svn_string_t *wanted_value = *old_value_p; > > + const svn_string_t *present_value = apr_hash_get(txn->proplist, name, > > + > > APR_HASH_KEY_STRING); > > + if ((!wanted_value != !present_value) > > + || (wanted_value && present_value > > + && !svn_string_compare(wanted_value, present_value))) > > + { > > + /* What we expected isn't what we found. */ > > + return svn_error_createf(SVN_ERR_BAD_PROPERTY_VALUE, NULL, > > + _("revprop '%s' has unexpected value in > > " > > + "filesystem"), > > + name); > > + } > > + /* Fall through. */ > > + } > > apr_hash_set(txn->proplist, name, APR_HASH_KEY_STRING, value); > > > Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c > > URL: > > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=955136&r1=955135&r2=955136&view=diff > > ============================================================================== > > --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original) > > +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Jun 16 06:05:17 > > 2010 > > @@ -7167,6 +7167,7 @@ struct change_rev_prop_baton { > > svn_fs_t *fs; > > svn_revnum_t rev; > > const char *name; > > + const svn_string_t **old_value_p; > > const svn_string_t *value; > > }; > > > > @@ -7182,6 +7183,23 @@ change_rev_prop_body(void *baton, apr_po > > > > SVN_ERR(svn_fs_fs__revision_proplist(&table, cb->fs, cb->rev, pool)); > > > > + if (cb->old_value_p) > > + { > > Again, that's not going to work for adds. > Same as above (it's the same code, copy-pasted). > > + const svn_string_t *wanted_value = *cb->old_value_p; > > + const svn_string_t *present_value = apr_hash_get(table, cb->name, > > + > > APR_HASH_KEY_STRING); > > + if ((!wanted_value != !present_value) > > + || (wanted_value && present_value > > + && !svn_string_compare(wanted_value, present_value))) > > + { > > + /* What we expected isn't what we found. */ > > + return svn_error_createf(SVN_ERR_BAD_PROPERTY_VALUE, NULL, > > + _("revprop '%s' has unexpected value in > > " > > + "filesystem"), > > + cb->name); > > + } > > + /* Fall through. */ > > + } > > > --- subversion/trunk/subversion/tests/libsvn_fs/fs-test.c (original) > > +++ subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Wed Jun 16 > > 06:05:17 2010 > > @@ -593,8 +593,11 @@ revision_props(const svn_test_opts_t *op > > svn_fs_t *fs; > > apr_hash_t *proplist; > > svn_string_t *value; > > + svn_error_t *err; > > int i; > > svn_string_t s1; > > + const svn_string_t s2 = { "wrong value", 11 }; > > + const svn_string_t *s2_p = &s2; > > > > const char *initial_props[4][2] = { > > { "color", "red" }, > > @@ -638,6 +641,14 @@ revision_props(const svn_test_opts_t *op > > s1.len = value->len; > > SVN_ERR(svn_fs_change_rev_prop(fs, 0, "flower", &s1, pool)); > > > > + err = svn_fs_change_rev_prop2(fs, 0, "flower", &s2_p, &s1, pool); > > + if (!err || err->apr_err != SVN_ERR_BAD_PROPERTY_VALUE) > > + return svn_error_create(SVN_ERR_TEST_FAILED, err, > > + "svn_fs_change_rev_prop2() failed to " > > + "detect unexpected old value"); > > + else > > + svn_error_clear(err); > > I'd like to see adds, modifications and deletes all tested to pass and > fail. > Sure, I can extend the test. > > + > > /* Obtain a list of all current properties, and make sure it matches > > the expected values. */ > > SVN_ERR(svn_fs_revision_proplist(&proplist, fs, 0, pool)); > > > > > >
