On Mon, Oct 29, 2012 at 03:26:36PM +0530, Prabhu Gnana Sundar wrote:
> Thanks much Stefan,
>
> Now, I have changed the code in libsvn_fs/fs-loader.c to handle the
> error. Passed the keep_going to the verify_fs so that it can be
> handled in both libsvn_fs_fs and libsvn_fs_base. I have attached the
> updated patch with this mail. Please share your reviews.
More review below (mostly stylistic nits).
> Index: subversion/libsvn_repos/dump.c
> ===================================================================
> --- subversion/libsvn_repos/dump.c (revision 1402414)
> +++ subversion/libsvn_repos/dump.c (working copy)
> @@ -1360,9 +1360,10 @@
> }
>
> svn_error_t *
> -svn_repos_verify_fs2(svn_repos_t *repos,
> +svn_repos_verify_fs3(svn_repos_t *repos,
> svn_revnum_t start_rev,
> svn_revnum_t end_rev,
> + svn_boolean_t keep_going,
> svn_repos_notify_func_t notify_func,
> void *notify_baton,
> svn_cancel_func_t cancel_func,
> @@ -1398,7 +1399,7 @@
>
> /* Verify global/auxiliary data and backend-specific data first. */
> SVN_ERR(svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton,
> - start_rev, end_rev, pool));
> + start_rev, end_rev, keep_going, pool));
>
> /* Create a notify object that we can reuse within the loop. */
> if (notify_func)
> @@ -1413,11 +1414,12 @@
> void *cancel_edit_baton;
> svn_fs_root_t *to_root;
> apr_hash_t *props;
> + svn_error_t *err;
>
> svn_pool_clear(iterpool);
>
> /* Get cancellable dump editor, but with our close_directory handler.
> */
> - SVN_ERR(get_dump_editor(&dump_editor, &dump_edit_baton,
> + err = get_dump_editor(&dump_editor, &dump_edit_baton,
> fs, rev, "",
> svn_stream_empty(iterpool),
> NULL, NULL,
Please also change indentation of function parameters on additional
lines to align at the opening bracket like this:
err = get_dump_editor(&dump_editor, &dump_edit_baton,
fs, rev, "",
svn_stream_empty(iterpool),
NULL, NULL,
With your patch it would look like this, which is wrong indentation
for our code base:
err = get_dump_editor(&dump_editor, &dump_edit_baton,
fs, rev, "",
svn_stream_empty(iterpool),
NULL, NULL,
> @@ -1425,7 +1427,19 @@
> notify_func, notify_baton,
> start_rev,
> FALSE, TRUE, /* use_deltas, verify */
> - iterpool));
> + iterpool);
> +
> + if (err && keep_going)
> + {
> + svn_repos_notify_t *notify2 =
> svn_repos_notify_create(svn_repos_notify_failure, iterpool);
Why call the variable 'notify2', not just 'notify'?
Also, the above line doesn't wrap at column 78.
I'd suggest using 'notify' as variable name and indenting like this:
svn_repos_notify_t *notify;
notify = svn_repos_notify_create(svn_repos_notify_failure,
iterpool);
> + notify2->err = err;
> + notify_func(notify_baton, notify2, iterpool);
> + svn_error_clear(err);
> + continue;
> + }
> + else
> + SVN_ERR(err);
> +
> SVN_ERR(svn_delta_get_cancellation_editor(cancel_func, cancel_baton,
> dump_editor, dump_edit_baton,
> &cancel_editor,
> @@ -1433,9 +1447,21 @@
> iterpool));
>
> SVN_ERR(svn_fs_revision_root(&to_root, fs, rev, iterpool));
> - SVN_ERR(svn_repos_replay2(to_root, "", SVN_INVALID_REVNUM, FALSE,
> + err = svn_repos_replay2(to_root, "", SVN_INVALID_REVNUM, FALSE,
> cancel_editor, cancel_edit_baton,
Again, please adjust indentation of function parameters.
> - NULL, NULL, iterpool));
> + NULL, NULL, iterpool);
> +
> + if (err && keep_going)
> + {
> + svn_repos_notify_t *notify2 =
> svn_repos_notify_create(svn_repos_notify_failure, iterpool);
Same as above suggestion for 'notify' + indentation.
> + notify2->err = err;
> + notify_func(notify_baton, notify2, iterpool);
> + svn_error_clear(err);
> + continue;
> + }
> + else
> + SVN_ERR(err);
> +
> /* While our editor close_edit implementation is a no-op, we still
> do this for completeness. */
> SVN_ERR(cancel_editor->close_edit(cancel_edit_baton, iterpool));
> Index: subversion/libsvn_repos/deprecated.c
> ===================================================================
> --- subversion/libsvn_repos/deprecated.c (revision 1402414)
> +++ subversion/libsvn_repos/deprecated.c (working copy)
> @@ -728,6 +728,27 @@
> }
>
> svn_error_t *
> +svn_repos_verify_fs2(svn_repos_t *repos,
> + svn_revnum_t start_rev,
> + svn_revnum_t end_rev,
> + svn_repos_notify_func_t notify_func,
> + void *notify_baton,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + apr_pool_t *pool)
> +{
> + return svn_error_trace(svn_repos_verify_fs3(repos,
> + start_rev,
> + end_rev,
> + FALSE,
> + notify_func,
> + notify_baton,
> + cancel_func,
> + cancel_baton,
> + pool));
> +}
> +
> +svn_error_t *
> svn_repos_verify_fs(svn_repos_t *repos,
> svn_stream_t *feedback_stream,
> svn_revnum_t start_rev,
> Index: subversion/libsvn_fs_fs/fs_fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 1402414)
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -10117,6 +10117,7 @@
> void *cancel_baton,
> svn_revnum_t start,
> svn_revnum_t end,
> + svn_boolean_t keep_going,
> apr_pool_t *pool)
> {
> fs_fs_data_t *ffd = fs->fsap_data;
> @@ -10151,6 +10152,7 @@
> commit-time checks in validate_root_noderev(). */
> {
> svn_revnum_t i;
> + svn_error_t *err;
We usually add a blank line between variable declarations and code.
So you should add a blank here.
> for (i = start; i <= end; i++)
> {
> svn_fs_root_t *root;
> @@ -10162,7 +10164,15 @@
> When this code is called in the library, we want to ensure we
> use the on-disk data --- rather than some data that was read
> in the possibly-distance past and cached since. */
> - SVN_ERR(svn_fs_fs__revision_root(&root, fs, i, iterpool));
> + err = svn_fs_fs__revision_root(&root, fs, i, iterpool);
> + if (err && keep_going)
> + {
> + svn_error_clear(err);
No notification about this error?
Instead of pushing the repos-layer notifier down into the FS-layer,
I'd suggest catching this error thrown from svn_fs_fs__verify() in the
repos-layer caller and notifying there if --keep-going was used.
And then this function won't even need a keep_going parameter and all
the FS-layer bits can be left unchanged, making your patch a bit smaller.
> + return SVN_NO_ERROR;
> + }
> + else
> + SVN_ERR(err);
> +
> SVN_ERR(svn_fs_fs__verify_root(root, iterpool));
> }
> }
> Index: subversion/libsvn_fs_fs/fs_fs.h
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.h (revision 1402414)
> +++ subversion/libsvn_fs_fs/fs_fs.h (working copy)
> @@ -38,12 +38,14 @@
> svn_error_t *svn_fs_fs__upgrade(svn_fs_t *fs,
> apr_pool_t *pool);
>
> -/* Verify the fsfs filesystem FS. Use POOL for temporary allocations. */
> +/* Verify the fsfs filesystem FS. Use POOL for temporary allocations.
> + * If KEEP_GOING is TRUE, do not stop upon failure. */
> svn_error_t *svn_fs_fs__verify(svn_fs_t *fs,
> svn_cancel_func_t cancel_func,
> void *cancel_baton,
> svn_revnum_t start,
> svn_revnum_t end,
> + svn_boolean_t keep_going,
> apr_pool_t *pool);
>
> /* Copy the fsfs filesystem SRC_FS at SRC_PATH into a new copy DST_FS at
> Index: subversion/libsvn_fs_fs/fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs.c (revision 1402414)
> +++ subversion/libsvn_fs_fs/fs.c (working copy)
> @@ -287,6 +287,7 @@
> void *cancel_baton,
> svn_revnum_t start,
> svn_revnum_t end,
> + svn_boolean_t keep_going,
> apr_pool_t *pool,
> apr_pool_t *common_pool)
> {
> @@ -295,7 +296,8 @@
> SVN_ERR(svn_fs_fs__open(fs, path, pool));
> SVN_ERR(svn_fs_fs__initialize_caches(fs, pool));
> SVN_ERR(fs_serialized_init(fs, common_pool, pool));
> - return svn_fs_fs__verify(fs, cancel_func, cancel_baton, start, end, pool);
> + return svn_fs_fs__verify(fs, cancel_func, cancel_baton, start, end,
> + keep_going, pool);
> }
>
> static svn_error_t *
> Index: subversion/libsvn_fs/fs-loader.c
> ===================================================================
> --- subversion/libsvn_fs/fs-loader.c (revision 1402414)
> +++ subversion/libsvn_fs/fs-loader.c (working copy)
> @@ -487,6 +487,7 @@
> void *cancel_baton,
> svn_revnum_t start,
> svn_revnum_t end,
> + svn_boolean_t keep_going,
> apr_pool_t *pool)
> {
> fs_library_vtable_t *vtable;
> @@ -497,7 +498,7 @@
>
> SVN_MUTEX__WITH_LOCK(common_pool_lock,
> vtable->verify_fs(fs, path, cancel_func, cancel_baton,
> - start, end, pool, common_pool));
> + start, end, keep_going, pool,
> common_pool));
> return SVN_NO_ERROR;
> }
>
> Index: subversion/libsvn_fs/fs-loader.h
> ===================================================================
> --- subversion/libsvn_fs/fs-loader.h (revision 1402414)
> +++ subversion/libsvn_fs/fs-loader.h (working copy)
> @@ -92,6 +92,7 @@
> svn_cancel_func_t cancel_func, void
> *cancel_baton,
> svn_revnum_t start,
> svn_revnum_t end,
> + svn_boolean_t keep_going,
> apr_pool_t *pool,
> apr_pool_t *common_pool);
> svn_error_t *(*delete_fs)(const char *path, apr_pool_t *pool);
> Index: subversion/svnadmin/main.c
> ===================================================================
> --- subversion/svnadmin/main.c (revision 1402414)
> +++ subversion/svnadmin/main.c (working copy)
> @@ -176,6 +176,7 @@
> {
> svnadmin__version = SVN_OPT_FIRST_LONGOPT_ID,
> svnadmin__incremental,
> + svnadmin__keep_going,
> svnadmin__deltas,
> svnadmin__ignore_uuid,
> svnadmin__force_uuid,
> @@ -288,6 +289,9 @@
> N_("use format compatible with Subversion versions\n"
> " earlier than 1.8")},
>
> + {"keep-going", svnadmin__keep_going, 0,
> + N_("continue verifying even if there is a corruption")},
> +
> {"memory-cache-size", 'M', 1,
> N_("size of the extra in-memory cache in MB used to\n"
> " minimize redundant operations.
> Default: 16.\n"
> @@ -475,7 +479,7 @@
> {"verify", subcommand_verify, {0}, N_
> ("usage: svnadmin verify REPOS_PATH\n\n"
> "Verifies the data stored in the repository.\n"),
> - {'r', 'q', 'M'} },
> + {'r', 'q', svnadmin__keep_going, 'M'} },
>
> { NULL, NULL, {0}, NULL, {0} }
> };
> @@ -505,6 +509,7 @@
> svn_boolean_t clean_logs; /* --clean-logs */
> svn_boolean_t bypass_hooks; /* --bypass-hooks */
> svn_boolean_t wait; /* --wait */
> + svn_boolean_t keep_going; /* --keep-going */
> svn_boolean_t bypass_prop_validation; /*
> --bypass-prop-validation */
> enum svn_repos_load_uuid uuid_action; /* --ignore-uuid,
> --force-uuid */
> @@ -738,6 +743,10 @@
> notify->warning_str));
> return;
>
> + case svn_repos_notify_failure:
> + svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
> "svnadmin: ");
Another overlong line. I'd suggest:
svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
"svnadmin: ");
> + return;
> +
> case svn_repos_notify_dump_rev_end:
> svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool,
> _("* Dumped revision %ld.\n"),
> @@ -1527,7 +1536,8 @@
> if (! opt_state->quiet)
> progress_stream = recode_stream_create(stderr, pool);
>
> - return svn_repos_verify_fs2(repos, lower, upper,
> + return svn_repos_verify_fs3(repos, lower, upper,
> + opt_state->keep_going,
> !opt_state->quiet
> ? repos_notify_handler : NULL,
> progress_stream, check_cancel, NULL, pool);
> @@ -1992,6 +2002,9 @@
> case svnadmin__pre_1_8_compatible:
> opt_state.pre_1_8_compatible = TRUE;
> break;
> + case svnadmin__keep_going:
> + opt_state.keep_going = TRUE;
> + break;
> case svnadmin__fs_type:
> SVN_INT_ERR(svn_utf_cstring_to_utf8(&opt_state.fs_type, opt_arg,
> pool));
> break;
> Index: subversion/libsvn_fs_base/fs.c
> ===================================================================
> --- subversion/libsvn_fs_base/fs.c (revision 1402414)
> +++ subversion/libsvn_fs_base/fs.c (working copy)
> @@ -896,6 +896,7 @@
> void *cancel_baton,
> svn_revnum_t start,
> svn_revnum_t end,
> + svn_boolean_t keep_going,
> apr_pool_t *pool,
> apr_pool_t *common_pool)
> {
> Index: subversion/include/svn_fs.h
> ===================================================================
> --- subversion/include/svn_fs.h (revision 1402414)
> +++ subversion/include/svn_fs.h (working copy)
> @@ -280,6 +280,7 @@
> void *cancel_baton,
> svn_revnum_t start,
> svn_revnum_t end,
> + svn_boolean_t keep_going,
> apr_pool_t *scratch_pool);
>
> /**
> Index: subversion/include/svn_repos.h
> ===================================================================
> --- subversion/include/svn_repos.h (revision 1402414)
> +++ subversion/include/svn_repos.h (working copy)
> @@ -193,6 +193,9 @@
> /** A warning message is waiting. */
> svn_repos_notify_warning = 0,
>
> + /** A revision is found with corruption/errors. */
> + svn_repos_notify_failure,
> +
This enum has already been released in 1.7!
We must add new values at the end to preserve binary compatibility.
So please put svn_repos_notify_failure after svn_repos_notify_load_skipped_rev.
And please add "@since New in 1.8" to the docstring, like this:
/** A revision is found with corruption/errors. @since New in 1.8. */
> /** A revision has finished being dumped. */
> svn_repos_notify_dump_rev_end,
>
> @@ -318,9 +321,13 @@
> /** For #svn_repos_notify_load_node_start, the path of the node. */
> const char *path;
>
> + /** The error chain. */
Please expand this comment to explain the intended use of this field.
I'd suggest:
/** For #svn_repos_notify_failure, this error chain indicates what
* went wrong during verification. */
> + svn_error_t *err;
> +
> /* NOTE: Add new fields at the end to preserve binary compatibility.
> Also, if you add fields here, you have to update
> svn_repos_notify_create(). */
> +
Why add a blank line here?
> } svn_repos_notify_t;
>
> /** Callback for providing notification from the repository.
> @@ -2517,8 +2524,29 @@
> * cancel_baton as argument to see if the caller wishes to cancel the
> * verification.
> *
> + * If @a keep_going is @c TRUE, the verify process prints the error message
> + * to the stderr and continues.
> + *
> + * @since New in 1.8.
> + */
> +svn_error_t *
> +svn_repos_verify_fs3(svn_repos_t *repos,
> + svn_revnum_t start_rev,
> + svn_revnum_t end_rev,
> + svn_boolean_t keep_going,
> + svn_repos_notify_func_t notify_func,
> + void *notify_baton,
> + svn_cancel_func_t cancel,
> + void *cancel_baton,
> + apr_pool_t *scratch_pool);
> +
> +/**
> + * Similar to svn_repos_verify_fs3(), but without force.
It's now called 'keep_going', not 'force'.
I'd suggest to say:
* Like svn_repos_verify_fs3(), but with @a keep_going set to @c FALSE.
> + *
> * @since New in 1.7.
> + * @deprecated Provided for backward compatibility with the 1.7 API.
> */
> +SVN_DEPRECATED
> svn_error_t *
> svn_repos_verify_fs2(svn_repos_t *repos,
> svn_revnum_t start_rev,
> Implement svnadmin verify --keep-going, which would continue the verification
> even if there is some corruption, after printing the errors to stderr.
>
> * subversion/svnadmin/main.c
> (svnadmin__cmdline_options_t):
Missing change description? Or has svnadmin__cmdline_options_t not changed?
> (svnadmin_opt_state) : add keep-going option.
The indentation style of the log message differs from the style
we usually use. I'd suggest formatting log entries like this:
* subversion/svnadmin/main.c
(svnadmin_opt_state): add keep-going option.
(subcommand_verify): uses the new svn_repos_verify_fs3 function.
(repos_notify_handler): svn_repos_notify_failure handles failure and
prints warning to stderr.
I'd also suggest using proper capitalisation.
* subversion/svnadmin/main.c
(svnadmin_opt_state): Add keep-going option.
What follows are stylistic suggestions. Log messages all differ based on
personal style and preferences. There are no strict guidelines and what
follows is just my personal opinion.
> (subcommand_verify) : uses the new svn_repos_verify_fs3 function.
The above doesn't describe the change itself but the result of the change.
Also, third-person simple present verbs like "X uses Y" makes it sounds as
if you're stating a fact, something that is always true and independent of
your change, rather than describing a change you are making.
Compare to this:
(subcommand_verify): Switch to the new svn_repos_verify_fs3 function
instead of svn_repos_verify_fs2, and pass the keep-going option.
I've added a minor detail by mentioning the keep-going option.
When describing changes I often try to describe each chunk of the diff
in a way that allows the reader of the log message to imagine what the
chunk might look like. This might seem overly detailed and it's not a
strict requirement of the project. But I found that this helps me with
writing log messages that I can make sense of myself when I read them
again half a year later :)
> (repos_notify_handler) : svn_repos_notify_failure handles failure and
> prints warning to stderr.
When expressing code changes in English, I prefer to phrase data structures
(in this case, svn_repos_notify_failure) as grammatical objects and executing
code acting on the data (in this case, repos_notify_handler) as the
grammatical subject acting on the object.
Your wording suggests that svn_repos_notify_failure (the data), and not
repos_notify_handler (the executing code), prints the warning to stderr.
Compare to phrasing it like this:
(repos_notify_handler): Handle svn_repos_notify_failure notification
by printing warnings to stderr.
That's all for now. I haven't tried to compile and run the patch yet.
You might also want to add a regression test for your change, by the way,
to make sure the --keep-going option doesn't break when we make further
changes in the future. But that can be done as a separate patch.
Thanks!
>
> * subversion/include/svn_repos.h
> (svn_repos_notify_action_t): add svn_repos_notify_failure to notify failure.
> (svn_repos_verify_fs3) : newly added to handle "keep-going" option.
>
> * subversion/include/svn_fs.h
> (svn_fs_verify) : handles keep-going. If keep-going is TRUE, svn_fs_verify
> would return no error.
>
> * subversion/libsvn_fs_fs/fs_fs.h
> (svn_fs_fs__verify) : added the keep_going parameter.
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (svn_fs_fs__verify) : handles keep_going. If keep_going is TRUE,
> SVN_NO_ERROR
> is returned upon failure too.
>
> * subversion/libsvn_fs/fs-loader.h
> (fs_library_vtable_t) : added keep_going parameter.
>
> * subversion/libsvn_fs/fs-loader.c
> (svn_fs_verify) : handles keep_going, which is also passed on to
> vtable->verify_fs.
>
> * subversion/libsvn_fs_base/fs.c
> (base_verify) : handles keep_going parameter.
>
> * subversion/libsvn_fs_fs/fs.c
> (fs_verify) : send keep_going to svn_fs_fs__verify function.
>
> * subversion/libsvn_repos/dump.c
> (svn_repos_verify_fs3): now handles "keep-going".
> If "keep-going" is TRUE, then the error message is
> written to the stderr and verify process continues.
>
> * subversion/libsvn_repos/deprecated.c
> (svn_repos_verify_fs2) : deprecated.
> Now calls svn_repos_verify_fs3 with keep-going as
> FALSE.
>
>
> Patch by: Prabhu Gnana Sundar <prabhugs{_AT_}collab.net>
