[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] svnadmin build-repcache command

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Wed, 18 Mar 2020 16:22:36 +0000

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
Received on 2020-03-18 17:23:00 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.