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.
>
Index: subversion/mod_dav_svn/util.c
===================================================================
--- subversion/mod_dav_svn/util.c (revision 999170)
+++ subversion/mod_dav_svn/util.c (working copy)
@@ -107,6 +107,9 @@ dav_svn__convert_err(svn_error_t *serr,
case SVN_ERR_FS_PATH_ALREADY_LOCKED:
status = HTTP_LOCKED;
break;
+ case SVN_ERR_FS_PROP_BASEVALUE_MISMATCH:
+ status = HTTP_PRECONDITION_FAILED;
+ break;
/* add other mappings here */
}
Index: subversion/libsvn_ra_neon/util.c
===================================================================
--- subversion/libsvn_ra_neon/util.c (revision 999170)
+++ subversion/libsvn_ra_neon/util.c (working copy)
@@ -167,6 +167,7 @@ typedef struct
svn_ra_neon__request_t *req;
svn_stringbuf_t *description;
svn_boolean_t contains_error;
+ svn_boolean_t contains_precondition_error;
} multistatus_baton_t;
/* Implements svn_ra_neon__startelm_cb_t. */
@@ -231,6 +232,9 @@ end_207_element(void *baton, int state,
return svn_error_create(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL,
_("The request response contained at least
"
"one error"));
+ else if (b->contains_precondition_error)
+ return svn_error_create(SVN_ERR_FS_PROP_BASEVALUE_MISMATCH, NULL,
+ b->description->data);
else
return svn_error_create(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL,
b->description->data);
@@ -260,6 +264,10 @@ end_207_element(void *baton, int state,
else
b->propstat_has_error = (status.klass != 2);
+ /* Handle "412 Precondition Failed" specially */
+ if (status.code == 412)
+ b->contains_precondition_error = TRUE;
+
free(status.reason_phrase);
}
else
Index: subversion/libsvn_ra_serf/util.c
===================================================================
--- subversion/libsvn_ra_serf/util.c (revision 999170)
+++ subversion/libsvn_ra_serf/util.c (working copy)
@@ -836,6 +836,7 @@ svn_ra_serf__handle_discard_body(serf_request_t *r
{
server_err->error = svn_error_create(APR_SUCCESS, NULL, NULL);
server_err->has_xml_response = TRUE;
+ server_err->contains_precondition_error = FALSE;
server_err->cdata = svn_stringbuf_create("", pool);
server_err->collect_cdata = FALSE;
server_err->parser.pool = server_err->error->pool;
@@ -945,6 +946,34 @@ svn_ra_serf__handle_status_only(serf_request_t *re
return svn_error_return(err);
}
+/* Given a string like "HTTP/1.1 500 (status)" in BUF, parse out the numeric
+ status code into *STATUS_CODE_OUT. Ignores leading whitespace. */
+static svn_error_t *
+parse_dav_status(int *status_code_out, svn_stringbuf_t *buf,
+ apr_pool_t *scratch_pool)
+{
+ svn_error_t *err;
+ const char *token;
+ char *tok_status;
+ svn_stringbuf_t *temp_buf = svn_stringbuf_dup(buf, scratch_pool);
+
+ svn_stringbuf_strip_whitespace(temp_buf);
+ token = apr_strtok(temp_buf->data, " \t\r\n", &tok_status);
+ if (token)
+ token = apr_strtok(NULL, " \t\r\n", &tok_status);
+ if (!token)
+ return svn_error_createf(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
+ "Malformed DAV:status CDATA '%s'",
+ buf->data);
+ err = svn_cstring_atoi(status_code_out, token);
+ if (err)
+ return svn_error_createf(SVN_ERR_RA_DAV_MALFORMED_DATA, err,
+ "Malformed DAV:status CDATA '%s'",
+ buf->data);
+
+ return SVN_NO_ERROR;
+}
+
/*
* Expat callback invoked on a start element tag for a 207 response.
*/
@@ -968,6 +997,14 @@ start_207(svn_ra_serf__xml_parser_t *parser,
svn_stringbuf_setempty(ctx->cdata);
ctx->collect_cdata = TRUE;
}
+ else if (ctx->in_error &&
+ strcmp(name.namespace, "DAV:") == 0 &&
+ strcmp(name.name, "status") == 0)
+ {
+ /* Start collecting cdata. */
+ svn_stringbuf_setempty(ctx->cdata);
+ ctx->collect_cdata = TRUE;
+ }
return SVN_NO_ERROR;
}
@@ -993,9 +1030,24 @@ end_207(svn_ra_serf__xml_parser_t *parser,
ctx->collect_cdata = FALSE;
ctx->error->message = apr_pstrmemdup(ctx->error->pool, ctx->cdata->data,
ctx->cdata->len);
- ctx->error->apr_err = SVN_ERR_RA_DAV_REQUEST_FAILED;
+ if (ctx->contains_precondition_error)
+ ctx->error->apr_err = SVN_ERR_FS_PROP_BASEVALUE_MISMATCH;
+ else
+ ctx->error->apr_err = SVN_ERR_RA_DAV_REQUEST_FAILED;
}
+ else if (ctx->in_error &&
+ strcmp(name.namespace, "DAV:") == 0 &&
+ strcmp(name.name, "status") == 0)
+ {
+ int status_code;
+ ctx->collect_cdata = FALSE;
+
+ SVN_ERR(parse_dav_status(&status_code, ctx->cdata, parser->pool));
+ if (status_code == 412)
+ ctx->contains_precondition_error = TRUE;
+ }
+
return SVN_NO_ERROR;
}
@@ -1044,6 +1096,7 @@ svn_ra_serf__handle_multistatus_only(serf_request_
{
server_err->error = svn_error_create(APR_SUCCESS, NULL, NULL);
server_err->has_xml_response = TRUE;
+ server_err->contains_precondition_error = FALSE;
server_err->cdata = svn_stringbuf_create("", pool);
server_err->collect_cdata = FALSE;
server_err->parser.pool = server_err->error->pool;
Index: subversion/libsvn_ra_serf/ra_serf.h
===================================================================
--- subversion/libsvn_ra_serf/ra_serf.h (revision 999170)
+++ subversion/libsvn_ra_serf/ra_serf.h (working copy)
@@ -673,6 +673,9 @@ typedef struct svn_ra_serf__server_error_t {
/* Have we seen an error tag? */
svn_boolean_t in_error;
+ /* Have we seen a HTTP "412 Precondition Failed" error? */
+ svn_boolean_t contains_precondition_error;
+
/* Should we be collecting the XML cdata? */
svn_boolean_t collect_cdata;
Index: subversion/tests/cmdline/svntest/actions.py
===================================================================
--- subversion/tests/cmdline/svntest/actions.py (revision 999158)
+++ subversion/tests/cmdline/svntest/actions.py (working copy)
@@ -152,7 +152,8 @@ def run_and_verify_atomic_ra_revprop_change(messag
expected_stderr,
expected_exit,
url, revision, propname,
- old_propval, propval):
+ old_propval, propval,
+ want_error):
"""Run atomic-ra-revprop-change helper and check its output and exit code.
Transforms OLD_PROPVAL and PROPVAL into a skel.
For HTTP, the default HTTP library is used."""
@@ -173,7 +174,8 @@ def run_and_verify_atomic_ra_revprop_change(messag
make_proplist_skel_part(KEY_NEW_PROPVAL, propval))
exit_code, out, err = main.run_atomic_ra_revprop_change(url, revision,
- propname, skel)
+ propname, skel,
+ want_error)
verify.verify_outputs("Unexpected output", out, err,
expected_stdout, expected_stderr)
verify.verify_exit_code(message, exit_code, expected_exit)
Index: subversion/tests/cmdline/svntest/main.py
===================================================================
--- subversion/tests/cmdline/svntest/main.py (revision 999158)
+++ subversion/tests/cmdline/svntest/main.py (working copy)
@@ -641,7 +641,7 @@ def run_entriesdump_subdirs(path):
0, 0, None, '--subdirs', path)
return [line.strip() for line in stdout_lines if not line.startswith("DBG:")]
-def run_atomic_ra_revprop_change(url, revision, propname, skel):
+def run_atomic_ra_revprop_change(url, revision, propname, skel, want_error):
"""Run the atomic-ra-revprop-change helper, returning its exit code, stdout,
and stderr. For HTTP, default HTTP library is used."""
# use spawn_process rather than run_command to avoid copying all the data
@@ -652,7 +652,7 @@ def run_entriesdump_subdirs(path):
# This passes HTTP_LIBRARY in addition to our params.
return run_command(atomic_ra_revprop_change_binary, True, False,
url, revision, propname, skel,
- options.http_library)
+ options.http_library, want_error and 1 or 0)
# Chmod recursively on a whole subtree
Index: subversion/tests/cmdline/atomic-ra-revprop-change.c
===================================================================
--- subversion/tests/cmdline/atomic-ra-revprop-change.c (revision 999158)
+++ subversion/tests/cmdline/atomic-ra-revprop-change.c (working copy)
@@ -41,13 +41,18 @@
#define KEY_NEW_PROPVAL "value"
#define USAGE_MSG \
- "Usage: %s URL REVISION PROPNAME VALUES_SKEL HTTP_LIBRARY\n" \
+ "Usage: %s URL REVISION PROPNAME VALUES_SKEL HTTP_LIBRARY WANT_ERROR\n" \
"\n" \
"VALUES_SKEL is a proplist skel containing pseudo-properties '%s' \n" \
"and '%s'. A pseudo-property missing from the skel is interpreted \n" \
- "as unset.\n"
+ "as unset.\n" \
+ "\n" \
+ "WANT_ERROR is 1 if the propchange is expected to fail due to the atomicity,"\
+ "and 0 if it is expected to succeed. If the expectation matches reality," \
+ "the exit code shall be zero.\n"
+
/* implements svn_auth_simple_prompt_func_t */
static svn_error_t *
aborting_simple_prompt_func(svn_auth_cred_simple_t **cred,
@@ -134,12 +139,14 @@ change_rev_prop(const char *url,
const svn_string_t *propval,
const svn_string_t *old_value,
const char *http_library,
+ svn_boolean_t want_error,
apr_pool_t *pool)
{
svn_ra_callbacks2_t *callbacks;
svn_ra_session_t *sess;
apr_hash_t *config;
svn_boolean_t capable;
+ svn_error_t *err;
SVN_ERR(svn_ra_create_callbacks(&callbacks, pool));
SVN_ERR(construct_auth_baton(&callbacks->auth_baton, pool));
@@ -152,15 +159,33 @@ change_rev_prop(const char *url,
SVN_RA_CAPABILITY_ATOMIC_REVPROPS,
pool));
if (capable)
- SVN_ERR(svn_ra_change_rev_prop2(sess, revision, propname,
- &old_value, propval, pool));
+ {
+ err = svn_ra_change_rev_prop2(sess, revision, propname,
+ &old_value, propval, pool);
+
+ if (want_error && err
+ && svn_error_has_cause(err, SVN_ERR_FS_PROP_BASEVALUE_MISMATCH))
+ {
+ /* Expectation was matched. Get out. */
+ fputs("<<<revprop 'flower' has unexpected value>>>", stderr);
+ svn_error_clear(err);
+ return SVN_NO_ERROR;
+ }
+ else if (! want_error && ! err)
+ /* Expectation was matched. Get out. */
+ return SVN_NO_ERROR;
+ else if (want_error && ! err)
+ return svn_error_create(SVN_ERR_TEST_FAILED, NULL,
+ "An error was expected but not seen");
+ else
+ /* A real (non-SVN_ERR_FS_PROP_BASEVALUE_MISMATCH) error. */
+ return svn_error_return(err);
+ }
else
/* Running under --server-minor-version? */
return svn_error_create(SVN_ERR_TEST_FAILED, NULL,
"Server doesn't advertise "
"SVN_RA_CAPABILITY_ATOMIC_REVPROPS");
-
- return SVN_NO_ERROR;
}
/* Parse SKEL_CSTR according to the description in USAGE_MSG. */
@@ -194,8 +219,9 @@ main(int argc, const char *argv[])
svn_string_t *old_propval;
const char *http_library;
char *digits_end = NULL;
+ svn_boolean_t want_error;
- if (argc != 6)
+ if (argc != 7)
{
fprintf(stderr, USAGE_MSG, argv[0], KEY_OLD_PROPVAL, KEY_NEW_PROPVAL);
exit(1);
@@ -216,6 +242,7 @@ main(int argc, const char *argv[])
propname = argv[3];
SVN_INT_ERR(extract_values_from_skel(&old_propval, &propval, argv[4], pool));
http_library = argv[5];
+ want_error = !strcmp(argv[6], "1");
if ((! SVN_IS_VALID_REVNUM(revision)) || (! digits_end) || *digits_end)
SVN_INT_ERR(svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
@@ -223,7 +250,7 @@ main(int argc, const char *argv[])
/* Do something. */
err = change_rev_prop(url, revision, propname, propval, old_propval,
- http_library, pool);
+ http_library, want_error, pool);
if (err)
{
svn_handle_error2(err, stderr, FALSE, "atomic-ra-revprop-change: ");
Index: subversion/tests/cmdline/prop_tests.py
===================================================================
--- subversion/tests/cmdline/prop_tests.py (revision 999158)
+++ subversion/tests/cmdline/prop_tests.py (working copy)
@@ -2010,8 +2010,8 @@ def atomic_over_ra(sbox):
if svntest.main.server_has_atomic_revprop():
expected_stderr = ".*revprop 'flower' has unexpected value.*"
svntest.actions.run_and_verify_atomic_ra_revprop_change(
- None, None, expected_stderr, 1, repo_url, 0, 'flower',
- not_the_old_value, proposed_value)
+ None, None, expected_stderr, 0, repo_url, 0, 'flower',
+ not_the_old_value, proposed_value, True)
else:
expect_old_server_fail(not_the_old_value, proposed_value)
@@ -2019,7 +2019,7 @@ def atomic_over_ra(sbox):
if svntest.main.server_has_atomic_revprop():
svntest.actions.run_and_verify_atomic_ra_revprop_change(
None, None, [], 0, repo_url, 0, 'flower',
- yes_the_old_value, proposed_value)
+ yes_the_old_value, proposed_value, False)
else:
expect_old_server_fail(yes_the_old_value, proposed_value)
