Hi,

Here is something I was working on at the hackathon.  This is the problem
I reported in http://svn.haxx.se/dev/archive-2014-02/0187.shtml
In brief, the problem lies in the way we use the recovery code when hotcopying
these old repositories.  We grab the YOUNGEST revision from the source, do the
hotcopy and then use the recovery procedure in order to determine the node-id
and the copy-id to stamp in the destination db/current file.  The problem is
that proper recovery requires a *full* scan of the FS, which we do not do.
As a consequence, the db/current in the destination is messed up and that is
shown by the XFail'ing fsfs_hotcopy_old_with_propchanges() test.

Anyway, this "use recover to obtain db/current contents" approach seems broken
from the very start.  I would like to fix this problem by switching to
something more obvious.  We could atomically read the (YOUNGEST, NODE-ID,
COPY-ID) tuple from the destination and write it to the destination at the
very moment the hotcopy finished working.  Here is a series of three patches,
which I am willing to commit one-by-one.  Please note that *I did not yet write
the log messages* for this patch series (probably they are going to include
parts of what I've written below).  I am working on them at this very moment.
And sorry for a lot of text :)

In brief, I have the following plan:

- Extend the existing fsfs_hotcopy_old_with_propchanges() test.  We now know
  that the problem is not actually in the property changes, but in the node-
  and copy-id changes.  We can extend the test (which would still be an
  @XFail on trunk) in order to test *more*.

  This is the first patch of the series.

- Fix the problem itself.  The idea of the fix is pretty simple -- we stop
  using the recover()-related functions when doing the hotcopy for repositories
  which have the old FS format.  Previously we did use the recovery mechanism
  in order to recover the next-node-id and the next-copy-id for the hotcopy
  target, but did it *wrong*:

  * The thing is that proper recovery actually needs a full rev-by-rev scan
    of the filesystem, otherwise it might actually miss some of the next- or
    copy-ids.  That's exactly what we encountered here, because now on trunk
    we run the recovery *only* for the youngest revision of the destination
    and hence are messing the node ids in the target 'current' file.

  * We could probably switch to a full recovery scan for this case (that
    would be simpler in the patch terms), but that actually sucks.  You do
    not want to run a full recovery scan in order for the backup (!) to work.
    So, I've fixed this by atomically reading the 'db/current' contents of the
    source and updating it as-is in the destination.  The diff might look a
    bit scary, but this is just a consequence of me extracting the revision
    copying logic into two separate functions (for old and new FS formats
    accordingly).

    The new logic is pretty simple -- if we encounted an FS with a new format,
    just stick to the old code.  If we see the FS format 1 or 2, we use the new
    approach, which is much simpler.  We do not have to deal with the packs and
    shards and stuff like that and simply copy the revisions and revprops (
    sticking to the old approach which copies only files which differ in terms
    of filestamps).  Then we atomically update the 'db/current' in the
    destination (we've atomically read the source 'db/current' contents just
    a bit earlier).

  This is the second patch of the series.

- Cleanup a bit.  Now, because the hotcopy code does not need the recovery
  code (which seems logical), we can remove the corresponding private API,
  namely svn_fs_fs__find_max_ids().  As a consequence, the id recovery code
  now is "private" in the subversion/libsvn_fs_fs/recovery.c file and that
  makes sense from the design point of view.

  This is the third patch of the series.

In case this might be interesting to you, here is a checklist I've run against
the trunk with all three patches applied:

- Run svnadmin_tests.py with Debug / Windows
- Run svnadmin_tests.py with Debug and --fsfs-packing / Windows
- Run svnadmin_tests.py with Debug and --fsfs-sharding / Windows
- Run svnadmin_tests.py with Debug and --server-minor-version=3 / Windows
- Run svnadmin_tests.py with Debug and --server-minor-version=6 / Windows
- Run svnadmin_tests.py with Debug and --server-minor-version=8 / Windows
- Run svnadmin_tests.py with Release / Windows
- Run svnadmin_tests.py with Release and --fs-type=bdb / Windows
- Run svnadmin_tests.py with Release and --fs-type=fsx / Windows
- Run svnadmin_tests.py with Release and --parallel / Windows
- Run svnadmin_tests.py with Release and --http-dir / Windows
- Run all tests / Ubuntu
- Run hotcopy for the new format serf repository, verify it, check db/current
- Run hotcopy for the new format googletest repository, verify it,
  check db/current
- Run hotcopy for the old format serf repository, verify it, check db/current
- Run hotcopy for the old format googletest repository, verify it,
  check db/current
- Run hotcopy for the PACKED new format serf repository, verify it,
  check db/current
- Run hotcopy for the PACKED new format googletest repository, verify it,
  check db/current
- Run hotcopy for the PACKED old format serf repository, verify it,
  check db/current
- Run hotcopy for the PACKED old format googletest repository, verify it,
  check db/current
- Somehow test the incremental hotcopy (say, abort the hotcopy for a
  sharded repository in the middle of it...)
- Do the previous test with packed shards
- Test that the test with changes still fails on trunk (itself)
- Patch the test in order to run it with a normal repository
  version (> 3) / Windows

What do you think?

Regards,
Evgeny Kotkov
Index: subversion/tests/cmdline/svnadmin_tests.py
===================================================================
--- subversion/tests/cmdline/svnadmin_tests.py  (revision 1603111)
+++ subversion/tests/cmdline/svnadmin_tests.py  (working copy)
@@ -2328,25 +2328,117 @@ def load_ignore_dates(sbox):
 
 @XFail()
 @SkipUnless(svntest.main.is_fs_type_fsfs)
-def fsfs_hotcopy_old_with_propchanges(sbox):
-  "hotcopy --compatible-version=1.3 with propchanges"
+def fsfs_hotcopy_old_with_id_changes(sbox):
+  "fsfs hotcopy old with node-id and copy-id changes"
 
   # Around trunk@1573728, running 'svnadmin hotcopy' for the
-  # --compatible-version=1.3 repository with property changes
-  # ended with mismatching db/current in source and destination:
-  # (source: "2 l 1", destination: "2 k 1").
+  # --compatible-version=1.3 repository with certain node-id and copy-id
+  # changes ended with mismatching db/current in source and destination:
+  #
+  #   source: "2 l 1"  destination: "2 k 1",
+  #           "3 l 2"               "3 4 2"
+  #           (and so on...)
+  #
+  # We test this case by creating a --compatible-version=1.3 repository
+  # and committing things that result in node-id and copy-id changes.
+  # After every commit, we hotcopy the repository to a new destination
+  # and check whether the source of the backup and the backup itself are
+  # identical.  We also maitain a separate --incremental backup, which
+  # is updated and checked after every commit.
+  sbox.build(create_wc=True, minor_version=3)
 
-  sbox.build(create_wc=True, minor_version=3)
+  inc_backup_dir, inc_backup_url = sbox.add_repo_path('incremental-backup')
+
+  # r1 = Initial greek tree sandbox.
+  backup_dir, backup_url = sbox.add_repo_path('backup-after-r1')
+  svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
+                                          sbox.repo_dir, backup_dir)
+  svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
+                                          "--incremental",
+                                          sbox.repo_dir, inc_backup_dir)
+  check_hotcopy_fsfs(sbox.repo_dir, backup_dir)
+  check_hotcopy_fsfs(sbox.repo_dir, inc_backup_dir)
+
+  # r2 = Add a new property.
   sbox.simple_propset('foo', 'bar', 'A/mu')
-  sbox.simple_commit()
+  sbox.simple_commit(message='r2')
 
-  backup_dir, backup_url = sbox.add_repo_path('backup')
+  backup_dir, backup_url = sbox.add_repo_path('backup-after-r2')
   svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
                                           sbox.repo_dir, backup_dir)
+  svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
+                                          "--incremental",
+                                          sbox.repo_dir, inc_backup_dir)
+  check_hotcopy_fsfs(sbox.repo_dir, backup_dir)
+  check_hotcopy_fsfs(sbox.repo_dir, inc_backup_dir)
 
+  # r3 = Copy a file.
+  sbox.simple_copy('A/B/E', 'A/B/E1')
+  sbox.simple_commit(message='r3')
+
+  backup_dir, backup_url = sbox.add_repo_path('backup-after-r3')
+  svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
+                                          sbox.repo_dir, backup_dir)
+  svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
+                                          "--incremental",
+                                          sbox.repo_dir, inc_backup_dir)
   check_hotcopy_fsfs(sbox.repo_dir, backup_dir)
+  check_hotcopy_fsfs(sbox.repo_dir, inc_backup_dir)
 
+  # r4 = Remove an existing file ...
+  sbox.simple_rm('A/D/gamma')
+  sbox.simple_commit(message='r4')
 
+  backup_dir, backup_url = sbox.add_repo_path('backup-after-r4')
+  svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
+                                          sbox.repo_dir, backup_dir)
+  svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
+                                          "--incremental",
+                                          sbox.repo_dir, inc_backup_dir)
+  check_hotcopy_fsfs(sbox.repo_dir, backup_dir)
+  check_hotcopy_fsfs(sbox.repo_dir, inc_backup_dir)
+
+  # r5 = ...and replace it with a new file here.
+  sbox.simple_add_text("This is the replaced file.\n", 'A/D/gamma')
+  sbox.simple_commit(message='r5')
+
+  backup_dir, backup_url = sbox.add_repo_path('backup-after-r5')
+  svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
+                                          sbox.repo_dir, backup_dir)
+  svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
+                                          "--incremental",
+                                          sbox.repo_dir, inc_backup_dir)
+  check_hotcopy_fsfs(sbox.repo_dir, backup_dir)
+  check_hotcopy_fsfs(sbox.repo_dir, inc_backup_dir)
+
+  # r6 = Add an entirely new file.
+  sbox.simple_add_text('This is an entirely new file.\n', 'A/C/mu1')
+  sbox.simple_commit(message='r6')
+
+  backup_dir, backup_url = sbox.add_repo_path('backup-after-r6')
+  svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
+                                          sbox.repo_dir, backup_dir)
+  svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
+                                          "--incremental",
+                                          sbox.repo_dir, inc_backup_dir)
+  check_hotcopy_fsfs(sbox.repo_dir, backup_dir)
+  check_hotcopy_fsfs(sbox.repo_dir, inc_backup_dir)
+
+  # r7 = Change the content of the existing file (this changeset does
+  #      not bump the next-id and copy-id counters in the repository).
+  sbox.simple_append('A/mu', 'This is change in the existing file.\n')
+  sbox.simple_commit(message='r7')
+
+  backup_dir, backup_url = sbox.add_repo_path('backup-after-r7')
+  svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
+                                          sbox.repo_dir, backup_dir)
+  svntest.actions.run_and_verify_svnadmin(None, None, [], "hotcopy",
+                                          "--incremental",
+                                          sbox.repo_dir, inc_backup_dir)
+  check_hotcopy_fsfs(sbox.repo_dir, backup_dir)
+  check_hotcopy_fsfs(sbox.repo_dir, inc_backup_dir)
+
+
 @SkipUnless(svntest.main.fs_has_pack)
 def verify_packed(sbox):
   "verify packed with small shards"
@@ -2554,7 +2646,7 @@ test_list = [ None,
               fsfs_recover_old_non_empty,
               fsfs_hotcopy_old_non_empty,
               load_ignore_dates,
-              fsfs_hotcopy_old_with_propchanges,
+              fsfs_hotcopy_old_with_id_changes,
               verify_packed,
               freeze_freeze,
               verify_metadata_only,
Index: subversion/libsvn_fs_fs/hotcopy.c
===================================================================
--- subversion/libsvn_fs_fs/hotcopy.c   (revision 1603111)
+++ subversion/libsvn_fs_fs/hotcopy.c   (working copy)
@@ -363,9 +363,10 @@ hotcopy_copy_packed_shard(svn_revnum_t *dst_min_un
   return SVN_NO_ERROR;
 }
 
-/* If NEW_YOUNGEST is younger than *DST_YOUNGEST, update the 'current'
- * file in DST_FS and set *DST_YOUNGEST to NEW_YOUNGEST.
- * Use SCRATCH_POOL for temporary allocations. */
+/* If NEW_YOUNGEST is younger than *DST_YOUNGEST, update the 'current' file
+ * in DST_FS and set *DST_YOUNGEST to NEW_YOUNGEST.  DST_FS is expected to
+ * be in a new format, i.e. storing only the youngest revision number in
+ * the 'current' file.  Use SCRATCH_POOL for temporary allocations. */
 static svn_error_t *
 hotcopy_update_current(svn_revnum_t *dst_youngest,
                        svn_fs_t *dst_fs,
@@ -372,31 +373,45 @@ hotcopy_update_current(svn_revnum_t *dst_youngest,
                        svn_revnum_t new_youngest,
                        apr_pool_t *scratch_pool)
 {
-  apr_uint64_t next_node_id = 0;
-  apr_uint64_t next_copy_id = 0;
   fs_fs_data_t *dst_ffd = dst_fs->fsap_data;
 
-  if (*dst_youngest >= new_youngest)
-    return SVN_NO_ERROR;
+  SVN_ERR_ASSERT(dst_ffd->format >= SVN_FS_FS__MIN_NO_GLOBAL_IDS_FORMAT);
 
-  /* If necessary, get new current next_node and next_copy IDs. */
-  if (dst_ffd->format < SVN_FS_FS__MIN_NO_GLOBAL_IDS_FORMAT)
+  if (*dst_youngest < new_youngest)
     {
-      SVN_ERR(svn_fs_fs__find_max_ids(dst_fs, new_youngest,
-                                      &next_node_id, &next_copy_id,
-                                      scratch_pool));
-
-      /* We store the _next_ ids. */
-      ++next_copy_id;
-      ++next_node_id;
+      SVN_ERR(svn_fs_fs__write_current(dst_fs, new_youngest,
+                                       0, 0, scratch_pool));
+      *dst_youngest = new_youngest;
     }
 
-  /* Update 'current'. */
-  SVN_ERR(svn_fs_fs__write_current(dst_fs, new_youngest, next_node_id,
-                                   next_copy_id, scratch_pool));
+  return SVN_NO_ERROR;
+}
 
-  *dst_youngest = new_youngest;
+/* If NEW_YOUNGEST is younger than *DST_YOUNGEST, update the 'current' file
+ * in DST_FS and set *DST_YOUNGEST to NEW_YOUNGEST.  DST_FS is expected to
+ * be in an old format, i.e. storing the youngest revision and the two next-ID
+ * parameters.  Those are updated with NEW_YOUNGEST, NEXT_NODE_ID and
+ * NEXT_COPY_ID respectively.  Use SCRATCH_POOL for temporary allocations. */
+static svn_error_t *
+hotcopy_update_current_old(svn_revnum_t *dst_youngest,
+                           svn_fs_t *dst_fs,
+                           svn_revnum_t new_youngest,
+                           apr_uint64_t next_node_id,
+                           apr_uint64_t next_copy_id,
+                           apr_pool_t *scratch_pool)
+{
+  fs_fs_data_t *dst_ffd = dst_fs->fsap_data;
 
+  SVN_ERR_ASSERT(dst_ffd->format < SVN_FS_FS__MIN_NO_GLOBAL_IDS_FORMAT);
+
+  if (*dst_youngest < new_youngest)
+    {
+      SVN_ERR(svn_fs_fs__write_current(dst_fs, new_youngest,
+                                       next_node_id, next_copy_id,
+                                       scratch_pool));
+      *dst_youngest = new_youngest;
+    }
+
   return SVN_NO_ERROR;
 }
 
@@ -573,140 +588,35 @@ remove_folder(const char *path,
   return svn_error_trace(err);
 }
 
-/* Baton for hotcopy_body(). */
-struct hotcopy_body_baton {
-  svn_fs_t *src_fs;
-  svn_fs_t *dst_fs;
-  svn_boolean_t incremental;
-  svn_cancel_func_t cancel_func;
-  void *cancel_baton;
-};
-
-/* Perform a hotcopy, either normal or incremental.
- *
- * Normal hotcopy assumes that the destination exists as an empty
- * directory. It behaves like an incremental hotcopy except that
- * none of the copied files already exist in the destination.
- *
- * An incremental hotcopy copies only changed or new files to the destination,
- * and removes files from the destination no longer present in the source.
- * While the incremental hotcopy is running, readers should still be able
- * to access the destintation repository without error and should not see
- * revisions currently in progress of being copied. Readers are able to see
- * new fully copied revisions even if the entire incremental hotcopy procedure
- * has not yet completed.
- *
- * Writers are blocked out completely during the entire incremental hotcopy
- * process to ensure consistency. This function assumes that the repository
- * write-lock is held.
+/* Copy the revision and revprop files (possibly sharded / packed) from
+ * SRC_FS to DST_FS.  Do not re-copy data which already exists in DST_FS.
+ * When copying packed or unpacked shards, checkpoint the result in DST_FS
+ * for every shard by updating the 'current' file if necessary.  Assume
+ * the >= SVN_FS_FS__MIN_NO_GLOBAL_IDS_FORMAT filesystem format without
+ * global next-ID counters.  Use POOL for temporary allocations.
  */
 static svn_error_t *
-hotcopy_body(void *baton, apr_pool_t *pool)
+hotcopy_revisions(svn_revnum_t *dst_youngest,
+                  svn_fs_t *src_fs,
+                  svn_fs_t *dst_fs,
+                  svn_revnum_t src_youngest,
+                  svn_boolean_t incremental,
+                  const char *src_revs_dir,
+                  const char *dst_revs_dir,
+                  const char *src_revprops_dir,
+                  const char *dst_revprops_dir,
+                  svn_cancel_func_t cancel_func,
+                  void* cancel_baton,
+                  apr_pool_t *pool)
 {
-  struct hotcopy_body_baton *hbb = baton;
-  svn_fs_t *src_fs = hbb->src_fs;
   fs_fs_data_t *src_ffd = src_fs->fsap_data;
-  svn_fs_t *dst_fs = hbb->dst_fs;
   fs_fs_data_t *dst_ffd = dst_fs->fsap_data;
   int max_files_per_dir = src_ffd->max_files_per_dir;
-  svn_boolean_t incremental = hbb->incremental;
-  svn_cancel_func_t cancel_func = hbb->cancel_func;
-  void* cancel_baton = hbb->cancel_baton;
-  svn_revnum_t src_youngest;
-  svn_revnum_t dst_youngest;
-  svn_revnum_t rev;
   svn_revnum_t src_min_unpacked_rev;
   svn_revnum_t dst_min_unpacked_rev;
-  const char *src_subdir;
-  const char *dst_subdir;
-  const char *revprop_src_subdir;
-  const char *revprop_dst_subdir;
+  svn_revnum_t rev;
   apr_pool_t *iterpool;
-  svn_node_kind_t kind;
 
-  /* Try to copy the config.
-   *
-   * ### We try copying the config file before doing anything else,
-   * ### because higher layers will abort the hotcopy if we throw
-   * ### an error from this function, and that renders the hotcopy
-   * ### unusable anyway. */
-  if (src_ffd->format >= SVN_FS_FS__MIN_CONFIG_FILE)
-    {
-      svn_error_t *err;
-
-      err = svn_io_dir_file_copy(src_fs->path, dst_fs->path, PATH_CONFIG,
-                                 pool);
-      if (err)
-        {
-          if (APR_STATUS_IS_ENOENT(err->apr_err))
-            {
-              /* 1.6.0 to 1.6.11 did not copy the configuration file during
-               * hotcopy. So if we're hotcopying a repository which has been
-               * created as a hotcopy itself, it's possible that fsfs.conf
-               * does not exist. Ask the user to re-create it.
-               *
-               * ### It would be nice to make this a non-fatal error,
-               * ### but this function does not get an svn_fs_t object
-               * ### so we have no way of just printing a warning via
-               * ### the fs->warning() callback. */
-
-              const char *msg;
-              const char *src_abspath;
-              const char *dst_abspath;
-              const char *config_relpath;
-              svn_error_t *err2;
-
-              config_relpath = svn_dirent_join(src_fs->path, PATH_CONFIG, 
pool);
-              err2 = svn_dirent_get_absolute(&src_abspath, src_fs->path, pool);
-              if (err2)
-                return svn_error_trace(svn_error_compose_create(err, err2));
-              err2 = svn_dirent_get_absolute(&dst_abspath, dst_fs->path, pool);
-              if (err2)
-                return svn_error_trace(svn_error_compose_create(err, err2));
-
-              /* ### hack: strip off the 'db/' directory from paths so
-               * ### they make sense to the user */
-              src_abspath = svn_dirent_dirname(src_abspath, pool);
-              dst_abspath = svn_dirent_dirname(dst_abspath, pool);
-
-              msg = apr_psprintf(pool,
-                                 _("Failed to create hotcopy at '%s'. "
-                                   "The file '%s' is missing from the source "
-                                   "repository. Please create this file, for "
-                                   "instance by running 'svnadmin upgrade 
%s'"),
-                                 dst_abspath, config_relpath, src_abspath);
-              return svn_error_quick_wrap(err, msg);
-            }
-          else
-            return svn_error_trace(err);
-        }
-    }
-
-  if (cancel_func)
-    SVN_ERR(cancel_func(cancel_baton));
-
-  /* Find the youngest revision in the source and destination.
-   * We only support hotcopies from sources with an equal or greater amount
-   * of revisions than the destination.
-   * This also catches the case where users accidentally swap the
-   * source and destination arguments. */
-  SVN_ERR(svn_fs_fs__youngest_rev(&src_youngest, src_fs, pool));
-  if (incremental)
-    {
-      SVN_ERR(svn_fs_fs__youngest_rev(&dst_youngest, dst_fs, pool));
-      if (src_youngest < dst_youngest)
-        return svn_error_createf(SVN_ERR_UNSUPPORTED_FEATURE, NULL,
-                 _("The hotcopy destination already contains more revisions "
-                   "(%lu) than the hotcopy source contains (%lu); are source "
-                   "and destination swapped?"),
-                  dst_youngest, src_youngest);
-    }
-  else
-    dst_youngest = 0;
-
-  if (cancel_func)
-    SVN_ERR(cancel_func(cancel_baton));
-
   /* Copy the min unpacked rev, and read its value. */
   if (src_ffd->format >= SVN_FS_FS__MIN_PACKED_FORMAT)
     {
@@ -743,10 +653,6 @@ static svn_error_t *
    * Copy the necessary rev files.
    */
 
-  src_subdir = svn_dirent_join(src_fs->path, PATH_REVS_DIR, pool);
-  dst_subdir = svn_dirent_join(dst_fs->path, PATH_REVS_DIR, pool);
-  SVN_ERR(svn_io_make_dir_recursively(dst_subdir, pool));
-
   iterpool = svn_pool_create(pool);
   /* First, copy packed shards. */
   for (rev = 0; rev < src_min_unpacked_rev; rev += max_files_per_dir)
@@ -764,7 +670,7 @@ static svn_error_t *
 
       /* If necessary, update 'current' to the most recent packed rev,
        * so readers can see new revisions which arrived in this pack. */
-      SVN_ERR(hotcopy_update_current(&dst_youngest, dst_fs,
+      SVN_ERR(hotcopy_update_current(dst_youngest, dst_fs,
                                      rev + max_files_per_dir - 1,
                                      iterpool));
 
@@ -794,13 +700,11 @@ static svn_error_t *
   if (cancel_func)
     SVN_ERR(cancel_func(cancel_baton));
 
+  SVN_ERR_ASSERT(rev == src_min_unpacked_rev);
+  SVN_ERR_ASSERT(src_min_unpacked_rev == dst_min_unpacked_rev);
+
   /* Now, copy pairs of non-packed revisions and revprop files.
    * If necessary, update 'current' after copying all files from a shard. */
-  SVN_ERR_ASSERT(rev == src_min_unpacked_rev);
-  SVN_ERR_ASSERT(src_min_unpacked_rev == dst_min_unpacked_rev);
-  revprop_src_subdir = svn_dirent_join(src_fs->path, PATH_REVPROPS_DIR, pool);
-  revprop_dst_subdir = svn_dirent_join(dst_fs->path, PATH_REVPROPS_DIR, pool);
-  SVN_ERR(svn_io_make_dir_recursively(revprop_dst_subdir, pool));
   for (; rev <= src_youngest; rev++)
     {
       svn_error_t *err;
@@ -811,7 +715,7 @@ static svn_error_t *
         SVN_ERR(cancel_func(cancel_baton));
 
       /* Copy the rev file. */
-      err = hotcopy_copy_shard_file(src_subdir, dst_subdir,
+      err = hotcopy_copy_shard_file(src_revs_dir, dst_revs_dir,
                                     rev, max_files_per_dir,
                                     svn_fs_fs__use_log_addressing(src_fs, rev),
                                     iterpool);
@@ -862,27 +766,233 @@ static svn_error_t *
         }
 
       /* Copy the revprop file. */
-      SVN_ERR(hotcopy_copy_shard_file(revprop_src_subdir,
-                                      revprop_dst_subdir,
+      SVN_ERR(hotcopy_copy_shard_file(src_revprops_dir,
+                                      dst_revprops_dir,
                                       rev, max_files_per_dir, FALSE,
                                       iterpool));
 
       /* After completing a full shard, update 'current'. */
       if (max_files_per_dir && rev % max_files_per_dir == 0)
-        SVN_ERR(hotcopy_update_current(&dst_youngest, dst_fs, rev, iterpool));
+        SVN_ERR(hotcopy_update_current(dst_youngest, dst_fs, rev, iterpool));
     }
   svn_pool_destroy(iterpool);
 
-  if (cancel_func)
-    SVN_ERR(cancel_func(cancel_baton));
-
   /* We assume that all revisions were copied now, i.e. we didn't exit the
    * above loop early. 'rev' was last incremented during exit of the loop. */
   SVN_ERR_ASSERT(rev == src_youngest + 1);
 
-  /* All revisions were copied. Update 'current'. */
-  SVN_ERR(hotcopy_update_current(&dst_youngest, dst_fs, src_youngest, pool));
+  return SVN_NO_ERROR;
+}
 
+/* Shortcut for the revision and revprop copying for old (1 or 2) format
+ * filesystems without sharding and packing.  Copy the non-sharded revision
+ * and revprop files from SRC_FS to DST_FS.  Do not re-copy data which
+ * already exists in DST_FS.  Do not somehow checkpoint the results in
+ * the 'current' file in DST_FS.  Use POOL for temporary allocations.
+ * Also see hotcopy_revisions().
+ */
+static svn_error_t *
+hotcopy_revisions_old(svn_fs_t *src_fs,
+                      svn_fs_t *dst_fs,
+                      svn_revnum_t src_youngest,
+                      const char *src_revs_dir,
+                      const char *dst_revs_dir,
+                      const char *src_revprops_dir,
+                      const char *dst_revprops_dir,
+                      svn_cancel_func_t cancel_func,
+                      void* cancel_baton,
+                      apr_pool_t *pool)
+{
+  apr_pool_t *iterpool = svn_pool_create(pool);
+  svn_revnum_t rev;
+
+  for (rev = 0; rev <= src_youngest; rev++)
+    {
+      svn_pool_clear(iterpool);
+
+      if (cancel_func)
+        SVN_ERR(cancel_func(cancel_baton));
+
+      SVN_ERR(hotcopy_io_dir_file_copy(src_revs_dir, dst_revs_dir,
+                                       apr_psprintf(iterpool, "%ld", rev),
+                                       iterpool));
+      SVN_ERR(hotcopy_io_dir_file_copy(src_revprops_dir, dst_revprops_dir,
+                                       apr_psprintf(iterpool, "%ld", rev),
+                                       iterpool));
+    }
+    svn_pool_destroy(iterpool);
+
+    return SVN_NO_ERROR;
+}
+
+/* Baton for hotcopy_body(). */
+struct hotcopy_body_baton {
+  svn_fs_t *src_fs;
+  svn_fs_t *dst_fs;
+  svn_boolean_t incremental;
+  svn_cancel_func_t cancel_func;
+  void *cancel_baton;
+};
+
+/* Perform a hotcopy, either normal or incremental.
+ *
+ * Normal hotcopy assumes that the destination exists as an empty
+ * directory. It behaves like an incremental hotcopy except that
+ * none of the copied files already exist in the destination.
+ *
+ * An incremental hotcopy copies only changed or new files to the destination,
+ * and removes files from the destination no longer present in the source.
+ * While the incremental hotcopy is running, readers should still be able
+ * to access the destintation repository without error and should not see
+ * revisions currently in progress of being copied. Readers are able to see
+ * new fully copied revisions even if the entire incremental hotcopy procedure
+ * has not yet completed.
+ *
+ * Writers are blocked out completely during the entire incremental hotcopy
+ * process to ensure consistency. This function assumes that the repository
+ * write-lock is held.
+ */
+static svn_error_t *
+hotcopy_body(void *baton, apr_pool_t *pool)
+{
+  struct hotcopy_body_baton *hbb = baton;
+  svn_fs_t *src_fs = hbb->src_fs;
+  fs_fs_data_t *src_ffd = src_fs->fsap_data;
+  svn_fs_t *dst_fs = hbb->dst_fs;
+  fs_fs_data_t *dst_ffd = dst_fs->fsap_data;
+  svn_boolean_t incremental = hbb->incremental;
+  svn_cancel_func_t cancel_func = hbb->cancel_func;
+  void* cancel_baton = hbb->cancel_baton;
+  svn_revnum_t src_youngest;
+  apr_uint64_t src_next_node_id;
+  apr_uint64_t src_next_copy_id;
+  svn_revnum_t dst_youngest;
+  const char *src_revprops_dir;
+  const char *dst_revprops_dir;
+  const char *src_revs_dir;
+  const char *dst_revs_dir;
+  const char *src_subdir;
+  const char *dst_subdir;
+  svn_node_kind_t kind;
+
+  /* Try to copy the config.
+   *
+   * ### We try copying the config file before doing anything else,
+   * ### because higher layers will abort the hotcopy if we throw
+   * ### an error from this function, and that renders the hotcopy
+   * ### unusable anyway. */
+  if (src_ffd->format >= SVN_FS_FS__MIN_CONFIG_FILE)
+    {
+      svn_error_t *err;
+
+      err = svn_io_dir_file_copy(src_fs->path, dst_fs->path, PATH_CONFIG,
+                                 pool);
+      if (err)
+        {
+          if (APR_STATUS_IS_ENOENT(err->apr_err))
+            {
+              /* 1.6.0 to 1.6.11 did not copy the configuration file during
+               * hotcopy. So if we're hotcopying a repository which has been
+               * created as a hotcopy itself, it's possible that fsfs.conf
+               * does not exist. Ask the user to re-create it.
+               *
+               * ### It would be nice to make this a non-fatal error,
+               * ### but this function does not get an svn_fs_t object
+               * ### so we have no way of just printing a warning via
+               * ### the fs->warning() callback. */
+
+              const char *msg;
+              const char *src_abspath;
+              const char *dst_abspath;
+              const char *config_relpath;
+              svn_error_t *err2;
+
+              config_relpath = svn_dirent_join(src_fs->path, PATH_CONFIG, 
pool);
+              err2 = svn_dirent_get_absolute(&src_abspath, src_fs->path, pool);
+              if (err2)
+                return svn_error_trace(svn_error_compose_create(err, err2));
+              err2 = svn_dirent_get_absolute(&dst_abspath, dst_fs->path, pool);
+              if (err2)
+                return svn_error_trace(svn_error_compose_create(err, err2));
+
+              /* ### hack: strip off the 'db/' directory from paths so
+               * ### they make sense to the user */
+              src_abspath = svn_dirent_dirname(src_abspath, pool);
+              dst_abspath = svn_dirent_dirname(dst_abspath, pool);
+
+              msg = apr_psprintf(pool,
+                                 _("Failed to create hotcopy at '%s'. "
+                                   "The file '%s' is missing from the source "
+                                   "repository. Please create this file, for "
+                                   "instance by running 'svnadmin upgrade 
%s'"),
+                                 dst_abspath, config_relpath, src_abspath);
+              return svn_error_quick_wrap(err, msg);
+            }
+          else
+            return svn_error_trace(err);
+        }
+    }
+
+  if (cancel_func)
+    SVN_ERR(cancel_func(cancel_baton));
+
+  /* Find the youngest revision in the source and destination.
+   * We only support hotcopies from sources with an equal or greater amount
+   * of revisions than the destination.
+   * This also catches the case where users accidentally swap the
+   * source and destination arguments. */
+  SVN_ERR(svn_fs_fs__read_current(&src_youngest, &src_next_node_id,
+                                  &src_next_copy_id, src_fs, pool));
+  if (incremental)
+    {
+      SVN_ERR(svn_fs_fs__youngest_rev(&dst_youngest, dst_fs, pool));
+      if (src_youngest < dst_youngest)
+        return svn_error_createf(SVN_ERR_UNSUPPORTED_FEATURE, NULL,
+                 _("The hotcopy destination already contains more revisions "
+                   "(%lu) than the hotcopy source contains (%lu); are source "
+                   "and destination swapped?"),
+                   dst_youngest, src_youngest);
+    }
+  else
+    dst_youngest = 0;
+
+  src_revs_dir = svn_dirent_join(src_fs->path, PATH_REVS_DIR, pool);
+  dst_revs_dir = svn_dirent_join(dst_fs->path, PATH_REVS_DIR, pool);
+  src_revprops_dir = svn_dirent_join(src_fs->path, PATH_REVPROPS_DIR, pool);
+  dst_revprops_dir = svn_dirent_join(dst_fs->path, PATH_REVPROPS_DIR, pool);
+
+  /* Ensure that the required folders exist in the destination
+   * before actually copying the revisions and revprops. */
+  SVN_ERR(svn_io_make_dir_recursively(dst_revs_dir, pool));
+  SVN_ERR(svn_io_make_dir_recursively(dst_revprops_dir, pool));
+
+  if (cancel_func)
+    SVN_ERR(cancel_func(cancel_baton));
+
+  /* Split the logic for new and old FS formats. The latter is much simpler
+   * due to the absense of sharding and packing. However, it requires special
+   * care when updating the 'current' file (which contains not just the
+   * revision number, but also the next-ID counters). */
+  if (src_ffd->format >= SVN_FS_FS__MIN_NO_GLOBAL_IDS_FORMAT)
+    {
+      SVN_ERR(hotcopy_revisions(&dst_youngest, src_fs, dst_fs, src_youngest,
+                                incremental, src_revs_dir, dst_revs_dir,
+                                src_revprops_dir, dst_revprops_dir,
+                                cancel_func, cancel_baton, pool));
+      SVN_ERR(hotcopy_update_current(&dst_youngest, dst_fs, src_youngest,
+                                     pool));
+    }
+  else
+    {
+      SVN_ERR(hotcopy_revisions_old(src_fs, dst_fs, src_youngest,
+                                    src_revs_dir, dst_revs_dir,
+                                    src_revprops_dir, dst_revprops_dir,
+                                    cancel_func, cancel_baton, pool));
+      SVN_ERR(hotcopy_update_current_old(&dst_youngest, dst_fs, src_youngest,
+                                         src_next_node_id, src_next_copy_id,
+                                         pool));
+    }
+
   /* Replace the locks tree.
    * This is racy in case readers are currently trying to list locks in
    * the destination. However, we need to get rid of stale locks.
Index: subversion/libsvn_fs_fs/util.c
===================================================================
--- subversion/libsvn_fs_fs/util.c      (revision 1603111)
+++ subversion/libsvn_fs_fs/util.c      (working copy)
@@ -441,6 +441,47 @@ svn_fs_fs__write_min_unpacked_rev(svn_fs_t *fs,
 }
 
 svn_error_t *
+svn_fs_fs__read_current(svn_revnum_t *rev,
+                        apr_uint64_t *next_node_id,
+                        apr_uint64_t *next_copy_id,
+                        svn_fs_t *fs,
+                        apr_pool_t *pool)
+{
+  fs_fs_data_t *ffd = fs->fsap_data;
+  svn_stringbuf_t *content;
+
+  SVN_ERR(svn_fs_fs__read_content(&content,
+                                  svn_fs_fs__path_current(fs, pool),
+                                  pool));
+
+  if (ffd->format >= SVN_FS_FS__MIN_NO_GLOBAL_IDS_FORMAT)
+    {
+      SVN_ERR(svn_revnum_parse(rev, content->data, NULL));
+
+      *next_node_id = 0;
+      *next_copy_id = 0;
+    }
+  else
+    {
+      const char *str;
+
+      SVN_ERR(svn_revnum_parse(rev, content->data, &str));
+      if (*str != ' ')
+        return svn_error_create(SVN_ERR_FS_CORRUPT, NULL,
+                                _("Corrupt 'current' file"));
+
+      *next_node_id = svn__base36toui64(&str, str + 1);
+      if (*str != ' ')
+        return svn_error_create(SVN_ERR_FS_CORRUPT, NULL,
+                                _("Corrupt 'current' file"));
+
+      *next_copy_id = svn__base36toui64(NULL, str + 1);
+    }
+
+  return SVN_NO_ERROR;
+}
+
+svn_error_t *
 svn_fs_fs__write_current(svn_fs_t *fs,
                          svn_revnum_t rev,
                          apr_uint64_t next_node_id,
Index: subversion/libsvn_fs_fs/util.h
===================================================================
--- subversion/libsvn_fs_fs/util.h      (revision 1603111)
+++ subversion/libsvn_fs_fs/util.h      (working copy)
@@ -322,6 +322,18 @@ svn_fs_fs__write_min_unpacked_rev(svn_fs_t *fs,
                                   svn_revnum_t revnum,
                                   apr_pool_t *scratch_pool);
 
+/* Set *REV, *NEXT_NODE_ID and *NEXT_COPY_ID to the values read from the
+ * 'current' file.  For new FS formats, which only store the youngest
+ * revision, set the *NEXT_NODE_ID and *NEXT_COPY_ID to 0.  Perform
+ * temporary allocations in POOL.
+ */
+svn_error_t *
+svn_fs_fs__read_current(svn_revnum_t *rev,
+                        apr_uint64_t *next_node_id,
+                        apr_uint64_t *next_copy_id,
+                        svn_fs_t *fs,
+                        apr_pool_t *pool);
+
 /* Atomically update the 'current' file to hold the specifed REV,
    NEXT_NODE_ID, and NEXT_COPY_ID.  (The two next-ID parameters are
    ignored and may be 0 if the FS format does not use them.)
Index: subversion/tests/cmdline/svnadmin_tests.py
===================================================================
--- subversion/tests/cmdline/svnadmin_tests.py  (revision 1603111)
+++ subversion/tests/cmdline/svnadmin_tests.py  (working copy)
@@ -2326,7 +2326,6 @@ def load_ignore_dates(sbox):
                             % (rev, str(rev_time), str(start_time)))
 
 
-@XFail()
 @SkipUnless(svntest.main.is_fs_type_fsfs)
 def fsfs_hotcopy_old_with_propchanges(sbox):
   "hotcopy --compatible-version=1.3 with propchanges"
Index: subversion/libsvn_fs_fs/recovery.c
===================================================================
--- subversion/libsvn_fs_fs/recovery.c  (revision 1603111)
+++ subversion/libsvn_fs_fs/recovery.c  (working copy)
@@ -321,29 +321,6 @@ recover_get_root_offset(apr_off_t *root_offset,
   return SVN_NO_ERROR;
 }
 
-svn_error_t *
-svn_fs_fs__find_max_ids(svn_fs_t *fs,
-                        svn_revnum_t youngest,
-                        apr_uint64_t *max_node_id,
-                        apr_uint64_t *max_copy_id,
-                        apr_pool_t *pool)
-{
-  fs_fs_data_t *ffd = fs->fsap_data;
-  apr_off_t root_offset;
-  svn_fs_fs__revision_file_t *rev_file;
-
-  /* call this function for old repo formats only */
-  SVN_ERR_ASSERT(ffd->format < SVN_FS_FS__MIN_NO_GLOBAL_IDS_FORMAT);
-
-  SVN_ERR(svn_fs_fs__open_pack_or_rev_file(&rev_file, fs, youngest, pool));
-  SVN_ERR(recover_get_root_offset(&root_offset, youngest, rev_file, pool));
-  SVN_ERR(recover_find_max_ids(fs, youngest, rev_file, root_offset,
-                               max_node_id, max_copy_id, pool));
-  SVN_ERR(svn_fs_fs__close_revision_file(rev_file));
-
-  return SVN_NO_ERROR;
-}
-
 /* Baton used for recover_body below. */
 struct recover_baton {
   svn_fs_t *fs;
@@ -425,13 +402,19 @@ recover_body(void *baton, apr_pool_t *pool)
 
       for (rev = 0; rev <= max_rev; rev++)
         {
+          svn_fs_fs__revision_file_t *rev_file;
+          apr_off_t root_offset;
+
           svn_pool_clear(iterpool);
 
           if (b->cancel_func)
             SVN_ERR(b->cancel_func(b->cancel_baton));
 
-          SVN_ERR(svn_fs_fs__find_max_ids(fs, rev, &next_node_id,
-                                          &next_copy_id, iterpool));
+          SVN_ERR(svn_fs_fs__open_pack_or_rev_file(&rev_file, fs, rev, pool));
+          SVN_ERR(recover_get_root_offset(&root_offset, rev, rev_file, pool));
+          SVN_ERR(recover_find_max_ids(fs, rev, rev_file, root_offset,
+                                       &next_node_id, &next_copy_id, pool));
+          SVN_ERR(svn_fs_fs__close_revision_file(rev_file));
         }
       svn_pool_destroy(iterpool);
 
Index: subversion/libsvn_fs_fs/recovery.h
===================================================================
--- subversion/libsvn_fs_fs/recovery.h  (revision 1603111)
+++ subversion/libsvn_fs_fs/recovery.h  (working copy)
@@ -25,16 +25,6 @@
 
 #include "fs.h"
 
-/* Find the "largest / max" node IDs in FS with the given YOUNGEST revision.
-   Return the result in the pre-allocated MAX_NODE_ID and MAX_COPY_ID data
-   buffer, respectively.   Use POOL for allocations.  */
-svn_error_t *
-svn_fs_fs__find_max_ids(svn_fs_t *fs,
-                        svn_revnum_t youngest,
-                        apr_uint64_t *max_node_id,
-                        apr_uint64_t *max_copy_id,
-                        apr_pool_t *pool);
-
 /* Recover the fsfs associated with filesystem FS.
    Use optional CANCEL_FUNC/CANCEL_BATON for cancellation support.
    Use POOL for temporary allocations. */

Reply via email to