On Wed, Jul 13, 2011 at 22:39, <danie...@apache.org> wrote:
>...
> +++ subversion/branches/fs-progress/subversion/include/svn_fs.h Thu Jul 14
> 02:39:52 2011
> @@ -246,6 +246,24 @@ svn_fs_upgrade(const char *path,
> apr_pool_t *pool);
>
> /**
> + * Callback function type for progress notification.
> + *
> + * @a progress is the number of steps already completed, @a total is
> + * the total number of steps in this stage, @a stage is the number of
> + * stages (for extensibility), @a baton is the callback baton.
> + *
> + * @note The number of stages may vary depending on the backend, library
> + * version, and so on. @a total may be a best-effort estimate.
> + *
> + * @since New in 1.8.
> + */
> +typedef void (*svn_fs_progress_notify_func_t)(apr_off_t progress,
> + apr_off_t total,
> + int stage,
> + void *baton,
> + apr_pool_t *scratch_pool);
How are PROGRESS and TOTAL logically associated with an apr_off_t?
That type is for file offsets. Progress information wouldn't seem to
have any correlation. Maybe just a long? Or an apr_int64_t ?
>...
> +++ subversion/branches/fs-progress/subversion/include/svn_repos.h Thu Jul 14
> 02:39:52 2011
> @@ -242,7 +242,19 @@ typedef enum svn_repos_notify_action_t
> svn_repos_notify_recover_start,
>
> /** Upgrade has started. */
> - svn_repos_notify_upgrade_start
> + svn_repos_notify_upgrade_start,
> +
> + /** Verifying global data has commenced
> + * @since New in 1.8. */
> + svn_repos_notify_verify_aux_start,
Why it is described as "global data", yet the symbol uses "aux"?
>...
> @@ -315,6 +327,12 @@ typedef struct svn_repos_notify_t
> /** For #svn_repos_notify_load_node_start, the path of the node. */
> const char *path;
>
> + /** For #svn_repos_notify_verify_aux_progress;
> + see svn_fs_progress_notify_func_t. */
> + apr_off_t progress_progress;
> + apr_off_t progress_total;
> + int progress_stage;
See above re: apr_off_t. And should "stage" be an integer, or is that
an enumerated constant?
>...
> +++ subversion/branches/fs-progress/subversion/libsvn_repos/dump.c Thu Jul 14
> 02:39:52 2011
>...
> @@ -1284,8 +1306,37 @@ svn_repos_verify_fs2(svn_repos_t *repos,
>
> /* Verify global/auxiliary data before verifying revisions. */
> if (start_rev == 0)
> - SVN_ERR(svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton,
> - pool));
> + {
> + struct progress_to_notify_baton ptnb = {
> + notify_func, notify_baton, NULL
> + };
> +
> + /* Create a notify object that we can reuse within the callback. */
> + if (notify_func)
> + ptnb.notify =
> svn_repos_notify_create(svn_repos_notify_verify_aux_progress,
> + iterpool);
> +
> + /* We're starting. */
> + if (notify_func)
> + notify_func(notify_baton,
> +
> svn_repos_notify_create(svn_repos_notify_verify_aux_start,
> + iterpool),
> + iterpool);
> +
> + /* Do the work. */
> + SVN_ERR(svn_fs_verify(svn_fs_path(fs, iterpool),
> + (notify_func ? progress_to_notify : NULL), &ptnb,
> + cancel_func, cancel_baton,
> + iterpool));
> +
> + /* We're finished. */
> + if (notify_func)
> + notify_func(notify_baton,
> + svn_repos_notify_create(svn_repos_notify_verify_aux_end,
> + iterpool),
> + iterpool);
> +
> + }
It seems the entire block above can be written more clearly with an
outer-block test of (notify_func), and then a direct call to
svn_fs_verify() without notify information, or a big block to set up
and do all the notification stuff. That should be clearer than four
tests of (notify_func).
Not to mention the conditional setting of .notify, yet there is an
unconditional usage in progress_to_notify() ... kinda throws you for a
bit. Until you realize that progress_to_notify() is *also*
conditionally used.
>...
Cheers,
-g
