On Fri, Sep 4, 2026 at 5:58 PM <[email protected]> wrote: > Author: rinrab > Date: Fri Sep 4 15:57:28 2026 > New Revision: 1937873 > > Log: > Add tests on the compat RA API (svn_ra_plugin_t) and fix a few crashes in > open_session that were making it completely unusable. > > * subversion/libsvn_ra/wrapper_template.h > (compat_open): Add scratch_pool and check that session_url is not NULL. > * subversion/tests/libsvn_ra/ra-test.c > (#includes): Rearrange includes to shut the deprecation warnings up. > (test_compat_vtable): New test. > (test_funcs): Run the test. > > Modified: > subversion/trunk/subversion/libsvn_ra/wrapper_template.h > subversion/trunk/subversion/tests/libsvn_ra/ra-test.c > > Modified: subversion/trunk/subversion/libsvn_ra/wrapper_template.h > > ============================================================================== > --- subversion/trunk/subversion/libsvn_ra/wrapper_template.h Fri Sep 4 > 15:54:51 2026 (r1937872) > +++ subversion/trunk/subversion/libsvn_ra/wrapper_template.h Fri Sep 4 > 15:57:28 2026 (r1937873) > @@ -71,7 +71,12 @@ static svn_error_t *compat_open(void **s > * the alternative (creating a new ra_util library) would be massive > * overkill for the time being. Just be sure to keep the following > * line and the code of svn_ra_create_callbacks in sync. */ > + > + /* Some RA modules (libsvn_ra_serf) want this exact pool configuration > and > + * refuse to work with the same pool as both scratch_pool and > result_pool. */ > apr_pool_t *sesspool = svn_pool_create(pool); > + apr_pool_t *scratch_pool = svn_pool_create(sesspool); > + > svn_ra_callbacks2_t *callbacks2 = apr_pcalloc(sesspool, > sizeof(*callbacks2)); > > @@ -93,9 +98,11 @@ static svn_error_t *compat_open(void **s > SVN_ERR(VTBL.open_session(sess, &session_url, NULL, repos_URL, > callbacks2, callback_baton, > callbacks ? callbacks->auth_baton : NULL, > - config, sesspool, sesspool)); > + config, sesspool, scratch_pool)); > + > + svn_pool_destroy(scratch_pool); > > - if (strcmp(repos_URL, session_url) != 0) > + if (session_url && strcmp(repos_URL, session_url) != 0) > { > svn_pool_destroy(sesspool); > return svn_error_createf(SVN_ERR_RA_SESSION_URL_MISMATCH, NULL, > > Modified: subversion/trunk/subversion/tests/libsvn_ra/ra-test.c > > ============================================================================== > --- subversion/trunk/subversion/tests/libsvn_ra/ra-test.c Fri Sep 4 > 15:54:51 2026 (r1937872) > +++ subversion/trunk/subversion/tests/libsvn_ra/ra-test.c Fri Sep 4 > 15:57:28 2026 (r1937873) > @@ -23,6 +23,9 @@ > > > > +#include "../svn_test.h" > +#include "../svn_test_fs.h" > + > #include <apr_general.h> > #include <apr_pools.h> > #include <apr_file_io.h> > @@ -37,8 +40,6 @@ > #include "svn_dirent_uri.h" > #include "svn_hash.h" > > -#include "../svn_test.h" > -#include "../svn_test_fs.h" > #include "../../libsvn_ra_local/ra_local.h" > > /*-------------------------------------------------------------------*/ > @@ -1914,6 +1915,49 @@ test_get_deleted_rev_errors(const svn_te > return SVN_NO_ERROR; > } > > +static svn_error_t * > +test_compat_vtable(const svn_test_opts_t *opts, > + apr_pool_t *pool) > +{ > + svn_ra_plugin_t *lib; > + void *ra_baton, *session_baton, *edit_baton, *root_baton, *dir_baton; > + const svn_delta_editor_t *editor; > + const char *url; > + svn_ra_callbacks_t cbtable = { 0 }; > + > + SVN_ERR(svn_test__create_repos2(NULL, &url, NULL, "test_compat_vtable", > opts, > + pool, pool)); > + SVN_ERR(svn_ra_initialize(pool)); > + > + SVN_ERR(svn_ra_init_ra_libs(&ra_baton, pool)); > + SVN_ERR(svn_ra_get_ra_library(&lib, ra_baton, url, pool)); > + > + SVN_ERR(svn_test__init_auth_baton(&cbtable.auth_baton, pool)); > + SVN_ERR(lib->open(&session_baton, url, &cbtable, NULL, NULL, pool)); > + > + /* mkdir A */ > + SVN_ERR(lib->get_commit_editor(session_baton, &editor, &edit_baton, > + "r1", NULL, NULL, pool)); > + SVN_ERR(editor->open_root(edit_baton, SVN_INVALID_REVNUM, > + pool, &root_baton)); > + SVN_ERR(editor->add_directory("A", 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(lib->get_commit_editor(session_baton, &editor, &edit_baton, > + "r2", NULL, NULL, 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; > +} > + > > /* The test table. */ > > @@ -1954,6 +1998,8 @@ static struct svn_test_descriptor_t test > "test get-deleted-rev no delete"), > SVN_TEST_OPTS_PASS(test_get_deleted_rev_errors, > "test get-deleted-rev errors"), > + SVN_TEST_OPTS_PASS(test_compat_vtable, > + "test compat svn_ra_plugin_t"), > SVN_TEST_NULL > }; > > > I discovered that the API we provide for backward compatibility hasn't been working for quite some time (I believe r1873487/1.14 was the revision it was first broken, maybe even earlier). I don't think anyone should still be using this code. So does anyone still test it.
The answer is probably we don't. Plus it seems wasteful to remember to update this wrapper any time something changes. I think, since it's no longer a compatibility promise, why spend time maintaining this code at all. This API has been deprecated since 1.1 and nobody is using it. I propose we remove it and make svn_ra_get_ra_library & friends return an error so that we atleast keep it technically binary compatible. Alternatively, we could remove the symbols alongside the svn_ra_plugin_t entirely, but it's probably more towards 2.0. -- Timofei Zhakov
