You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Peter N. Lundblad" <pe...@famlundblad.se> on 2005/02/28 22:03:52 UTC
Notification API
Hi,
We need to revise the notification API, i.e. svn_wc_notify_func_t for the,
guess what, locking branch. We want to notify users when locks get
defunct during an update. We can't add another action, since this can
happen at the same time as text changes was updated or something. We
shouldn't call the notify function more than once per path.
It is not like this is just three or four calls... There are plenty of
these calls all over the place.
I've got a feeling that we might want to extend this again in the future.
To make it more extensible, without having to revise it again, I propose:
- Instead of just adding another argument, move all arguments to a struct.
Pass a pointer to that struct to the new notify function.
- REquire that this struct is created bya an svn_wc_notify_args_create (or
some name), that initialize everything to default arguments.
Then we can add fields in the future without having to revise again.
Also, svn_client_ctx_t is extensible by design. It is possible to add the
new notify function/baton as new fields. svn_client_create_context could
init them to a function/baton that forwards to the old function. The other
alternative is to revise svn_client_ctx_t, leading to revision of all
client functions. Which do you prefer as the least ugly way? Or do I miss
an obvious other alternative?
Regards,
//Peter the API revisor:-(
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Notification API
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 28 Feb 2005, Greg Hudson wrote:
> On Mon, 2005-02-28 at 17:03, Peter N. Lundblad wrote:
> > - REquire that this struct is created bya an svn_wc_notify_args_create (or
> > some name), that initialize everything to default arguments.
>
> Who are we imposing this requirement on? I don't think callers invoke
> notification callbacks.
>
On our selves.
> If the requirement is on us, the Subversion libraries, then
> svn_wc_notify_args_create or whatever does not need to be part of the
> API.
>
Well, at least libsvn_client will have to create this struct, since it
calls the notify function in a few places. (For some strange reason, this
API is in svn_wc.h; I don't plan to change that.)
> > Then we can add fields in the future without having
to revise > > again.
>
> If these fields are for the benefit of callers, how will they know when
> they can safely use new fields?
>
The point of having a creation function that allocates the struct in a
pool is so it can initialize all fields to a known value (NULL,
SVN_INVALID_REVNUM, etc.) It is the same thinking that motivated
svn_client_create_context, but to make life easier for ourselves the next
time we want to add a field to this new struct.
Hope this clarifies,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Notification API
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2005-02-28 at 17:03, Peter N. Lundblad wrote:
> - REquire that this struct is created bya an svn_wc_notify_args_create (or
> some name), that initialize everything to default arguments.
Who are we imposing this requirement on? I don't think callers invoke
notification callbacks.
If the requirement is on us, the Subversion libraries, then
svn_wc_notify_args_create or whatever does not need to be part of the
API.
> Then we can add fields in the future without having to revise
> again.
If these fields are for the benefit of callers, how will they know when
they can safely use new fields?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Notification API
Posted by "Brian W. Fitzpatrick" <fi...@collab.net>.
On Wed, 2005-03-02 at 11:11 -0600, kfogel@collab.net wrote:
> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
> > What you asked for, Sir. A patch is below with this, obviously incomplete,
> > just for illustration.
> >
> > the idea is that svn_client_create_context set the new func/baton to
> > something that will forward to the old deprecated stuff. If the user sets
> > the old func/baton, she will get notification as before. If she set the
> > new function, the old stuff will not be called, but that shouldn't be a
> > problem.
>
> Thank you. I don't know why I didn't see that myself, it makes
> perfect sense now. +1.
Same here... up until now, for some reason, I was thinking you proposed
an opaque baton. +1
-Fitz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Notification API
Posted by kf...@collab.net.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> What you asked for, Sir. A patch is below with this, obviously incomplete,
> just for illustration.
>
> the idea is that svn_client_create_context set the new func/baton to
> something that will forward to the old deprecated stuff. If the user sets
> the old func/baton, she will get notification as before. If she set the
> new function, the old stuff will not be called, but that shouldn't be a
> problem.
Thank you. I don't know why I didn't see that myself, it makes
perfect sense now. +1.
-Karl
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 13218)
> +++ subversion/include/svn_wc.h (arbetskopia)
> @@ -564,6 +564,26 @@
> } svn_wc_notify_state_t;
>
>
> +/** @since New in 1.2. */
> +typedef struct svn_wc_notify_t {
> + svn_wc_notify_action_t action;
> + svn_node_kind_t kind;
> + const char *mime_type;
> + svn_wc_notify_state_t content_state;
> + svn_wc_notify_state_t prop_state;
> + svn_revnum_t revision;
> +} svn_wc_notify_t;
> +
> +/** @since New in 1.2. */
> +svn_wc_notify_t *
> +svn_wc_create_notify (apr_pool_t *pool);
> +
> +/** @since New in 1.2. */
> +typedef void (*svn_wc_notify_func2_t) (void *baton,
> + const char *path,
> + const svn_wc_notify_t *notify,
> + apr_pool_t *pool);
> +
> /** Notify the world that @a action has happened to @a path. @a path is
> * either absolute or relative to cwd (i.e., not relative to an anchor).
> *
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 13218)
> +++ subversion/include/svn_client.h (arbetskopia)
> @@ -360,10 +360,12 @@
> /** main authentication baton. */
> svn_auth_baton_t *auth_baton;
>
> - /** notification callback function */
> + /** @deprecated Provided for backwards compatibility with the 1.1 API.
> + notification callback function.
> + This will be called by @c notify_func2 by default. */
> svn_wc_notify_func_t notify_func;
>
> - /** notification callback baton */
> + /** notification callback baton for the above */
> void *notify_baton;
>
> /** log message callback function */
> @@ -386,6 +388,11 @@
> /** a baton to pass to the cancellation callback. */
> void *cancel_baton;
>
> + /** notification function, defaulting to a function that fowrads
> + to @c notify_func. */
> + svn_wc_notify_func2_t notify_func2;
> + /** notification baton for the above. */
> + void *notify_baton2;
> } svn_client_ctx_t;
>
>
> Index: subversion/libsvn_client/ctx.c
> ===================================================================
> --- subversion/libsvn_client/ctx.c (revision 13218)
> +++ subversion/libsvn_client/ctx.c (arbetskopia)
> @@ -29,10 +29,25 @@
>
> /*** Code. ***/
>
> +/* Call the notify_func of the context provided by BATON, if non-NULL. */
> +static void
> +call_notify_func (void *baton, const char *path, const svn_wc_notify_t *n,
> + apr_pool_t *pool)
> +{
> + const svn_client_ctx_t *ctx = baton;
> +
> + if (ctx->notify_func)
> + ctx->notify_func (ctx->notify_baton, path, n->action, n->kind,
> + n->mime_type, n->content_state, n->prop_state,
> + n->revision);
> +}
> +
> svn_error_t *
> svn_client_create_context (svn_client_ctx_t **ctx,
> apr_pool_t *pool)
> {
> *ctx = apr_pcalloc (pool, sizeof (svn_client_ctx_t));
> + (*ctx)->notify_func2 = call_notify_func;
> + (*ctx)->notify_baton2 = *ctx;
> return SVN_NO_ERROR;
> }
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Notification API
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Wed, 2 Mar 2005 kfogel@collab.net wrote:
> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
> > > > Also, svn_client_ctx_t is extensible by design. It is possible
> > > > to add the new notify function/baton as new
> > > > fields. svn_client_create_context could init them to a
> > > > function/baton that forwards to the old function. The other
> > > > alternative is to revise svn_client_ctx_t, leading to revision
> > > > of all client functions. Which do you prefer as the least ugly
> > > > way? Or do I miss an obvious other alternative?
> >
> > I still want an opinion about the above paragraph.
>
> So the idea is to put
>
> svn_wc_notify_func_t2 notify_func2;
> void *notify_baton2;
>
> at the end of svn_client_ctx_t? But I'm unclear on exactly how
> svn_client_create_context is supposed forward after that. That
> function doesn't have a handle on the actual notify_func and baton the
> caller is going to put in the ctx, so how can it set up the call
> forwarding? All svn_client_create_context does is allocate the
> structure; it doesn't currently initialize any fields.
>
What you asked for, Sir. A patch is below with this, obviously incomplete,
just for illustration.
the idea is that svn_client_create_context set the new func/baton to
something that will forward to the old deprecated stuff. If the user sets
the old func/baton, she will get notification as before. If she set the
new function, the old stuff will not be called, but that shouldn't be a
problem.
Thanks,
//Peter
Index: subversion/include/svn_wc.h
===================================================================
--- subversion/include/svn_wc.h (revision 13218)
+++ subversion/include/svn_wc.h (arbetskopia)
@@ -564,6 +564,26 @@
} svn_wc_notify_state_t;
+/** @since New in 1.2. */
+typedef struct svn_wc_notify_t {
+ svn_wc_notify_action_t action;
+ svn_node_kind_t kind;
+ const char *mime_type;
+ svn_wc_notify_state_t content_state;
+ svn_wc_notify_state_t prop_state;
+ svn_revnum_t revision;
+} svn_wc_notify_t;
+
+/** @since New in 1.2. */
+svn_wc_notify_t *
+svn_wc_create_notify (apr_pool_t *pool);
+
+/** @since New in 1.2. */
+typedef void (*svn_wc_notify_func2_t) (void *baton,
+ const char *path,
+ const svn_wc_notify_t *notify,
+ apr_pool_t *pool);
+
/** Notify the world that @a action has happened to @a path. @a path is
* either absolute or relative to cwd (i.e., not relative to an anchor).
*
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 13218)
+++ subversion/include/svn_client.h (arbetskopia)
@@ -360,10 +360,12 @@
/** main authentication baton. */
svn_auth_baton_t *auth_baton;
- /** notification callback function */
+ /** @deprecated Provided for backwards compatibility with the 1.1 API.
+ notification callback function.
+ This will be called by @c notify_func2 by default. */
svn_wc_notify_func_t notify_func;
- /** notification callback baton */
+ /** notification callback baton for the above */
void *notify_baton;
/** log message callback function */
@@ -386,6 +388,11 @@
/** a baton to pass to the cancellation callback. */
void *cancel_baton;
+ /** notification function, defaulting to a function that fowrads
+ to @c notify_func. */
+ svn_wc_notify_func2_t notify_func2;
+ /** notification baton for the above. */
+ void *notify_baton2;
} svn_client_ctx_t;
Index: subversion/libsvn_client/ctx.c
===================================================================
--- subversion/libsvn_client/ctx.c (revision 13218)
+++ subversion/libsvn_client/ctx.c (arbetskopia)
@@ -29,10 +29,25 @@
/*** Code. ***/
+/* Call the notify_func of the context provided by BATON, if non-NULL. */
+static void
+call_notify_func (void *baton, const char *path, const svn_wc_notify_t *n,
+ apr_pool_t *pool)
+{
+ const svn_client_ctx_t *ctx = baton;
+
+ if (ctx->notify_func)
+ ctx->notify_func (ctx->notify_baton, path, n->action, n->kind,
+ n->mime_type, n->content_state, n->prop_state,
+ n->revision);
+}
+
svn_error_t *
svn_client_create_context (svn_client_ctx_t **ctx,
apr_pool_t *pool)
{
*ctx = apr_pcalloc (pool, sizeof (svn_client_ctx_t));
+ (*ctx)->notify_func2 = call_notify_func;
+ (*ctx)->notify_baton2 = *ctx;
return SVN_NO_ERROR;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Notification API
Posted by kf...@collab.net.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> > > Also, svn_client_ctx_t is extensible by design. It is possible
> > > to add the new notify function/baton as new
> > > fields. svn_client_create_context could init them to a
> > > function/baton that forwards to the old function. The other
> > > alternative is to revise svn_client_ctx_t, leading to revision
> > > of all client functions. Which do you prefer as the least ugly
> > > way? Or do I miss an obvious other alternative?
>
> I still want an opinion about the above paragraph.
So the idea is to put
svn_wc_notify_func_t2 notify_func2;
void *notify_baton2;
at the end of svn_client_ctx_t? But I'm unclear on exactly how
svn_client_create_context is supposed forward after that. That
function doesn't have a handle on the actual notify_func and baton the
caller is going to put in the ctx, so how can it set up the call
forwarding? All svn_client_create_context does is allocate the
structure; it doesn't currently initialize any fields.
I guess I'm not sure exactly what you're proposing. Could you
describe it in more detail, or in patch form?
Thanks,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Notification API
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 28 Feb 2005, Brian W. Fitzpatrick wrote:
> On Mon, 2005-02-28 at 23:03 +0100, Peter N. Lundblad wrote:
> > I've got a feeling that we might want to extend this again in the future.
> > To make it more extensible, without having to revise it again, I propose:
> > - Instead of just adding another argument, move all arguments to a struct.
> > Pass a pointer to that struct to the new notify function.
> > - REquire that this struct is created bya an svn_wc_notify_args_create (or
> > some name), that initialize everything to default arguments.
> >
> > Then we can add fields in the future without having to revise again.
> >
> > Also, svn_client_ctx_t is extensible by design. It is possible to add the
> > new notify function/baton as new fields. svn_client_create_context could
> > init them to a function/baton that forwards to the old function. The other
> > alternative is to revise svn_client_ctx_t, leading to revision of all
> > client functions. Which do you prefer as the least ugly way? Or do I miss
> > an obvious other alternative?
>
I still want an opinion about the above paragraph.
> *sigh* I'm OK with this... I just went through the same thing you're
> going through as I'm making svn_client_lock and svn_ra_lock take
> multiple targets, but I punted and went the route of creating an
> svn_wc_lock_info_func_t with an opaque baton as I'm just not up for the
Since we have to revise the notify API anyway, I think this
svn_wc_lock_info_func_t should go again.
> API juggling that you seem so good at. :-)
>
Since you really needed it first (but shamelessly worked around it:-), I
offer you the opportunity to be the one hero who did this...
> Anyway, +1.
>
Thank you for the support...
Seriously, I'll do it if you don't. It will take some days, though since
my time is limited currently.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Notification API
Posted by "Brian W. Fitzpatrick" <fi...@collab.net>.
On Mon, 2005-02-28 at 23:03 +0100, Peter N. Lundblad wrote:
> Hi,
>
> We need to revise the notification API, i.e. svn_wc_notify_func_t for the,
> guess what, locking branch. We want to notify users when locks get
> defunct during an update. We can't add another action, since this can
> happen at the same time as text changes was updated or something. We
> shouldn't call the notify function more than once per path.
>
> It is not like this is just three or four calls... There are plenty of
> these calls all over the place.
>
> I've got a feeling that we might want to extend this again in the future.
> To make it more extensible, without having to revise it again, I propose:
> - Instead of just adding another argument, move all arguments to a struct.
> Pass a pointer to that struct to the new notify function.
> - REquire that this struct is created bya an svn_wc_notify_args_create (or
> some name), that initialize everything to default arguments.
>
> Then we can add fields in the future without having to revise again.
>
> Also, svn_client_ctx_t is extensible by design. It is possible to add the
> new notify function/baton as new fields. svn_client_create_context could
> init them to a function/baton that forwards to the old function. The other
> alternative is to revise svn_client_ctx_t, leading to revision of all
> client functions. Which do you prefer as the least ugly way? Or do I miss
> an obvious other alternative?
*sigh* I'm OK with this... I just went through the same thing you're
going through as I'm making svn_client_lock and svn_ra_lock take
multiple targets, but I punted and went the route of creating an
svn_wc_lock_info_func_t with an opaque baton as I'm just not up for the
API juggling that you seem so good at. :-)
Anyway, +1.
-Fitz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org