On Wed, Apr 20, 2011 at 9:50 AM, Julian Foad <julian.foad_at_wandisco.com> wrote:
>> Author: hwright
>> Date: Tue Apr 19 20:33:21 2011
>> New Revision: 1095195
>>
>> URL: http://svn.apache.org/viewvc?rev=1095195&view=rev
>> Log:
>> Make the libsvn_client propset API operate on an array of targets, rather than
>> just one.
>
> Nice change.
>
> The doc string looks like it could do with a re-write, partly because of
> the multiple-targets aspect, and partly to clarify things that weren't
> well described before. How about this?
Looks good.
>
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 1095347)
> +++ subversion/include/svn_client.h (working copy)
> @@ -4115,25 +4115,52 @@ svn_client_move(svn_client_commit_info_t
>
>
> /**
> - * Set @a propname to @a propval on @a targets.
> + * Set @a propname to @a propval on each (const char *) target in @a
> + * targets. The targets must be either all working copy paths or all URLs.
> * A @a propval of @c NULL will delete the property.
> *
> - * If @a depth is #svn_depth_empty, set the property on each member of
> - * @a targets only; if #svn_depth_files, set it on @a targets and their file
> - * children (if any); if #svn_depth_immediates, on @a targets and all
> - * of their immediate children (both files and directories); if
> - * #svn_depth_infinity, on @a targets and everything beneath them.
> - *
> - * Targets must either be all working copy paths or URLs. The @a targets may
> - * only be an URL if @a base_revision_for_url is not
> - * #SVN_INVALID_REVNUM; in this case, the property will only be set
> - * if it has not changed since revision @a base_revision_for_url.
> - * @a base_revision_for_url must be #SVN_INVALID_REVNUM if @a targets
> - * are not URLs. @a depth deeper than #svn_depth_empty is not
> - * supported on URLs. The authentication baton in @a ctx and @a
> - * ctx->log_msg_func3/@a ctx->log_msg_baton3 will be used to
> - * immediately attempt to commit the property change in the
> - * repository.
> + * If @a targets are URLs:
> + *
> + * Immediately attempt to commit the property change in the repository,
> + * using the authentication baton in @a ctx and @a
> + * ctx->log_msg_func3/@a ctx->log_msg_baton3.
> + *
> + * ### Currently a separate commit for each target. TODO: single commit.
> + *
> + * If the property has changed on any target since revision
> + * @a base_revision_for_url (which must not be #SVN_INVALID_REVNUM), no
> + * change will be made and an error will be returned.
> + *
> + * ### Currently processes targets in order, committing if successful,
> + * stopping when one hits this error. TODO: commit all or none.
> + *
> + * If non-NULL, @a revprop_table is a hash table holding additional,
> + * custom revision properties (<tt>const char *</tt> names mapped to
> + * <tt>svn_string_t *</tt> values) to be set on the new revision. This
> + * table cannot contain any standard Subversion properties.
> + *
> + * If @a commit_callback is non-NULL, then for each successful commit,
> + * call @a commit_callback with @a commit_baton and a #svn_commit_info_t
> + * for the commit.
> + *
> + * @a depth must be #svn_depth_empty. @a changelists is ignored.
> + *
> + * If @a targets are working copy paths:
> + *
> + * If @a depth is #svn_depth_empty, set the property on each member of
> + * @a targets only; if #svn_depth_files, set it on @a targets and their
> + * file children (if any); if #svn_depth_immediates, on @a targets and all
> + * of their immediate children (both files and directories); if
> + * #svn_depth_infinity, on @a targets and everything beneath them.
> + *
> + * @a changelists is an array of <tt>const char *</tt> changelist
> + * names, used as a restrictive filter on items whose properties are
> + * set; that is, don't set properties on any item unless it's a member
> + * of one of those changelists. If @a changelists is empty (or
> + * altogether @c NULL), no changelist filtering occurs.
> + *
> + * @a base_revision_for_url must be #SVN_INVALID_REVNUM. @a revprop_table,
> + * @a commit_callback and @a commit_baton are ignored.
> *
> * If @a propname is an svn-controlled property (i.e. prefixed with
> * #SVN_PROP_PREFIX), then the caller is responsible for ensuring that
> @@ -4146,25 +4173,9 @@ svn_client_move(svn_client_commit_info_t
> * #SVN_ERR_BAD_MIME_TYPE (if @a propname is "svn:mime-type", but @a
> * propval is not a valid mime-type).
> *
> - * @a changelists is an array of <tt>const char *</tt> changelist
> - * names, used as a restrictive filter on items whose properties are
> - * set; that is, don't set properties on any item unless it's a member
> - * of one of those changelists. If @a changelists is empty (or
> - * altogether @c NULL), no changelist filtering occurs.
> - *
> - * If non-NULL, @a revprop_table is a hash table holding additional,
> - * custom revision properties (<tt>const char *</tt> names mapped to
> - * <tt>svn_string_t *</tt> values) to be set on the new revision in
> - * the event that this is a committing operation. This table cannot
> - * contain any standard Subversion properties.
> - *
> * If @a ctx->cancel_func is non-NULL, invoke it passing @a
> * ctx->cancel_baton at various places during the operation.
> *
> - * If @a commit_callback is non-NULL, then for each successful commit, call
> - * @a commit_callback with @a commit_baton and a #svn_commit_info_t for
> - * the commit.
> - *
> * Use @a pool for all memory allocation.
> *
> * @since New in 1.7.
>
>
> And noticing that several of the parameters are applicable to only one
> kind of targets, this also makes me think the API would be better split
> into two: one for URLs and one for WC paths. Thoughts?
It sounds reasonable, but I'm not sure how that jives with the overall
philosophy of libsvn_client. Most libsvn_client APIs allow the caller
to throw either URLs or working copy paths at the API and then it just
does the right thing. However, in this context, where the various
operations are quite different, it may make sense to divorce the two
APIs.
I'm interested to hear what others may think.
-Hyrum
Received on 2011-04-20 17:34:41 CEST