subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From julianf...@apache.org
Subject svn commit: r1643074 - in /subversion/trunk/subversion: include/svn_repos.h libsvn_repos/load-fs-vtable.c tests/libsvn_repos/dump-load-test.c
Date Mon, 15 Dec 2014 17:12:20 GMT
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
   };
 



Mime
View raw message