You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Garrett Rooney <ro...@electricjellyfish.net> on 2003/02/16 07:12:14 UTC
[PATCH] cancellation, take one
ok, i was bored this weekend and tired of real work, so i decided to
start on the cancellation stuff earlier than i had originally planned.
here's a proof of concept patch that adds a cancellation editor, which
can be wrapped around any other editor to insert calls to the client
provided cancellation callback function to see if we should stop the
editor's drive.
so far i've only tried this with repos to repos diffs, but it seems to
work ok for that. the usual warnings, relatively untested code, could
eat your dog and email your homework to your worst enemy...
i haven't even put this through make check yet, but i just wanted to
get people's opinions. does this seem like a reasonable approach?
something that just occurred to me is that
svn_cancel_get_cancellation_editor should be smarter, it can check to
see if the cancellation function is NULL, and if so just return the
underlying editor it was passed, with no extra cancellation editor
layered on top. that would also remove the checks in this patch that
ensure the cancellation function is non-null before calling it.
-garrett
Re: [PATCH] cancellation, take one
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2003-02-17 at 21:56, Garrett Rooney wrote:
> i don't have any real problem with moving the code in there, other than
> it being kind of odd to have something associated with 'cancelation' in
> the svn_delta namespace. it just seems more natural for it to be
> called svn_cancel_get_cancellation_editor than
> svn_delta_get_cancellation_editor.
Although we must rigidly adhere to the svn_* prefix on our symbols in
general, libsvn_delta does not deal exclusively with symbols named
svn_delta_*. Observe that most of the symbols in svn_delta.h actually
have the prefix svn_txdelta.
On the other hand, the XML editor was named svn_delta_get_xml_editor()
when we had it, and the name svn_cancel_get_cancellation_editor() seems
dreadfully redundant, so I would favor
svn_delta_get_cancellation_editor() anyway.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cancellation, take one
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On Monday, February 17, 2003, at 09:40 PM, Greg Hudson wrote:
> On Mon, 2003-02-17 at 20:32, Garrett Rooney wrote:
>> well, in that case, if it could potentially be a problem, we don't
>> *have* to use svn_delta_default_editor, we can just as easily declare
>> the editor structure statically in cancel.c and return a pointer to
>> it.
>> any objections to that?
>
> Although there are no immediately identifiable practical problems with
> having libsvn_subr depend on data structures in libsvn_delta, it's
> still
> a very bad practice in my mind.
>
> And as I previously said, libsvn_delta is a fine place for the
> cancellation editor to live. There's no reason to create circles in
> our
> library MDD for this feature.
i don't have any real problem with moving the code in there, other than
it being kind of odd to have something associated with 'cancelation' in
the svn_delta namespace. it just seems more natural for it to be
called svn_cancel_get_cancellation_editor than
svn_delta_get_cancellation_editor.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cancellation, take one
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2003-02-17 at 20:32, Garrett Rooney wrote:
> well, in that case, if it could potentially be a problem, we don't
> *have* to use svn_delta_default_editor, we can just as easily declare
> the editor structure statically in cancel.c and return a pointer to it.
> any objections to that?
Although there are no immediately identifiable practical problems with
having libsvn_subr depend on data structures in libsvn_delta, it's still
a very bad practice in my mind.
And as I previously said, libsvn_delta is a fine place for the
cancellation editor to live. There's no reason to create circles in our
library MDD for this feature.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cancellation, take one
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On Monday, February 17, 2003, at 06:57 PM, Greg Hudson wrote:
> On Mon, 2003-02-17 at 17:42, Branko Čibej wrote:
>> There's absolutely nothing wrong with that kind of dependencies. Any
>> normal linker can resolve them; at worst, you'll get one of those
>> files
>> on the link line twice.
>
> Not so.
>
> In the Unix world, with shared libraries, circular dependencies are not
> generally an issue because the entire library is linked in regardless
> of
> what symbols are called for. (Though IRIX manages to produce link
> failures anyway, presumably through misguided heroic efforts.) With
> static libraries, you have to resolve symbols after they are used
> because only the necessary objects are pulled in at link time.
>
> No problem, you say, we'll just link in -lsvn_subr -lsvn_delta
> -lsvn_subr? That doesn't play nice with modern libtool. In an effort
> to avoid the "three million -lz" problem, modern libtool collapses
> redundant library dependencies, under the assumption (perfectly valid
> in
> ~100% of all real-world packages) that there are no circular
> dependencies in the mix. There is a command-line option to prevent
> this
> problem, but I don't believe there's a way to tell that a particular
> pair of libraries depend on each other.
well, in that case, if it could potentially be a problem, we don't
*have* to use svn_delta_default_editor, we can just as easily declare
the editor structure statically in cancel.c and return a pointer to it.
any objections to that?
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cancellation, take one
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2003-02-17 at 17:42, Branko Čibej wrote:
> There's absolutely nothing wrong with that kind of dependencies. Any
> normal linker can resolve them; at worst, you'll get one of those files
> on the link line twice.
Not so.
In the Unix world, with shared libraries, circular dependencies are not
generally an issue because the entire library is linked in regardless of
what symbols are called for. (Though IRIX manages to produce link
failures anyway, presumably through misguided heroic efforts.) With
static libraries, you have to resolve symbols after they are used
because only the necessary objects are pulled in at link time.
No problem, you say, we'll just link in -lsvn_subr -lsvn_delta
-lsvn_subr? That doesn't play nice with modern libtool. In an effort
to avoid the "three million -lz" problem, modern libtool collapses
redundant library dependencies, under the assumption (perfectly valid in
~100% of all real-world packages) that there are no circular
dependencies in the mix. There is a command-line option to prevent this
problem, but I don't believe there's a way to tell that a particular
pair of libraries depend on each other.
> All our copyright headers are the same in all files, for good reason --
> they're updated by a script.
It's not clear to me that this is really acceptable, but I doubt it will
ever matter in practice.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cancellation, take one
Posted by Branko Čibej <br...@xbc.nu>.
Philip Martin wrote:
>Garrett Rooney <ro...@electricjellyfish.net> writes:
>
>
>
>>* subversion/libsvn_subr/cancel.c: new file, implements the
>>* cancellation editor.
>>
>>
>
>I'm a little worried about the organisation of this stuff. The
>cancellation editor is an svn_delta_editor_t editor but you have
>implemented it in libsvn_subr. Doesn't that introduce a cyclic
>dependency? You have libsvn_subr call svn_delta_default_editor but
>libsvn_delta already uses libsvn_subr routines.
>
There's absolutely nothing wrong with that kind of dependencies. Any
normal linker can resolve them; at worst, you'll get one of those files
on the link line twice.
>>Index: subversion/include/svn_cancel.h
>>===================================================================
>>--- subversion/include/svn_cancel.h (working copy)
>>+++ subversion/include/svn_cancel.h (working copy)
>>@@ -0,0 +1,51 @@
>>+/**
>>+ * @copyright
>>+ * ====================================================================
>>+ * Copyright (c) 2000-2003 CollabNet. All rights reserved.
>>
>>
>
>2000-2003? Should a new file use that or just 2003?
>
All our copyright headers are the same in all files, for good reason --
they're updated by a script.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cancellation, take one
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2003-02-17 at 13:08, Garrett Rooney wrote:
> i was wondering about that myself... perhaps
> svn_cancel_get_cancelation_editor should move into libsvn_delta
> somewhere? what do people think?
I think the XML editor also used to live in libsvn_delta, so that's a
fine place for the cancellation editor to live.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cancellation, take one
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On Monday, February 17, 2003, at 01:03 PM, Philip Martin wrote:
> Garrett Rooney <ro...@electricjellyfish.net> writes:
>
>> * subversion/libsvn_subr/cancel.c: new file, implements the
>> * cancellation editor.
>
> I'm a little worried about the organisation of this stuff. The
> cancellation editor is an svn_delta_editor_t editor but you have
> implemented it in libsvn_subr. Doesn't that introduce a cyclic
> dependency? You have libsvn_subr call svn_delta_default_editor but
> libsvn_delta already uses libsvn_subr routines.
i was wondering about that myself... perhaps
svn_cancel_get_cancelation_editor should move into libsvn_delta
somewhere? what do people think?
thanks for the comments on main.c... that part of the patch hadn't
been polished up yet, since i hadn't planned on committing it until
more of this stuff had been done.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cancellation, take one
Posted by Philip Martin <ph...@codematters.co.uk>.
Garrett Rooney <ro...@electricjellyfish.net> writes:
> * subversion/libsvn_subr/cancel.c: new file, implements the
> * cancellation editor.
I'm a little worried about the organisation of this stuff. The
cancellation editor is an svn_delta_editor_t editor but you have
implemented it in libsvn_subr. Doesn't that introduce a cyclic
dependency? You have libsvn_subr call svn_delta_default_editor but
libsvn_delta already uses libsvn_subr routines.
> Index: subversion/include/svn_cancel.h
> ===================================================================
> --- subversion/include/svn_cancel.h (working copy)
> +++ subversion/include/svn_cancel.h (working copy)
> @@ -0,0 +1,51 @@
> +/**
> + * @copyright
> + * ====================================================================
> + * Copyright (c) 2000-2003 CollabNet. All rights reserved.
2000-2003? Should a new file use that or just 2003?
> + *
> + * This software is licensed as described in the file COPYING, which
> + * you should have received as part of this distribution. The terms
> + * are also available at http://subversion.tigris.org/license-1.html.
> + * If newer versions of this license are posted there, you may use a
> + * newer version instead, at your option.
> + *
> + * This software consists of voluntary contributions made by many
> + * individuals. For exact contribution history, see the revision
> + * history and logs, available at http://subversion.tigris.org/.
> + * ====================================================================
> + * @endcopyright
> + *
> + * @file svn_cancel.h
> + * @brief Routines to support cancelation of running subversion functions.
> + */
> +
> +#include "svn_delta.h"
> +
> +#ifndef SVN_CANCEL_H
> +#define SVN_CANCEL_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +/** A user defined callback that subversion will call with a user defined
> + * baton to see if the current operation should be continued. If the operation
> + * should continue, the function should return @c SVN_NO_ERROR, if not, it
> + * should return @c SVN_ERR_CANCELLED.
> + */
> +typedef svn_error_t *(*svn_cancel_func_t) (void *cancel_baton);
> +
> +svn_error_t *
> +svn_cancel_get_cancellation_editor (svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + const svn_delta_editor_t *wrapped_editor,
> + void *wrapped_baton,
> + const svn_delta_editor_t **editor,
> + void **edit_baton,
> + apr_pool_t *pool);
> +
> +#ifdef __cplusplus
> +}
> +#endif /* __cplusplus */
> +
> +#endif /* SVN_CANCEL_H */
> Index: subversion/clients/cmdline/main.c
> ===================================================================
> --- subversion/clients/cmdline/main.c (revision 4912)
> +++ subversion/clients/cmdline/main.c (working copy)
> @@ -29,6 +29,7 @@
> #include <apr_tables.h>
> #include <apr_general.h>
> #include <apr_lib.h>
> +#include <apr_signal.h>
>
> #include "svn_pools.h"
> #include "svn_wc.h"
> @@ -506,7 +507,23 @@
> { NULL, NULL, {0}, NULL, {0} }
> };
>
/* Flag used by the SIGINT signal handler */
> +static volatile sig_atomic_t interrupted = 0;
>
/* A SIGINT signal handler */
> +static void
> +sig_int (int unused)
> +{
> + interrupted = 1;
> +}
> +
/* A cancellation editor cancel callback */
> +static svn_error_t *
> +check_cancel (void *baton)
> +{
> + if (interrupted)
> + return svn_error_create (SVN_ERR_CANCELLED, NULL, "caught SIGINT");
> + else
> + return SVN_NO_ERROR;
> +}
> +
>
> /*** Main. ***/
>
> @@ -945,6 +962,10 @@
> ctx.log_msg_baton = svn_cl__make_log_msg_baton (&opt_state, NULL,
> ctx.config, pool);
>
> + apr_signal (SIGINT, sig_int);
> +
> + ctx.cancel_func = check_cancel;
> +
> err = (*subcommand->cmd_func) (os, &command_baton, pool);
> if (err)
> {
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cancellation, take one
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On Sunday, February 16, 2003, at 02:12 AM, Garrett Rooney wrote:
> i haven't even put this through make check yet, but i just wanted to
> get people's opinions. does this seem like a reasonable approach?
> something that just occurred to me is that
> svn_cancel_get_cancellation_editor should be smarter, it can check to
> see if the cancellation function is NULL, and if so just return the
> underlying editor it was passed, with no extra cancellation editor
> layered on top. that would also remove the checks in this patch that
> ensure the cancellation function is non-null before calling it.
ok, here's a revised copy of this patch.
it implements the cancellation editor and assuming that a cancellation
callback is specified in the client context, it uses it for 'svn diff'.
this allows (as far as i can tell) us to cleanly cancel out of 'svn
diff', which is less than entirely useful, but makes a decent proof of
concept.
if nobody has any objections, i'd like to commit this today, minus the
changes in clients/cmdline/main.c, so it won't actually have any effect
for users (i'd rather not remove their ability to control-c out of long
running operations until the cancellation editor is wrapped around more
of our functionality).
-garrett
* subversion/include/svn_error_codes.h
(SVN_ERR_CANCELLED): new error code to signify that an operation has
been
cancelled.
* subversion/include/svn_cancel.h: new file, holds prototypes for
cancellation
stuff.
* subversion/include/svn_wc.h
(svn_wc_get_diff_editor): add cancellation function/baton arguments.
* subversion/include/svn_client.h
(svn_client_ctx_t): add cancellation function/baton members.
* subversion/libsvn_wc/diff.c
(svn_wc_get_diff_editor): add cancellation function/baton, wrap our
editor in
a cancellation editor.
* subversion/libsvn_subr/cancel.c: new file, implements the
cancellation editor.
* subversion/libsvn_client/diff.c
(do_diff): pass cancellation function/baton into
svn_client__get_diff_editor
and svn_wc_get_diff_editor.
(do_merge): pass cancellation function/baton into
svn_client__get_diff_editor.
* subversion/libsvn_client/repos_diff.c
(svn_client__get_diff_editor): add cancellation function/baton
arguments,
wrap our editor in a cancellation editor.
* subversion/libsvn_client/client.h
(svn_client__get_diff_editor): add cancellation function/baton
arguments.
* subversion/clients/cmdline/main.c
(sig_int): signal handler for cancellation.
(check_cancel): cancellation callback function.
(main): set up signal handler for SIGINT, add check_cancel to our
client
context.