You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Eric Gillespie <ep...@pretzelnet.org> on 2008/04/02 17:53:39 UTC
[PATCH] Cancellation editor discards abort_edit
[[[
The cancellation editor has been discarding the wrapped editor's
abort_edit callback, and all cancellation-wrapped editors have
been using the default editor's no-op abort_edit.
* subversion/libsvn_delta/cancel.c
(abort_edit): Add new wrapper.
(svn_delta_get_cancellation_editor): Set abort_edit.
]]]
Index: subversion/libsvn_delta/cancel.c
===================================================================
--- subversion/libsvn_delta/cancel.c (revision 30158)
+++ subversion/libsvn_delta/cancel.c (working copy)
@@ -336,6 +336,19 @@ close_edit(void *edit_baton,
return SVN_NO_ERROR;
}
+static svn_error_t *
+abort_edit(void *edit_baton,
+ apr_pool_t *pool)
+{
+ struct edit_baton *eb = edit_baton;
+
+ SVN_ERR(eb->cancel_func(eb->cancel_baton));
+
+ SVN_ERR(eb->wrapped_editor->abort_edit(eb->wrapped_edit_baton, pool));
+
+ return SVN_NO_ERROR;
+}
+
svn_error_t *
svn_delta_get_cancellation_editor(svn_cancel_func_t cancel_func,
void *cancel_baton,
@@ -365,6 +378,7 @@ svn_delta_get_cancellation_editor(svn_cancel_func_
tree_editor->close_file = close_file;
tree_editor->absent_file = absent_file;
tree_editor->close_edit = close_edit;
+ tree_editor->abort_edit = abort_edit;
eb->wrapped_editor = wrapped_editor;
eb->wrapped_edit_baton = wrapped_edit_baton;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Cancellation editor discards abort_edit
Posted by Karl Fogel <kf...@red-bean.com>.
Eric Gillespie <ep...@pretzelnet.org> writes:
>> Yeah, I believe that you *can* add new callbacks; you just can't add
>> new callbacks that must ~always have a non-default implementation.
>
> I'm not sure how well that's going to work out in practice,
> especially with something like the editor interface, but OK. Is
> this what you had in mind?
>
> Index: svn_delta.h
> ===================================================================
> --- svn_delta.h (revision 30158)
> +++ svn_delta.h (working copy)
> @@ -922,6 +922,8 @@
> svn_error_t *(*abort_edit)(void *edit_baton,
> apr_pool_t *pool);
>
> + /* Be sure to update svn_delta_get_cancellation_editor and
> + * svn_delta_default_editor if you add a new callback here. */
> } svn_delta_editor_t;
Yes, with "()"s. Also, we should add something to the main doc string
indicating that one should never allocate these things oneself.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Cancellation editor discards abort_edit
Posted by Eric Gillespie <ep...@pretzelnet.org>.
"David Glasser" <gl...@davidglasser.net> writes:
> On Wed, Apr 2, 2008 at 6:53 PM, Karl Fogel <kf...@red-bean.com> wrote:
> > Hmm. I thought we deliberately designed all our structures now to take
> > additions at the end, so that old code referring into them would never
> > miss. In fact, I thought this was one reason we insist that
> > 'svn_delta_editor_t' be allocated by 'svn_delta_default_editor() instead
> > of by apr_p[c]alloc() ? (We seem to have neglected to document that,
> > but I remember it being The Plan.)
> >
> > This strategy is obvious with opaque structures, of course, but I
> > thought we were doing it for some non-opaque ones too...
>
> Yeah, I believe that you *can* add new callbacks; you just can't add
> new callbacks that must ~always have a non-default implementation.
I'm not sure how well that's going to work out in practice,
especially with something like the editor interface, but OK. Is
this what you had in mind?
Index: svn_delta.h
===================================================================
--- svn_delta.h (revision 30158)
+++ svn_delta.h (working copy)
@@ -922,6 +922,8 @@
svn_error_t *(*abort_edit)(void *edit_baton,
apr_pool_t *pool);
+ /* Be sure to update svn_delta_get_cancellation_editor and
+ * svn_delta_default_editor if you add a new callback here. */
} svn_delta_editor_t;
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Cancellation editor discards abort_edit
Posted by David Glasser <gl...@davidglasser.net>.
On Wed, Apr 2, 2008 at 6:53 PM, Karl Fogel <kf...@red-bean.com> wrote:
> Eric Gillespie <ep...@pretzelnet.org> writes:
> > I suppose you meant "svn_delta_default_editor and
> > svn_delta_get_cancellation_editor". But, I'm not sure it helps;
> > you can't add new editor callbacks, you have to create a revved
> > editor interface, complete with new default/cancellation/god only
> > knows what else.
>
> Hmm. I thought we deliberately designed all our structures now to take
> additions at the end, so that old code referring into them would never
> miss. In fact, I thought this was one reason we insist that
> 'svn_delta_editor_t' be allocated by 'svn_delta_default_editor() instead
> of by apr_p[c]alloc() ? (We seem to have neglected to document that,
> but I remember it being The Plan.)
>
> This strategy is obvious with opaque structures, of course, but I
> thought we were doing it for some non-opaque ones too...
Yeah, I believe that you *can* add new callbacks; you just can't add
new callbacks that must ~always have a non-default implementation.
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Cancellation editor discards abort_edit
Posted by Karl Fogel <kf...@red-bean.com>.
Eric Gillespie <ep...@pretzelnet.org> writes:
> I suppose you meant "svn_delta_default_editor and
> svn_delta_get_cancellation_editor". But, I'm not sure it helps;
> you can't add new editor callbacks, you have to create a revved
> editor interface, complete with new default/cancellation/god only
> knows what else.
Hmm. I thought we deliberately designed all our structures now to take
additions at the end, so that old code referring into them would never
miss. In fact, I thought this was one reason we insist that
'svn_delta_editor_t' be allocated by 'svn_delta_default_editor() instead
of by apr_p[c]alloc() ? (We seem to have neglected to document that,
but I remember it being The Plan.)
This strategy is obvious with opaque structures, of course, but I
thought we were doing it for some non-opaque ones too...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Cancellation editor discards abort_edit
Posted by Eric Gillespie <ep...@pretzelnet.org>.
Karl Fogel <kf...@red-bean.com> writes:
> My goodness. +1 to commit that. I'd suggest adding a note to the end
> of svn_delta.h:svn_delta_editor_t, too, saying that if you add a new
> callback there, you have to remember to implement it in the editor
> returned by svn_delta_default_editor().
I suppose you meant "svn_delta_default_editor and
svn_delta_get_cancellation_editor". But, I'm not sure it helps;
you can't add new editor callbacks, you have to create a revved
editor interface, complete with new default/cancellation/god only
knows what else.
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Cancellation editor discards abort_edit
Posted by Karl Fogel <kf...@red-bean.com>.
Eric Gillespie <ep...@pretzelnet.org> writes:
> [[[
> The cancellation editor has been discarding the wrapped editor's
> abort_edit callback, and all cancellation-wrapped editors have
> been using the default editor's no-op abort_edit.
>
> * subversion/libsvn_delta/cancel.c
> (abort_edit): Add new wrapper.
> (svn_delta_get_cancellation_editor): Set abort_edit.
> ]]]
My goodness. +1 to commit that. I'd suggest adding a note to the end
of svn_delta.h:svn_delta_editor_t, too, saying that if you add a new
callback there, you have to remember to implement it in the editor
returned by svn_delta_default_editor().
-K
> Index: subversion/libsvn_delta/cancel.c
> ===================================================================
> --- subversion/libsvn_delta/cancel.c (revision 30158)
> +++ subversion/libsvn_delta/cancel.c (working copy)
> @@ -336,6 +336,19 @@ close_edit(void *edit_baton,
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *
> +abort_edit(void *edit_baton,
> + apr_pool_t *pool)
> +{
> + struct edit_baton *eb = edit_baton;
> +
> + SVN_ERR(eb->cancel_func(eb->cancel_baton));
> +
> + SVN_ERR(eb->wrapped_editor->abort_edit(eb->wrapped_edit_baton, pool));
> +
> + return SVN_NO_ERROR;
> +}
> +
> svn_error_t *
> svn_delta_get_cancellation_editor(svn_cancel_func_t cancel_func,
> void *cancel_baton,
> @@ -365,6 +378,7 @@ svn_delta_get_cancellation_editor(svn_cancel_func_
> tree_editor->close_file = close_file;
> tree_editor->absent_file = absent_file;
> tree_editor->close_edit = close_edit;
> + tree_editor->abort_edit = abort_edit;
>
> eb->wrapped_editor = wrapped_editor;
> eb->wrapped_edit_baton = wrapped_edit_baton;
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org