Subversion 1.9.0-rc1 introduced a new svnadmin verify --keep-going mode [1].
In order to achieve this, we added a svn_repos_verify_fs3() API function and
deprecated its predecessor, svn_repos_verify_fs2(), that now calls the newer
function with keep_going argument set to FALSE.
However, svn_repos_verify_fs2() behaves differently in 1.9.0-rc1 and 1.8.13
when it comes to error reporting. As an example, the following code ...
SVN_ERR(svn_repos_verify_fs2(repos, 1, 5, NULL, NULL, NULL, NULL, pool));
...would return two different errors depending on the binaries being used,
assuming that one of the revisions in the [r1:r5] range is corrupted:
(With 1.8.13) E160004: Checksum mismatch in item at offset 0 of
length 59 bytes in file path/asf/db/revs/0/2
(With 1.9.0-rc1) E165011: Repository 'path/asf' failed to verify
Please note that the second error is generic, and that the actual information
about the error is lost. Existing API users of svn_repos_verify_fs2() are
going to lose an important bit of information about the root cause of the
corruption unless they update the code. Furthermore, even if an API caller
subscribes to notifications with a notify_func / notify_baton pair, it would
still be necessary to update the code to handle svn_repos_notify_failure that
did not exist before 1.9.
I did not find any discussion on the matter or the corresponding entry in
/notes/api-errata [2] that would describe this behavior change, so I am
inclined to think that this is accidental and, probably, undesirable.
There is an option of restoring the 1.8 behavior when svn_repos_verify_fs3()
is called with keep_going set to FALSE. We could immediately yield the error
in this case, and only use the notifications / generic error when keep_going
is set to TRUE. Doing so would change the output of svnadmin verify without
--keep-going, because now it wouldn't include the generic error message in
the end. I attached a proof-of-concept patch, that doesn't change the test
expectations and the documentation, i.e., it only includes the core change,
mostly as an illustration to these points.
Thoughts?
[1] https://svn.apache.org/r1492651
[2] https://svn.apache.org/repos/asf/subversion/trunk/notes/api-errata/1.9
Regards,
Evgeny Kotkov
Index: subversion/libsvn_repos/dump.c
===================================================================
--- subversion/libsvn_repos/dump.c (revision 1680667)
+++ subversion/libsvn_repos/dump.c (working copy)
@@ -2423,33 +2423,21 @@ svn_repos_verify_fs3(svn_repos_t *repos,
verify_notify, verify_notify_baton,
cancel_func, cancel_baton, pool);
- if (err)
+ if (err && err->apr_err == SVN_ERR_CANCELLED)
{
- if (err->apr_err == SVN_ERR_CANCELLED)
- return svn_error_trace(err);
-
+ return svn_error_trace(err);
+ }
+ else if (err && !keep_going)
+ {
+ /* Return the error, as the caller doesn't want us to continue. */
+ return svn_error_trace(err);
+ }
+ else if (err && keep_going)
+ {
+ /* Notify about the error and keep going. */
found_corruption = TRUE;
notify_verification_error(SVN_INVALID_REVNUM, err, notify_func,
notify_baton, iterpool);
-
- /* If we already reported the error, reset it. */
- if (notify_func)
- {
- svn_error_clear(err);
- err = NULL;
- }
-
- /* If we abort the verification now, combine yet unreported error
- info with the generic one we return. */
- if (!keep_going)
- /* ### Jump to "We're done" and so send the final notification,
- for consistency? */
- return svn_error_createf(SVN_ERR_REPOS_CORRUPTED, err,
- _("Repository '%s' failed to verify"),
- svn_dirent_local_style(svn_repos_path(repos,
- pool),
- pool));
-
svn_error_clear(err);
}
@@ -2464,24 +2452,26 @@ svn_repos_verify_fs3(svn_repos_t *repos,
cancel_func, cancel_baton,
iterpool);
- if (err)
+ if (err && err->apr_err == SVN_ERR_CANCELLED)
{
- if (err->apr_err == SVN_ERR_CANCELLED)
- return svn_error_trace(err);
-
+ return svn_error_trace(err);
+ }
+ else if (err && !keep_going)
+ {
+ /* Return the error, as the caller doesn't want us to continue. */
+ return svn_error_trace(err);
+ }
+ else if (err && keep_going)
+ {
+ /* Notify about the error and keep going. */
found_corruption = TRUE;
notify_verification_error(rev, err, notify_func, notify_baton,
iterpool);
svn_error_clear(err);
-
- if (keep_going)
- continue;
- else
- break;
}
-
- if (notify_func)
+ else if (!err && notify_func)
{
+ /* Tell the caller that we're done with this revision. */
notify->revision = rev;
notify_func(notify_baton, notify, iterpool);
}
