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

Re: svn commit: r1146547 - in /subversion/branches/fs-progress/subversion: include/ libsvn_fs/ libsvn_fs_base/ libsvn_fs_fs/ libsvn_repos/ svnadmin/ tests/cmdline/

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Fri, 15 Jul 2011 11:02:54 +0300

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, <danielsh_at_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,
>
Received on 2011-07-15 10:04:59 CEST

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