> 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