You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jon Foster <Jo...@cabot.co.uk> on 2010/09/20 09:48:44 UTC
[PATCH 2/3] atomic-revprop: Change tests
Hi,
This patch changes the tests for the atomic-revprop feature, so that
they test the error number rather than parsing the error text.
This doesn't work with Serf or Neon yet - they're still TODO. This
gives you a way to test Serf & Neon patches. (You might like to
hold off on committing this until Serf and Neon are done).
The patch is against the atomic-revprop branch, and requires
Patch 1 to be applied first. It doesn't apply to trunk (I don't
think the tests have been merged to trunk?)
[[[
Change atomic-revprop tests to look at the error number rather
than parsing the error text.
* subversion/tests/cmdline/atomic-ra-revprop-change.c:
(main): When printing an error, check if it's SVN_ERR_BAD_OLD_VALUE
and print a special message if it is.
* subversion/tests/cmdline/prop_tests.py:
(FAILS_WITH_BPV): Look for the special message.
Patch by: Jon Foster <jo...@cabot.co.uk>
]]]
Kind regards,
Jon
**********************************************************************
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd.
If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone.
Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232
Co. Registered in England number 02817269
Please contact the sender if you believe you have received this email in error.
**********************************************************************
______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________
Re: [PATCH 2/3],[PATCH 3/3] atomic-revprop: status
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Done in r999179, r999182, r999184 on the branch.
[PATCH 2/3],[PATCH 3/3] atomic-revprop: status
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Tue, Sep 21, 2010 at 01:19:06 +0200:
> Thanks for the review.
>
> As discussed on IRC, I'll proceed as follows:
>
> 1. Rename the error codes (per Bert and Blair)
Done.
> 2. Commit your patch 3/3 about using 412 to preserve err->apr_err over DAV
Running tests on the attached patch_atomic_revprops_dav3.txt.tweaked-by-me.
> 3. Commit my want_error patch (which will pass thanks to #2)
>
I'm satisfied with the attached crp-want_error-v3.diff on top of the
above patch.
> I think after this there will be only 1-2 items left in BRANCH-README.
Once I commit those patches, I'll update STATUS too and we'll see what's left.
>
[PATCH 2/3],[PATCH 3/3] atomic-revprop: status
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Tue, Sep 21, 2010 at 01:19:06 +0200:
> Thanks for the review.
>
> As discussed on IRC, I'll proceed as follows:
>
> 1. Rename the error codes (per Bert and Blair)
Done.
> 2. Commit your patch 3/3 about using 412 to preserve err->apr_err over DAV
Running tests on the attached patch_atomic_revprops_dav3.txt.tweaked-by-me.
> 3. Commit my want_error patch (which will pass thanks to #2)
>
I'm satisfied with the attached crp-want_error-v3.diff on top of the
above patch.
> I think after this there will be only 1-2 items left in BRANCH-README.
Once I commit those patches, I'll update STATUS too and we'll see what's left.
>
Re: [PATCH 2/3] atomic-revprop: Change tests
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Thanks for the review.
As discussed on IRC, I'll proceed as follows:
1. Rename the error codes (per Bert and Blair)
2. Commit your patch 3/3 about using 412 to preserve err->apr_err over DAV
3. Commit my want_error patch (which will pass thanks to #2)
I think after this there will be only 1-2 items left in BRANCH-README.
RE: [PATCH 2/3] atomic-revprop: Change tests
Posted by Jon Foster <Jo...@cabot.co.uk>.
Hi,
Daniel Shahaf wrote:
> Jon Foster wrote on Mon, Sep 20, 2010 at 12:39:31 +0100:
> > Daniel Shahaf wrote:
> > > Jon Foster wrote on Mon, Sep 20, 2010 at 10:48:44 +0100:
> > > > [[[
> > > > Change atomic-revprop tests to look at the error number rather
> > > > than parsing the error text.
> > > So, this is trying to capture the current ra_dav situation, where
the
> > > err->message is correct but err->apr_err isn't?
> > Correct.
> Okay. Perhaps this could have been called out more explicitly in
> the log message --- i.e., have it say not only *what* the patch
> does, but also *why* it does that.
Argh, I made one of the classic log-message-writing blunders!
> > > In other news, I've been thinking about moving the entire
validation
> > > logic to the C helper; that is, to have it get an extra argv
argument
> > > saying whether the revpropchange should pass or fail, and test by
> > itself
> > > that it passed/failed as expected; and the Python tests would
always
> > > expect it to exit(0). What do you think?
> >
> > Sounds like a good idea.
> Okay. I started a patch in that way at a time; I've touched it up now
> a tiny bit and I'm attaching it. Let me know what you think...
As discussed on IRC...
> Index: subversion/tests/cmdline/atomic-ra-revprop-change.c
> ===================================================================
> --- subversion/tests/cmdline/atomic-ra-revprop-change.c
(revision 998887)
> +++ subversion/tests/cmdline/atomic-ra-revprop-change.c (working
copy)
[...]
> @@ -117,6 +122,8 @@
> SVN_ERR(svn_config_create(&servers, pool));
> svn_config_set(servers, SVN_CONFIG_SECTION_GLOBAL,
> SVN_CONFIG_OPTION_HTTP_LIBRARY, http_library);
> + svn_config_set(servers, SVN_CONFIG_SECTION_GLOBAL,
> + SVN_CONFIG_OPTION_NEON_DEBUG_MASK,
getenv("SVN_NEON_DEBUG_MASK") /* "131" */);
>
> /* Populate CONFIG. */
> config = apr_hash_make(pool);
If you drop that hunk (which looks totally unrelated) then:
+1 (non-binding)
Tested (both "should pass" and "should fail") and it works.
Kind regards,
Jon
**********************************************************************
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd.
If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone.
Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232
Co. Registered in England number 02817269
Please contact the sender if you believe you have received this email in error.
**********************************************************************
______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________
Re: [PATCH 2/3] atomic-revprop: Change tests
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Jon Foster wrote on Mon, Sep 20, 2010 at 12:39:31 +0100:
> Hi,
>
> Daniel Shahaf wrote:
> > Jon Foster wrote on Mon, Sep 20, 2010 at 10:48:44 +0100:
> > > Hi,
> > >
> > > This patch changes the tests for the atomic-revprop feature, so that
> > > they test the error number rather than parsing the error text.
> > > This doesn't work with Serf or Neon yet - they're still TODO. This
> > > gives you a way to test Serf & Neon patches. (You might like to
> > > hold off on committing this until Serf and Neon are done).
> > >
> > > The patch is against the atomic-revprop branch, and requires
> > > Patch 1 to be applied first. It doesn't apply to trunk (I don't
> > > think the tests have been merged to trunk?)
> > >
> >
> > On trunk there are only the libsvn_fs and libsvn_repos parts. The
> > libsvn_ra parts, and the associated Python tests, are only on the
> > branch.
> >
> > > [[[
> > > Change atomic-revprop tests to look at the error number rather
> > > than parsing the error text.
> > >
> > > * subversion/tests/cmdline/atomic-ra-revprop-change.c:
> > > (main): When printing an error, check if it's
> SVN_ERR_BAD_OLD_VALUE
> > > and print a special message if it is.
> > >
> > > * subversion/tests/cmdline/prop_tests.py:
> > > (FAILS_WITH_BPV): Look for the special message.
> > >
> > > Patch by: Jon Foster <jo...@cabot.co.uk>
> > > ]]]
> > >
> >
> > So, this is trying to capture the current ra_dav situation, where the
> > err->message is correct but err->apr_err isn't?
>
> Correct.
>
Okay. Perhaps this could have been called out more explicitly in the
log message --- i.e., have it say not only *what* the patch does, but
also *why* it does that.
> > (and will make the test fail over ra_neon/ra_serf)
>
> Correct.
>
> > In other news, I've been thinking about moving the entire validation
> > logic to the C helper; that is, to have it get an extra argv argument
> > saying whether the revpropchange should pass or fail, and test by
> itself
> > that it passed/failed as expected; and the Python tests would always
> > expect it to exit(0). What do you think?
>
> Sounds like a good idea.
>
Okay. I started a patch in that way at a time; I've touched it up now
a tiny bit and I'm attaching it. Let me know what you think...
(I've tested it when applied on top of your patch 3/3.)
> > > Index: subversion/tests/cmdline/atomic-ra-revprop-change.c
> > > ===================================================================
> > > --- subversion/tests/cmdline/atomic-ra-revprop-change.c
> (revision 998620)
> > > +++ subversion/tests/cmdline/atomic-ra-revprop-change.c (working
> copy)
> > > @@ -226,6 +226,8 @@
> > > http_library, pool);
> > > if (err)
> > > {
> > > + if (svn_error_has_cause(err, SVN_ERR_BAD_OLD_VALUE))
> > > + fprintf(stderr, "atomic-ra-revprop-change failed due to
> incorrect
> >
> > Or even more directly:
> >
> > - if (err)
> > + if (err && err->apr_err == SVN_ERR_BAD_OLD_VALUE)
> >
>
> Wouldn't that leak an error if the error isn't SVN_ERR_BAD_OLD_VALUE?
>
Yes, it would.
> Also, the current* design requires us to use svn_error_has_cause()
> because the error is wrapped inside an error with a different code.
Yup, I've noticed this in the verbose test output I've pasted
elsethread, but you beat me to replying with the correction :)
> (FWIW, I think the RA layer shouldn't do that - I think we should
> change svn_ra_change_rev_prop() so that the simple test of
> err->apr_err works. But that's not urgent, and could even be
> postponed to 1.8).
First of all, agreed that this is small change; I think it's more
important to be able to promise that the error situation will be
/recognizable/ then whether the recognition will be with or without
svn_error_has_cause().
For reference, this is the error chain currently (with your patch 3/3):
[[[
subversion/tests/cmdline/atomic-ra-revprop-change.c:156: (apr_err=175002)
subversion/libsvn_ra_neon/fetch.c:1210: (apr_err=175002)
atomic-ra-revprop-change: DAV request failed; it's possible that the repository's pre-revprop-change hook either failed or is
+non-existent
subversion/libsvn_ra_neon/props.c:1231: (apr_err=175008)
atomic-ra-revprop-change: At least one property change failed; repository is unchanged
subversion/libsvn_ra_neon/util.c:1550: (apr_err=125014)
subversion/libsvn_ra_neon/util.c:236: (apr_err=125014)
atomic-ra-revprop-change: Error setting property 'flower':
revprop 'flower' has unexpected value in filesystem
]]]
>
> Kind regards,
>
> Jon
>
> (* Unless I missed something and the design's been changed?)
RE: [PATCH 2/3] atomic-revprop: Change tests
Posted by Jon Foster <Jo...@cabot.co.uk>.
Hi,
Daniel Shahaf wrote:
> Jon Foster wrote on Mon, Sep 20, 2010 at 10:48:44 +0100:
> > Hi,
> >
> > This patch changes the tests for the atomic-revprop feature, so that
> > they test the error number rather than parsing the error text.
> > This doesn't work with Serf or Neon yet - they're still TODO. This
> > gives you a way to test Serf & Neon patches. (You might like to
> > hold off on committing this until Serf and Neon are done).
> >
> > The patch is against the atomic-revprop branch, and requires
> > Patch 1 to be applied first. It doesn't apply to trunk (I don't
> > think the tests have been merged to trunk?)
> >
>
> On trunk there are only the libsvn_fs and libsvn_repos parts. The
> libsvn_ra parts, and the associated Python tests, are only on the
> branch.
>
> > [[[
> > Change atomic-revprop tests to look at the error number rather
> > than parsing the error text.
> >
> > * subversion/tests/cmdline/atomic-ra-revprop-change.c:
> > (main): When printing an error, check if it's
SVN_ERR_BAD_OLD_VALUE
> > and print a special message if it is.
> >
> > * subversion/tests/cmdline/prop_tests.py:
> > (FAILS_WITH_BPV): Look for the special message.
> >
> > Patch by: Jon Foster <jo...@cabot.co.uk>
> > ]]]
> >
>
> So, this is trying to capture the current ra_dav situation, where the
> err->message is correct but err->apr_err isn't?
Correct.
> (and will make the test fail over ra_neon/ra_serf)
Correct.
> In other news, I've been thinking about moving the entire validation
> logic to the C helper; that is, to have it get an extra argv argument
> saying whether the revpropchange should pass or fail, and test by
itself
> that it passed/failed as expected; and the Python tests would always
> expect it to exit(0). What do you think?
Sounds like a good idea.
> > Index: subversion/tests/cmdline/atomic-ra-revprop-change.c
> > ===================================================================
> > --- subversion/tests/cmdline/atomic-ra-revprop-change.c
(revision 998620)
> > +++ subversion/tests/cmdline/atomic-ra-revprop-change.c (working
copy)
> > @@ -226,6 +226,8 @@
> > http_library, pool);
> > if (err)
> > {
> > + if (svn_error_has_cause(err, SVN_ERR_BAD_OLD_VALUE))
> > + fprintf(stderr, "atomic-ra-revprop-change failed due to
incorrect
>
> Or even more directly:
>
> - if (err)
> + if (err && err->apr_err == SVN_ERR_BAD_OLD_VALUE)
>
Wouldn't that leak an error if the error isn't SVN_ERR_BAD_OLD_VALUE?
Also, the current* design requires us to use svn_error_has_cause()
because
the error is wrapped inside an error with a different code. (FWIW,
I think the RA layer shouldn't do that - I think we should change
svn_ra_change_rev_prop() so that the simple test of err->apr_err works.
But that's not urgent, and could even be postponed to 1.8).
Kind regards,
Jon
(* Unless I missed something and the design's been changed?)
**********************************************************************
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd.
If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone.
Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232
Co. Registered in England number 02817269
Please contact the sender if you believe you have received this email in error.
**********************************************************************
______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________
Re: [PATCH 2/3] atomic-revprop: Change tests
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Jon Foster wrote on Mon, Sep 20, 2010 at 10:48:44 +0100:
> Hi,
>
> This patch changes the tests for the atomic-revprop feature, so that
> they test the error number rather than parsing the error text.
> This doesn't work with Serf or Neon yet - they're still TODO. This
> gives you a way to test Serf & Neon patches. (You might like to
> hold off on committing this until Serf and Neon are done).
>
> The patch is against the atomic-revprop branch, and requires
> Patch 1 to be applied first. It doesn't apply to trunk (I don't
> think the tests have been merged to trunk?)
>
On trunk there are only the libsvn_fs and libsvn_repos parts. The
libsvn_ra parts, and the associated Python tests, are only on the
branch.
> [[[
> Change atomic-revprop tests to look at the error number rather
> than parsing the error text.
>
> * subversion/tests/cmdline/atomic-ra-revprop-change.c:
> (main): When printing an error, check if it's SVN_ERR_BAD_OLD_VALUE
> and print a special message if it is.
>
> * subversion/tests/cmdline/prop_tests.py:
> (FAILS_WITH_BPV): Look for the special message.
>
> Patch by: Jon Foster <jo...@cabot.co.uk>
> ]]]
>
So, this is trying to capture the current ra_dav situation, where the
err->message is correct but err->apr_err isn't? (and will make the test
fail over ra_neon/ra_serf)
In other news, I've been thinking about moving the entire validation
logic to the C helper; that is, to have it get an extra argv argument
saying whether the revpropchange should pass or fail, and test by itself
that it passed/failed as expected; and the Python tests would always
expect it to exit(0). What do you think?
> Index: subversion/tests/cmdline/atomic-ra-revprop-change.c
> ===================================================================
> --- subversion/tests/cmdline/atomic-ra-revprop-change.c (revision 998620)
> +++ subversion/tests/cmdline/atomic-ra-revprop-change.c (working copy)
> @@ -226,6 +226,8 @@
> http_library, pool);
> if (err)
> {
> + if (svn_error_has_cause(err, SVN_ERR_BAD_OLD_VALUE))
> + fprintf(stderr, "atomic-ra-revprop-change failed due to incorrect old value.\n");
Or even more directly:
- if (err)
+ if (err && err->apr_err == SVN_ERR_BAD_OLD_VALUE)
> svn_handle_error2(err, stderr, FALSE, "atomic-ra-revprop-change: ");
> svn_error_clear(err);
> exit_code = EXIT_FAILURE;