Branko Čibej <brane_at_wandisco.com> writes:
> Go ahead; I won't be able to for the next few days. Please remember to
> remove the new error code, too, if we won't be using it any more.
With a bit of trial and error, I was able to come up with a complete and
tested solution that I find suitable in terms of the API design and the
calling site (svnadmin.c). The ideas are partly borrowed from the already
mentioned svn_fs_lock_many() and svn_fs_lock_callback_t implementations.
See the attached patch. Please note that the patch is not a continuation of
what I posted in my last e-mail (verify-fixup-squashed-v1.patch.txt), but from
my point of view is better and should address the raised concerns, such as
having a non-trivial compatibility wrapper.
Here is the log message:
[[[
Reimplement svn_repos_verify_fs3() to support an arbitrary callback that
receives the information about an encountered problem and lets the caller
decide on what happens next. This supersedes the keep_going argument for
this API. A callback is optional; the behavior of this API if the callback
is not provided is equivalent to how svn_repos_verify_fs2() deals with
encountered errors. This allows seamless migration to the new API, if the
callback is not necessary. The idea is partly taken from how our existing
svn_fs_lock_many() API works with a svn_fs_lock_callback_t and passes error
information to the caller.
Immediately use the new API to provide an alternative solution for the
encountered problem with 'svnadmin verify --keep-going -q' (see r1684940)
being useless in terms that it was only giving an indication of whether a
particular repository passes the verification or not, without providing a
root cause (details) of what's wrong.
Discussion can be found in http://svn.haxx.se/dev/archive-2015-05/0141.shtml
(Subject: "Possible incompatibility of svn_repos_verify_fs2() in 1.9.0-rc1")
* subversion/include/svn_error_codes.h
(SVN_ERR_REPOS_VERIFY_FAILED): Remove this error code, as we no longer
need to send a specific error from within svn_repos_verify_fs3().
(SVN_ERR_CL_REPOS_VERIFY_FAILED): New.
* subversion/include/svn_repos.h
(svn_repos_notify_action_t): Remove svn_repos_notify_failure.
(svn_repos_notify_t): Remove 'err' field, as it is no longer needed.
(svn_repos_verify_callback_t): New optional callback type to be used with
svn_repos_verify_fs3().
(svn_repos_verify_fs3): Drop 'keep_going' argument in favor of accepting a
svn_repos_verify_callback_t. Update the docstring accordingly.
(svn_repos_verify_fs3): Update the docstring for this deprecated function.
* subversion/libsvn_repos/deprecated.c
(svn_repos_verify_fs2): Update the call to svn_repos_verify_fs3() in this
compatibility wrapper. Don't pass the verify callback.
* subversion/libsvn_repos/dump.c
(notify_verification_error): Remove; this function is no longer required.
(report_error): New helper function.
(svn_repos_verify_fs3): In case we've got a svn_repos_verify_callback_t,
call it upon receiving an FS-specific structure failure or a revision
verification failure. Delegate this action to the new report_error()
helper function. Doing so makes the caller responsible for what's going
to happen with the error. The caller can choose to store the error,
ignore it or use it in any other necessary way. If a callback returns an
error, stop the verification process and immediately return that error.
If no callback is provided, mimic the behavior of svn_repos_verify_fs2()
and return the first encountered error. Drop the logic related to error
formatting, as we no longer need it at this layer. We are going to make
a simpler replacement for it is the UI code (svnadmin.c), where it is
supposed to live.
* subversion/svnadmin/svnadmin.c
(struct repos_verify_callback_baton): New. Contains the fields that are
required to track the --keep-going errors taken from ...
(struct repos_notify_handler_baton): ...this baton. After the previous
step, this baton only contains the 'feedback_stream' field, so inline it
into every calling site.
(repos_notify_handler): Baton is now simply an svn_stream_t. Remove the
boolean-based filtering logic from this handler and drop the handling of
svn_repos_notify_failure. The latter is moved, with a bit of tweaking,
into ...
(repos_verify_callback): ...this new function, that implements a callback
for svn_repos_verify_fs3(). Depending on whether we are in --keep-going
mode or not, either dump the failure details to stderr and track them to
produce a summary, or immediately return it through the callback, thus
ending the verification process. Remember all errors in the --keep-going
mode, not only those that are associated with a particular revision.
Prior to handling the error itself, tell that we failed to verify the
revision or metadata by writing corresponding messages to stderr.
(subcommand_dump, subcommand_load, subcommand_recover, subcommand_upgrade,
subcommand_hotcopy, subcommand_pack): Inline repos_notify_handler_baton
here, as it now contains a single svn_stream_t field.
(subcommand_verify): Inline repos_notify_handler_baton here, as it now
contains a single svn_stream_t field. Avoid manipulations with boolean
fields like b->silent_errors and b->silent_running, because we no longer
need them, and the fields themselves are gone. Create a feedback stream
only in non-quiet mode, as we do in other subcommand implementations.
Create a baton for repos_verify_callback() and adjust the calling site of
svn_repos_verify_fs3(), that now needs a callback. Adjust --keep-going
summary printing to the new approach with the verification callback.
Finally, provide a simple error if we encountered at least one failure
in the --keep-going mode.
* subversion/tests/cmdline/svnadmin_tests.py
(verify_keep_going, verify_keep_going_quiet, verify_invalid_path_changes):
Adjust the expectations, because now errors go straight to stderr in both
--keep-going and ordinary modes. Where possible, make the expectations a
bit stricter by extending the lines that we check with RegexListOutput().
* subversion/tests/libsvn_fs_fs/fs-fs-private-test.c
(load_index, load_index_keep_going): Squash two tests into one; basically,
undo the corresponding hunk from r1683311. As we no longer have separate
keep_going mode in svn_repos_verify_fs3(), and the caller decides if the
verification continues or not, we don't have to check two different
scenarios.
(test_funcs): Track the test changes.
* subversion/tests/libsvn_fs_fs/fs-fs-fuzzy-test.c
(fuzzing_1_byte_1_rev): Adjust the call to svn_repos_verify_fs3().
]]]
What do you think?
Regards,
Evgeny Kotkov
Received on 2015-06-25 18:13:23 CEST