You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by da...@apache.org on 2010/06/16 08:05:18 UTC
svn commit: r955136 - in /subversion/trunk/subversion: include/ libsvn_fs/
libsvn_fs_base/ libsvn_fs_fs/ tests/libsvn_fs/
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.
* subversion/libsvn_fs/fs-loader.h
(fs_vtable_t.change_rev_prop):
Add OLD_VALUE_P parameter (as with svn_fs_change_rev_prop2()).
* subversion/libsvn_fs/fs-loader.c
(svn_fs_change_rev_prop2): Pass OLD_VALUE_P to change_rev_prop().
(svn_fs_change_rev_prop): Now a wrapper around svn_fs_change_rev_prop2().
* subversion/tests/libsvn_fs/fs-test.c
(revision_props):
Extend existing test to cover the new API.
* subversion/libsvn_fs_base/revs-txns.c
(svn_fs_base__set_rev_prop): Use OLD_VALUE_P parameter.
(change_rev_prop_args): Add OLD_VALUE_P member.
(txn_body_change_rev_prop): Pass OLD_VALUE_P.
(svn_fs_base__change_rev_prop): Drill OLD_VALUE_P through.
* subversion/libsvn_fs_base/revs-txns.h
(svn_fs_base__set_rev_prop, svn_fs_base__change_rev_prop):
Revv to add OLD_VALUE_P parameter.
* subversion/libsvn_fs_base/dag.c
(txn_body_dag_init_fs, svn_fs_base__dag_commit_txn):
Update calls to svn_fs_base__set_rev_prop().
* subversion/libsvn_fs_fs/fs_fs.c
(change_rev_prop_baton): Add OLD_VALUE_P member.
(change_rev_prop_body): Use OLD_VALUE_P.
(svn_fs_fs__change_rev_prop): Drill OLD_VALUE_P through.
* subversion/libsvn_fs_fs/fs_fs.h
(svn_fs_fs__change_rev_prop): Add OLD_VALUE_P parameter.
Modified:
subversion/trunk/subversion/include/svn_fs.h
subversion/trunk/subversion/libsvn_fs/fs-loader.c
subversion/trunk/subversion/libsvn_fs/fs-loader.h
subversion/trunk/subversion/libsvn_fs_base/dag.c
subversion/trunk/subversion/libsvn_fs_base/revs-txns.c
subversion/trunk/subversion/libsvn_fs_base/revs-txns.h
subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h
subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Modified: subversion/trunk/subversion/include/svn_fs.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_fs.h?rev=955136&r1=955135&r2=955136&view=diff
==============================================================================
--- 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
* 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);
+
+
+/**
+ * Similar to svn_fs_change_rev_prop2(), but with @a old_value_p passed as
+ * @c NULL.
+ *
+ * @deprecated Provided for backward compatibility with the 1.6 API.
*/
+SVN_DEPRECATED
svn_error_t *
svn_fs_change_rev_prop(svn_fs_t *fs,
svn_revnum_t rev,
Modified: subversion/trunk/subversion/libsvn_fs/fs-loader.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs/fs-loader.c?rev=955136&r1=955135&r2=955136&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs/fs-loader.c (original)
+++ subversion/trunk/subversion/libsvn_fs/fs-loader.c Wed Jun 16 06:05:17 2010
@@ -1199,11 +1199,21 @@ svn_fs_revision_proplist(apr_hash_t **ta
}
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)
+{
+ return svn_error_return(fs->vtable->change_rev_prop(fs, rev, name,
+ old_value_p,
+ value, pool));
+}
+
+svn_error_t *
svn_fs_change_rev_prop(svn_fs_t *fs, svn_revnum_t rev, const char *name,
const svn_string_t *value, apr_pool_t *pool)
{
- return svn_error_return(fs->vtable->change_rev_prop(fs, rev, name, value,
- pool));
+ return svn_error_return(
+ svn_fs_change_rev_prop2(fs, rev, name, NULL, value, pool));
}
svn_error_t *
Modified: subversion/trunk/subversion/libsvn_fs/fs-loader.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs/fs-loader.h?rev=955136&r1=955135&r2=955136&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs/fs-loader.h (original)
+++ subversion/trunk/subversion/libsvn_fs/fs-loader.h Wed Jun 16 06:05:17 2010
@@ -158,6 +158,7 @@ typedef struct fs_vtable_t
svn_revnum_t rev, apr_pool_t *pool);
svn_error_t *(*change_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,
apr_pool_t *pool);
svn_error_t *(*get_uuid)(svn_fs_t *fs, const char **uuid, apr_pool_t *pool);
Modified: subversion/trunk/subversion/libsvn_fs_base/dag.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_base/dag.c?rev=955136&r1=955135&r2=955136&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_base/dag.c (original)
+++ subversion/trunk/subversion/libsvn_fs_base/dag.c Wed Jun 16 06:05:17 2010
@@ -251,7 +251,7 @@ txn_body_dag_init_fs(void *baton,
/* Set a date on revision 0. */
date.data = svn_time_to_cstring(apr_time_now(), trail->pool);
date.len = strlen(date.data);
- return svn_fs_base__set_rev_prop(fs, 0, SVN_PROP_REVISION_DATE, &date,
+ return svn_fs_base__set_rev_prop(fs, 0, SVN_PROP_REVISION_DATE, NULL, &date,
trail, trail->pool);
}
@@ -1644,7 +1644,7 @@ svn_fs_base__dag_commit_txn(svn_revnum_t
date.data = svn_time_to_cstring(apr_time_now(), pool);
date.len = strlen(date.data);
return svn_fs_base__set_rev_prop(fs, *new_rev, SVN_PROP_REVISION_DATE,
- &date, trail, pool);
+ NULL, &date, trail, pool);
}
/* Modify all entries in the "node-origins" table that have a txn-id of
Modified: subversion/trunk/subversion/libsvn_fs_base/revs-txns.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_base/revs-txns.c?rev=955136&r1=955135&r2=955136&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_base/revs-txns.c (original)
+++ subversion/trunk/subversion/libsvn_fs_base/revs-txns.c Wed Jun 16 06:05:17 2010
@@ -241,6 +241,7 @@ svn_error_t *
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)
+ {
+ 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);
/* Overwrite the revision. */
@@ -269,6 +287,7 @@ svn_fs_base__set_rev_prop(svn_fs_t *fs,
struct change_rev_prop_args {
svn_revnum_t rev;
const char *name;
+ const svn_string_t **old_value_p;
const svn_string_t *value;
};
@@ -279,7 +298,7 @@ txn_body_change_rev_prop(void *baton, tr
struct change_rev_prop_args *args = baton;
return svn_fs_base__set_rev_prop(trail->fs, args->rev,
- args->name, args->value,
+ args->name, args->old_value_p, args->value,
trail, trail->pool);
}
@@ -288,6 +307,7 @@ svn_error_t *
svn_fs_base__change_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,
apr_pool_t *pool)
{
@@ -297,6 +317,7 @@ svn_fs_base__change_rev_prop(svn_fs_t *f
args.rev = rev;
args.name = name;
+ args.old_value_p = old_value_p;
args.value = value;
return svn_fs_base__retry_txn(fs, txn_body_change_rev_prop, &args,
TRUE, pool);
Modified: subversion/trunk/subversion/libsvn_fs_base/revs-txns.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_base/revs-txns.h?rev=955136&r1=955135&r2=955136&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_base/revs-txns.h (original)
+++ subversion/trunk/subversion/libsvn_fs_base/revs-txns.h Wed Jun 16 06:05:17 2010
@@ -61,6 +61,7 @@ svn_error_t *svn_fs_base__rev_get_txn_id
svn_error_t *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);
@@ -180,6 +181,7 @@ svn_error_t *svn_fs_base__revision_propl
svn_error_t *svn_fs_base__change_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,
apr_pool_t *pool);
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)
+ {
+ 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. */
+ }
apr_hash_set(table, cb->name, APR_HASH_KEY_STRING, cb->value);
return set_revision_proplist(cb->fs, cb->rev, table, pool);
@@ -7191,6 +7209,7 @@ svn_error_t *
svn_fs_fs__change_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,
apr_pool_t *pool)
{
@@ -7201,6 +7220,7 @@ svn_fs_fs__change_rev_prop(svn_fs_t *fs,
cb.fs = fs;
cb.rev = rev;
cb.name = name;
+ cb.old_value_p = old_value_p;
cb.value = value;
return svn_fs_fs__with_write_lock(fs, change_rev_prop_body, &cb, pool);
Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h?rev=955136&r1=955135&r2=955136&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.h Wed Jun 16 06:05:17 2010
@@ -441,9 +441,11 @@ svn_error_t *svn_fs_fs__revision_prop(sv
/* Change, add, or delete a property on a revision REV in filesystem
FS. NAME gives the name of the property, and value, if non-NULL,
gives the new contents of the property. If value is NULL, then the
- property will be deleted. Do any temporary allocation in POOL. */
+ property will be deleted. If OLD_VALUE_P is not NULL, do nothing unless the
+ preexisting value is *OLD_VALUE_P. Do any temporary allocation in POOL. */
svn_error_t *svn_fs_fs__change_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,
apr_pool_t *pool);
Modified: subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c?rev=955136&r1=955135&r2=955136&view=diff
==============================================================================
--- 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);
+
/* 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));
Re: svn commit: r955136 - in /subversion/trunk/subversion: include/
libsvn_fs/ libsvn_fs_base/ libsvn_fs_fs/ tests/libsvn_fs/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Wed, 16 Jun 2010 at 15:05 +0100:
> On Wed, 2010-06-16 at 14:30 +0100, Philip Martin wrote:
> > Daniel Shahaf <d....@daniel.shahaf.name> writes:
> >
> > > Philip Martin wrote on Wed, 16 Jun 2010 at 11:17 -0000:
> > >> 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).
> > >
> >
> > OK, I didn't read the code carefully enough.
>
> I didn't spot that either. Please could you add a second "const" in the
> function prototype to make it clearer? Otherwise it looks like an
> output.
>
> "const svn_string_t *const *old_p"
>
Done in r955306.
Also extended the fs-test.c unit test in r955303.
Re: svn commit: r955136 - in /subversion/trunk/subversion:
include/ libsvn_fs/ libsvn_fs_base/ libsvn_fs_fs/ tests/libsvn_fs/
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2010-06-16 at 14:30 +0100, Philip Martin wrote:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> > Philip Martin wrote on Wed, 16 Jun 2010 at 11:17 -0000:
> >> 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).
> >
>
> OK, I didn't read the code carefully enough.
I didn't spot that either. Please could you add a second "const" in the
function prototype to make it clearer? Otherwise it looks like an
output.
"const svn_string_t *const *old_p"
- Julian
> >> 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.
>
> Agreed.
>
Re: svn commit: r955136 - in /subversion/trunk/subversion: include/ libsvn_fs/ libsvn_fs_base/ libsvn_fs_fs/ tests/libsvn_fs/
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> Philip Martin wrote on Wed, 16 Jun 2010 at 11:17 -0000:
>> 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).
>
OK, I didn't read the code carefully enough.
>> 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.
Agreed.
--
Philip
Re: svn commit: r955136 - in /subversion/trunk/subversion: include/
libsvn_fs/ libsvn_fs_base/ libsvn_fs_fs/ tests/libsvn_fs/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Wed, 16 Jun 2010 at 11:17 -0000:
> danielsh@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));
> >
> >
>
>
Re: svn commit: r955136 - in /subversion/trunk/subversion: include/
libsvn_fs/ libsvn_fs_base/ libsvn_fs_fs/ tests/libsvn_fs/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Wed, 16 Jun 2010 at 11:17 -0000:
> danielsh@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));
> >
> >
>
>
Re: svn commit: r955136 - in /subversion/trunk/subversion: include/
libsvn_fs/ libsvn_fs_base/ libsvn_fs_fs/ tests/libsvn_fs/
Posted by "C. Michael Pilato" <cm...@collab.net>.
Philip Martin wrote:
> danielsh@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.
If the old interface is not to be deprecated, then please don't use our
simple numeric interface revision naming scheme. Give the thing a new name,
such as svn_fs_change_matching_rev_prop() or somesuch.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r955136 - in /subversion/trunk/subversion: include/ libsvn_fs/ libsvn_fs_base/ libsvn_fs_fs/ tests/libsvn_fs/
Posted by Philip Martin <ph...@wandisco.com>.
danielsh@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
Re: svn commit: r955136 - in /subversion/trunk/subversion: include/ libsvn_fs/ libsvn_fs_base/ libsvn_fs_fs/ tests/libsvn_fs/
Posted by Philip Martin <ph...@wandisco.com>.
danielsh@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