On 25.06.2015 18:11, Evgeny Kotkov wrote:
> 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.
Typo; you mean 'svn_repos_verify_fs2' here.
> * 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?
I like this a lot. Much better than my hacks upon hacks. :)
Please go ahead and commit this. As you said, JavaHL would break after
this change. If you like, I can fix it afterwards, but I'm on a rather
tight schedule tomorrow and on the week-end so I very likely couldn't do
that before Monday.
-- Brane
Received on 2015-06-26 00:03:02 CEST