[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

[PATCH] Re: "svn revert" performance

From: Gareth McCaughan <gareth.mccaughan_at_pobox.com>
Date: 2003-11-01 04:47:28 CET

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Received on Sat Nov 1 04:48:19 2003

This is an archived mail posted to the Subversion Dev mailing list.