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)