Denis Kovalchuk wrote on Wed, 18 Mar 2020 17:32 +0300:
> +++ subversion/include/private/svn_fs_fs_private.h (working copy)
> @@ -353,6 +353,16 @@
Please use the -p option to diff.
> +typedef struct svn_fs_fs__ioctl_build_rep_cache_input_t
> +{
> + svn_revnum_t start_rev;
> + svn_revnum_t end_rev;
> + svn_fs_progress_notify_func_t progress_func;
> + void *progress_baton;
> +} svn_fs_fs__ioctl_build_rep_cache_input_t;
> +
Add:
/* See svn_fs_fs__build_rep_cache(). */
> +SVN_FS_DECLARE_IOCTL_CODE(SVN_FS_FS__IOCTL_BUILD_REP_CACHE,
> SVN_FS_TYPE_FSFS, 1004);
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> +static svn_error_t *
> +reindex_node(svn_fs_t *fs,
⋮
> +{
⋮
> + if (noderev->data_rep && noderev->data_rep->revision == rev &&
> + noderev->kind == svn_node_file)
> + {
> + SVN_ERR(ensure_representation_sha1(fs, noderev->data_rep, pool));
> + SVN_ERR(svn_fs_fs__set_rep_reference(fs, noderev->data_rep, pool));
STMT_SET_REP uses «INSERT OR FAIL». Therefore, this call will fail if
any of the reps in REV are in rep-cache.db. In particular, I expect it
will fail in the common case that _all_ of the reps in REV are already
in the database (which can happen if enable-rep-sharing was toggled
during the filesystem's lifetime). Shouldn't it silently succeed in
that case? I.e., shouldn't the API be idempotent?
> + }
> +
> + if (noderev->prop_rep && noderev->prop_rep->revision == rev)
> + {
> + SVN_ERR(ensure_representation_sha1(fs, noderev->prop_rep, pool));
> + SVN_ERR(svn_fs_fs__set_rep_reference(fs, noderev->prop_rep, pool));
Ditto.
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +svn_fs_fs__build_rep_cache(svn_fs_t *fs,
⋮
> +{
> + if (!ffd->rep_sharing_allowed)
> + {
> + return SVN_NO_ERROR;
Shouldn't this case be an error? This isn't a case of "Nothing done,
nothing said" as above, but a case of "the precondition is unmet".
If need be, rep_sharing_allowed can be exposed via `svnadmin info`.
> + }
> + /* Do not build rep-cache for revision zero to match
> + * svn_fs_fs__create() behavior. */
> + for (rev = start_rev; rev <= end_rev; rev++)
This comment should be placed near the «start_rev = 1;» line (snipped),
rather than here.
> + {
⋮
> + SVN_ERR(svn_sqlite__begin_transaction(ffd->rep_cache_db));
> + err = reindex_node(fs, root_id, rev, file, cancel_func, cancel_baton,
> iterpool);
> + SVN_ERR(svn_sqlite__finish_transaction(ffd->rep_cache_db, err));
Shouldn't this function take a write lock — at least, to prevent
«svnadmin upgrade» from running concurrently?
> static const svn_opt_subcommand_desc3_t cmd_table[] =
> {
> + {"build-repcache", subcommand_build_repcache, {0}, {N_(
> + "usage: svnadmin build-repcache REPOS_PATH [-r LOWER[:UPPER]]\n"
> + "\n") N_(
> + "Add missing entries to the representation cache for the repository\n"
> + "at REPOS_PATH. Process data in revisions LOWER through UPPER.\n"
> + "If no revision arguments are given, process all revisions. If only\n"
> + "LOWER revision argument is given, process only that single revision.\n"
> + )},
Why isn't there a comma between the two string literals, as in the other
array entries?
> + {'r', 'q', 'M'} },
> +++ subversion/tests/cmdline/svnadmin_tests.py (working copy)
> @@ -4036,7 +4036,28 @@
> +@SkipUnless(svntest.main.is_fs_type_fsfs)
> +@SkipUnless(svntest.main.fs_has_rep_sharing)
Sounds like we should have fs_has_rep_sharing() call is_fs_type_fsfs().
Cheers,
Daniel
