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
Received on Tue Dec 21 03:36:42 2004