On Tuesday 22 July 2003 12:49 am, cmpilato@collab.net wrote:
> John Szakmeister <john@szakmeister.net> writes:
> > Resolves Issue #1074 (Need a svnadmin command to verify that the
> > repository is not corrupted).
>
> [...]
>
> Nice log message. That's a good start! :-)
Thanks!
>
> > +/* Verify */
> > +
> > +/** Verify the filesystem data associated with @ fs. */
> > +svn_error_t *
> > +svn_fs_verify(svn_fs_t *fs,
> > + apr_pool_t *pool);
> > +
> > +
> > +
>
> Couple of comments. I notice you used a weird mix of coding styles
> through your patch (sometimes doing "fn(args)", sometimes "fn
> (args)"). There are lines that won't fit on an 80-column screen, and
> some minor alignment issues. Please stick with the coding style
> already used in a given module. Sounds picky, I know, but it really
> does make life easier for everyone.
Not picky at all... I certainly understand the need to stay within a set of
guidelines. I tried to be consistent within a given module, but it appears
something slipped through the cracks. :-) I'll go back and take a look at
the formatting again.
> Now, about that docstring for svn_fs_verify(). Seems a little
> lightweight for a module API docstring, but more than that -- it's
> kinda wrong. This function doesn't actually verify all the filesystem
> data. It only verifies the file contents, file and directory property
> lists, and directory entries lists.
Honestly, I felt the same way when naming it. At the time nothing better came
to mind, so I stuck with it.
> How about if we call the function svn_fs_verify_contents(), and
> compose a slightly more revealing docstring for it?
>
> > +svn_error_t *svn_fs__bdb_walk_reps (svn_fs_t *fs,
> > + svn_fs__bdb_rep_handler_t
> > rep_handler, + trail_t *trail)
> > +{
>
> [...]
>
> > + /* Turn the key into a null-terminated string to be used by the
> > handler */ + rep_key = svn_string_ncreate(key.data, key.size,
> > subpool); +
> > + /* Run the handler */
> > + error = (*rep_handler)(fs, rep_key->data, trail);
>
> We don't really need to build a full string structure here. Consider
> using apr_pstrmemdup() to make a regular 'const char *'.
I actually hunted through svn looking for something that would do exactly
that... didn't even think to look at APR. I'll change this too.
> > +/* Verifying data integrity */
> > +
> > +/* A simple callback function used when walking the reps */
> > +svn_error_t *
> > +verify_reps_handler (svn_fs_t *fs,
> > + const char *rep_key,
> > + trail_t *trail)
> > +{
> > + svn_string_t str;
> > +
> > + SVN_ERR (svn_fs__rep_contents (&str, fs, rep_key, trail));
> > +
> > + return SVN_NO_ERROR;
> > +}
>
> You know, I'm really thinking it's not such a good idea to hold the
> entire contents of a file in memory. I'm thinking about those
> gigabyte-sized files some folks are putting into their repositories...
>
> Perhaps we should reconsider this part of the verification plan, hm? :-)
Agreed. I remember looking at that function the first time and thinking the
same thing. Any ideas?
> > +
> > +/* A simple callback function used when initiating the
> > + walking of the repository */
> > +svn_error_t *
> > +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;
> > +}
>
> We have a longstanding convention of naming this svn_fs__retry_txn()
> callbacks 'txn_body_function_name' -- so, in this case,
> txn_body_verify_reps().
Will fix this as well.
Thanks for the helpful comments. I'll work the changes in (except the whole
reading the entire contents of a file in) and re-post the patch. Meanwhile,
if you've got an idea on how you would like to see this problem of storing
the entire contents of a file in memory, I'll get started on a fix.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jul 22 08:01:24 2003