On Thu, Oct 18, 2012 at 08:35:03PM +0530, Prabhu Gnana Sundar wrote:
> 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.
I believe we want to discourage new --force options. The reason
is that it is a poor option name. It is not self-evident what is being
forced. And worse, the option already has different meanings for
different subcommands. Try to find out what 'svn merge --force' does,
for instance, and compare that to what --force does for 'svn diff'.
It's usually impossible to figure out what --force really does without
looking deep into the svnbook or the source code.
> Attaching a patch and the log message for the same. Please share
> your thoughts...
I like the idea, but I would prefer an option name such as
"--continue-on-verification-failure", or... something that sounds a
bit snappier than that -- but nothing else comes to mind right now :)
Another approach might be to just change the default behaviour instead
of adding an option. I've never understood why 'svnadmin verify' errors
out once a revision fails. I cannot think of any good reason for this
behaviour.
Some more comments below...
> Implement svnadmin verify --force, 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):
> (svnadmin_opt_state): add force option.
> (subcommand_verify) : uses the new svn_repos_verify_fs3 function.
>
> * subversion/include/svn_repos.h
> (svn_repos_verify_fs3): newly added to handle "force" option.
>
> * subversion/libsvn_repos/dump.c
> (svn_repos_verify_fs3): now handles "force".
> If "force" 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 force as FALSE.
>
>
> Patch by: Prabhu Gnana Sundar <prabhugs{_AT_}collab.net>
> @@ -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);
> }
This looks wrong. Why are you removing error tracing?
>
> /*** 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"
The repository layer should not use cmdline functionality.
> @@ -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: ");
Subversion libraries should never print anything to stdout or stderr.
This code might not be running within svnadmin but within some
third-party product based on Subversion, for example. In which case
printing something is undesirable and could cause problems (cosmetic
problems with application output, or worse).
I see two possible solutions to this problem:
1) Use the notification system that invokes a callback provided by the
API user to report the warning. See repos_notify_handler() in
svnadmin/main.c for code which receives such notifications.
You might need to extend svn_repos_notify_action_t with a new
notification code to support this. For instance, you could call
it 'svn_repos_notify_verify_error'. Or maybe you can use
svn_repos_notify_warning, which already exists.
2) Error out all the way back to svnadmin, and find a way to make it run
a new verification which starts at the next revision after the one that
failed to verify.
Your patch looks fine otherwise and I'm looking forward to the final
revision of it. Thanks!
Received on 2012-10-18 18:06:52 CEST