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.
Received on Wed Apr 3 23:52:52 2002