Merge the r1860936 group from trunk:
* r1860936, r1860951, r1860958
Provide a way for svnserve's 'get-deleted-rev' API to return 'not deleted'.
Justification:
Error handling was inconsistent across RA layers.
Votes:
+1: julianfoad, stsp
git-svn-id: https://svn.apache.org/repos/asf/subversion/branches/1.12.x@1863290 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/STATUS b/STATUS
index 07e319a..ba635cc 100644
--- a/STATUS
+++ b/STATUS
@@ -20,10 +20,3 @@
Approved changes:
=================
-
- * r1860936, r1860951, r1860958
- Provide a way for svnserve's 'get-deleted-rev' API to return 'not deleted'.
- Justification:
- Error handling was inconsistent across RA layers.
- Votes:
- +1: julianfoad, stsp
diff --git a/subversion/libsvn_ra_svn/client.c b/subversion/libsvn_ra_svn/client.c
index ab1aa58..dd60dbe 100644
--- a/subversion/libsvn_ra_svn/client.c
+++ b/subversion/libsvn_ra_svn/client.c
@@ -3105,6 +3105,7 @@
{
svn_ra_svn__session_baton_t *sess_baton = session->priv;
svn_ra_svn_conn_t *conn = sess_baton->conn;
+ svn_error_t *err;
path = reparent_path(session, path, pool);
@@ -3116,8 +3117,20 @@
SVN_ERR(handle_unsupported_cmd(handle_auth_request(sess_baton, pool),
N_("'get-deleted-rev' not implemented")));
- return svn_error_trace(svn_ra_svn__read_cmd_response(conn, pool, "r",
- revision_deleted));
+ err = svn_error_trace(svn_ra_svn__read_cmd_response(conn, pool, "r",
+ revision_deleted));
+ /* The protocol does not allow for a reply of SVN_INVALID_REVNUM directly.
+ Instead, a new enough server returns SVN_ERR_ENTRY_MISSING_REVISION to
+ indicate the answer to the query is SVN_INVALID_REVNUM. (An older server
+ closes the connection and returns SVN_ERR_RA_SVN_CONNECTION_CLOSED.) */
+ if (err && err->apr_err == SVN_ERR_ENTRY_MISSING_REVISION)
+ {
+ *revision_deleted = SVN_INVALID_REVNUM;
+ svn_error_clear(err);
+ }
+ else
+ SVN_ERR(err);
+ return SVN_NO_ERROR;
}
static svn_error_t *
diff --git a/subversion/svnserve/serve.c b/subversion/svnserve/serve.c
index 5b58e14..ba65340 100644
--- a/subversion/svnserve/serve.c
+++ b/subversion/svnserve/serve.c
@@ -3516,8 +3516,21 @@
svn_relpath_canonicalize(path, pool), pool);
SVN_ERR(log_command(b, conn, pool, "get-deleted-rev"));
SVN_ERR(trivial_auth_request(conn, pool, b));
- SVN_ERR(svn_repos_deleted_rev(b->repository->fs, full_path, peg_revision,
- end_revision, &revision_deleted, pool));
+ SVN_CMD_ERR(svn_repos_deleted_rev(b->repository->fs, full_path, peg_revision,
+ end_revision, &revision_deleted, pool));
+
+ /* The protocol does not allow for a reply of SVN_INVALID_REVNUM directly.
+ Instead, return SVN_ERR_ENTRY_MISSING_REVISION. A new enough client
+ knows that this means the answer to the query is SVN_INVALID_REVNUM.
+ (An older client reports this as an error.) */
+ if (revision_deleted == SVN_INVALID_REVNUM)
+ SVN_CMD_ERR(svn_error_createf(SVN_ERR_ENTRY_MISSING_REVISION, NULL,
+ "svn protocol command 'get-deleted-rev': "
+ "path '%s' was not deleted in r%ld-%ld; "
+ "NOTE: newer clients handle this case "
+ "and do not report it as an error",
+ full_path, peg_revision, end_revision));
+
SVN_ERR(svn_ra_svn__write_cmd_response(conn, pool, "r", revision_deleted));
return SVN_NO_ERROR;
}
@@ -4125,7 +4138,7 @@
serve_params_t *params,
apr_pool_t *scratch_pool)
{
- svn_error_t *err, *io_err;
+ svn_error_t *err;
apr_uint64_t ver;
const char *client_url, *ra_client_string, *client_string;
svn_ra_svn__list_t *caplist;
@@ -4267,11 +4280,12 @@
}
if (err)
{
- log_error(err, b);
- io_err = svn_ra_svn__write_cmd_failure(conn, scratch_pool, err);
- svn_error_clear(err);
- SVN_ERR(io_err);
- return svn_ra_svn__flush(conn, scratch_pool);
+ /* Report these errors to the client before closing the connection. */
+ err = svn_error_compose_create(err,
+ svn_ra_svn__write_cmd_failure(conn, scratch_pool, err));
+ err = svn_error_compose_create(err,
+ svn_ra_svn__flush(conn, scratch_pool));
+ return err;
}
SVN_ERR(svn_fs_get_uuid(b->repository->fs, &b->repository->uuid,
diff --git a/subversion/tests/libsvn_ra/ra-test.c b/subversion/tests/libsvn_ra/ra-test.c
index 061b19a..beee607 100644
--- a/subversion/tests/libsvn_ra/ra-test.c
+++ b/subversion/tests/libsvn_ra/ra-test.c
@@ -94,6 +94,41 @@
return SVN_NO_ERROR;
}
+/* Commit two revisions: add 'B', then delete 'A' */
+static svn_error_t *
+commit_two_changes(svn_ra_session_t *session,
+ apr_pool_t *pool)
+{
+ apr_hash_t *revprop_table = apr_hash_make(pool);
+ const svn_delta_editor_t *editor;
+ void *edit_baton;
+ void *root_baton, *dir_baton;
+
+ /* mkdir B */
+ SVN_ERR(svn_ra_get_commit_editor3(session, &editor, &edit_baton,
+ revprop_table,
+ NULL, NULL, NULL, TRUE, pool));
+ SVN_ERR(editor->open_root(edit_baton, SVN_INVALID_REVNUM,
+ pool, &root_baton));
+ SVN_ERR(editor->add_directory("B", root_baton, NULL, SVN_INVALID_REVNUM,
+ pool, &dir_baton));
+ SVN_ERR(editor->close_directory(dir_baton, pool));
+ SVN_ERR(editor->close_directory(root_baton, pool));
+ SVN_ERR(editor->close_edit(edit_baton, pool));
+
+ /* delete A */
+ SVN_ERR(svn_ra_get_commit_editor3(session, &editor, &edit_baton,
+ revprop_table,
+ NULL, NULL, NULL, TRUE, pool));
+ SVN_ERR(editor->open_root(edit_baton, SVN_INVALID_REVNUM,
+ pool, &root_baton));
+ SVN_ERR(editor->delete_entry("A", SVN_INVALID_REVNUM, root_baton, pool));
+ SVN_ERR(editor->close_directory(root_baton, pool));
+ SVN_ERR(editor->close_edit(edit_baton, pool));
+
+ return SVN_NO_ERROR;
+}
+
static svn_error_t *
commit_tree(svn_ra_session_t *session,
apr_pool_t *pool)
@@ -1784,6 +1819,63 @@
return SVN_NO_ERROR;
}
+/* Cases of 'get-deleted-rev' that should return SVN_INVALID_REVNUM. */
+static svn_error_t *
+test_get_deleted_rev_no_delete(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ svn_ra_session_t *ra_session;
+ svn_revnum_t revision_deleted;
+
+ SVN_ERR(make_and_open_repos(&ra_session,
+ "test-repo-get-deleted-rev-no-delete", opts,
+ pool));
+ SVN_ERR(commit_changes(ra_session, pool));
+ SVN_ERR(commit_two_changes(ra_session, pool));
+
+ /* expect 'no deletion' in the range up to r2, when it is deleted in r3 */
+ /* This was failing over RA-SVN where the 'get-deleted-rev' wire command's
+ prototype cannot directly represent that result. A new enough client and
+ server collaborate on a work-around implemented using an error code. */
+ SVN_ERR(svn_ra_get_deleted_rev(ra_session, "A", 1, 2,
+ &revision_deleted, pool));
+ SVN_TEST_INT_ASSERT(revision_deleted, SVN_INVALID_REVNUM);
+
+ /* this connection should still be open: a simple case should still work */
+ SVN_ERR(svn_ra_get_deleted_rev(ra_session, "A", 1, 3,
+ &revision_deleted, pool));
+ SVN_TEST_INT_ASSERT(revision_deleted, 3);
+
+ return SVN_NO_ERROR;
+}
+
+/* Cases of 'get-deleted-rev' that should return an error. */
+static svn_error_t *
+test_get_deleted_rev_errors(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ svn_ra_session_t *ra_session;
+ svn_revnum_t revision_deleted;
+ svn_error_t *err;
+
+ SVN_ERR(make_and_open_repos(&ra_session,
+ "test-repo-get-deleted-rev-errors", opts, pool));
+ SVN_ERR(commit_changes(ra_session, pool));
+
+ /* expect an error when searching up to r3, when repository head is r1 */
+ err = svn_ra_get_deleted_rev(ra_session, "A", 1, 3, &revision_deleted, pool);
+
+ /* mod_dav_svn returns a generic error code for "500 Internal Server Error";
+ * the other RA layers return the specific error code for "no such revision".
+ * We should make these consistent, but for now that's how it is. */
+ if (opts->repos_url && strncmp(opts->repos_url, "http", 4) == 0)
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_RA_DAV_REQUEST_FAILED);
+ else
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_FS_NO_SUCH_REVISION);
+
+ return SVN_NO_ERROR;
+}
+
/* The test table. */
@@ -1820,6 +1912,10 @@
"check how last change applies to empty commit"),
SVN_TEST_OPTS_PASS(commit_locked_file,
"check commit editor for a locked file"),
+ SVN_TEST_OPTS_PASS(test_get_deleted_rev_no_delete,
+ "test get-deleted-rev no delete"),
+ SVN_TEST_OPTS_PASS(test_get_deleted_rev_errors,
+ "test get-deleted-rev errors"),
SVN_TEST_NULL
};
