On Fri, Sep 4, 2026 at 7:41 PM Branko Čibej <[email protected]> wrote:
> On 4. 9. 2026 18:22, Timofei Zhakov wrote: > > 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. > > > > Hmm. While I agree in principle that maintaining unused APIs is a burden, > our compatibility promises are quite strict. Removing a 1.0 API, even if it > was deprecated in 1.1, is a no-go. I'd prefer if we could come up with a > way, even if only in maintainer mode, to verify that the backwards > compatibility shims are up to date. > > I agree. But what about throwing a "not implemented" error instead? Is it still breaking backward compatibility or it could actually be a way to go? I'm not an expert in this field, but I'm pretty sure a similar thing was done to ra_dav. The symbols still exist, but the module itself was removed a long time ago. -- Timofei Zhakov
