You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by da...@apache.org on 2011/07/14 04:39:53 UTC
svn commit: r1146547 - in /subversion/branches/fs-progress/subversion:
include/ libsvn_fs/ libsvn_fs_base/ libsvn_fs_fs/ libsvn_repos/ svnadmin/
tests/cmdline/
Author: danielsh
Date: Thu Jul 14 02:39:52 2011
New Revision: 1146547
URL: http://svn.apache.org/viewvc?rev=1146547&view=rev
Log:
On the fs-progress branch, sketch an implementation of progress notification
for svn_fs_verify().
The FS API and the output of 'svnadmin' could both use some tweaking;
suggestions welcome.
Add a new FS API:
* subversion/include/svn_fs.h
(svn_fs_verify): Grow PROGRESS_FUNC and PROGRESS_BATON arguments.
(svn_fs_progress_notify_func_t): New, like svn_ra_progress_notify_func_t
but with an extra STAGE parameter.
* subversion/libsvn_fs/fs-loader.c
(svn_fs_verify): Pass new arguments to verify_fs().
* subversion/libsvn_fs/fs-loader.h
(verify_fs): Grow PROGRESS_FUNC and PROGRESS_BATON arguments.
* subversion/libsvn_fs_base/fs.c
(base_verify): Track signature change.
* subversion/libsvn_fs_fs/fs.c
(fs_verify): Track signature change.
* subversion/libsvn_fs_fs/fs_fs.h
(svn_fs_fs__verify): Track signature change.
* subversion/libsvn_fs_fs/fs_fs.c
(verify_baton): New baton type.
(svn_fs_fs__verify): Track signature change. Initialize and pass a baton.
(verify_walker): Call PROGRESS_FUNC occasionally.
Add a new repos API:
* subversion/include/svn_repos.h
(svn_repos_notify_action_t):
New values:
svn_repos_notify_verify_aux_start,
svn_repos_notify_verify_aux_progress,
svn_repos_notify_verify_aux_end.
(svn_repos_notify_t):
New members PROGRESS_PROGRESS, PROGRESS_TOTAL, PROGRESS_STAGE.
* subversion/libsvn_repos/dump.c
(progress_to_notify_baton): New baton type.
(progress_to_notify): New helper.
(svn_repos_verify_fs2): Notify about global/auxiliary data verification.
Use the new API:
* subversion/svnadmin/main.c
(repos_notify_handler): Handle the three new notification kinds.
Adjust regression tests:
* subversion/tests/cmdline/svnadmin_tests.py
(verify_windows_paths_in_repos): Adjust expected output.
Modified:
subversion/branches/fs-progress/subversion/include/svn_fs.h
subversion/branches/fs-progress/subversion/include/svn_repos.h
subversion/branches/fs-progress/subversion/libsvn_fs/fs-loader.c
subversion/branches/fs-progress/subversion/libsvn_fs/fs-loader.h
subversion/branches/fs-progress/subversion/libsvn_fs_base/fs.c
subversion/branches/fs-progress/subversion/libsvn_fs_fs/fs.c
subversion/branches/fs-progress/subversion/libsvn_fs_fs/fs_fs.c
subversion/branches/fs-progress/subversion/libsvn_fs_fs/fs_fs.h
subversion/branches/fs-progress/subversion/libsvn_repos/dump.c
subversion/branches/fs-progress/subversion/svnadmin/main.c
subversion/branches/fs-progress/subversion/tests/cmdline/svnadmin_tests.py
Modified: subversion/branches/fs-progress/subversion/include/svn_fs.h
URL: http://svn.apache.org/viewvc/subversion/branches/fs-progress/subversion/include/svn_fs.h?rev=1146547&r1=1146546&r2=1146547&view=diff
==============================================================================
--- subversion/branches/fs-progress/subversion/include/svn_fs.h (original)
+++ 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);
+
+/**
* Perform backend-specific data consistency and correctness validations
* to the Subversion filesystem located in the directory @a path.
* Use @a pool for necessary allocations.
@@ -258,6 +276,8 @@ svn_fs_upgrade(const char *path,
*/
svn_error_t *
svn_fs_verify(const char *path,
+ svn_fs_progress_notify_func_t progress_func,
+ void *progress_baton,
svn_cancel_func_t cancel_func,
void *cancel_baton,
apr_pool_t *pool);
@@ -2293,7 +2313,6 @@ svn_fs_pack(const char *db_path,
void *cancel_baton,
apr_pool_t *pool);
-
/** @} */
Modified: subversion/branches/fs-progress/subversion/include/svn_repos.h
URL: http://svn.apache.org/viewvc/subversion/branches/fs-progress/subversion/include/svn_repos.h?rev=1146547&r1=1146546&r2=1146547&view=diff
==============================================================================
--- subversion/branches/fs-progress/subversion/include/svn_repos.h (original)
+++ 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,
+
+ /** Verifying global data is progressing
+ * @since New in 1.8. */
+ svn_repos_notify_verify_aux_progress,
+
+ /** Verifying global data has finished
+ * @since New in 1.8. */
+ svn_repos_notify_verify_aux_end
} svn_repos_notify_action_t;
@@ -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;
+
/* NOTE: Add new fields at the end to preserve binary compatibility.
Also, if you add fields here, you have to update
svn_repos_notify_create(). */
Modified: subversion/branches/fs-progress/subversion/libsvn_fs/fs-loader.c
URL: http://svn.apache.org/viewvc/subversion/branches/fs-progress/subversion/libsvn_fs/fs-loader.c?rev=1146547&r1=1146546&r2=1146547&view=diff
==============================================================================
--- subversion/branches/fs-progress/subversion/libsvn_fs/fs-loader.c (original)
+++ subversion/branches/fs-progress/subversion/libsvn_fs/fs-loader.c Thu Jul 14 02:39:52 2011
@@ -461,6 +461,8 @@ svn_fs_upgrade(const char *path, apr_poo
svn_error_t *
svn_fs_verify(const char *path,
+ svn_fs_progress_notify_func_t progress_func,
+ void *progress_baton,
svn_cancel_func_t cancel_func,
void *cancel_baton,
apr_pool_t *pool)
@@ -473,7 +475,9 @@ svn_fs_verify(const char *path,
SVN_ERR(fs_library_vtable(&vtable, path, pool));
fs = fs_new(NULL, pool);
SVN_ERR(acquire_fs_mutex());
- err = vtable->verify_fs(fs, path, cancel_func, cancel_baton,
+ err = vtable->verify_fs(fs, path,
+ progress_func, progress_baton,
+ cancel_func, cancel_baton,
pool, common_pool);
err2 = release_fs_mutex();
if (err)
Modified: subversion/branches/fs-progress/subversion/libsvn_fs/fs-loader.h
URL: http://svn.apache.org/viewvc/subversion/branches/fs-progress/subversion/libsvn_fs/fs-loader.h?rev=1146547&r1=1146546&r2=1146547&view=diff
==============================================================================
--- subversion/branches/fs-progress/subversion/libsvn_fs/fs-loader.h (original)
+++ subversion/branches/fs-progress/subversion/libsvn_fs/fs-loader.h Thu Jul 14 02:39:52 2011
@@ -87,7 +87,8 @@ typedef struct fs_library_vtable_t
svn_error_t *(*upgrade_fs)(svn_fs_t *fs, const char *path, apr_pool_t *pool,
apr_pool_t *common_pool);
svn_error_t *(*verify_fs)(svn_fs_t *fs, const char *path,
- /* ### notification? */
+ svn_fs_progress_notify_func_t progress_func,
+ void *progress_baton,
svn_cancel_func_t cancel_func, void *cancel_baton,
apr_pool_t *pool,
apr_pool_t *common_pool);
Modified: subversion/branches/fs-progress/subversion/libsvn_fs_base/fs.c
URL: http://svn.apache.org/viewvc/subversion/branches/fs-progress/subversion/libsvn_fs_base/fs.c?rev=1146547&r1=1146546&r2=1146547&view=diff
==============================================================================
--- subversion/branches/fs-progress/subversion/libsvn_fs_base/fs.c (original)
+++ subversion/branches/fs-progress/subversion/libsvn_fs_base/fs.c Thu Jul 14 02:39:52 2011
@@ -875,6 +875,8 @@ base_upgrade(svn_fs_t *fs, const char *p
static svn_error_t *
base_verify(svn_fs_t *fs, const char *path,
+ svn_fs_progress_notify_func_t progress_func,
+ void *progress_baton,
svn_cancel_func_t cancel_func,
void *cancel_baton,
apr_pool_t *pool,
Modified: subversion/branches/fs-progress/subversion/libsvn_fs_fs/fs.c
URL: http://svn.apache.org/viewvc/subversion/branches/fs-progress/subversion/libsvn_fs_fs/fs.c?rev=1146547&r1=1146546&r2=1146547&view=diff
==============================================================================
--- subversion/branches/fs-progress/subversion/libsvn_fs_fs/fs.c (original)
+++ subversion/branches/fs-progress/subversion/libsvn_fs_fs/fs.c Thu Jul 14 02:39:52 2011
@@ -257,6 +257,8 @@ fs_upgrade(svn_fs_t *fs, const char *pat
static svn_error_t *
fs_verify(svn_fs_t *fs, const char *path,
+ svn_fs_progress_notify_func_t progress_func,
+ void *progress_baton,
svn_cancel_func_t cancel_func,
void *cancel_baton,
apr_pool_t *pool,
@@ -267,7 +269,8 @@ fs_verify(svn_fs_t *fs, const char *path
SVN_ERR(svn_fs_fs__open(fs, path, pool));
SVN_ERR(svn_fs_fs__initialize_caches(fs, pool));
SVN_ERR(fs_serialized_init(fs, common_pool, pool));
- return svn_fs_fs__verify(fs, cancel_func, cancel_baton, pool);
+ return svn_fs_fs__verify(fs, progress_func, progress_baton,
+ cancel_func, cancel_baton, pool);
}
static svn_error_t *
Modified: subversion/branches/fs-progress/subversion/libsvn_fs_fs/fs_fs.c
URL: http://svn.apache.org/viewvc/subversion/branches/fs-progress/subversion/libsvn_fs_fs/fs_fs.c?rev=1146547&r1=1146546&r2=1146547&view=diff
==============================================================================
--- subversion/branches/fs-progress/subversion/libsvn_fs_fs/fs_fs.c (original)
+++ subversion/branches/fs-progress/subversion/libsvn_fs_fs/fs_fs.c Thu Jul 14 02:39:52 2011
@@ -7780,6 +7780,13 @@ svn_fs_fs__pack(svn_fs_t *fs,
/** Verifying. **/
+struct verify_baton
+{
+ svn_fs_progress_notify_func_t progress_func;
+ void *progress_baton;
+ apr_int64_t reps_seen;
+};
+
/* Body of svn_fs_fs__verify().
Implements svn_fs_fs__walk_rep_reference().walker. */
static svn_error_t *
@@ -7789,9 +7796,16 @@ verify_walker(representation_t *rep,
apr_int64_t reps_count,
apr_pool_t *scratch_pool)
{
+ struct verify_baton *vb = baton;
struct rep_state *rs;
struct rep_args *rep_args;
+ if (vb->progress_func && (vb->reps_seen++ % 1024 == 0))
+ /* ### Symbolic names for the stages? */
+ vb->progress_func(vb->reps_seen, reps_count, 1,
+ vb->progress_baton,
+ scratch_pool);
+
/* ### Should this be using read_rep_line() directly? */
SVN_ERR(create_rep_state(&rs, &rep_args, rep, fs, scratch_pool));
@@ -7800,17 +7814,20 @@ verify_walker(representation_t *rep,
svn_error_t *
svn_fs_fs__verify(svn_fs_t *fs,
+ svn_fs_progress_notify_func_t progress_func,
+ void *progress_baton,
svn_cancel_func_t cancel_func,
void *cancel_baton,
apr_pool_t *pool)
{
fs_fs_data_t *ffd = fs->fsap_data;
+ struct verify_baton vb = { progress_func, progress_baton, 0 };
if (ffd->format < SVN_FS_FS__MIN_REP_SHARING_FORMAT)
return SVN_NO_ERROR;
/* Don't take any lock. */
- SVN_ERR(svn_fs_fs__walk_rep_reference(fs, verify_walker, NULL,
+ SVN_ERR(svn_fs_fs__walk_rep_reference(fs, verify_walker, &vb,
cancel_func, cancel_baton,
pool));
Modified: subversion/branches/fs-progress/subversion/libsvn_fs_fs/fs_fs.h
URL: http://svn.apache.org/viewvc/subversion/branches/fs-progress/subversion/libsvn_fs_fs/fs_fs.h?rev=1146547&r1=1146546&r2=1146547&view=diff
==============================================================================
--- subversion/branches/fs-progress/subversion/libsvn_fs_fs/fs_fs.h (original)
+++ subversion/branches/fs-progress/subversion/libsvn_fs_fs/fs_fs.h Thu Jul 14 02:39:52 2011
@@ -40,6 +40,8 @@ svn_error_t *svn_fs_fs__upgrade(svn_fs_t
/* Verify the fsfs filesystem FS. Use POOL for temporary allocations. */
svn_error_t *svn_fs_fs__verify(svn_fs_t *fs,
+ svn_fs_progress_notify_func_t progress_func,
+ void *progress_baton,
svn_cancel_func_t cancel_func,
void *cancel_baton,
apr_pool_t *pool);
Modified: subversion/branches/fs-progress/subversion/libsvn_repos/dump.c
URL: http://svn.apache.org/viewvc/subversion/branches/fs-progress/subversion/libsvn_repos/dump.c?rev=1146547&r1=1146546&r2=1146547&view=diff
==============================================================================
--- subversion/branches/fs-progress/subversion/libsvn_repos/dump.c (original)
+++ subversion/branches/fs-progress/subversion/libsvn_repos/dump.c Thu Jul 14 02:39:52 2011
@@ -1245,6 +1245,28 @@ verify_close_directory(void *dir_baton,
return close_directory(dir_baton, pool);
}
+struct progress_to_notify_baton
+{
+ svn_repos_notify_func_t notify_func;
+ void *notify_baton;
+ svn_repos_notify_t *notify;
+};
+
+/* Implements svn_fs_progress_notify_func_t. */
+static void
+progress_to_notify(apr_off_t progress,
+ apr_off_t total,
+ int stage,
+ void *baton,
+ apr_pool_t *scratch_pool)
+{
+ struct progress_to_notify_baton *ptnb = baton;
+ ptnb->notify->progress_progress = progress;
+ ptnb->notify->progress_total = total;
+ ptnb->notify->progress_stage = stage;
+ ptnb->notify_func(ptnb->notify_baton, ptnb->notify, scratch_pool);
+}
+
svn_error_t *
svn_repos_verify_fs2(svn_repos_t *repos,
svn_revnum_t start_rev,
@@ -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);
+
+ }
/* Create a notify object that we can reuse within the loop. */
if (notify_func)
Modified: subversion/branches/fs-progress/subversion/svnadmin/main.c
URL: http://svn.apache.org/viewvc/subversion/branches/fs-progress/subversion/svnadmin/main.c?rev=1146547&r1=1146546&r2=1146547&view=diff
==============================================================================
--- subversion/branches/fs-progress/subversion/svnadmin/main.c (original)
+++ subversion/branches/fs-progress/subversion/svnadmin/main.c Thu Jul 14 02:39:52 2011
@@ -815,6 +815,21 @@ repos_notify_handler(void *baton,
" repository may take some time...\n")));
return;
+ case svn_repos_notify_verify_aux_start:
+ /* ### Somehow use PROGRESS_TOTAL here? */
+ svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool,
+ _("* Verifying global structures")
+ ));
+ return;
+
+ case svn_repos_notify_verify_aux_progress:
+ svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool, "."));
+ return;
+
+ case svn_repos_notify_verify_aux_end:
+ svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool, "\n"));
+ return;
+
default:
return;
}
Modified: subversion/branches/fs-progress/subversion/tests/cmdline/svnadmin_tests.py
URL: http://svn.apache.org/viewvc/subversion/branches/fs-progress/subversion/tests/cmdline/svnadmin_tests.py?rev=1146547&r1=1146546&r2=1146547&view=diff
==============================================================================
--- subversion/branches/fs-progress/subversion/tests/cmdline/svnadmin_tests.py (original)
+++ subversion/branches/fs-progress/subversion/tests/cmdline/svnadmin_tests.py Thu Jul 14 02:39:52 2011
@@ -449,11 +449,15 @@ def verify_windows_paths_in_repos(sbox):
exit_code, output, errput = svntest.main.run_svnadmin("verify",
sbox.repo_dir)
+ expected_stderr = [
+ "* Verifying global structures.\n",
+ "* Verified revision 0.\n",
+ "* Verified revision 1.\n",
+ "* Verified revision 2.\n"
+ ]
svntest.verify.compare_and_display_lines(
"Error while running 'svnadmin verify'.",
- 'STDERR', ["* Verified revision 0.\n",
- "* Verified revision 1.\n",
- "* Verified revision 2.\n"], errput)
+ 'STDERR', expected_stderr, errput)
#----------------------------------------------------------------------
Re: svn commit: r1146547 - in
/subversion/branches/fs-progress/subversion: include/ libsvn_fs/
libsvn_fs_base/ libsvn_fs_fs/ libsvn_repos/ svnadmin/ tests/cmdline/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
I'm not familiar with GUI progress notification paradigms and the first
example I checked (scp) wasn't too instructive. Perhaps someone has
a readily available pointer to share?
If not... I'm not sure. I'm not after svn_grand_unified_progress_func_t,
only after some distraction to let people know that 'svnadmin verify'
hasn't hung. And STAGE isn't needed yet; it's only there for
adding more information in the future.
So I'll go ahead and change STAGE's type to a pointer to an
declare-but-not-defined struct; if we want to define that struct in
1.next, fine. Until then, we pass NULL.
Julian Foad wrote on Thu, Jul 14, 2011 at 14:09:11 +0100:
> Daniel Shahaf wrote:
> > Greg Stein wrote on Thu, Jul 14, 2011 at 00:50:21 -0400:
> > > On Wed, Jul 13, 2011 at 22:39, <da...@apache.org> wrote:
> > > > /**
> > > > + * 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);
>
> Isn't progress notification pretty much standard in GUI environments -
> are there not standard notification paradigms which you could use, which
> have a notion of "how much out of how much in total" and have some
> string parameter to convey "the thing I'm currently processing" and even
> have the notion of stages?
>
> I would expect to see us move towards using a single progress callback
> type throughout Subversion instead of an FS-specific thing.
>
> - Julian
>
> > > > @@ -315,6 +327,12 @@ typedef struct svn_repos_notify_t
> > > >
> > > > + /** 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?
> >
> > Dunno. The idea was to not have to revv the API if we ever decide to
> > add some other checks besides rep-cache.db.
> >
> > Perhaps even a C string instead of either an int (counter) or an
> > enum type (which would be backend and library-version specific).
> >
> > Ideas welcome.
>
>
>
Re: svn commit: r1146547 - in
/subversion/branches/fs-progress/subversion: include/ libsvn_fs/
libsvn_fs_base/ libsvn_fs_fs/ libsvn_repos/ svnadmin/ tests/cmdline/
Posted by Julian Foad <ju...@wandisco.com>.
Daniel Shahaf wrote:
> Greg Stein wrote on Thu, Jul 14, 2011 at 00:50:21 -0400:
> > On Wed, Jul 13, 2011 at 22:39, <da...@apache.org> wrote:
> > > /**
> > > + * 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);
Isn't progress notification pretty much standard in GUI environments -
are there not standard notification paradigms which you could use, which
have a notion of "how much out of how much in total" and have some
string parameter to convey "the thing I'm currently processing" and even
have the notion of stages?
I would expect to see us move towards using a single progress callback
type throughout Subversion instead of an FS-specific thing.
- Julian
> > > @@ -315,6 +327,12 @@ typedef struct svn_repos_notify_t
> > >
> > > + /** 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?
>
> Dunno. The idea was to not have to revv the API if we ever decide to
> add some other checks besides rep-cache.db.
>
> Perhaps even a C string instead of either an int (counter) or an
> enum type (which would be backend and library-version specific).
>
> Ideas welcome.
Re: svn commit: r1146547 - in
/subversion/branches/fs-progress/subversion: include/ libsvn_fs/
libsvn_fs_base/ libsvn_fs_fs/ libsvn_repos/ svnadmin/ tests/cmdline/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Finished applying these in r1147003.
Thanks.
Daniel
Daniel Shahaf wrote on Thu, Jul 14, 2011 at 15:23:10 +0300:
> Greg Stein wrote on Thu, Jul 14, 2011 at 00:50:21 -0400:
> > On Wed, Jul 13, 2011 at 22:39, <da...@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 ?
> >
>
> I just copied them from svn_ra_progress_notify_func_t. Will fix.
>
> > >...
> > > +++ 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"?
> >
>
> Because I haven't decided which way to color the bike shed.
>
> > >...
> > > @@ -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?
> >
>
> Dunno. The idea was to not have to revv the API if we ever decide to
> add some other checks besides rep-cache.db.
>
> Perhaps even a C string instead of either an int (counter) or an
> enum type (which would be backend and library-version specific).
>
> Ideas welcome.
>
> > >...
> > > +++ 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.
> >
>
> Yeah, I wasn't happy with it; the notifications take up more visual
> space than the code. I'll rework it later for simplicity, perhaps along
> the lines you mention.
>
> > >...
> >
> > Cheers,
> > -g
>
> Thanks for the review,
>
Re: svn commit: r1146547 - in
/subversion/branches/fs-progress/subversion: include/ libsvn_fs/
libsvn_fs_base/ libsvn_fs_fs/ libsvn_repos/ svnadmin/ tests/cmdline/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Greg Stein wrote on Thu, Jul 14, 2011 at 00:50:21 -0400:
> On Wed, Jul 13, 2011 at 22:39, <da...@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 ?
>
I just copied them from svn_ra_progress_notify_func_t. Will fix.
> >...
> > +++ 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"?
>
Because I haven't decided which way to color the bike shed.
> >...
> > @@ -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?
>
Dunno. The idea was to not have to revv the API if we ever decide to
add some other checks besides rep-cache.db.
Perhaps even a C string instead of either an int (counter) or an
enum type (which would be backend and library-version specific).
Ideas welcome.
> >...
> > +++ 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.
>
Yeah, I wasn't happy with it; the notifications take up more visual
space than the code. I'll rework it later for simplicity, perhaps along
the lines you mention.
> >...
>
> Cheers,
> -g
Thanks for the review,
Re: svn commit: r1146547 - in /subversion/branches/fs-progress/subversion:
include/ libsvn_fs/ libsvn_fs_base/ libsvn_fs_fs/ libsvn_repos/ svnadmin/ tests/cmdline/
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Jul 13, 2011 at 22:39, <da...@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