Re: [PATCH] Implement svnadmin verify --force

Thanks to Stefan and Daniel.

Attaching a new patch addressing the suggestions given by Stefan and
Daniel. Hope this is good :)
Edited the log message also.

Thanks and regards
Prabhu

On 10/29/2012 11:53 PM, Stefan Sperling wrote:
> On Mon, Oct 29, 2012 at 10:45:19PM +0530, Prabhu Gnana Sundar wrote:
>> Thank you so much Stefan for your patience in reviewing and guiding
>> me through.
>>
>> I have addressed your points in the patch attached in this mail. I
>> hope I addressed all the suggestions given by you :)
>> Please share your views.
> Sure, see below.
>
> I don't think we're quite done yet but we're getting close :)
>
>> Index: subversion/svnadmin/main.c
>> ===================================================================
>> --- subversion/svnadmin/main.c (revision 1402414)
>> +++ subversion/svnadmin/main.c (working copy)
>> @@ -738,6 +743,11 @@
>> notify->warning_str));
>> return;
>>
>> + case svn_repos_notify_failure:
>> + svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
>> + "svnadmin: ");
>> + return;
>> +
> Running this on a repository with 5 revisions, where r3 is corrupt,
> the output looks as follows.
>
> $ svnadmin verify --keep-going repos
> * Verified revision 0.
> * Verified revision 1.
> * Verified revision 2.
> svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787)
> svnadmin: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed
> * Verified revision 5.
> $
>
> I've removed error tracing output as shown by a maintainer-mode build
> to match what it would look like in a release build.
>
> What I don't like is that there is no visual indication at all that
> tells me which revision an error corresponds to.
> The first error message is from r3, which is obvious only because the
> error message text itself happens to mention the revision number.
> The second error message is from r4, but I know that only because I
> already know what the corruption is because I've deliberately corrupted
> the repository myself :)
>
> So I'd suggest printing the revision number as part of the error message.
> Something like this should do:
>
> $ svnadmin verify --keep-going repos
> * Verified revision 0.
> * Verified revision 1.
> * Verified revision 2.
> svnadmin: Error verifying revision 3: E160004: Missing node-id in node-rev at r3 (offset 787)
> svnadmin: Error verifying revision 4: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed
> * Verified revision 5.
> $
>
> And while we're here, it would be great to do the same if the user did
> not pass the --keep-going option:
>
> $ svnadmin verify repos
> * Verified revision 0.
> * Verified revision 1.
> * Verified revision 2.
> svnadmin: Error verifying revision 3: E160004: Missing node-id in node-rev at r3 (offset 787)
> $
>
> Do you agree that this is better? If so, please amend the patch accordingly.
> You can use the existing 'revision' field in svn_repos_notify_t to communicate
> a revision number to the notification handler.
>
>> 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,
>> @@ -1374,6 +1375,7 @@
>> svn_revnum_t rev;
>> apr_pool_t *iterpool = svn_pool_create(pool);
>> svn_repos_notify_t *notify;
>> + svn_error_t *error;
> We usually call these variables 'err', not 'error'.
> Please follow this stylistic pattern.
>
>>
>> /* Determine the current youngest revision of the filesystem. */
>> SVN_ERR(svn_fs_youngest_rev(&youngest, fs, pool));
>> @@ -1397,9 +1399,17 @@
>> end_rev, youngest);
>>
>> /* 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));
>> + error = svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton,
>> + start_rev, end_rev, pool);
>> + if(error&& !keep_going)
> Please always add a space between 'if' and the opening brace, like this:
>
> if (err&& !keep_going)
>
> We do the same for 'while', 'for', and a bunch of other C keywords.
> The goal here is consistency, there's no point in arguing which style
> is better. We're just following the style that the project has agreed
> on using.
>
>> + SVN_ERR(error);
> If an error is known to be non-NULL (i.e. it is not SVN_NO_ERROR),
> you can use svn_error_trace() instead of SVN_ERR():
>
> return svn_error_trace(err);
>
>>
>> + /* We don't have to notify this failure when keep_going is TRUE since
>> + * that is taken care by the svn_repos_notify_failure notification.
>> + * Notifying it here too would make it notified twice when keep_going
>> + * is TRUE. */
>> + svn_error_clear(error);
>> +
> Hmm... does this mean errors from svn_fs_verify() are never reported
> to the notification handler? That doesn't seem right.
>
> I think you should report this error because svn_fs_verify() function
> can catch corruption outside of the revision number space. For example,
> it can catch a corrupt rep-cache.db file, which the user would surely
> like to know about!
>
> To tell the notification handler that this error does not relate
> to any particular revision, you can set the 'revision' field of
> svn_repos_notify_t to SVN_INVALID_REVNUM. In which case the handler
> can simply omit the "Error verifying revision X" annotation I suggested
> above, and print the error as usual...
>
> svnadmin: E12345: blah blah blah
>
> ... and then we keep going if --keep-going was passed.
>
>> Index: subversion/include/svn_repos.h
>> ===================================================================
>> --- subversion/include/svn_repos.h (revision 1402414)
>> +++ subversion/include/svn_repos.h (working copy)
>> @@ -318,6 +321,10 @@
>> /** For #svn_repos_notify_load_node_start, the path of the node. */
>> const char *path;
>>
>> + /** For #svn_repos_notify_failure, this error chain indicates what
>> + went wrong during verification. */
>> + svn_error_t *err;
>> +
> Note this comment which is part of the existing context:
>
>> /* 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(). */
> It doesn't look like you adjusted svn_repos_notify_create(), did you?
> I think you should initialise the 'err' chain to SVN_NO_ERROR in
> svn_repos_notify_create().
>
>> 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):
>> (svnadmin_opt_state): Add keep-going option.
>> (subcommand_verify): Switch to the new svn_repos_verify_fs3 function instead
>> of svn_repos_verify_fs2, and pass the keep-going option.
> I'd intend multi-line log entries like this to provide more horizontal
> space for the second and consecutive lines:
>
> (subcommand_verify): Switch to the new svn_repos_verify_fs3 function instead
> of svn_repos_verify_fs2, and pass the keep-going option.
>
> Looks great otherwise!