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

Re: [PATCH] Re: "svn revert" performance

From: Gareth McCaughan <gareth.mccaughan_at_pobox.com>
Date: 2003-11-01 02:57:41 CET

> 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
Received on Sat Nov 1 02:58:24 2003

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