On Thu, Apr 21, 2011 at 4:25 PM, Julian Foad <julian.f...@wandisco.com> wrote:
> Hyrum K Wright wrote:
>> On Thu, Apr 21, 2011 at 1:33 PM, Hyrum K Wright <hy...@hyrumwright.org>
>> wrote:
>> > On Thu, Apr 21, 2011 at 10:00 AM, Julian Foad <julian.f...@wandisco.com>
>> > wrote:
>> >> This is a post-1.7 RFC.
>> >>
>> >> 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. But does
>> >> this paradigm make sense for APIs such as this one?
>> >
>> > Julian,
>> > Does this look reasonable for the API split?
>
> Yes, this looks great.
>
> ...
>
>> > /**
>> > * Set @a propname to @a propval on @a url. A @a propval of @c NULL will
>> > * delete the property.
>> > *
>> > * 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.
>> > *
>> > * If the property has changed on @a url 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.
>> > *
>> > * 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 call @a commit_callback with
>> > * @a commit_baton and a #svn_commit_info_t for the commit.
>> > *
>> > * If @a propname is an svn-controlled property (i.e. prefixed with
>> > * #SVN_PROP_PREFIX), then the caller is responsible for ensuring that
>> > * the value is UTF8-encoded and uses LF line-endings.
>> > *
>> > * If @a skip_checks is TRUE, do no validity checking. But if @a
>> > * skip_checks is FALSE, and @a propname is not a valid property for @a
>> > * targets, return an error, either #SVN_ERR_ILLEGAL_TARGET (if the
>
> s/targets/url/
>
>> > * property is not appropriate for @a targets), or
>
> s/targets/url/
Fixed both of these.
>> > * #SVN_ERR_BAD_MIME_TYPE (if @a propname is "svn:mime-type", but @a
>> > * propval is not a valid mime-type).
>> > *
>> > * Use @a scratch_pool for all memory allocation.
>> > *
>> > * @since New in 1.7.
>> > */
>> > svn_error_t *
>> > svn_client_propset_remote(const char *propname,
>> > const svn_string_t *propval,
>> > const char *url,
>> > svn_boolean_t skip_checks,
>> > svn_revnum_t base_revision_for_url,
>> > const apr_hash_t *revprop_table,
>> > svn_commit_callback2_t commit_callback,
>> > void *commit_baton,
>> > svn_client_ctx_t *ctx,
>> > apr_pool_t *scratch_pool);
>> >
>> > /**
>> > * Set @a propname to @a propval on each (const char *) target in @a
>> > * targets. The targets must be either all working copy paths.
>
> -------------------------------------^^^^^^
> s/either//.
Fixed.
>> > * 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.
>> > *
>> > * @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 @a propname is an svn-controlled property (i.e. prefixed with
>> > * #SVN_PROP_PREFIX), then the caller is responsible for ensuring that
>> > * the value is UTF8-encoded and uses LF line-endings.
>> > *
>> > * If @a skip_checks is TRUE, do no validity checking. But if @a
>> > * skip_checks is FALSE, and @a propname is not a valid property for @a
>> > * targets, return an error, either #SVN_ERR_ILLEGAL_TARGET (if the
>> > * property is not appropriate for @a targets), or
>> > * #SVN_ERR_BAD_MIME_TYPE (if @a propname is "svn:mime-type", but @a
>> > * propval is not a valid mime-type).
>
> (Related to the multi-targets commit rather than to this one.) What
> semantics should this paragraph promise now that there are multiple
> targets?
I don't know yet. Since the goal is to move these to one txn, I
suspect it will be an all-or-nothing proposition. We can come back
and update the docstring in a bit when we better know what the
implementation is doing.
>> > * If @a ctx->cancel_func is non-NULL, invoke it passing @a
>> > * ctx->cancel_baton at various places during the operation.
>> > *
>> > * Use @a scratch_pool for all memory allocation.
>> > *
>> > * @since New in 1.7.
>> > */
>> > svn_error_t *
>> > svn_client_propset_local(const char *propname,
>> > const svn_string_t *propval,
>> > const apr_array_header_t *targets,
>> > svn_depth_t depth,
>> > svn_boolean_t skip_checks,
>> > const apr_array_header_t *changelists,
>> > svn_client_ctx_t *ctx,
>> > apr_pool_t *scratch_pool);
>> > ]]]
>>
>> Done in r1095802.
>
> Ahh, thus neatly side-stepping the issue that multi-URL prop-sets were
> not being combined into a single commit. That problem is now more
> neatly left as a future improvement.
>
> It also resolves an issue I didn't notice before: the old doc string
> promised cancellation support, but it was not implemented for the
> multi-URLs code path.
Yep. I figured we'd wait to add multi-path support to this API until
we have true multiple-paths-in-one-commit support.
>> +static svn_error_t *
>> +check_prop_name(const char *propname,
>> + const svn_string_t *propval)
>
> Doc string? I added one in r1095824 since I think I know the rationale
> for this particular set of checks.
Thanks.
>> if (targets_are_urls)
>> return svn_error_create(SVN_ERR_ILLEGAL_TARGET, NULL,
>> _("Targets must be URLs"));
>
> Wrong error message.
Fixed.
-Hyrum
