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