You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2010/06/16 08:17:12 UTC

Re: svn commit: r955136 - in /subversion/trunk/subversion: include/ libsvn_fs/ libsvn_fs_base/ libsvn_fs_fs/ tests/libsvn_fs/

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 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