On Wed, Apr 03, 2002 at 03:51:36PM -0600, Ben Collins-Sussman wrote:
>...
> See my last mail. Many commits tests still fail, but for *real* reasons
> now, no longer because the output is wonky. Now Cmike can start fixing
> bugs.
Yup. Saw it after sending mine :-)
>...
> > > +++ trunk/subversion/include/svn_client.h Wed Apr 3 14:49:50 2002
> > > @@ -463,6 +463,10 @@
> > > AFTER_EDIT_BATON are pre- and post-commit hook editors. They are
> > > optional; pass four NULLs here if you don't need them.
> > >
> > > + Additionally, NOTIFY_FUNC/BATON will be called as the commit
> > > + progresses, as a way of describing actions to the application
> > > + layer.
> >
> > Is it reasonable now to remove the before/after editor concept [from across
> > the interface] ? i.e. stop composing editors and shift to a pure
> > notification system?
>
> Definitely, they're no longer strictly necessary. I mean, our
> application layer is no longer passing any before- or after- editors
> into svn_client_commit().
I'm not talking about *just* commit, but the whole API. I see these other
functions use before/after editors:
- checkout: passed a trace_update editor
- update: passed a trace_update editor
- switch: passed a trace_update editor
- import: passed a trace_commit editor
- commit: not used, per your change
- copy: passed a trace_update editor
Looks like only after-editors, also.
> Maybe some future client *might* want to do
> that, dunno. Maybe a GUI client wants a write a trace-commit-editor
> that draws pretty animations. :-)
I don't think we should add features for "maybe somebody would want to". My
favorite quote here is, "just because we *can*, doesn't mean we *should*."
I'd much rather remove all the before_editor stuff -- it seems that concept
has never been used, so its basis in any future reality is in question.
Then, I'd like to see if the after editors are needed. If they are replaced
by the notification callback, then I'd also like to toss the after editors.
>...
> > > + else if ((item->state_flags & SVN_CLIENT_COMMIT_ITEM_TEXT_MODS)
> > > + || (item->state_flags & SVN_CLIENT_COMMIT_ITEM_PROP_MODS))
> > > + (*notify_func) (notify_baton, svn_wc_notify_commit_modified, path);
> >
> > This sequence of else/if makes it look like the "state_flags" should
> > actually be an enumerated constant, rather than bit fields.
>
> Hmm, yeah. We should direct this design question to Cmike... I'm not
> too familiar with this new commit system yet.
Right. Mike?
>...
> > > +static void
> > > +notify_commit_postfix_txdelta (void *baton,
> > > + const char *path)
> > > +{
> > > + struct notify_baton *nb = (struct notify_baton *) baton;
> >
> > Note that the cast here and above aren't needed. Casting from 'void *' to
> > 'struct notify_baton *' is automatically done by the compiler.
>
> Really?
Definitely.
> That seems so hard for me to believe. Our code is full of
> batons, and in just about *every* instance,
Not *every*. Just the ones you wrote ;-) [well, and some other people]
I never put those casts in, and I've removed a number of them, too, as I run
across them.
> we explicitly cast the void * back into a real type. Is this a stylistic
> wrongness? Or should we just continue using that style for clarity's sake?
I'd call it stylistic, so I don't aggressively remove them. However, I'd
prefer that we remove them because, one day, it would be nice to search for
all casts and double-check they should be there. In many cases, casts point
to problems, so removing them from the code would reduce the "noise".
>...
> > > void *svn_cl__make_notify_baton (apr_pool_t *pool)
> > > {
> > > - return (void *)pool;
> > > + struct notify_baton *nb = apr_pcalloc (pool, sizeof(*nb));
> > > +
> > > + nb->pool = pool;
> > > + nb->sent_first_txdelta = 0;
> >
> > If you're using pcalloc(), there isn't a need to set to zero. Alternatively,
> > keep the assignment, but use palloc().
>
> Does it hurt to be paranoid-ly redundant? :-)
Not in this case. It gets called once. Just pointing it out.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Apr 4 00:37:00 2002