On 25.06.2015 18:11, Evgeny Kotkov wrote: > Branko Čibej <br...@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
