danie...@apache.org writes:
> 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.
> ==============================================================================
> --- 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.
> * be removed altogether.
> *
> @@ -1902,7 +1906,25 @@ svn_fs_revision_proplist(apr_hash_t **ta
> * via transactions.
> *
> * Do any necessary temporary allocation in @a pool.
> + *
> + * @since New in 1.7.
> + */
> +svn_error_t *
> +svn_fs_change_rev_prop2(svn_fs_t *fs,
> + svn_revnum_t rev,
> + const char *name,
> + const svn_string_t **old_value_p,
> + const svn_string_t *value,
> + apr_pool_t *pool);
> +
> +
> 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.
> + 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.
> + 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.
> +
> /* 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));
>
>
--
Philip
