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
Received on Sun Nov 30 22:35:00 2003