philip@tigris.org writes:
> Log:
> Issue 1831, make libsvn_client's update function accept multiple targets.
>
> * subversion/include/svn_client.h
> (svn_client_update2): New.
> (svn_client_update): Deprecated.
>
> * subversion/libsvn_client/update.c
> (svn_client_update2): New.
>
> * subversion/clients/cmdline/update-cmd.c
> (svn_cl_update): Use svn_client_update2.
>
> * subversion/tests/clients/cmdline/basic_tests.py
> (basic_update): Add test of skipping unversioned targets.
>
>
> --- trunk/subversion/clients/cmdline/update-cmd.c (original)
> +++ trunk/subversion/clients/cmdline/update-cmd.c Mon Dec 20 17:36:16 2004
> @@ -40,9 +40,6 @@
> svn_cl__opt_state_t *opt_state = ((svn_cl__cmd_baton_t *) baton)->opt_state;
> svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
> apr_array_header_t *targets;
> - apr_array_header_t *condensed_targets;
> - apr_pool_t *subpool = svn_pool_create (pool);
> - int i;
>
> SVN_ERR (svn_opt_args_to_target_array2 (&targets, os,
> opt_state->targets, pool));
> @@ -50,43 +47,14 @@
> /* Add "." if user passed 0 arguments */
> svn_opt_push_implicit_dot_target (targets, pool);
>
> - /* Remove redundancies from the target list while preserving order. */
> - SVN_ERR (svn_path_remove_redundancies (&condensed_targets,
> - targets,
> - pool));
> + if (! opt_state->quiet)
> + svn_cl__get_notifier (&ctx->notify_func, &ctx->notify_baton,
> + FALSE, FALSE, FALSE, pool);
>
> - for (i = 0; i < condensed_targets->nelts; i++)
> - {
> - const char *target = ((const char **) (condensed_targets->elts))[i];
> - svn_error_t *err;
> -
> - svn_pool_clear (subpool);
> - SVN_ERR (svn_cl__check_cancel (ctx->cancel_baton));
> -
> - if (! opt_state->quiet)
> - svn_cl__get_notifier (&ctx->notify_func, &ctx->notify_baton,
> - FALSE, FALSE, FALSE, subpool);
> -
> - err = svn_client_update (NULL, target,
> + SVN_ERR (svn_client_update2 (NULL, targets,
> &(opt_state->start_revision),
> opt_state->nonrecursive ? FALSE : TRUE,
> - ctx, subpool);
> - if (err)
> - {
> - if (err->apr_err == SVN_ERR_ENTRY_NOT_FOUND)
> - {
> - if (!opt_state->quiet)
> - {
> - svn_handle_warning (stderr, err);
> - }
> - svn_error_clear (err);
> - continue;
> - }
> - else
> - return err;
> - }
> - }
> + ctx, pool));
>
> - svn_pool_destroy (subpool);
> return SVN_NO_ERROR;
> }
It looks like we've lost the warning behavior (the ENTRY_NOT_FOUND
case above), because we don't test 'err' on every iteration of the
loop anymore.
We can't call svn_handle_warning() inside svn_client_update2(),
obviously. I see that the new code uses the notifier func to warn
about unversioned items (see later comments regarding that), but the
new behavior is surprising:
$ touch unversioned_but_existing
$ ls unversioned_but_existing
unversioned_but_existing
$ ls unversioned_and_nonexistent
ls: unversioned_and_nonexistent: No such file or directory
$ svn st -v README COPYING
12471 12286 maxb README
12471 8016 kfogel COPYING
$
$ ### Okay, try to update some targets explicitly. ###
$ svn up README unversioned_but_existing COPYING unversioned_and_nonexistent
At revision 12471.
At revision 12471.
At revision 12471.
At revision 12471.
$
That's rather odd :-). Even unversioned files -- both existing and
totally vaporous ones! -- report being at r12471.
> --- trunk/subversion/include/svn_client.h (original)
> +++ trunk/subversion/include/svn_client.h Mon Dec 20 17:36:16 2004
> @@ -461,23 +461,49 @@
> apr_pool_t *pool);
>
>
> -/** Update working tree @a path to @a revision, authenticating with
> - * the authentication baton cached in @a ctx. If @a result_rev is not
> - * @c NULL, set @a *result_rev to the value of the revision to which
> - * the working copy was actually updated.
> +/**
> + * @since New in 1.2.
> + *
> + * Update working trees @a paths to @a revision, authenticating with the
> + * authentication baton cached in @a ctx. @a paths is an array of const
> + * char * paths to be updated. Unversioned paths that are direct children
> + * of a versioned path will cause an update that attempts to add that path,
> + * other unversioned paths are skipped. If @a result_revs is not
> + * @c NULL an array of svn_revnum_t will be returned with each element set
> + * to the value of the actual revision to which the corresponding path was
> + * updated.
When you say "actual revision" there, do you mean last committed rev
for that path, or the revision to which the '*revision' parameter was
resolved? (I'm pretty sure the latter, but maybe the doc string
should be explicit about it.)
> * @a revision must be of kind @c svn_opt_revision_number,
> * @c svn_opt_revision_head, or @c svn_opt_revision_date. If @a
> * revision does not meet these requirements, return the error
> * @c SVN_ERR_CLIENT_BAD_REVISION.
> *
> - * If @a ctx->notify_func is non-null, invoke @a ctx->notify_func with
> - * @a ctx->notify_baton for each item handled by the update, and also for
> - * files restored from text-base.
> + * The paths in @a paths can be from multiple working copies from multiple
> + * repositories, but even if they all come from the same repository there
> + * is no guarantee that revision represented by @c svn_opt_revision_head
> + * will remain the same as each path is updated.
In issue #1831, Stefan Kueng wrote:
> When implementing svn_client_update2() which takes an array of
> targets to update, I suggest also that the function first checks
> (by comparing the repository uuids of each target) if the targets
> are from the same repository. And if so, first get the HEAD
> revision and then update to that specific revision. Otherwise,
> you could end up with the following scenario:
>
> svn up file1 file2 file3 file4
> at revision 100
> at revision 100
> //now here someone else finishes a commit
> at revision 101
> at revision 101
Is this something you consider undesirable, or is it just an
enhancement to be left for later?
> - * If @a path is not found, return the error @c SVN_ERR_ENTRY_NOT_FOUND.
> + * If @a ctx->notify_func is non-null, invoke @a ctx->notify_func with
> + * @a ctx->notify_baton for each item handled by the update, and also for
> + * files restored from text-base. If @a ctx->cancel_func is non-null, invoke
> + * it passing @a ctx->cancel_baton at various places during the update.
> *
> * Use @a pool for any temporary allocation.
> + */
> +svn_error_t *
> +svn_client_update2 (apr_array_header_t **result_revs,
> + const apr_array_header_t *paths,
> + const svn_opt_revision_t *revision,
> + svn_boolean_t recurse,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool);
> +
> +/**
> + * @deprecated Provided for backward compatibility with the 1.1 API.
> + *
> + * Similar to svn_client_update2 except that it accepts only a single
> + * target in @a path and returns a single revision if @a result_rev is not
> + * NULL.
> */
> svn_error_t *
> svn_client_update (svn_revnum_t *result_rev,
>
> --- trunk/subversion/libsvn_client/update.c (original)
> +++ trunk/subversion/libsvn_client/update.c Mon Dec 20 17:36:16 2004
> @@ -30,6 +30,7 @@
> #include "svn_config.h"
> #include "svn_time.h"
> #include "svn_path.h"
> +#include "svn_pools.h"
> #include "client.h"
>
> #include "svn_private_config.h"
> @@ -183,6 +184,58 @@
> *result_rev = revnum;
>
> return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_client_update2 (apr_array_header_t **result_revs,
> + const apr_array_header_t *paths,
> + const svn_opt_revision_t *revision,
> + svn_boolean_t recurse,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + int i;
> + svn_error_t *err = SVN_NO_ERROR;
> + apr_pool_t *subpool = svn_pool_create (pool);
> +
> + if (result_revs)
> + *result_revs = apr_array_make (pool, paths->nelts, sizeof (svn_revnum_t));
> +
> + for (i = 0; i < paths->nelts; ++i)
> + {
> + svn_boolean_t sleep;
> + svn_revnum_t result_rev;
> + const char *path = APR_ARRAY_IDX (paths, i, const char *);
> +
> + svn_pool_clear (subpool);
> +
> + if (ctx->cancel_func && (err = ctx->cancel_func (ctx->cancel_baton)))
> + break;
> +
> + err = svn_client__update_internal (&result_rev, path, revision,
> + recurse, &sleep, ctx, subpool);
> + if (err && err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)
> + return err;
> + else if (err)
> + {
> + svn_error_clear (err);
> + err = SVN_NO_ERROR;
> + result_rev = SVN_INVALID_REVNUM;
> + if (ctx->notify_func)
> + (*ctx->notify_func) (ctx->notify_baton, path,
> + svn_wc_notify_skip, svn_node_unknown,
> + NULL, svn_wc_notify_state_unknown,
> + svn_wc_notify_state_unknown,
> + SVN_INVALID_REVNUM);
> + }
Do we really want to clear all errors besides WC_NOT_DIRECTORY here?
I would expect the reverse logic: clear a certain specific set of
errors -- those that are legitimate reasons for skipping a file, such
as that the item is unversioned -- and return all other errors.
Even if we want the update to continue, and do the best it can despite
errors, wouldn't it be good to save up the errors and return them at
the end somehow? Just tossing them seems reckless.
Regarding notification: if we examined the type of error, we could use
it to drive a more informative notification. For example, would we
want to use slightly different notifications for files that exist but
are not versioned, versus those that don't exist and are unversioned?
We would currently have to overload 'svn_wc_notify_state_unknown' for
that, but I wouldn't mind adding another notification code in order to
distinguish them.
Or maybe it's okay to simply use the same 'unknown' code for both. My
main concern is that, currently, we have no idea what the error was,
and neither does the user.
In any case, looking at the above code, and comparing with the test
results I quoted earlier, I'm baffled as to how it could be reporting
"At revision 12471" for the unversioned targets. It should give a
"Skipped" notification, right?
Unrelated to the above, a minor comment: Mike Pilato has suggested
that whenever any block of a chained conditional requires curly
braces, they should all get curlies. Thus:
if (err && err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)
{
return err;
}
else if (err)
{
svn_error_clear (err);
err = SVN_NO_ERROR;
result_rev = SVN_INVALID_REVNUM;
if (ctx->notify_func)
(*ctx->notify_func) (ctx->notify_baton, path,
svn_wc_notify_skip, svn_node_unknown,
NULL, svn_wc_notify_state_unknown,
svn_wc_notify_state_unknown,
SVN_INVALID_REVNUM);
}
I too find that more readable. However, if you prefer the way it is,
that's the end of the matter as far as I'm concerned. This particular
style question is worth exactly as much space I've devoted to it, up
to and including the end of this sentence, and not a bit more :-).
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Dec 21 02:37:10 2004