You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ben Collins-Sussman <su...@collab.net> on 2002/04/03 21:51:36 UTC
Re: svn commit: rev 1623 - trunk/subversion/include
trunk/subversion/libsvn_client trunk/subversion/clients/cmdline
On Wed, 2002-04-03 at 15:41, Greg Stein wrote:
> On Wed, Apr 03, 2002 at 02:49:50PM -0600, sussman@tigris.org wrote:
> >...
> > Stop using the trace-commit editor in a half-XXXed way. Instead, send
> > detailed feedback to the application that *looks* like the old
> > trace-commit editor output. This should cause our python tests to
> > pass again with the new commit system.
>
> "should" or "do" ? :-)
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.
>
> I presume this was coordinated with Mike?
>
Definitely. :-)
> btw, I *really* like this much better than the trace editor. It is
> definitely much more straight-forward, and (IMO) will be easier for clients
> to use.
Um, sure. See below.
> > + svn_wc_notify_commit_postfix_txdelta,
> > } svn_wc_notify_action_t;
>
> The last enumerated constant in a list cannot have a trailing comma. This
> keeps coming up...
>
> [ there are compilers which don't like it, even tho GCC seems fine ]
Fixed.
>
> >...
> > +++ 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(). 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. :-)
> + if (notify_func)
> > + {
> > + /* Convert an absolute path into a relative one (for feedback.) */
> > + const char *path = item->path->data + (display_dir->len + 1);
> > +
> > + if ((item->state_flags & SVN_CLIENT_COMMIT_ITEM_DELETE)
> > + && (item->state_flags & SVN_CLIENT_COMMIT_ITEM_ADD))
> > + (*notify_func) (notify_baton, svn_wc_notify_commit_replaced, path);
> > +
> > + else if (item->state_flags & SVN_CLIENT_COMMIT_ITEM_DELETE)
> > + (*notify_func) (notify_baton, svn_wc_notify_commit_deleted, path);
> > +
> > + else if (item->state_flags & SVN_CLIENT_COMMIT_ITEM_ADD)
> > + (*notify_func) (notify_baton, svn_wc_notify_commit_added, path);
> > +
> > + 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.
> > static void
> > notify_added (void *baton, const char *path)
> > {
> > /* the pool (BATON) is typically the global pool; don't keep filling it */
> > - apr_pool_t *subpool = svn_pool_create (baton);
> > + struct notify_baton *nb = (struct notify_baton *) baton;
> > + apr_pool_t *subpool = svn_pool_create (nb->pool);
>
> The comment doesn't match the code any more.
Fixed.
>
> [ should I reiterate my position on comments? :-) ]
Don't go there. :-)
> > +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? That seems so hard for me to believe. Our code is full of
batons, and in just about *every* instance, 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?
>
> > + if (nb->sent_first_txdelta == FALSE)
>
> You compare against FALSE, but initialize this field to 0. I'd recommend
> just using the ! operator.
OK.
> > 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? :-)
>
> > +
> > + return (void *)nb;
>
> The cast isn't needed.
Right. Someone else wrote that, I think.
Re: svn commit: rev 1623 - trunk/subversion/include trunk/subversion/libsvn_client trunk/subversion/clients/cmdline
Posted by Greg Stein <gs...@lyra.org>.
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