You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@wandisco.com> on 2011/04/20 16:50:37 UTC
Re: svn commit: r1095195 - make svn_client_propset4() operate on
an array of targets
> 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?
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?
- Julian
Re: RFC: Split svn_client_propset4() into propset-for-URLs and
propset-for-WC-paths? [was: svn commit: r1095195 ...]
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Thu, Apr 21, 2011 at 10:00 AM, Julian Foad <ju...@wandisco.com> wrote:
> This is a post-1.7 RFC.
I'm happy to make this a 1.7 RFC, since we're already rev'd the client
API, and the question is coming up now. The effort shouldn't be that
involved.[1]
-Hyrum
[1] Yes, I'm volunteering, and yes those are famous last words.
>
> 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?
>
> svn_client_propset4() operates on either WC paths or URLs. Although its
> purpose is the same either way in general terms, the differences are
> substantial, and in particular the parameters needed by the API are
> substantially disjoint. The differences are highlighted by its doc
> string which looks like this in outline:
>
> /**
> * 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 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.
> *
> * - 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.
> *
> * - If non-NULL, @a revprop_table is a hash table holding additional,
> * custom revision properties (...) to be set on the new revision. ...
> *
> * - If @a commit_callback is non-NULL, then for each successful commit,
> * call @a commit_callback with @a commit_baton ...
> *
> * - @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, ...; if #svn_depth_immediates,
> * ...; if #svn_depth_infinity, ...
> *
> * - @a changelists is ... used as a restrictive filter on items whose
> * properties are set ...
> *
> * - @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
> * 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 ...
> *
> * If @a ctx->cancel_func is non-NULL, invoke it passing @a
> * ctx->cancel_baton at various places during the operation.
> */
>
> Notice that apart from the basic inputs (propname, propval, targets) and
> cancellation, there are about four parameters that are relevant just for
> URLs, two that are relevant just for WC paths, and only one interesting
> parameter (skip_checks) that is common to both.
>
> There are currently three callers in "svn": "propset" and "propdel" only
> support using WC paths, while "propedit" supports using both forms of
> target.
>
> Thoughts?
>
> - Julian
>
>
> On Wed, 2011-04-20, Hyrum K Wright wrote:
>> On Wed, Apr 20, 2011 at 9:50 AM, Julian Foad <ju...@wandisco.com> wrote:
>> > 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.
>
>
>
Re: RFC: Split svn_client_propset4() into propset-for-URLs and propset-for-WC-paths?
[was: svn commit: r1095195 ...]
Posted by Stefan Küng <to...@gmail.com>.
On 21.04.2011 17:00, Julian Foad 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?
>
> svn_client_propset4() operates on either WC paths or URLs. Although its
> purpose is the same either way in general terms, the differences are
> substantial, and in particular the parameters needed by the API are
> substantially disjoint. The differences are highlighted by its doc
> string which looks like this in outline:
I'm all for it. In TSVN I separated the two situations (urls and paths)
into two separate classes already since it's just easier to deal with it
that way.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: RFC: Split svn_client_propset4() into propset-for-URLs and
propset-for-WC-paths? [was: svn commit: r1095195 ...]
Posted by Stefan Küng <to...@gmail.com>.
On Thu, Apr 21, 2011 at 20:33, Hyrum K Wright <hy...@hyrumwright.org> wrote:
> On Thu, Apr 21, 2011 at 10:00 AM, Julian Foad <ju...@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?
>
> -Hyrum
[snip]
Looks good. But to avoid confusion with file:/// access (those
repositories are local too), I suggest to name the APIs not 'local'
and 'remove' but 'wc' and 'repo' or something like that.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: RFC: Split svn_client_propset4() into propset-for-URLs and
propset-for-WC-paths? [was: svn commit: r1095195 ...]
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Thu, Apr 21, 2011 at 4:25 PM, Julian Foad <ju...@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 <ju...@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
Re: RFC: Split svn_client_propset4() into propset-for-URLs and
propset-for-WC-paths? [was: svn commit: r1095195 ...]
Posted by Julian Foad <ju...@wandisco.com>.
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 <ju...@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/
> > * #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//.
> > * 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?
> > * 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.
> +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.
> if (targets_are_urls)
> return svn_error_create(SVN_ERR_ILLEGAL_TARGET, NULL,
> _("Targets must be URLs"));
Wrong error message.
- Julian
Re: RFC: Split svn_client_propset4() into propset-for-URLs and
propset-for-WC-paths? [was: svn commit: r1095195 ...]
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
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 <ju...@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?
>
> -Hyrum
>
> [[[
> /**
> * 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
> * 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).
> *
> * 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.
> * 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).
> *
> * 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.
-Hyrum
Re: RFC: Split svn_client_propset4() into propset-for-URLs and
propset-for-WC-paths? [was: svn commit: r1095195 ...]
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Thu, Apr 21, 2011 at 10:00 AM, Julian Foad <ju...@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?
-Hyrum
[[[
/**
* 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
* 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).
*
* 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.
* 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).
*
* 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);
]]]
Re: RFC: Split svn_client_propset4() into propset-for-URLs and
propset-for-WC-paths? [was: svn commit: r1095195 ...]
Posted by Greg Stein <gs...@gmail.com>.
On Thu, Apr 21, 2011 at 11:00, Julian Foad <ju...@wandisco.com> wrote:
> This is a post-1.7 RFC.
If you have time and can do it for 1.7, then I don't think we can stop you :-P
>...
> Thoughts?
Yeah. Split them!
Cheers,
-g
RFC: Split svn_client_propset4() into propset-for-URLs and
propset-for-WC-paths? [was: svn commit: r1095195 ...]
Posted by Julian Foad <ju...@wandisco.com>.
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?
svn_client_propset4() operates on either WC paths or URLs. Although its
purpose is the same either way in general terms, the differences are
substantial, and in particular the parameters needed by the API are
substantially disjoint. The differences are highlighted by its doc
string which looks like this in outline:
/**
* 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 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.
*
* - 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.
*
* - If non-NULL, @a revprop_table is a hash table holding additional,
* custom revision properties (...) to be set on the new revision. ...
*
* - If @a commit_callback is non-NULL, then for each successful commit,
* call @a commit_callback with @a commit_baton ...
*
* - @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, ...; if #svn_depth_immediates,
* ...; if #svn_depth_infinity, ...
*
* - @a changelists is ... used as a restrictive filter on items whose
* properties are set ...
*
* - @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
* 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 ...
*
* If @a ctx->cancel_func is non-NULL, invoke it passing @a
* ctx->cancel_baton at various places during the operation.
*/
Notice that apart from the basic inputs (propname, propval, targets) and
cancellation, there are about four parameters that are relevant just for
URLs, two that are relevant just for WC paths, and only one interesting
parameter (skip_checks) that is common to both.
There are currently three callers in "svn": "propset" and "propdel" only
support using WC paths, while "propedit" supports using both forms of
target.
Thoughts?
- Julian
On Wed, 2011-04-20, Hyrum K Wright wrote:
> On Wed, Apr 20, 2011 at 9:50 AM, Julian Foad <ju...@wandisco.com> wrote:
> > 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.
Re: svn commit: r1095195 - make svn_client_propset4() operate on an
array of targets
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Wed, Apr 20, 2011 at 9:50 AM, Julian Foad <ju...@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