Prabhu Gnana Sundar wrote on Tue, Oct 30, 2012 at 19:22:31 +0530:
>
> Thanks to Stefan and Daniel.
>
> Attaching a new patch addressing the suggestions given by Stefan and
> Daniel. Hope this is good :)
> Edited the log message also.
> Index: subversion/libsvn_repos/dump.c
> ===================================================================
> --- subversion/libsvn_repos/dump.c (revision 1402414)
> +++ subversion/libsvn_repos/dump.c (working copy)
> @@ -1360,9 +1360,10 @@
> }
>
> svn_error_t *
> -svn_repos_verify_fs2(svn_repos_t *repos,
> +svn_repos_verify_fs3(svn_repos_t *repos,
> svn_revnum_t start_rev,
> svn_revnum_t end_rev,
> + svn_boolean_t keep_going,
> svn_repos_notify_func_t notify_func,
> void *notify_baton,
> svn_cancel_func_t cancel_func,
> @@ -1374,6 +1375,7 @@
> svn_revnum_t rev;
> apr_pool_t *iterpool = svn_pool_create(pool);
> svn_repos_notify_t *notify;
> + svn_error_t *err;
>
> /* Determine the current youngest revision of the filesystem. */
> SVN_ERR(svn_fs_youngest_rev(&youngest, fs, pool));
> @@ -1397,8 +1399,19 @@
> end_rev, youngest);
>
> /* Verify global/auxiliary data and backend-specific data first. */
> - SVN_ERR(svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton,
> - start_rev, end_rev, pool));
> + err = svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton,
> + start_rev, end_rev, pool);
> + if (err && keep_going)
> + {
> + svn_repos_notify_t *notify_failure;
> + notify_failure = svn_repos_notify_create(svn_repos_notify_failure,
> + iterpool);
> + notify_failure->err = err;
> + notify_func(notify_baton, notify_failure, iterpool);
> + svn_error_clear(err);
> + }
> + else
> + return svn_error_trace(err);
This pattern repeats three times, maybe introduce a macro (like SVN_ERR,
SVN_INT_ERR, etc) to improve readability?
Oh, and NOTIFY_FUNC may be NULL. (if that happens, trying to call it
will segfault) I think the docstring needs to document what happens
when NOTIFY_FUNC is NULL and KEEP_GOING is TRUE.
> * subversion/libsvn_repos/dump.c
> (svn_repos_verify_fs3): Handle "keep-going". If "keep-going" is TRUE, the
> error message is notified and verification process continues.
>
Normally I mention svn_repos_verify_fs2() too here, with a text
description "Deprecate.". (in the imperative, not past)
> * subversion/libsvn_repos/deprecated.c
> (svn_repos_verify_fs2): Deprecated. Call svn_repos_verify_fs3 with
> keep_going as FALSE by default to keep the old default implementation.
>
> * subversion/libsvn_repos/notify.c
> (svn_repos_notify_create): Initialise the error chain "err" to SVN_NO_ERROR.
>
> Patch by: Prabhu Gnana Sundar <prabhugs{_AT_}collab.net>
> Reviewed by: Stefan Sperling <stsp{_AT_}elego.de>
Looks good otherwise.
Received on 2012-10-30 15:08:32 CET