Hi!
I've noticed inconsistency in svn_repos_create() behavior. Despite the fact
that creation of repositories over empty directories is allowed, in some cases
this function will remove an empty directory that existed before repository
creation.
I've attached the patch with test and fix.
[[[
When creating a repository over an existing (empty) folder, don't remove this
empty folder if the creation fails.
* include/svn_io.h
(svn_io_remove_dir_contents): Declare.
* libsvn_repos/repos.c
(svn_repos_create): Don't remove the top-level repository directory if it
existed before repository creation.
* subversion/libsvn_subr/io.c
(remove_dir_helper,
svn_io_remove_dir2): Extract core logic to new remove_dir_helper() function.
(svn_io_remove_dir_contents): New.
* subversion/tests/libsvn_repos/repos-test.c
(fail_to_create_repos_over_empty_dir): New test.
(test_funcs): Add new test.
Patch by: sergey.raevskiy{_AT_}visualsvn.com
]]]
Index: subversion/include/svn_io.h
===================================================================
--- subversion/include/svn_io.h (revision 1669349)
+++ subversion/include/svn_io.h (working copy)
@@ -1662,6 +1662,22 @@ svn_error_t *
svn_io_remove_dir(const char *path,
apr_pool_t *pool);
+/** Recursively remove contents of directory specified by @a path. @a path is
+ * utf8-encoded. If @a ignore_enoent is @c TRUE, don't fail if the target
+ * directory doesn't exist. Use @a pool for temporary allocations.
+ *
+ * Because recursive delete of a directory tree can be a lengthy operation,
+ * provide @a cancel_func and @a cancel_baton for interruptibility.
+ *
+ * @since New in 1.10.
+ */
+svn_error_t *
+svn_io_remove_dir_contents(const char *path,
+ svn_boolean_t ignore_enoent,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ apr_pool_t *pool);
+
/** Read all of the disk entries in directory @a path, a utf8-encoded
* path. Set @a *dirents to a hash mapping dirent names (<tt>char *</tt>) to
* undefined non-NULL values, allocated in @a pool.
Index: subversion/libsvn_repos/repos.c
===================================================================
--- subversion/libsvn_repos/repos.c (revision 1669349)
+++ subversion/libsvn_repos/repos.c (working copy)
@@ -1136,6 +1136,7 @@ svn_repos_create(svn_repos_t **repos_p,
apr_pool_t *scratch_pool = svn_pool_create(result_pool);
const char *root_path;
const char *local_abspath;
+ svn_node_kind_t kind;
/* Allocate a repository object, filling in the format we will create. */
repos = create_svn_repos_t(path, result_pool);
@@ -1167,6 +1168,9 @@ svn_repos_create(svn_repos_t **repos_p,
scratch_pool));
}
+ /* Check if directory exists before creating the repository structure. */
+ SVN_ERR(svn_io_check_path(path, &kind, scratch_pool));
+
/* Create the various files and subdirectories for the repository. */
SVN_ERR_W(create_repos_structure(repos, path, fs_config, scratch_pool),
_("Repository creation failed"));
@@ -1180,16 +1184,28 @@ svn_repos_create(svn_repos_t **repos_p,
{
/* If there was an error making the filesytem, e.g. unknown/supported
* filesystem type. Clean up after ourselves. Yes this is safe because
- * create_repos_structure will fail if the path existed before we started
- * so we can't accidentally remove a directory that previously existed.
- */
+ * create_repos_structure will fail if the top-level directory existed
+ * and was non-empty before we started. */
svn_pool_destroy(scratch_pool); /* Release lock to allow deleting dir */
- return svn_error_trace(
- svn_error_compose_create(
- err,
- svn_io_remove_dir2(path, FALSE, NULL, NULL,
- result_pool)));
+ if (kind == svn_node_dir)
+ {
+ /* Don't remove the top-level repository directory if it existed
+ * before repository creation. */
+ return svn_error_trace(
+ svn_error_compose_create(
+ err,
+ svn_io_remove_dir_contents(path, FALSE, NULL, NULL,
+ result_pool)));
+ }
+ else
+ {
+ return svn_error_trace(
+ svn_error_compose_create(
+ err,
+ svn_io_remove_dir2(path, FALSE, NULL, NULL,
+ result_pool)));
+ }
}
/* This repository is ready. Stamp it with a format number. */
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 1669349)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -2549,14 +2549,13 @@ svn_io_remove_dir(const char *path, apr_pool_t *po
http://subversion.tigris.org/issues/show_bug.cgi?id=3501.
*/
-/* Neither windows nor unix allows us to delete a non-empty
- directory.
-
- This is a function to perform the equivalent of 'rm -rf'. */
-svn_error_t *
-svn_io_remove_dir2(const char *path, svn_boolean_t ignore_enoent,
- svn_cancel_func_t cancel_func, void *cancel_baton,
- apr_pool_t *pool)
+static svn_error_t *
+remove_dir_helper(const char *path,
+ svn_boolean_t ignore_enoent,
+ svn_boolean_t remove_top_dir,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ apr_pool_t *pool)
{
svn_error_t *err;
apr_pool_t *subpool;
@@ -2594,8 +2593,8 @@ svn_io_remove_dir(const char *path, apr_pool_t *po
if (dirent->kind == svn_node_dir)
{
/* Don't check for cancellation, the callee will immediately do so */
- SVN_ERR(svn_io_remove_dir2(fullpath, FALSE, cancel_func,
- cancel_baton, subpool));
+ SVN_ERR(remove_dir_helper(fullpath, FALSE, TRUE, cancel_func,
+ cancel_baton, subpool));
}
else
{
@@ -2612,10 +2611,37 @@ svn_io_remove_dir(const char *path, apr_pool_t *po
svn_pool_destroy(subpool);
- return svn_io_dir_remove_nonrecursive(path, pool);
+ if (remove_top_dir)
+ SVN_ERR(svn_io_dir_remove_nonrecursive(path, pool));
+
+ return SVN_NO_ERROR;
}
+/* Neither windows nor unix allows us to delete a non-empty
+ directory.
+
+ This is a function to perform the equivalent of 'rm -rf'. */
svn_error_t *
+svn_io_remove_dir2(const char *path, svn_boolean_t ignore_enoent,
+ svn_cancel_func_t cancel_func, void *cancel_baton,
+ apr_pool_t *pool)
+{
+ return svn_error_trace(remove_dir_helper(path, ignore_enoent, TRUE,
+ cancel_func, cancel_baton, pool));
+}
+
+svn_error_t *
+svn_io_remove_dir_contents(const char *path,
+ svn_boolean_t ignore_enoent,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ apr_pool_t *pool)
+{
+ return svn_error_trace(remove_dir_helper(path, ignore_enoent, FALSE,
+ cancel_func, cancel_baton, pool));
+}
+
+svn_error_t *
svn_io_get_dir_filenames(apr_hash_t **dirents,
const char *path,
apr_pool_t *pool)
Index: subversion/tests/libsvn_repos/repos-test.c
===================================================================
--- subversion/tests/libsvn_repos/repos-test.c (revision 1669349)
+++ subversion/tests/libsvn_repos/repos-test.c (working copy)
@@ -3612,6 +3612,37 @@ deprecated_access_context_api(const svn_test_opts_
return SVN_NO_ERROR;
}
+static svn_error_t *
+fail_to_create_repos_over_empty_dir(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ const char *test_dir = "test-fail-to-create-repos-over-empty-dir";
+ svn_repos_t *repos;
+ apr_hash_t *fs_config;
+ svn_node_kind_t kind;
+
+ /* Remove the test directory from previous runs. */
+ SVN_ERR(svn_io_remove_dir2(test_dir, TRUE, NULL, NULL, pool));
+
+ /* Create an empty directory. */
+ SVN_ERR(svn_io_dir_make(test_dir, APR_OS_DEFAULT, pool));
+ svn_test_add_dir_cleanup(test_dir);
+
+ /* Try to create repository over an existing empty directory. We're forcing
+ svn_repos_create() to fail by specifying an invalid fs-type. */
+ fs_config = apr_hash_make(pool);
+ svn_hash_sets(fs_config, SVN_FS_CONFIG_FS_TYPE, "invalid-fs-type");
+
+ SVN_TEST_ASSERT_ANY_ERROR(svn_repos_create(&repos, test_dir, NULL, NULL,
+ NULL, fs_config, pool));
+
+ /* Directory should exist. */
+ SVN_ERR(svn_io_check_path(test_dir, &kind, pool));
+ SVN_TEST_ASSERT(kind == svn_node_dir);
+
+ return SVN_NO_ERROR;
+}
+
/* The test table. */
static int max_threads = 4;
@@ -3667,6 +3698,8 @@ static struct svn_test_descriptor_t test_funcs[] =
"test test_repos_fs_type"),
SVN_TEST_OPTS_PASS(deprecated_access_context_api,
"test deprecated access context api"),
+ SVN_TEST_OPTS_PASS(fail_to_create_repos_over_empty_dir,
+ "test repository creation over existing empty dir"),
SVN_TEST_NULL
};
