> -----Original Message-----
> From: Prabhu Gnana Sundar [mailto:prabhugs_at_collab.net]
> Sent: donderdag 18 oktober 2012 17:05
> To: dev_at_subversion.apache.org
> Subject: [PATCH] Implement svnadmin verify --force
>
> Hi all,
>
> Currently svnadmin verify would stop verification process once an
> error/corruption is found in the repo. It does not go till the HEAD of
> the repo to see if there are further corruptions/errors.
>
> It would be helpful if "--force" switch would do this.
>
> Attaching a patch and the log message for the same. Please share your
> thoughts...
>
>
> Thanks and regards
> Prabhu
Comments inline.
> Index: subversion/libsvn_repos/deprecated.c
> ===================================================================
> --- subversion/libsvn_repos/deprecated.c (revision 1398254)
> +++ 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,
> + notify_func,
> + notify_baton,
> + cancel_func,
> + cancel_baton,
> + FALSE,
> + pool));
> +}
> +
> +svn_error_t *
> svn_repos_verify_fs(svn_repos_t *repos,
> svn_stream_t *feedback_stream,
> svn_revnum_t start_rev,
> @@ -736,16 +757,16 @@
> void *cancel_baton,
> apr_pool_t *pool)
> {
> - return svn_error_trace(svn_repos_verify_fs2(repos,
> - start_rev,
> - end_rev,
> - feedback_stream
> - ? repos_notify_handler
> - : NULL,
> - feedback_stream,
> - cancel_func,
> - cancel_baton,
> - pool));
> + return svn_repos_verify_fs2(repos,
> + start_rev,
> + end_rev,
> + feedback_stream
> + ? repos_notify_handler
> + : NULL,
> + feedback_stream,
> + cancel_func,
> + cancel_baton,
> + pool);
> }
Please keep the svn_error_trace() wrapping. This improves error messages in
maintainer builds.
>
> /*** From load.c ***/
> Index: subversion/libsvn_repos/dump.c
> ===================================================================
> --- subversion/libsvn_repos/dump.c (revision 1398254)
> +++ subversion/libsvn_repos/dump.c (working copy)
> @@ -35,6 +35,7 @@
> #include "svn_checksum.h"
> #include "svn_props.h"
> #include "svn_sorts.h"
> +#include "svn_cmdline.h"
>
> #include "private/svn_mergeinfo_private.h"
> #include "private/svn_fs_private.h"
> @@ -1360,13 +1361,14 @@
> }
>
> 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_repos_notify_func_t notify_func,
> void *notify_baton,
> svn_cancel_func_t cancel_func,
> void *cancel_baton,
> + svn_boolean_t force,
> apr_pool_t *pool)
The new norm is that we try to keep callbacks at the end of the argument
list.
An argument name as 'skip_errors' or 'continue_on_errors' would be more
self-explaining than 'force'.
> {
> svn_fs_t *fs = svn_repos_fs(repos);
> @@ -1397,6 +1399,7 @@
> end_rev, youngest);
>
> /* Verify global/auxiliary data and backend-specific data first. */
> + if (!force)
> SVN_ERR(svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton,
> start_rev, end_rev, pool));
>
> @@ -1413,11 +1416,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,
> @@ -1425,7 +1429,15 @@
> notify_func, notify_baton,
> start_rev,
> FALSE, TRUE, /* use_deltas, verify */
> - iterpool));
> + iterpool);
> + if (err && force)
> + {
> + svn_handle_error2(err, stderr, FALSE /* non-fatal */, "svnadmin:
");
> + continue;
> + }
> + else
> + SVN_ERR(err);
> +
Please use our standard indent style.
But wait we are in subversion/libsvn_repos/dump.c, not svnadmin, so we can't
just write something to the console!
This will need some callback function. Can we just use the existing notify
function?
> SVN_ERR(svn_delta_get_cancellation_editor(cancel_func,
cancel_baton,
> dump_editor,
dump_edit_baton,
> &cancel_editor,
> Index: subversion/include/svn_repos.h
> ===================================================================
> --- subversion/include/svn_repos.h (revision 1398254)
> +++ subversion/include/svn_repos.h (working copy)
> @@ -2517,8 +2517,29 @@
> * cancel_baton as argument to see if the caller wishes to cancel the
> * verification.
> *
> + * If @a force 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_repos_notify_func_t notify_func,
> + void *notify_baton,
> + svn_cancel_func_t cancel,
> + void *cancel_baton,
> + svn_boolean_t force,
> + apr_pool_t *scratch_pool);
Similar as above: move force to before the notify func and probably rename
force to something less confusing.
> +
> +/**
> + * Similar to svn_repos_verify_fs3(), but without force.
> + *
> * @since New in 1.7.
> + * @deprecated Provided for backward compatibility with the 1.7 API.
> */
> +
> svn_error_t *
> svn_repos_verify_fs2(svn_repos_t *repos,
> svn_revnum_t start_rev,
> Index: subversion/svnadmin/main.c
> ===================================================================
> --- subversion/svnadmin/main.c (revision 1398254)
> +++ subversion/svnadmin/main.c (working copy)
> @@ -193,7 +193,8 @@
> svnadmin__pre_1_4_compatible,
> svnadmin__pre_1_5_compatible,
> svnadmin__pre_1_6_compatible,
> - svnadmin__pre_1_8_compatible
> + svnadmin__pre_1_8_compatible,
> + svnadmin__force
> };
>
> /* Option codes and descriptions.
> @@ -286,6 +287,9 @@
> N_("use format compatible with Subversion versions\n"
> " earlier than 1.8")},
>
> + {"force", svnadmin__force, 0,
> + N_("continue verifying even if there is a corruption")},
> +
For svnadmin something similar applies: why use such a generic --force.
If we could break compatibility we would probably have removed --force from
'svn' as it is not that user friendly to have such a catch-all argument.
> {"memory-cache-size", 'M', 1,
> N_("size of the extra in-memory cache in MB used to\n"
> " minimize redundant operations.
Default: 16.\n"
> @@ -473,7 +477,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__force, 'M'} },
>
> { NULL, NULL, {0}, NULL, {0} }
> };
> @@ -503,6 +507,7 @@
> svn_boolean_t clean_logs; /* --clean-logs */
> svn_boolean_t bypass_hooks; /* --bypass-hooks */
> svn_boolean_t wait; /* --wait */
> + svn_boolean_t force; /* --force */
> svn_boolean_t bypass_prop_validation; /*
--bypass-prop-validation */
> enum svn_repos_load_uuid uuid_action; /* --ignore-uuid,
> --force-uuid */
> @@ -1525,10 +1530,11 @@
> 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->quiet
> ? repos_notify_handler : NULL,
> - progress_stream, check_cancel, NULL, pool);
> + progress_stream, check_cancel, NULL,
> + opt_state->force, pool);
> }
>
> /* This implements `svn_opt_subcommand_t'. */
> @@ -1990,6 +1996,9 @@
> case svnadmin__pre_1_8_compatible:
> opt_state.pre_1_8_compatible = TRUE;
> break;
> + case svnadmin__force:
> + opt_state.force = TRUE;
> + break;
> case svnadmin__fs_type:
> SVN_INT_ERR(svn_utf_cstring_to_utf8(&opt_state.fs_type, opt_arg,
pool));
> break;
>
Received on 2012-10-18 18:06:28 CEST