Philip Martin wrote on Wed, 16 Jun 2010 at 11:17 -0000:
> danielsh_at_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));
> >
> >
>
>
Received on 2010-06-16 14:22:27 CEST