You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2004/12/21 01:33:58 UTC
Re: svn commit: r12465 - in trunk/subversion: clients/cmdline include libsvn_client tests/clients/cmdline
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
Re: svn commit: r12465 - in trunk/subversion: clients/cmdline include libsvn_client tests/clients/cmdline
Posted by kf...@collab.net.
Philip Martin <ph...@codematters.co.uk> writes:
> Look again, the logic is the other way round, it only clears
> WC_NOT_DIRECTORY.
Ack, sorry about that.
> > 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.
>
> :)
Uh, yes... "<grin>" :-).
> > 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?
>
> It's in the docstring. versioned/unversioned doesn't skip, you get a
> skip with unversioned/unversioned.
>
> I did expect versioned/unversioned to give an error if unversioned
> didn't get added, but that's not the way the update editor works. As
> far as I know I didn't change anything here.
Okay. I had thought the old current was different, but I didn't have
an old client to test with and didn't have time to build one. I'll
try when I go home, before I update there.
> I don't really care, but given that you misuderstood what I wrote I'll
> change it and add a comment :)
Oh, I'm perfectly capable of misunderstanding it the new way too,
don't change it on that account :-).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12465 - in trunk/subversion: clients/cmdline
include libsvn_client tests/clients/cmdline
Posted by Philip Martin <ph...@codematters.co.uk>.
kfogel@collab.net writes:
> 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.
There is an equivalent test in the library. It does however raise an
interesting point. There was no regression test for ENTRY_NOT_FOUND
and svn_client_update didn't return it, the old code didn't work, I
suspect it might have been my svn_wc_adm_open_anchor change. I
suppose this counts as an ABI change, perhaps I should make the
deprecated svn_client_update convert WC_NOT_DIRECTORY into
ENTRY_NOT_FOUND.
> 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.
That's a side effect of code that supports
svn up versioned/unversioned_and_nonexistent
which is can be used to add unversioned_and_nonexistent if it has been
recently added to the repository. As far as I know I didn't really
change this behaviour, other than to move the notification from the
client into the library. Once again, it's possible that the earlier
svn_wc_adm_open_anchor change may have altered some behaviour that was
not regression tested.
>
>> --- 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.)
The latter. Note, the previous docstring refered to "the revision to
which the working copy was actually updated", I didn't really change
anything.
>> * @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?
It's tricky to implement. The client can do it if required. I left
the issue open.
>> + 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?
Look again, the logic is the other way round, it only clears
WC_NOT_DIRECTORY.
> 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.
:)
> 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?
It's in the docstring. versioned/unversioned doesn't skip, you get a
skip with unversioned/unversioned.
I did expect versioned/unversioned to give an error if unversioned
didn't get added, but that's not the way the update editor works. As
far as I know I didn't change anything here.
> 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 :-).
I don't really care, but given that you misuderstood what I wrote I'll
change it and add a comment :)
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org