Re: [PATCH] Implement svnadmin verify --force

Thanks Stefan,

    I have written a new patch as per your suggestions and Julian's
suggestions. Attaching the new patch and the log message with this mail.
Please share your thoughts.

Thanks and regards
Prabhu

On 10/18/2012 09:26 PM, Stefan Sperling wrote:
> 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!