On Thursday 31 July 2003 10:14, cmpilato@collab.net wrote:
> 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.
Weird my copy of the patch seems to be fine... maybe I did something wrong...
for some reason the one that I posted is slightly different that the one
stored on my drive. I'll have to keep an eye on this... perhaps my e-mail
client is messing this up. Hopefully this one applies cleanly!
>
> > 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.
Removed. I'll send in another one to fix that.
>
> > +/* Verify */
>
> Really should toss a ^L above this line (and maybe "Verification" --
> note that other headings are nouns).
The diff actually borrowed the one that used be there for "Non-historical
Properties" and added another at the end of the block. I only mention it
because this patch looks the same way. BTW, changed "Verify" to "Content
Verification."
>
> > + /* 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 ^^^^^^^^^^^^^^^^^^
Why would you ever think that? :-)
>
> (I think you missed some s/transactions/representations/)
I fixed up the comments better. Hopefully they tell the truth now. :-)
>
> > 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".
Removed.
>
> > 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.
Heh, definitely missed that. Sometimes no matter how hard you look they slip
right on by.
>
> > +
> > +/* 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.
Understood. Removed the docstrings.
>
> > 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."
Hope you don't mind me copying that. :-)
>
> > +/* 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:
Grrr! SlickEdit is driving me crazy with this. Must learn to use emacs,
again!
>
> 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;
> }
Try this patch and see if things look any better.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Aug 1 02:36:26 2003