You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ju...@apache.org on 2014/12/15 18:12:20 UTC
svn commit: r1643074 - in /subversion/trunk/subversion: include/svn_repos.h
libsvn_repos/load-fs-vtable.c tests/libsvn_repos/dump-load-test.c
Author: julianfoad
Date: Wed Dec 3 09:30:51 2014
New Revision: 1643074
URL: http://svn.apache.org/r1643074
Log:
Fix the 'svnadmin load' part of issue #4476 "Mergeinfo containing r0 makes
svnsync and svnadmin dump fail".
Make 'svnadmin load' accept mergeinfo containing r0 (with a warning) if the
option to validate properties is not enabled. In that case that mergeinfo
property will not be adjusted for revision numbers or paths. Add a test.
* subversion/include/svn_repos.h
(svn_repos_notify_warning_t): Add a new value for invalid mergeinfo.
* subversion/libsvn_repos/load-fs-vtable.c
(adjust_mergeinfo_property): New, factored out of ...
(set_node_property): ... here. If adjusting the mergeinfo fails, return an
error if validating properties, or give a warning and use the invalid
value otherwise.
* subversion/tests/libsvn_repos/dump-load-test.c
(test_dump_bad_props): New, factored out of...
(test_dump_r0_mergeinfo): ... here.
(test_load_bad_props,
load_r0_mergeinfo_notifier,
test_load_r0_mergeinfo): New.
(test_funcs): Add the new test.
Modified:
subversion/trunk/subversion/include/svn_repos.h
subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c
subversion/trunk/subversion/tests/libsvn_repos/dump-load-test.c
Modified: subversion/trunk/subversion/include/svn_repos.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_repos.h?rev=1643074&r1=1643073&r2=1643074&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_repos.h (original)
+++ subversion/trunk/subversion/include/svn_repos.h Wed Dec 3 09:30:51 2014
@@ -287,7 +287,14 @@ typedef enum svn_repos_notify_warning_t
*
* @since New in 1.9.
*/
- svn_repos_notify_warning_mergeinfo_collision
+ svn_repos_notify_warning_mergeinfo_collision,
+
+ /**
+ * Detected invalid mergeinfo.
+ *
+ * @since New in 1.9.
+ */
+ svn_repos_notify_warning_invalid_mergeinfo
} svn_repos_notify_warning_t;
/**
Modified: subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c?rev=1643074&r1=1643073&r2=1643074&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c (original)
+++ subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c Wed Dec 3 09:30:51 2014
@@ -753,6 +753,67 @@ set_revision_property(void *baton,
}
+/* Adjust mergeinfo:
+ * - normalize line endings (if all CRLF, change to LF; but error if mixed);
+ * - adjust revision numbers (see renumber_mergeinfo_revs());
+ * - adjust paths (see prefix_mergeinfo_paths()).
+ */
+static svn_error_t *
+adjust_mergeinfo_property(struct revision_baton *rb,
+ svn_string_t **new_value_p,
+ const svn_string_t *old_value,
+ apr_pool_t *result_pool)
+{
+ struct parse_baton *pb = rb->pb;
+ svn_string_t prop_val = *old_value;
+
+ /* Tolerate mergeinfo with "\r\n" line endings because some
+ dumpstream sources might contain as much. If so normalize
+ the line endings to '\n' and make a notification to
+ PARSE_BATON->FEEDBACK_STREAM that we have made this
+ correction. */
+ if (strstr(prop_val.data, "\r"))
+ {
+ const char *prop_eol_normalized;
+
+ SVN_ERR(svn_subst_translate_cstring2(prop_val.data,
+ &prop_eol_normalized,
+ "\n", /* translate to LF */
+ FALSE, /* no repair */
+ NULL, /* no keywords */
+ FALSE, /* no expansion */
+ result_pool));
+ prop_val.data = prop_eol_normalized;
+ prop_val.len = strlen(prop_eol_normalized);
+
+ if (pb->notify_func)
+ {
+ /* ### TODO: Use proper scratch pool instead of pb->notify_pool */
+ svn_repos_notify_t *notify
+ = svn_repos_notify_create(
+ svn_repos_notify_load_normalized_mergeinfo,
+ pb->notify_pool);
+
+ pb->notify_func(pb->notify_baton, notify, pb->notify_pool);
+ svn_pool_clear(pb->notify_pool);
+ }
+ }
+
+ /* Renumber mergeinfo as appropriate. */
+ SVN_ERR(renumber_mergeinfo_revs(new_value_p, &prop_val, rb,
+ result_pool));
+ if (pb->parent_dir)
+ {
+ /* Prefix the merge source paths with PB->parent_dir. */
+ /* ASSUMPTION: All source paths are included in the dump stream. */
+ SVN_ERR(prefix_mergeinfo_paths(new_value_p, *new_value_p,
+ pb->parent_dir, result_pool));
+ }
+
+ return SVN_NO_ERROR;
+}
+
+
static svn_error_t *
set_node_property(void *baton,
const char *name,
@@ -766,55 +827,42 @@ set_node_property(void *baton,
if (rb->skipped)
return SVN_NO_ERROR;
+ /* Adjust mergeinfo. If this fails, presumably because the mergeinfo
+ property has an ill-formed value, then we must not fail to load
+ the repository (at least if it's a simple load with no revision
+ offset adjustments, path changes, etc.) so just warn and leave it
+ as it is. */
if (strcmp(name, SVN_PROP_MERGEINFO) == 0)
{
- svn_string_t prop_val = *value;
- svn_string_t *renumbered_mergeinfo;
+ svn_string_t *new_value;
+ svn_error_t *err;
- /* Tolerate mergeinfo with "\r\n" line endings because some
- dumpstream sources might contain as much. If so normalize
- the line endings to '\n' and make a notification to
- PARSE_BATON->FEEDBACK_STREAM that we have made this
- correction. */
- if (strstr(prop_val.data, "\r"))
+ err = adjust_mergeinfo_property(rb, &new_value, value, nb->pool);
+ if (err)
{
- const char *prop_eol_normalized;
-
- SVN_ERR(svn_subst_translate_cstring2(prop_val.data,
- &prop_eol_normalized,
- "\n", /* translate to LF */
- FALSE, /* no repair */
- NULL, /* no keywords */
- FALSE, /* no expansion */
- nb->pool));
- prop_val.data = prop_eol_normalized;
- prop_val.len = strlen(prop_eol_normalized);
-
+ if (pb->validate_props)
+ {
+ return svn_error_quick_wrap(
+ err,
+ _("Invalid svn:mergeinfo value"));
+ }
if (pb->notify_func)
{
- /* ### TODO: Use proper scratch pool instead of pb->notify_pool */
svn_repos_notify_t *notify
- = svn_repos_notify_create(
- svn_repos_notify_load_normalized_mergeinfo,
- pb->notify_pool);
+ = svn_repos_notify_create(svn_repos_notify_warning,
+ pb->notify_pool);
+ notify->warning = svn_repos_notify_warning_invalid_mergeinfo;
+ notify->warning_str = _("Invalid svn:mergeinfo value; "
+ "leaving unchanged");
pb->notify_func(pb->notify_baton, notify, pb->notify_pool);
svn_pool_clear(pb->notify_pool);
}
+ svn_error_clear(err);
}
-
- /* Renumber mergeinfo as appropriate. */
- SVN_ERR(renumber_mergeinfo_revs(&renumbered_mergeinfo, &prop_val, rb,
- nb->pool));
- value = renumbered_mergeinfo;
- if (pb->parent_dir)
+ else
{
- /* Prefix the merge source paths with PB->parent_dir. */
- /* ASSUMPTION: All source paths are included in the dump stream. */
- svn_string_t *mergeinfo_val;
- SVN_ERR(prefix_mergeinfo_paths(&mergeinfo_val, value,
- pb->parent_dir, nb->pool));
- value = mergeinfo_val;
+ value = new_value;
}
}
Modified: subversion/trunk/subversion/tests/libsvn_repos/dump-load-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_repos/dump-load-test.c?rev=1643074&r1=1643073&r2=1643074&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_repos/dump-load-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_repos/dump-load-test.c Wed Dec 3 09:30:51 2014
@@ -35,7 +35,114 @@
-/* Notification receiver for test_dump_bad_mergeinfo(). This does not
+/* Test dumping in the presence of the property PROP_NAME:PROP_VAL.
+ * Return the dumped data in *DUMP_DATA_P (if DUMP_DATA_P is not null).
+ * REPOS is an empty repository.
+ * See svn_repos_dump_fs3() for START_REV, END_REV, NOTIFY_FUNC, NOTIFY_BATON.
+ */
+static svn_error_t *
+test_dump_bad_props(svn_stringbuf_t **dump_data_p,
+ svn_repos_t *repos,
+ const char *prop_name,
+ const svn_string_t *prop_val,
+ svn_revnum_t start_rev,
+ svn_revnum_t end_rev,
+ svn_repos_notify_func_t notify_func,
+ void *notify_baton,
+ apr_pool_t *pool)
+{
+ const char *test_path = "/bar";
+ svn_fs_t *fs = svn_repos_fs(repos);
+ svn_fs_txn_t *txn;
+ svn_fs_root_t *txn_root;
+ svn_revnum_t youngest_rev = 0;
+ svn_stringbuf_t *dump_data = svn_stringbuf_create_empty(pool);
+ svn_stream_t *stream = svn_stream_from_stringbuf(dump_data, pool);
+ const char *expected_str;
+
+ /* Revision 1: Any commit will do, here */
+ SVN_ERR(svn_fs_begin_txn2(&txn, fs, youngest_rev, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
+ SVN_ERR(svn_fs_make_dir(txn_root, test_path , pool));
+ SVN_ERR(svn_repos_fs_commit_txn(NULL, repos, &youngest_rev, txn, pool));
+ SVN_TEST_ASSERT(SVN_IS_VALID_REVNUM(youngest_rev));
+
+ /* Revision 2: Add the bad property */
+ SVN_ERR(svn_fs_begin_txn2(&txn, fs, youngest_rev, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
+ SVN_ERR(svn_fs_change_node_prop(txn_root, test_path , prop_name, prop_val,
+ pool));
+ SVN_ERR(svn_repos_fs_commit_txn(NULL, repos, &youngest_rev, txn, pool));
+ SVN_TEST_ASSERT(SVN_IS_VALID_REVNUM(youngest_rev));
+
+ /* Test that a dump completes without error. */
+ SVN_ERR(svn_repos_dump_fs3(repos, stream, start_rev, end_rev,
+ FALSE, FALSE,
+ notify_func, notify_baton,
+ NULL, NULL,
+ pool));
+ svn_stream_close(stream);
+
+ /* Check that the property appears in the dump data */
+ expected_str = apr_psprintf(pool, "K %d\n%s\n"
+ "V %d\n%s\n"
+ "PROPS-END\n",
+ (int)strlen(prop_name), prop_name,
+ (int)prop_val->len, prop_val->data);
+ SVN_TEST_ASSERT(strstr(dump_data->data, expected_str));
+
+ if (dump_data_p)
+ *dump_data_p = dump_data;
+ return SVN_NO_ERROR;
+}
+
+/* Test loading in the presence of the property PROP_NAME:PROP_VAL.
+ * Load data from DUMP_DATA.
+ * REPOS is an empty repository.
+ */
+static svn_error_t *
+test_load_bad_props(svn_stringbuf_t *dump_data,
+ svn_repos_t *repos,
+ const char *prop_name,
+ const svn_string_t *prop_val,
+ const char *parent_fspath,
+ svn_boolean_t validate_props,
+ svn_repos_notify_func_t notify_func,
+ void *notify_baton,
+ apr_pool_t *pool)
+{
+ const char *test_path = apr_psprintf(pool, "%s%s",
+ parent_fspath ? parent_fspath : "",
+ "/bar");
+ svn_stream_t *stream = svn_stream_from_stringbuf(dump_data, pool);
+ svn_fs_t *fs;
+ svn_fs_root_t *rev_root;
+ svn_revnum_t youngest_rev;
+ svn_string_t *loaded_prop_val;
+
+ SVN_ERR(svn_repos_load_fs5(repos, stream,
+ SVN_INVALID_REVNUM, SVN_INVALID_REVNUM,
+ svn_repos_load_uuid_default,
+ parent_fspath,
+ FALSE, FALSE, /*use_*_commit_hook*/
+ validate_props,
+ FALSE /*ignore_dates*/,
+ notify_func, notify_baton,
+ NULL, NULL, /*cancellation*/
+ pool));
+ svn_stream_close(stream);
+
+ /* Check the loaded property */
+ fs = svn_repos_fs(repos);
+ SVN_ERR(svn_fs_youngest_rev(&youngest_rev, fs, pool));
+ SVN_ERR(svn_fs_revision_root(&rev_root, fs, youngest_rev, pool));
+ SVN_ERR(svn_fs_node_prop(&loaded_prop_val,
+ rev_root, test_path, prop_name, pool));
+ SVN_TEST_ASSERT(svn_string_compare(loaded_prop_val, prop_val));
+ return SVN_NO_ERROR;
+}
+
+/* Notification receiver for test_dump_r0_mergeinfo(). This does not
need to do anything, it just needs to exist.
*/
static void
@@ -51,44 +158,115 @@ static svn_error_t *
test_dump_r0_mergeinfo(const svn_test_opts_t *opts,
apr_pool_t *pool)
{
- svn_repos_t *repos;
- svn_fs_t *fs;
- svn_fs_txn_t *txn;
- svn_fs_root_t *txn_root;
- svn_revnum_t youngest_rev = 0;
+ const char *prop_name = "svn:mergeinfo";
const svn_string_t *bad_mergeinfo = svn_string_create("/foo:0", pool);
+ svn_repos_t *repos;
SVN_ERR(svn_test__create_repos(&repos, "test-repo-dump-r0-mergeinfo",
opts, pool));
- fs = svn_repos_fs(repos);
-
- /* Revision 1: Any commit will do, here */
- SVN_ERR(svn_fs_begin_txn2(&txn, fs, youngest_rev, 0, pool));
- SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
- SVN_ERR(svn_fs_make_dir(txn_root, "/bar", pool));
- SVN_ERR(svn_repos_fs_commit_txn(NULL, repos, &youngest_rev, txn, pool));
- SVN_TEST_ASSERT(SVN_IS_VALID_REVNUM(youngest_rev));
-
- /* Revision 2: Add bad mergeinfo */
- SVN_ERR(svn_fs_begin_txn2(&txn, fs, youngest_rev, 0, pool));
- SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
- SVN_ERR(svn_fs_change_node_prop(txn_root, "/bar", "svn:mergeinfo", bad_mergeinfo, pool));
- SVN_ERR(svn_repos_fs_commit_txn(NULL, repos, &youngest_rev, txn, pool));
- SVN_TEST_ASSERT(SVN_IS_VALID_REVNUM(youngest_rev));
-
- /* Test that a dump completes without error. In order to exercise the
+ /* In order to exercise the
functionality under test -- that is, in order for the dump to try to
parse the mergeinfo it is dumping -- the dump must start from a
revision greater than 1 and must take a notification callback. */
+ SVN_ERR(test_dump_bad_props(NULL, repos,
+ prop_name, bad_mergeinfo,
+ 2, SVN_INVALID_REVNUM,
+ dump_r0_mergeinfo_notifier, NULL,
+ pool));
+
+ return SVN_NO_ERROR;
+}
+
+static void
+load_r0_mergeinfo_notifier(void *baton,
+ const svn_repos_notify_t *notify,
+ apr_pool_t *scratch_pool)
+{
+ svn_boolean_t *had_mergeinfo_warning = baton;
+
+ if (notify->action == svn_repos_notify_warning)
+ {
+ if (notify->warning == svn_repos_notify_warning_invalid_mergeinfo)
+ {
+ *had_mergeinfo_warning = TRUE;
+ }
+ }
+}
+
+/* Regression test for the 'load' part of issue #4476 "Mergeinfo
+ * containing r0 makes svnsync and svnadmin dump fail".
+ *
+ * Bad mergeinfo should not prevent loading a backup, at least when we do not
+ * require mergeinfo revision numbers or paths to be adjusted during loading.
+ */
+static svn_error_t *
+test_load_r0_mergeinfo(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ const char *prop_name = "svn:mergeinfo";
+ const svn_string_t *prop_val = svn_string_create("/foo:0", pool);
+ svn_stringbuf_t *dump_data = svn_stringbuf_create_empty(pool);
+
+ /* Produce a dump file containing bad mergeinfo */
{
- svn_stringbuf_t *stringbuf = svn_stringbuf_create_empty(pool);
- svn_stream_t *stream = svn_stream_from_stringbuf(stringbuf, pool);
+ svn_repos_t *repos;
+
+ SVN_ERR(svn_test__create_repos(&repos, "test-repo-load-r0-mi-1",
+ opts, pool));
+ SVN_ERR(test_dump_bad_props(&dump_data, repos,
+ prop_name, prop_val,
+ SVN_INVALID_REVNUM, SVN_INVALID_REVNUM,
+ NULL, NULL, pool));
+ }
+
+ /* Test loading without validating properties: should warn and succeed */
+ {
+ svn_repos_t *repos;
+ svn_boolean_t had_mergeinfo_warning = FALSE;
+
+ SVN_ERR(svn_test__create_repos(&repos, "test-repo-load-r0-mi-2",
+ opts, pool));
+
+ /* Without changing revision numbers or paths */
+ SVN_ERR(test_load_bad_props(dump_data, repos,
+ prop_name, prop_val,
+ NULL /*parent_dir*/, FALSE /*validate_props*/,
+ load_r0_mergeinfo_notifier, &had_mergeinfo_warning,
+ pool));
+ SVN_TEST_ASSERT(had_mergeinfo_warning);
+
+ /* With changing revision numbers and/or paths (by loading the same data
+ again, on top of existing revisions, into subdirectory 'bar') */
+ had_mergeinfo_warning = FALSE;
+ SVN_ERR(test_load_bad_props(dump_data, repos,
+ prop_name, prop_val,
+ "/bar", FALSE /*validate_props*/,
+ load_r0_mergeinfo_notifier, &had_mergeinfo_warning,
+ pool));
+ SVN_TEST_ASSERT(had_mergeinfo_warning);
+ }
+
+ /* Test loading with validating properties: should return an error */
+ {
+ svn_repos_t *repos;
+
+ SVN_ERR(svn_test__create_repos(&repos, "test-repo-load-r0-mi-3",
+ opts, pool));
- SVN_ERR(svn_repos_dump_fs3(repos, stream, 2, SVN_INVALID_REVNUM,
- FALSE, FALSE,
- dump_r0_mergeinfo_notifier, NULL,
- NULL, NULL,
- pool));
+ /* Without changing revision numbers or paths */
+ SVN_TEST_ASSERT_ANY_ERROR(test_load_bad_props(dump_data, repos,
+ prop_name, prop_val,
+ NULL /*parent_dir*/, TRUE /*validate_props*/,
+ NULL, NULL,
+ pool));
+
+ /* With changing revision numbers and/or paths (by loading the same data
+ again, on top of existing revisions, into subdirectory 'bar') */
+ SVN_TEST_ASSERT_ANY_ERROR(test_load_bad_props(dump_data, repos,
+ prop_name, prop_val,
+ "/bar", TRUE /*validate_props*/,
+ NULL, NULL,
+ pool));
}
return SVN_NO_ERROR;
@@ -103,6 +281,8 @@ static struct svn_test_descriptor_t test
SVN_TEST_NULL,
SVN_TEST_OPTS_PASS(test_dump_r0_mergeinfo,
"test dumping with r0 mergeinfo"),
+ SVN_TEST_OPTS_PASS(test_load_r0_mergeinfo,
+ "test loading with r0 mergeinfo"),
SVN_TEST_NULL
};