John Szakmeister <john@szakmeister.net> writes:
> Here is take 2 on this patch. I re-worked a couple of things. I
> removed the typedef for the function prototype and changed it to
> match retry_txn's style of decleration, and fixed some of the
> formatting problems I had before (I hopefully got them all this time
> :-).
This patch doesn't apply cleanly for me. I get failed hunks in
svn_fs.h and fs.c.
> Index: subversion/include/svn_fs.h
> ===================================================================
> --- subversion/include/svn_fs.h (revision 6521)
> +++ subversion/include/svn_fs.h (working copy)
> @@ -1327,18 +1327,30 @@
>
> /** Populate @a *uuid with the UUID associated with @a fs. */
> svn_error_t *
> -svn_fs_get_uuid(svn_fs_t *fs,
> - const char **uuid,
> - apr_pool_t *pool);
> +svn_fs_get_uuid (svn_fs_t *fs,
> + const char **uuid,
> + apr_pool_t *pool);
>
>
> /** Associate @a *uuid with @a fs. */
> svn_error_t *
> -svn_fs_set_uuid(svn_fs_t *fs,
> - const char *uuid,
> - apr_pool_t *pool);
> +svn_fs_set_uuid (svn_fs_t *fs,
> + const char *uuid,
> + apr_pool_t *pool);
These should really be in a separate patch, but whatever.
> +/* Verify */
Really should toss a ^L above this line (and maybe "Verification" --
note that other headings are nouns).
> + /* Create a database cursor to list the transaction names. */
> + SVN_ERR (BDB_WRAP (fs, "reading transaction list (opening cursor)",
> + fs->representations->cursor (fs->representations,
> + trail->db_txn,
> + &cursor,
> + 0)));
> + /* Build a null-terminated array of keys in the transactions table. */
Hmm, I wonder if maybe you copied this code from the ^^^^^^^^^^^^^^^^^^
(I think you missed some s/transactions/representations/)
> Index: subversion/libsvn_fs/bdb/reps-table.h
> ===================================================================
> --- subversion/libsvn_fs/bdb/reps-table.h (revision 6521)
> +++ subversion/libsvn_fs/bdb/reps-table.h (working copy)
> @@ -74,6 +74,17 @@
> trail_t *trail);
>
>
> +
> +/*** Walking the representation table. ***/
No need for the section boundary -- this is still "storing and
retrieving reps".
> Index: subversion/libsvn_fs/fs.c
> ===================================================================
> --- subversion/libsvn_fs/fs.c (revision 6521)
> +++ subversion/libsvn_fs/fs.c (working copy)
> @@ -33,6 +33,8 @@
> #include "err.h"
> #include "dag.h"
> #include "svn_private_config.h"
> +#include "reps-strings.h"
> +#include "svn_string.h"
Oops. Leftover #include from the previous attempt at this patch. You
don't actually use svn_string_t's anymore.
> +
> +/* A simple callback function used when initiating the
> + walking of the repository */
> +static svn_error_t *
> +txn_body_verify_reps (void *baton,
> + trail_t *trail)
> +{
> + svn_fs_t *fs = baton;
> +
> + SVN_ERR (svn_fs__bdb_walk_reps (fs, verify_reps_handler, trail));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* The exposed function used to kick off the verification process */
> +svn_error_t *
> +svn_fs_verify_contents (svn_fs_t *fs,
> + apr_pool_t *pool)
> +{
> + SVN_ERR (svn_fs__retry_txn (fs, txn_body_verify_reps, fs, pool));
> +
> + return SVN_NO_ERROR;
> +}
We don't typically use docstrings on the implementation of public
functions (that's just asking for them to get out of sync with the
public header file's docstring of the same function. And we don't put
docstrings with the txn_body_ functions either, since their names and
types are dead giveaways as to what they exist to do.
> Index: subversion/libsvn_fs/reps-strings.h
> ===================================================================
> --- subversion/libsvn_fs/reps-strings.h (revision 6521)
> +++ subversion/libsvn_fs/reps-strings.h (working copy)
> @@ -103,7 +103,9 @@
>
> If USE_TRAIL_FOR_READS is TRUE, the stream's reads are part
> of TRAIL; otherwise, each read happens in an internal, one-off
> - trail (though TRAIL is still required). POOL may be TRAIL->pool. */
> + trail (though TRAIL is still required). POOL may be TRAIL->pool.
> +
> + Note: this function will verify the checksum as chunks are being read. */
"... and return the error SVN_ERR_FS_CORRUPT if the stored checksum
doesn't match the one calculated from the data."
> +/* This implements `svn_opt_subcommand_t'. */
> +static svn_error_t *
> +subcommand_verify (apr_getopt_t *os, void *baton, apr_pool_t *pool)
> +{
> +
> + struct svnadmin_opt_state *opt_state = baton;
> + svn_repos_t *repos;
> + svn_fs_t *fs;
> +
> + SVN_ERR (svn_repos_open (&repos, opt_state->repository_path, pool));
> + fs = svn_repos_fs (repos);
> +
> + SVN_ERR (svn_fs_verify_contents (fs, pool));
> +
> + return SVN_NO_ERROR;
> +}
Nits: your indentation is off here (function bodies start at column
2). Also, you could avoid the 'fs' convenience variable and do:
subcommand_verify (apr_getopt_t *os, void *baton, apr_pool_t *pool)
{
struct svnadmin_opt_state *opt_state = baton;
svn_repos_t *repos;
SVN_ERR (svn_repos_open (&repos, opt_state->repository_path, pool));
SVN_ERR (svn_fs_verify_contents (svn_repos_fs (repos), pool));
return SVN_NO_ERROR;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jul 31 16:34:30 2003