You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David Glasser <gl...@mit.edu> on 2006/09/29 15:08:29 UTC
svn propedit URL
(I assume there must have been discussion of this before, but I'm
having trouble finding it.)
It would be useful to support "svn propedit" on URLs. The obvious
problem with modifying properties on URLs is accidentally overwriting
somebody else's changes. While this is clearly a problem for propset
and propdel, it's not as big of a deal (in theory) for propedit: svn
could remember the revision of the property being edited, and pass it
through the various layers of propset, and the operation would fail if
it was out of date. This can be implemented with the current RA API;
it just needs a new client API (and 'svn' changes).
I'm specifically proposing the following API change. What do folks
think?
--- subversion/include/svn_client.h (revision 102401)
+++ subversion/include/svn_client.h (local)
@@ -2086,6 +2086,12 @@
*
* A @a propval of @c NULL will delete the property.
*
+ * The @a target may only be an URL if @a base_revision_for_url is not
+ * @c 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 @c SVN_INVALID_REVNUM if @a target is
+ * not an URL.
+ *
* If @a propname is an svn-controlled property (i.e. prefixed with
* @c SVN_PROP_PREFIX), then the caller is responsible for ensuring that
* the value is UTF8-encoded and uses LF line-endings.
@@ -2102,9 +2108,25 @@
*
* Use @a pool for all memory allocation.
*
- * @since New in 1.2.
+ * @since New in 1.5.
*/
svn_error_t *
+svn_client_propset3(const char *propname,
+ const svn_string_t *propval,
+ const char *target,
+ svn_boolean_t recurse,
+ svn_boolean_t skip_checks,
+ svn_revnum_t base_revision_for_url,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool);
+
+/**
+ * Like svn_client_propset2(), but with @a base_revision_for_url
+ * always @c SVN_INVALID_REVNUM.
+ *
+ * @deprecated Provided for backward compatibility with the 1.4 API.
+ */
+svn_error_t *
svn_client_propset2(const char *propname,
const svn_string_t *propval,
const char *target,
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn propedit URL
Posted by David Glasser <gl...@mit.edu>.
On 9/29/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> Second, finding a UI for
> entry of log messages, since it's kind of lame to pop up one editor to
> make your change, then a second one to enter a log. I don't
> personally have an issue with that being the UI, but other people have
> expressed problems with it in the past.
I also think that's not much of an inconvenience, compared to the
inconvenience of not being able to do this at all. I think that this
choice is independent of the svn_client API change, though: if "svn
propedit" wanted to provide one buffer with two parts or something,
then it can just use a custom log_msg_func2 into the client ctx when
calling svn_client_propset3.
(I'll write this the straightforward way, with two editor invocations.
Whether or not changing from "two editor invocations" to "one editor
invocation" is an incompatible API change is unclear to me, since it's
most likely to occur in interactive use, but in any case the UI could
be tweaked at any time before 1.5.)
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn propedit URL
Posted by "C. Michael Pilato" <cm...@collab.net>.
Garrett Rooney wrote:
> On 9/29/06, C. Michael Pilato <cm...@collab.net> wrote:
>
>> Oh, no you don't! It's before noon here, too, and I don't have my
>> bikeshed painter gear on yet!
>
> Heh.
>
>> Seriously, was just thinking something like:
>>
>> Property 'foo' on '/some/path' [added|modified|removed].
>
> Would that just be something that pops up in your editor, or are you
> seriously talking about defaulting to a generated log? I'd have no
> objection to a default log showing up in the editor, but I'd really
> dislike using a generated log message by default.
I was actually considering the use of a generated log, but only if the
folks than can't handle a pair of EDITOR pop-ups are too noisy.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn propedit URL
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 9/29/06, C. Michael Pilato <cm...@collab.net> wrote:
> Oh, no you don't! It's before noon here, too, and I don't have my
> bikeshed painter gear on yet!
Heh.
> Seriously, was just thinking something like:
>
> Property 'foo' on '/some/path' [added|modified|removed].
Would that just be something that pops up in your editor, or are you
seriously talking about defaulting to a generated log? I'd have no
objection to a default log showing up in the editor, but I'd really
dislike using a generated log message by default.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn propedit URL
Posted by "C. Michael Pilato" <cm...@collab.net>.
Garrett Rooney wrote:
> On 9/29/06, C. Michael Pilato <cm...@collab.net> wrote:
>
>> Did you miss the part of David's mail where he said pretty much exactly
>> that? :-)
>
> Hey, it's still before noon here, I'm allowed to be half asleep ;-)
Heheh.
>> I think it's fine as a default UI. Remember, folks can do the following
>> to get down to a single editor pop-up:
>>
>> svn propedit http://server.com/some/path foo \
>> -m "Modify the `foo' property on /some/path"
>
> True.
>
>> In fact, we could consider having Subversion just use a stock log
>> message for such a single-property-only change (it can always be tweaked
>> later).
>
> What would the stock message be?
Oh, no you don't! It's before noon here, too, and I don't have my
bikeshed painter gear on yet!
Seriously, was just thinking something like:
Property 'foo' on '/some/path' [added|modified|removed].
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn propedit URL
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 9/29/06, C. Michael Pilato <cm...@collab.net> wrote:
> Did you miss the part of David's mail where he said pretty much exactly
> that? :-)
Hey, it's still before noon here, I'm allowed to be half asleep ;-)
> I think it's fine as a default UI. Remember, folks can do the following
> to get down to a single editor pop-up:
>
> svn propedit http://server.com/some/path foo \
> -m "Modify the `foo' property on /some/path"
True.
> In fact, we could consider having Subversion just use a stock log
> message for such a single-property-only change (it can always be tweaked
> later).
What would the stock message be?
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn propedit URL
Posted by "C. Michael Pilato" <cm...@collab.net>.
Garrett Rooney wrote:
> On 9/29/06, David Glasser <gl...@mit.edu> wrote:
>> (I assume there must have been discussion of this before, but I'm
>> having trouble finding it.)
>>
>> It would be useful to support "svn propedit" on URLs. The obvious
>> problem with modifying properties on URLs is accidentally overwriting
>> somebody else's changes. While this is clearly a problem for propset
>> and propdel, it's not as big of a deal (in theory) for propedit: svn
>> could remember the revision of the property being edited, and pass it
>> through the various layers of propset, and the operation would fail if
>> it was out of date. This can be implemented with the current RA API;
>> it just needs a new client API (and 'svn' changes).
>>
>> I'm specifically proposing the following API change. What do folks
>> think?
>
> There are two gotchas with edit type commands on URLs. Neither are
> showstoppers, but you need to be aware of them. First, you need to be
> sure to avoid race conditions where you start editing, someone commits
> a change, and then you hit save and commit. With files this can be
> handled by remembering what version of the file you started with and
> using that as your initial rev during the commit. There should be a
> similar thing you can do with properties.
Did you miss the part of David's mail where he said pretty much exactly
that? :-)
> Second, finding a UI for
> entry of log messages, since it's kind of lame to pop up one editor to
> make your change, then a second one to enter a log. I don't
> personally have an issue with that being the UI, but other people have
> expressed problems with it in the past.
I think it's fine as a default UI. Remember, folks can do the following
to get down to a single editor pop-up:
svn propedit http://server.com/some/path foo \
-m "Modify the `foo' property on /some/path"
In fact, we could consider having Subversion just use a stock log
message for such a single-property-only change (it can always be tweaked
later).
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn propedit URL
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 9/29/06, David Glasser <gl...@mit.edu> wrote:
> (I assume there must have been discussion of this before, but I'm
> having trouble finding it.)
>
> It would be useful to support "svn propedit" on URLs. The obvious
> problem with modifying properties on URLs is accidentally overwriting
> somebody else's changes. While this is clearly a problem for propset
> and propdel, it's not as big of a deal (in theory) for propedit: svn
> could remember the revision of the property being edited, and pass it
> through the various layers of propset, and the operation would fail if
> it was out of date. This can be implemented with the current RA API;
> it just needs a new client API (and 'svn' changes).
>
> I'm specifically proposing the following API change. What do folks
> think?
There are two gotchas with edit type commands on URLs. Neither are
showstoppers, but you need to be aware of them. First, you need to be
sure to avoid race conditions where you start editing, someone commits
a change, and then you hit save and commit. With files this can be
handled by remembering what version of the file you started with and
using that as your initial rev during the commit. There should be a
similar thing you can do with properties. Second, finding a UI for
entry of log messages, since it's kind of lame to pop up one editor to
make your change, then a second one to enter a log. I don't
personally have an issue with that being the UI, but other people have
expressed problems with it in the past.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn propedit URL
Posted by David Glasser <gl...@mit.edu>.
On 9/29/06, David Glasser <gl...@mit.edu> wrote:
> It would be useful to support "svn propedit" on URLs. The obvious
> problem with modifying properties on URLs is accidentally overwriting
> somebody else's changes. While this is clearly a problem for propset
> and propdel, it's not as big of a deal (in theory) for propedit: svn
> could remember the revision of the property being edited, and pass it
> through the various layers of propset, and the operation would fail if
> it was out of date. This can be implemented with the current RA API;
> it just needs a new client API (and 'svn' changes).
Committed as r21887.
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn propedit URL
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 9/29/06, David Glasser <gl...@mit.edu> wrote:
> On 9/29/06, David Glasser <gl...@mit.edu> wrote:
> > I'm specifically proposing the following API change. What do folks
> > think?
>
> The previous API change wouldn't be good enough, since it'll need to
> return an svn_commit_info_t as well. (Should I explicitly state that
> passing in NULL for commit_info_p is OK if the target isn't an URL?)
Yes, I'd say that makes sense to document.
> There's also some validation and canonicalization in svn_wc_prop_set2
> that need to be factored out into libsvn_subr or something. Some of
> this is easy (don't set svn:ignore on files, strip whitespace around
> svn:keywords). Some of it... less so: there's a check that a
> svn:eol-style is reasonable, and a full-blown attempt to parse
> svn:externals with svn_wc_parse_externals_description2 (which doesn't
> take a wc_adm_access_t, at least). Dealing with these two might be a
> little hairy.
>
> This patch shows my current plan for svn_client_propset3:
Seems reasonable enough to me.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] Re: svn propedit URL
Posted by David Glasser <gl...@mit.edu>.
On 10/1/06, David Glasser <gl...@mit.edu> wrote:
> I think this patch is kind of ugly, but necessary to achieve this
> functionality. I would appreciate eyes before committing it.
Oops, and here's the log message:
[[[
Extract the validation and canonicalization of svn:* properties from
svn_wc_prop_set2 into a new API, which could theoretically be used
for "svn propset URL".
* subversion/include/svn_wc.h
(svn_wc_canonicalize_svn_prop): New API to validate and canonicalize
svn:* properties.
(svn_wc_canonicalize_svn_prop_get_file_t): New callback type for above API.
* subversion/libsvn_wc/props.c
(svn_wc_canonicalize_svn_prop): Implement new API (extracted from
svn_wc_prop_set2).
(getter_baton): New baton implementation for new callback.
(get_file_for_validation): New function, implementing new callback
type; contents
extracted from validate_eol_prop_against_file.
(validate_eol_prop_against_file): Use callback instead of direct WC access.
(svn_wc_prop_set2): Use new API for property validation and canonicalization
instead of doing it directly.
(validate_prop_against_node_kind, validate_eol_prop_against_file,
svn_wc_parse_externals_description2): Allow an argument which is
only used in error
messages to be an URL.
]]]
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn propedit URL
Posted by David Glasser <gl...@mit.edu>.
On 10/1/06, David Glasser <gl...@mit.edu> wrote:
> OK, I ended up just making this a WC api (since it needs to call
> svn_wc_parse_externals_description2 anyway). Attached is a patch to
> create an API for validating and canonicalizing svn:* props. This
> will then be usable by svn_client_propset3 for checking properties
> that are being set on URLs.
>
> I think this patch is kind of ugly, but necessary to achieve this
> functionality. I would appreciate eyes before committing it.
Ah, well, they can be post-commit eyes. Committed in r21739.
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn propedit URL
Posted by David Glasser <gl...@mit.edu>.
On 9/30/06, David Glasser <gl...@mit.edu> wrote:
> So, either: how do I refer to svn_stream_t in svn_props.h, or what
> design simplification am I totally missing?
OK, I ended up just making this a WC api (since it needs to call
svn_wc_parse_externals_description2 anyway). Attached is a patch to
create an API for validating and canonicalizing svn:* props. This
will then be usable by svn_client_propset3 for checking properties
that are being set on URLs.
I think this patch is kind of ugly, but necessary to achieve this
functionality. I would appreciate eyes before committing it.
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
Re: svn propedit URL
Posted by David Glasser <gl...@mit.edu>.
On 9/29/06, David Glasser <gl...@mit.edu> wrote:
> There's also some validation and canonicalization in svn_wc_prop_set2
> that need to be factored out into libsvn_subr or something. Some of
> this is easy (don't set svn:ignore on files, strip whitespace around
> svn:keywords). Some of it... less so: there's a check that a
> svn:eol-style is reasonable, and a full-blown attempt to parse
> svn:externals with svn_wc_parse_externals_description2 (which doesn't
> take a wc_adm_access_t, at least). Dealing with these two might be a
> little hairy.
OK, so the deal here is that svn_wc_prop_set2 does validation and
canonicalization on svn:* properties. I want to factor that out. The
toughest part is svn:eol-style checking: it looks at the MIME type,
and also tries to make sure that the file has consistent newlines at
all. It does this by opening the file and writing it to a
substituting stream (which is connected to an empty stream) and seeing
if there's an error.
I've added the following to svn_props.h:
/** Callback type used by @c svn_prop_canonicalize_svn_prop.
*
* It should set @a mime_type to the value of @a SVN_PROP_MIME_TYPE
* for the path passed to @c svn_prop_mime_type (allocated from @a
* pool), and then write the contents of the file to @a stream.
*
* (Currently, this is used if you are attempting to set the @a
* SVN_PROP_EOL_STYLE property, to make sure that the value matches
* the mime type and contents.)
*/
typedef svn_error_t *(*svn_prop_canonicalize_get_file_t)
(const svn_string_t **mime_type,
svn_stream_t *stream,
void *baton,
apr_pool_t *pool);
/** Canonicalize the value of an svn:* property @a propname with
* value @propval.
*
* If the property is not appropriate for a node of kind @a kind, or
* is otherwise invalid, throw an error. Otherwise, set @a *propval_p
* to a canonicalized version of the property value. If @a
* skip_some_checks is true, only some validity checks are taken.
*
* Some validity checks require access to the contents and MIME type
* of the target if it is a file; they will call @a getter with @a
* getter_baton, which then needs to set the MIME type and print the
* contents of the file to the given stream.
*
* @a path should be the path of the file in question; it is only used
* for error messages.
*/
svn_error_t *svn_prop_canonicalize_svn_prop(svn_string_t **propval_p,
const char *propname,
const svn_string_t *propval,
const char *path,
svn_node_kind_t kind,
svn_boolean_t skip_some_checks,
svn_prop_canonicalize_get_file_t getter,
void *getter_baton,
apr_pool_t *pool);
The idea is that in the wc propset case, the getter function would use
wc prop getting functions and print the file from disk to the given
stream, whereas for URL propsets, it would use svn_ra_get_file.
First of all, this seems like overkill. But it feels necessary to
preserve the current behavior for URL propsets.
Secondly, I'm running into a technical problem -- putting a reference
to svn_stream_t into svn_props.h make circular includes, since svn_io
includes svn_error includes svn_error_codes includes svn_props. (The
last is for no reason other than backwards compatibility -- it doesn't
even use it!)
So, either: how do I refer to svn_stream_t in svn_props.h, or what
design simplification am I totally missing?
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn propedit URL
Posted by David Glasser <gl...@mit.edu>.
On 9/29/06, David Glasser <gl...@mit.edu> wrote:
> I'm specifically proposing the following API change. What do folks
> think?
The previous API change wouldn't be good enough, since it'll need to
return an svn_commit_info_t as well. (Should I explicitly state that
passing in NULL for commit_info_p is OK if the target isn't an URL?)
There's also some validation and canonicalization in svn_wc_prop_set2
that need to be factored out into libsvn_subr or something. Some of
this is easy (don't set svn:ignore on files, strip whitespace around
svn:keywords). Some of it... less so: there's a check that a
svn:eol-style is reasonable, and a full-blown attempt to parse
svn:externals with svn_wc_parse_externals_description2 (which doesn't
take a wc_adm_access_t, at least). Dealing with these two might be a
little hairy.
This patch shows my current plan for svn_client_propset3:
--- subversion/include/svn_client.h (revision 21714)
+++ subversion/include/svn_client.h (patch - level 1)
@@ -2086,6 +2086,16 @@
*
* A @a propval of @c NULL will delete the property.
*
+ * The @a target may only be an URL if @a base_revision_for_url is not
+ * @c 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 @c SVN_INVALID_REVNUM if @a target is
+ * not an URL. @a recurse is not supported on URLs. The
+ * authentication baton in @a ctx and @a ctx->log_msg_func/@a
+ * ctx->log_msg_baton will be used to immediately attempt to commit
+ * the property change in the repository. If the commit succeeds,
+ * allocate (in @a pool) and populate @a *commit_info_p.
+ *
* If @a propname is an svn-controlled property (i.e. prefixed with
* @c SVN_PROP_PREFIX), then the caller is responsible for ensuring that
* the value is UTF8-encoded and uses LF line-endings.
@@ -2102,9 +2112,26 @@
*
* Use @a pool for all memory allocation.
*
- * @since New in 1.2.
+ * @since New in 1.5.
*/
svn_error_t *
+svn_client_propset3(svn_commit_info_t **commit_info_p,
+ const char *propname,
+ const svn_string_t *propval,
+ const char *target,
+ svn_boolean_t recurse,
+ svn_boolean_t skip_checks,
+ svn_revnum_t base_revision_for_url,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool);
+
+/**
+ * Like svn_client_propset2(), but with @a base_revision_for_url
+ * always @c SVN_INVALID_REVNUM, and @a commit_info_p always NULL.
+ *
+ * @deprecated Provided for backward compatibility with the 1.4 API.
+ */
+svn_error_t *
svn_client_propset2(const char *propname,
const svn_string_t *propval,
const char *target,
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org