[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Possible incompatibility of svn_repos_verify_fs2() in 1.9.0-rc1

From: Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com>
Date: Thu, 25 Jun 2015 19:11:48 +0300

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().

* 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): 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
  (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?

Evgeny Kotkov

Received on 2015-06-25 18:13:23 CEST

This is an archived mail posted to the Subversion Dev mailing list.