You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Gareth McCaughan <ga...@pobox.com> on 2003/11/01 00:17:16 UTC
[PATCH] Re: "svn revert" performance
Karl wrote:
> Huh, that seems wrong. We ought to sleep once at the end of the
> overall operation, whether or not there were multiple targets.
...
> > So it would seem that the best way to get the behavior you want is to
> > passing the whole target array to the core libraries and iterated over
> > them there instead of in the client program itself. I'm fine with
> > such an API change. Got patch?
>
> That would fix it, yep.
A first pass at a patch is attached (rather than included,
because I have reason to suspect that my MUA sometimes
wraps lines: sorry). Things I'm not entirely happy about:
1. The new svn_client_revert_list function never returns
an error when an entry doesn't exist. That's the behaviour
svn_cl__revert (currently its only caller) wants, but it's
not necessarily what every imaginable caller would want.
Should there be another flag passed in to determine
whether nonexistent entries should be treated just like
other errors?
2. The old svn_cl__revert used to call svn_cl__check_cancel
after each individual reversion. It's not so easy to do that
down in the depths, and in any case there's less need now
that we aren't sleeping for 1s per reverted path. But, still,
it makes me uneasy not to have the check there at all for
an operation that can in principle take an unbounded amount
of time. Should there be a callback or something?
3. Is "svn_client_revert_list" the best name for the new function?
Is it right for both this and the old function that reverts a single
path to continue to exist? (I think the answer to the latter question
is "yes", but am open to persuasion.)
And, as an aside, something puzzles me looking at svn_cl__revert:
it calls svn_cl__get_notifier, but doesn't seem to do anything with
the results. Does svn_cl__get_notifier have some important side
effect? (If so, it should surely be documented.) If not, it looks to me
as if that call can just go away. But I'd be happier if someone wiser
in the ways of Subversion than I could confirm that.
--
Gareth McCaughan
Re: [PATCH] Re: "svn revert" performance
Posted by Gareth McCaughan <ga...@pobox.com>.
On Sunday 09 November 2003 2:52 am, kfogel@collab.net wrote:
> Gareth McCaughan <ga...@pobox.com> writes:
> > I've not had any further feedback, so I assume the patch
> > is utterly perfect and without flaw :-). Um, should I open an
> > issue for this, or what?
>
> Usually the patch manager will do that after waiting for a day or two
> (sorry, haven't had a chance to actually review or apply it myself,
> just been extremely busy with the authz fixes).
No need for apology. That stuff is much more important!
--
g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: "svn revert" performance
Posted by kf...@collab.net.
Gareth McCaughan <ga...@pobox.com> writes:
> I've not had any further feedback, so I assume the patch
> is utterly perfect and without flaw :-). Um, should I open an
> issue for this, or what?
Usually the patch manager will do that after waiting for a day or two
(sorry, haven't had a chance to actually review or apply it myself,
just been extremely busy with the authz fixes).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: "svn revert" performance
Posted by Sander Roobol <ph...@wanadoo.nl>.
On Sat, Nov 08, 2003 at 11:00:41AM +0000, Gareth McCaughan wrote:
> On Monday 03 November 2003 2:00 am, Gareth McCaughan wrote:
> > I wrote:
> > > Anyway... my feelings on this are not strong, and it seems
> > > as if the general preference is for having only the one function
> > > in the API. If I haven't heard any dissenting voices in, say,
> > > a day's time then I'll send in a version of the patch that
> > > has only one function. (Called svn_client_revert, of course.)
> >
> > ... So I'm doing so. Patch and log message are attached.
>
> I've not had any further feedback, so I assume the patch
> is utterly perfect and without flaw :-). Um, should I open an
> issue for this, or what?
Filed as issue 1603:
http://subversion.tigris.org/issues/show_bug.cgi?id=1603
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: "svn revert" performance
Posted by Gareth McCaughan <ga...@pobox.com>.
On Monday 03 November 2003 2:00 am, Gareth McCaughan wrote:
> I wrote:
> > Anyway... my feelings on this are not strong, and it seems
> > as if the general preference is for having only the one function
> > in the API. If I haven't heard any dissenting voices in, say,
> > a day's time then I'll send in a version of the patch that
> > has only one function. (Called svn_client_revert, of course.)
>
> ... So I'm doing so. Patch and log message are attached.
I've not had any further feedback, so I assume the patch
is utterly perfect and without flaw :-). Um, should I open an
issue for this, or what?
--
g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: "svn revert" performance
Posted by Julian Foad <ju...@btopenworld.com>.
Gareth McCaughan wrote:
>
> ... So I'm doing so. Patch and log message are attached.
I don't think there was a later version so I'll review this one for you.
I like the patch in general but have several specific comments on it.
>>I forgot to mention: all the tests pass with the patch applied.
>>This is interesting because there *is* an externally visible
>>change: without --quiet, the way in which nonexistent paths
>>are reported is a bit different from before. I'm guessing that
>>this isn't a problem.
>
> Still true.
How did it and how does it report non-existent paths with and without --quiet? As I point out below, I only get an error.
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 7591)
> +++ subversion/include/svn_client.h (working copy)
> @@ -901,18 +901,27 @@
> apr_pool_t *pool);
>
>
> -/** Restore the pristine version of a working copy @a path, effectively
> - * undoing any local mods. If @a path is a directory, and @a recursive
> - * is @a true, this will be a recursive operation.
> +/** Restore the pristine version of each working copy named in @a paths,
The original comment was meant to be read as "a working copy path (@a path)", so:
"of each working copy path named in @a paths" or
"of each of the working copy @a paths"
> + * effectively undoing any local mods. If any of those paths is a
> + * directory and @a recursive is @a true, it will be reverted
"@c TRUE" or just "true" (I think "@a" is used for arguments and "@c" for other identifiers)
> + * recursively.
> *
> * If @a ctx->notify_func is non-null, then for each item reverted, call
> * @a ctx->notify_func with @a ctx->notify_baton and the path of the reverted
> * item.
> *
> - * If @a path is not found, return the error @c SVN_ERR_ENTRY_NOT_FOUND.
> + * If any path is not found and @a ctx->notify_func is non-null,
> + * then @a ctx->notify_func will be called with @a ctx->notify_baton
> + * and each nonexistent path, with kind @a svn_wc_notify_skip.
"and each" -> "for each" or "on each"
"@c svn_wc_notify_skip"
> + * Regardless of the state of ctx->notify_func, this will not
> + * cause an error to be returned and will not prevent processing
> + * of subsequent paths in the list.
I'm not sure that this is true in practice. When I try "svn revert foo" within a working copy, when "foo" does not exist in Subversion's mind or on disk, I get:
/home/julianfoad/src/subversion/subversion/libsvn_wc/update_editor.c:2716: (apr_err=200005)
svn: Tried a versioning operation on an unversioned resource
svn: 'foo' is not under version control
Have I misunderstood?
> + *
> + * After each path has been processed, @a ctx->cancel_func is called
> + * with @a ctx->cancel_baton provided @a ctx->cancel_func is non-null.
> */
> svn_error_t *
> -svn_client_revert (const char *path,
> +svn_client_revert (apr_array_header_t *paths,
"const apr_array_header_t"
> svn_boolean_t recursive,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
> Index: subversion/libsvn_client/revert.c
> ===================================================================
> --- subversion/libsvn_client/revert.c (revision 7591)
> +++ subversion/libsvn_client/revert.c (working copy)
> @@ -36,11 +36,15 @@
>
> /*** Code. ***/
>
> -svn_error_t *
> -svn_client_revert (const char *path,
> - svn_boolean_t recursive,
> - svn_client_ctx_t *ctx,
> - apr_pool_t *pool)
> +
> +/** The core of svn_client_revert and svn_client_revert_list.
That comment is out of date.
> + * Reverts a single path.
> + */
> +static svn_error_t *
> +revert_1 (const char *path,
I think the project prefers more verbose names, like "revert_single_path" here and "revert_multiple_paths" below.
> + svn_boolean_t recursive,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> {
> svn_wc_adm_access_t *adm_access;
> svn_boolean_t wc_root;
> @@ -107,7 +111,65 @@
> out:
>
> SVN_ERR (svn_wc_adm_close (adm_access));
> + return err;
> +}
>
> +
> +/** Revert a list of paths. The @a pool is private to @a revert_n,
> + * which may clear it at will. It will be destroyed (by a wrapper
> + * function) on exit.
I think the conventions on pool usage are that the function does not own the pool that is passed to it, so it may allocate things in the pool but should not clear the pool. If it needs a private pool it should create one.
> + */
> +static svn_error_t *
> +revert_n (apr_array_header_t *paths,
"revert_multiple_paths"
> + svn_boolean_t recursive,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + int i;
> +
> + for (i = 0; i < paths->nelts; i++)
> + {
> + const char *path = ((const char **) (paths->elts))[i];
> + svn_error_t * err;
> +
> + svn_pool_clear (pool);
> +
> + err = revert_1 (path, recursive, ctx, pool);
> +
> + if (err)
> + {
> + if (err->apr_err != SVN_ERR_ENTRY_NOT_FOUND)
> + return err;
> +
> + if (ctx->notify_func)
> + ctx->notify_func (ctx->notify_baton, path,
> + svn_wc_notify_skip,
> + svn_node_none, NULL,
> + svn_wc_notify_state_missing,
> + svn_wc_notify_state_missing,
> + SVN_INVALID_REVNUM);
> + svn_error_clear (err);
> + }
> +
> + if (ctx->cancel_func)
> + SVN_ERR (ctx->cancel_func (ctx->cancel_baton));
The cancellation callback function is called within revert_1 so there is no need to call it here. (And when it is called, it should be called before doing something rather than after doing it.)
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> +svn_error_t *
> +svn_client_revert (apr_array_header_t *paths,
> + svn_boolean_t recursive,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + apr_pool_t * subpool = svn_pool_create (pool);
> + svn_error_t * err = revert_n (paths, recursive, ctx, subpool);
> +
> + svn_pool_destroy (subpool);
Re. my comment on pool usage above, I think you should move the "create" and "destroy" into the function above.
> +
> /* Sleep to ensure timestamp integrity. */
> svn_sleep_for_timestamps ();
>
> Index: subversion/clients/cmdline/revert-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/revert-cmd.c (revision 7591)
> +++ subversion/clients/cmdline/revert-cmd.c (working copy)
> @@ -44,7 +44,6 @@
> apr_array_header_t *targets;
> int i;
revert-cmd.c:45: warning: unused variable `i'
> svn_boolean_t recursive = opt_state->recursive;
> - apr_pool_t *subpool;
>
> SVN_ERR (svn_opt_args_to_target_array (&targets, os,
> opt_state->targets,
> @@ -60,31 +59,5 @@
> svn_cl__get_notifier (&ctx->notify_func, &ctx->notify_baton, FALSE, FALSE,
> FALSE, pool);
>
> - subpool = svn_pool_create (pool);
> - for (i = 0; i < targets->nelts; i++)
> - {
> - const char *target = ((const char **) (targets->elts))[i];
> - svn_error_t *err;
> -
> - err = svn_client_revert (target, recursive, ctx, subpool);
> - if (err)
> - {
> - if (err->apr_err == SVN_ERR_ENTRY_NOT_FOUND)
> - {
> - if (!opt_state->quiet)
> - {
> - svn_handle_warning (stderr, err);
> - }
> - continue;
> - }
> - else
> - return err;
> - }
> -
> - SVN_ERR (svn_cl__check_cancel (ctx->cancel_baton));
> - svn_pool_clear (subpool);
> - }
> -
> - svn_pool_destroy (subpool);
> - return SVN_NO_ERROR;
> + return svn_client_revert(targets, recursive, ctx, pool);
> }
>
>
> ------------------------------------------------------------------------
>
> Make "svn revert" not sleep for approximately 1 second per
> path named on the command line; instead, do the "sleep for
> timestamp integrity" once at the end of the operation. See
> brief discussion in dev@subversion starting at message 48749.
That's a good log message summary.
>
> * svn_client.h
> (svn_client_revert_list) New function.
>
> * revert.c
> (revert_1) New function, doing what svn_client_revert
> used to do apart from the final sleep.
> (revert_n) New function, calling revert_1 multiple times.
> (svn_client_revert) Eviscerated, so that it now just calls
> revert_1 and then sleeps.
> (svn_client_revert) Now takes a list of paths, calls
> revert_n and then sleeps.
>
> * revert-cmd.c
> (svn_cl__revert) Call the new svn_client_revert once,
> instead of the old one many times.
Some of those descriptions are out of date.
If you are willing to revise this again (I know you have done quite a bit on it already) I'd be glad to review it again. If you're getting tired of it, just say, and I might help out (if someone like Philip or Karl agrees with my comments) :-)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: "svn revert" performance
Posted by Julian Foad <ju...@btopenworld.com>.
On 2003-11-03 02:00, Gareth McCaughan wrote:
>
> ... So I'm doing so. Patch and log message are attached.
Gareth, I have come across the same thing so am interested in your patch. If this message that I am replying to has the latest version of your patch, then please could you update the log message (the one attached corresponds to an earlier version of the patch) and re-send. If this isn't the latest version, could you send or point me to the latest.
Thanks.
- Julian
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 7591)
> +++ subversion/include/svn_client.h (working copy)
> @@ -901,18 +901,27 @@
> apr_pool_t *pool);
>
>
> -/** Restore the pristine version of a working copy @a path, effectively
> - * undoing any local mods. If @a path is a directory, and @a recursive
> - * is @a true, this will be a recursive operation.
> +/** Restore the pristine version of each working copy named in @a paths,
> + * effectively undoing any local mods. If any of those paths is a
> + * directory and @a recursive is @a true, it will be reverted
> + * recursively.
> *
> * If @a ctx->notify_func is non-null, then for each item reverted, call
> * @a ctx->notify_func with @a ctx->notify_baton and the path of the reverted
> * item.
> *
> - * If @a path is not found, return the error @c SVN_ERR_ENTRY_NOT_FOUND.
> + * If any path is not found and @a ctx->notify_func is non-null,
> + * then @a ctx->notify_func will be called with @a ctx->notify_baton
> + * and each nonexistent path, with kind @a svn_wc_notify_skip.
> + * Regardless of the state of ctx->notify_func, this will not
> + * cause an error to be returned and will not prevent processing
> + * of subsequent paths in the list.
> + *
> + * After each path has been processed, @a ctx->cancel_func is called
> + * with @a ctx->cancel_baton provided @a ctx->cancel_func is non-null.
> */
> svn_error_t *
> -svn_client_revert (const char *path,
> +svn_client_revert (apr_array_header_t *paths,
> svn_boolean_t recursive,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
[...]
> ------------------------------------------------------------------------
>
> Make "svn revert" not sleep for approximately 1 second per
> path named on the command line; instead, do the "sleep for
> timestamp integrity" once at the end of the operation. See
> brief discussion in dev@subversion starting at message 48749.
>
> * svn_client.h
> (svn_client_revert_list) New function.
>
> * revert.c
> (revert_1) New function, doing what svn_client_revert
> used to do apart from the final sleep.
> (revert_n) New function, calling revert_1 multiple times.
> (svn_client_revert) Eviscerated, so that it now just calls
> revert_1 and then sleeps.
> (svn_client_revert) Now takes a list of paths, calls
> revert_n and then sleeps.
>
> * revert-cmd.c
> (svn_cl__revert) Call the new svn_client_revert once,
> instead of the old one many times.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] Re: "svn revert" performance
Posted by Gareth McCaughan <ga...@pobox.com>.
I wrote:
> Anyway... my feelings on this are not strong, and it seems
> as if the general preference is for having only the one function
> in the API. If I haven't heard any dissenting voices in, say,
> a day's time then I'll send in a version of the patch that
> has only one function. (Called svn_client_revert, of course.)
... So I'm doing so. Patch and log message are attached.
> I forgot to mention: all the tests pass with the patch applied.
> This is interesting because there *is* an externally visible
> change: without --quiet, the way in which nonexistent paths
> are reported is a bit different from before. I'm guessing that
> this isn't a problem.
Still true.
--
g
Re: [PATCH] Re: "svn revert" performance
Posted by Gareth McCaughan <ga...@pobox.com>.
On Saturday 01 November 2003 4:00 am, Garrett Rooney wrote:
> > 2. The function presently called svn_client_revert_list
> > doesn't report nonexistent-path errors. (Maybe it should
> > do, but the only caller is the commandline client, which
> > ignores them.) It feels wrong to expose *no* version of
> > "revert" that just reports an error when a path doesn't
> > exist.
>
> As Philip said, that could be taken care of by the notification
> callback.
Well, sort of. The notification callback doesn't get to report
any information back to its caller, so the trouble can be
reported but the information can't be used to stop the
reversion. (Well ... you could cheat and have the notification
callback set a variable that the cancel callback checks.
But that's ludicrous.)
Anyway... my feelings on this are not strong, and it seems
as if the general preference is for having only the one function
in the API. If I haven't heard any dissenting voices in, say,
a day's time then I'll send in a version of the patch that
has only one function. (Called svn_client_revert, of course.)
I forgot to mention: all the tests pass with the patch applied.
This is interesting because there *is* an externally visible
change: without --quiet, the way in which nonexistent paths
are reported is a bit different from before. I'm guessing that
this isn't a problem.
Before:
svn: warning: svn_wc_is_wc_root: '<name>' is not a versioned resource
After:
Skipped missing target: <name>
--
g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: "svn revert" performance
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On Oct 31, 2003, at 8:57 PM, Gareth McCaughan wrote:
>> I'm reviewing your comments, not your patch. Just so you know.
>
> OK. I see Philip has taken a look at the patch and found a few
> horrors... :-)
>
>> Hm... I really don't think we need both svn_client_revert() and
>> svn_client_revert_list(). Just make svn_client_revert() *not* take a
>> const char * path, and start taking an apr_array_header_t * targets
>> list.
>
> Two reasons for not doing it that way:
>
> 1. There may be other things out there using the client
> library. It would be better not to break them.
Too bad for them. This is pre-1.0 code, we reserve the right to change
the APIs if necessary up till 1.0. Having two APIs just seem ugly, and
it's better to change now than to have to live with an ugly interface
for the entire 1.0 lifecycle.
> 2. The function presently called svn_client_revert_list
> doesn't report nonexistent-path errors. (Maybe it should
> do, but the only caller is the commandline client, which
> ignores them.) It feels wrong to expose *no* version of
> "revert" that just reports an error when a path doesn't
> exist.
As Philip said, that could be taken care of by the notification
callback.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: "svn revert" performance
Posted by Gareth McCaughan <ga...@pobox.com>.
> I'm reviewing your comments, not your patch. Just so you know.
OK. I see Philip has taken a look at the patch and found a few
horrors... :-)
> Hm... I really don't think we need both svn_client_revert() and
> svn_client_revert_list(). Just make svn_client_revert() *not* take a
> const char * path, and start taking an apr_array_header_t * targets
> list.
Two reasons for not doing it that way:
1. There may be other things out there using the client
library. It would be better not to break them.
2. The function presently called svn_client_revert_list
doesn't report nonexistent-path errors. (Maybe it should
do, but the only caller is the commandline client, which
ignores them.) It feels wrong to expose *no* version of
"revert" that just reports an error when a path doesn't
exist.
> > 2. The old svn_cl__revert used to call svn_cl__check_cancel
> > after each individual reversion. It's not so easy to do that
> > down in the depths, and in any case there's less need now
> > that we aren't sleeping for 1s per reverted path. But, still,
> > it makes me uneasy not to have the check there at all for
> > an operation that can in principle take an unbounded amount
> > of time. Should there be a callback or something?
>
> svn_client_revert() should be handed a 'ctx' client callback back,
> from which it should be able to call ctx->cancel_func(ctx->cancel_baton).
Aha. Philip pointed out the same thing. Will do. My ignorance
is showing here :-).
> > 3. Is "svn_client_revert_list" the best name for the new function?
> > Is it right for both this and the old function that reverts a single
> > path to continue to exist? (I think the answer to the latter question
> > is "yes", but am open to persuasion.)
>
> I answered this already. Also, I'd prefer not to see a function named
> svn_client__revert1() -- first of all, the "1" is just ... ugly. But
> also, if this function is never called from outside of revert.c, we
> need to drop the "svn_client__" prefix (which implies intra-module
> globalness).
OK. More ignorance on my part. I should read the code more
carefully, and then I'd notice what sort of functions have __ and
what don't. If the function survives at all then I'll change its name.
--
g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] Re: "svn revert" performance
Posted by Gareth McCaughan <ga...@pobox.com>.
On Saturday 01 November 2003 1:47 pm, Philip Martin wrote:
> > I was just following what revert-cmd.c does. What's the benefit
> > of clearing at the start of the loop?
>
> See the HACKING file. The project's knowledge about the use of pools
> has developed over time, not all of the existing code follows the best
> practice.
I hadn't read that bit of HACKING (mea culpa), but it still
doesn't explain what the benefit is. Brian observes that if
one uses the "continue" statement then clearing at the
start of the loop body will make sure everything gets freed
each iteration, which seems plausible.
> >> You don't appear to have deleted the subpool variable.
> >
> > Are we reading the same code? :-) The subpool is destroyed
> > immediately on exit from the loop.
>
> You don't appear to have deleted the declaration.
Ohhhhh. So I haven't. My apologies for misinterpreting
your comment.
> > 4. The name "out" for a clean-everything-up-and-return label
> > is taken from the original revert.c code, but I don't like it. I'd
> > prefer "finish" or something. Oh, and I'd like it painted dark
> > green.
>
> I dislike the way svn_client_revert uses labels irrespective of the
> name, it's too easy to introduce a line "SVN_ERR (some_func())" that
> isn't "label aware", witness my first comment above. When there is
> some essential post-processing that must always be done I prefer to
> use a wrapper function, see svn_repos_finish_report in
> libsvn_repos/reporter.c.
Sounds reasonable.
Yet another version of the patch is attached. Changes:
- Delete superfluous subpool variable in rename-cmd.c.
- Factor svn_revert_list into a "core" and a "wrapper"
for more robust handling of errors and subpool.
- The warn_on_nonexistent flag was still there, even
though it wasn't being used. Feh. Now removed.
- I now check for nullity of ctx->cancel_func. At present
the only caller always makes it non-null, but I shouldn't
have been relying on that.
- We now always check for cancellation, even after an
"entry not found" notification. If someone manages to
feed an enormous list of nonexistent paths to "svn revert"
then they'll thank me for this. And it makes the code
a little easier to understand.
- Better commenting.
There are still two public functions (svn_client_revert
and svn_client_revert_list). I'm waiting to see if there are
dissenting opinions before eliminating the former and
renaming the latter.
The function now called revert_1 still uses the "if (err=...) goto out;"
strategy (as it did before I touched the file). It would be possible to
wrap this too, but I'm not sure that doing so really belongs in the
same patch as this one.
--
g
Re: [PATCH] Re: "svn revert" performance
Posted by Brian Denny <br...@briandenny.net>.
On Sat, Nov 01, 2003 at 03:47:28AM +0000, Gareth McCaughan wrote:
>
> I was just following what revert-cmd.c does. What's the benefit
> of clearing at the start of the loop? It replaces a pointless "clear,
> then destroy" at the end with an equally pointless "create, then
> clear" at the start. I don't see any way in which it's more robust.
> Is it just a matter of idiomatic consistency, or is there an advantage
> I'm missing? -- Anyway, even if it's only for the sake of consistency
> I might as well go along :-).
Certain control-flow constructs (e.g. "continue") can result in the
pool not getting cleared, if it's cleared at the end of the loop.
Putting the clear at the beginning avoids that problem.
The specific code you're looking at may not have a "continue" statement,
but that's where the idiomatic consistency comes in. :)
-brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: "svn revert" performance
Posted by Philip Martin <ph...@codematters.co.uk>.
Gareth McCaughan <ga...@pobox.com> writes:
> On Saturday 01 November 2003 1:42 am, Philip Martin wrote:
>
> Yes, that's dumb. Will fix. I was led astray by the fact that the
> original code had a similar "feature": there's a line
>
> SVN_ERR (svn_wc_adm_close (adm_access));
>
> immediately before the sleep.
Our current code has bugs :)
>> Clear pools at the start of the loop.
>
> I was just following what revert-cmd.c does. What's the benefit
> of clearing at the start of the loop?
See the HACKING file. The project's knowledge about the use of pools
has developed over time, not all of the existing code follows the best
practice.
>> You don't appear to have deleted the subpool variable.
>
> Are we reading the same code? :-) The subpool is destroyed
> immediately on exit from the loop.
You don't appear to have deleted the declaration.
> 4. The name "out" for a clean-everything-up-and-return label
> is taken from the original revert.c code, but I don't like it. I'd
> prefer "finish" or something. Oh, and I'd like it painted dark
> green.
I dislike the way svn_client_revert uses labels irrespective of the
name, it's too easy to introduce a line "SVN_ERR (some_func())" that
isn't "label aware", witness my first comment above. When there is
some essential post-processing that must always be done I prefer to
use a wrapper function, see svn_repos_finish_report in
libsvn_repos/reporter.c.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] Re: "svn revert" performance
Posted by Gareth McCaughan <ga...@pobox.com>.
On Saturday 01 November 2003 1:42 am, Philip Martin wrote:
[in reply to my patch]
> > +svn_error_t *
> > +svn_client_revert_list (apr_array_header_t *paths,
> > + svn_boolean_t recursive,
> > + svn_client_ctx_t *ctx,
> > + svn_boolean_t warn_on_nonexistence,
>
> That parameter looks odd, I think the notify callback should make
> that decision.
Yes, it should. When I wrote the code I hadn't even heard of a
notify callback.
> > +svn_error_t *
> > +svn_client_revert (const char *path,
> > + svn_boolean_t recursive,
> > + svn_client_ctx_t *ctx,
> > + apr_pool_t *pool)
> > +{
> > + SVN_ERR (svn_client__revert1 (path, recursive, ctx, pool));
>
> If an error is returned here it will bypass the sleep, but timestamps
> may have been changed.
Yes, that's dumb. Will fix. I was led astray by the fact that the
original code had a similar "feature": there's a line
SVN_ERR (svn_wc_adm_close (adm_access));
immediately before the sleep. Obviously that's insufficient
excuse: for all I know, maybe that's either (1) an existing
bug or (2) safe for some clever reason that doesn't apply
in general.
> The client library should not call svn_handle_warning, it's not much
> use to a GUI application. You should be calling the notify callback
> instead.
Yes, I felt bad about that. I don't know why I failed to
include that in my list of things I wasn't comfortable
about. Will fix.
> Errors that are not returned need to cleared using svn_error_clear.
OK. I plead in mitigation that I was misled by the code in
revert-cmd.c, which didn't bother.
> > + continue;
> > + }
> > + svn_pool_clear (subpool);
>
> Clear pools at the start of the loop.
I was just following what revert-cmd.c does. What's the benefit
of clearing at the start of the loop? It replaces a pointless "clear,
then destroy" at the end with an equally pointless "create, then
clear" at the start. I don't see any way in which it's more robust.
Is it just a matter of idiomatic consistency, or is there an advantage
I'm missing? -- Anyway, even if it's only for the sake of consistency
I might as well go along :-).
> You don't appear to have deleted the subpool variable.
Are we reading the same code? :-) The subpool is destroyed
immediately on exit from the loop.
> > Index: autogen.sh
> > ===================================================================
> > --- autogen.sh (revision 7591)
> > +++ autogen.sh (working copy)
>
> Don't include this file in the diff.
Oops, that was a blunder. Especially as the reason for the change
is a dirty hack to make Subversion compile correctly on my box.
* * *
Attached is another attempt at a patch. I've made the
changes I said I would. I still have separate svn_client_revert
and svn_client_revert_list functions for now.
Things I'm now not entirely happy about:
1. As before: svn_client_revert_list never returns an error for
nonexistent entries. Probably OK, but it seems as if there
maybe should be a way to make it do so.
2. I'm still not sure whether it's right to have both the 1-path
and the multi-path version. Your preference for having only
the latter is noted.
3. I've diddled with the error-handling logic in revert.c. This
doesn't make any change to *what* it does, merely to how
it's organized. The result is nice and concise, but perhaps
some people might find it confusing. I'd be glad of feedback.
4. The name "out" for a clean-everything-up-and-return label
is taken from the original revert.c code, but I don't like it. I'd
prefer "finish" or something. Oh, and I'd like it painted dark
green.
5. Elsewhere in the Subversion source, it seems to be normal
to write "(*ctx->notify_func) (...)", but "ctx->cancel_func (...)".
I can't be consistent with both *and* internally consistent :-).
I've chosen the un-starred form for both, but if this makes
people feel ill then it could easily be changed. I'd like this
bikeshed painted blue.
--
g
Re: [PATCH] Re: "svn revert" performance
Posted by Philip Martin <ph...@codematters.co.uk>.
Gareth McCaughan <ga...@pobox.com> writes:
> +/** Restore the pristine version of each working copy named in @a paths,
> + * effectively undoing any local mods. This is equivalent to calling
> + * svn_client_revert once for each path, except that (1) it may be faster
> + * because it avoids redundant sleeping and (2) nonexistent paths
> + * will not cause an error to be returned -- instead a warning
> + * will be issued via @a svn_handle_warning if @a
> + * warn_on_nonexistence is true, or the problem will be completely
> + * ignored if not.
> + */
> +svn_error_t *
> +svn_client_revert_list (apr_array_header_t *paths,
> + svn_boolean_t recursive,
> + svn_client_ctx_t *ctx,
> + svn_boolean_t warn_on_nonexistence,
That parameter looks odd, I think the notify callback should make
that decision.
> + apr_pool_t *pool);
> +
> +
> /** Remove the 'conflicted' state on a working copy @a path. This will
> * not semantically resolve conflicts; it just allows @a path to be
> * committed in the future. The implementation details are opaque.
> Index: subversion/libsvn_client/revert.c
> ===================================================================
> --- subversion/libsvn_client/revert.c (revision 7591)
> +++ subversion/libsvn_client/revert.c (working copy)
> @@ -36,11 +36,11 @@
>
> /*** Code. ***/
>
> -svn_error_t *
> -svn_client_revert (const char *path,
> - svn_boolean_t recursive,
> - svn_client_ctx_t *ctx,
> - apr_pool_t *pool)
> +static svn_error_t *
> +svn_client__revert1 (const char *path,
> + svn_boolean_t recursive,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> {
> svn_wc_adm_access_t *adm_access;
> svn_boolean_t wc_root;
> @@ -107,9 +107,52 @@
> out:
>
> SVN_ERR (svn_wc_adm_close (adm_access));
> + return err;
> +}
>
> +svn_error_t *
> +svn_client_revert (const char *path,
> + svn_boolean_t recursive,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + SVN_ERR (svn_client__revert1 (path, recursive, ctx, pool));
If an error is returned here it will bypass the sleep, but timestamps
may have been changed.
> +
> /* Sleep to ensure timestamp integrity. */
> svn_sleep_for_timestamps ();
> + return SVN_NO_ERROR;
> +}
>
> - return err;
> +svn_error_t *
> +svn_client_revert_list (apr_array_header_t *paths,
> + svn_boolean_t recursive,
> + svn_client_ctx_t *ctx,
> + svn_boolean_t warn_on_nonexistence,
> + apr_pool_t *pool)
> +{
> + int i;
> + apr_pool_t *subpool = svn_pool_create (pool);
> +
> + for (i = 0; i < paths->nelts; i++)
> + {
> + const char *path = ((const char **) (paths->elts))[i];
> + svn_error_t *err;
> +
> + err = svn_client__revert1 (path, recursive, ctx, subpool);
> + if (err)
> + {
> + if (err->apr_err != SVN_ERR_ENTRY_NOT_FOUND)
> + return err;
> + if (warn_on_nonexistence)
> + svn_handle_warning (stderr, err);
The client library should not call svn_handle_warning, it's not much
use to a GUI application. You should be calling the notify callback
instead.
Errors that are not returned need to cleared using svn_error_clear.
> + continue;
> + }
> + svn_pool_clear (subpool);
Clear pools at the start of the loop.
> + }
> +
> + svn_pool_destroy (subpool);
> +
> + /* Sleep to ensure timestamp integrity. */
> + svn_sleep_for_timestamps ();
> + return SVN_NO_ERROR;
> }
> Index: subversion/clients/cmdline/revert-cmd.c
> ===================================================================
> --- subversion/clients/cmdline/revert-cmd.c (revision 7591)
> +++ subversion/clients/cmdline/revert-cmd.c (working copy)
> @@ -60,31 +60,6 @@
> svn_cl__get_notifier (&ctx->notify_func, &ctx->notify_baton, FALSE, FALSE,
> FALSE, pool);
>
> - subpool = svn_pool_create (pool);
You don't appear to have deleted the subpool variable.
> - for (i = 0; i < targets->nelts; i++)
> - {
> - const char *target = ((const char **) (targets->elts))[i];
> - svn_error_t *err;
> -
> - err = svn_client_revert (target, recursive, ctx, subpool);
> - if (err)
> - {
> - if (err->apr_err == SVN_ERR_ENTRY_NOT_FOUND)
> - {
> - if (!opt_state->quiet)
> - {
> - svn_handle_warning (stderr, err);
> - }
> - continue;
> - }
> - else
> - return err;
> - }
> -
> - SVN_ERR (svn_cl__check_cancel (ctx->cancel_baton));
> - svn_pool_clear (subpool);
> - }
> -
> - svn_pool_destroy (subpool);
> - return SVN_NO_ERROR;
> + return svn_client_revert_list(targets, recursive, ctx,
> + !opt_state->quiet, pool);
> }
> Index: autogen.sh
> ===================================================================
> --- autogen.sh (revision 7591)
> +++ autogen.sh (working copy)
Don't include this file in the diff.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: "svn revert" performance
Posted by "C. Michael Pilato" <cm...@collab.net>.
Gareth McCaughan <ga...@pobox.com> writes:
> A first pass at a patch is attached (rather than included,
> because I have reason to suspect that my MUA sometimes
> wraps lines: sorry). Things I'm not entirely happy about:
I'm reviewing your comments, not your patch. Just so you know.
> 1. The new svn_client_revert_list function never returns
> an error when an entry doesn't exist. That's the behaviour
> svn_cl__revert (currently its only caller) wants, but it's
> not necessarily what every imaginable caller would want.
> Should there be another flag passed in to determine
> whether nonexistent entries should be treated just like
> other errors?
Hm... I really don't think we need both svn_client_revert() and
svn_client_revert_list(). Just make svn_client_revert() *not* take a
const char * path, and start taking an apr_array_header_t * targets
list.
> 2. The old svn_cl__revert used to call svn_cl__check_cancel
> after each individual reversion. It's not so easy to do that
> down in the depths, and in any case there's less need now
> that we aren't sleeping for 1s per reverted path. But, still,
> it makes me uneasy not to have the check there at all for
> an operation that can in principle take an unbounded amount
> of time. Should there be a callback or something?
svn_client_revert() should be handed a 'ctx' client callback back,
from which it should be able to call ctx->cancel_func(ctx->cancel_baton).
> 3. Is "svn_client_revert_list" the best name for the new function?
> Is it right for both this and the old function that reverts a single
> path to continue to exist? (I think the answer to the latter question
> is "yes", but am open to persuasion.)
I answered this already. Also, I'd prefer not to see a function named
svn_client__revert1() -- first of all, the "1" is just ... ugly. But
also, if this function is never called from outside of revert.c, we
need to drop the "svn_client__" prefix (which implies intra-module
globalness).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org