Hi,
Thank you for your comments. I'm sorry that I did respond to your
previous two emails. My (broken) email system seems to have eaten
them, so I never saw them. :-( I'm fortunate the list is archived.
So, let me first respond to the points you made in those emails.
On 2/20/2007 4:19 PM, Peter Lundblad wrote:
>
> Gerco Ballintijn writes:
>>> Yes, as I realize now there are three ways to go: (1) Provide the revision
>>> properties as part of the get_commit_editor() call, what I did in my patch;
>>> (2) Provide the revision properties through a new change_file_prop() "method"
>>> in the svn_delta_editor_t struct; or (3) Provide the revision properties as
>>> part of the close_edit() method of the svn_delta_editor_t struct. Method (1)
>>> and (3) seem to have the least impact on the existing code, but method (2)
>>> is the cleanest and most generic. (Method (2) and (3) both allow the revision
>>> properties to be provided at the end).
>>>
>
> Unfortunately, the editor is not documented to be extensible, so
> approaches 2 and 3 will require revising that struct, which will be a lot
> of work.
>
I'm not sure what you mean with "not documented to be extensible."
Other structures have changed, right? I'm also not sure why it
would be a lot of work. By bumping the version number on struct
svn_delta_editor_t and constructor svn_delta_default_editor you're
done. Current edit producer and consumer will simply ignore the
extra method, whether using the old or the new version of struct
svn_delta_editor_t.
>
> If someone wants to take that (which we will need to do at some
> point for other things), that'd be great, but I'm not sure I think
> this feature need to block on that, especially since it can be useful
> without the ideal genericity.
>
Any implementation of this functionality will impact functions and/or
structures in the public APIs and the changes thus have to stay to
"2.0". So, avoiding an unsatisfactory solutions here seems desirable
to me.
>>>>>
>>>>> A separate issue was (is) whether to ignore, to filter, or to flag the
>>>>> standard revision properties as errors when passed in the table.
>>>>>
>>>>
>>>> I think it would make sense to disallow any changes to the svn:
>>>> properties, since they're already set through other processes (and you
>>>> wouldn't want, for example, to have to work out whether someone could
>>>> override svn:author).
>>>>
>>>
>>> But would such a prohibition be for the command-line client *only* or
>>> for all (relevant) public APIs?
>>>
>
> To make sense, this has to be prohibited on the server side.
>
This is certainly possible for the svn protocol. I am not so sure
you can easily recongize the distinction between valid and invalid
revprop changes in the DAV implementation. I was also wondering
whether, beside the svn client, libsvn_client would also have to
perform this check, and possible some of the other libraries.
On 2/24/2007 10:04 PM, Peter Lundblad wrote:
>
> Also, while thinking of this, I'm not sure that this commit-specific
> function belongs in the general editor interface.
>
Well, I don't think it is commit-specific. The interface changes could
also be used to implement the svnsync and load/dump stuff. I would say
its more generic than that. The method should probably be called
change_edit_prop() to make it more generic. :-)
>
> An alternative is to have svn_ra_get_commit_editor() return
> a callback/baton that can be used for this purpose.
>
Do you mean a callback and baton parallel to the existing edit
callback and batton? Even "only" changing this method still has a
significant ripple effect, as I discovered when writing my first
patch...
>
> We should restrict the use of this callback to either before
> any editing operations to simplify the implementation.
>
Why this restriction? This makes it impossible to do the after-
editing changing of revprops, which people seem to like, and the
current implementation doesn't seem to need it. Besides in that
case you can just give the properties straightaway, like I did
in my first patch.
On 2/26/2007 10:08 AM, Peter Lundblad wrote:
>
> Gerco Ballintijn writes:
>> Index: subversion/include/svn_ra_svn.h
>> ===================================================================
>> --- subversion/include/svn_ra_svn.h (revision 23480)
>> +++ subversion/include/svn_ra_svn.h (working copy)
>> @@ -43,6 +43,7 @@
>> #define SVN_RA_SVN_CAP_EDIT_PIPELINE "edit-pipeline"
>> #define SVN_RA_SVN_CAP_SVNDIFF1 "svndiff1"
>> #define SVN_RA_SVN_CAP_ABSENT_ENTRIES "absent-entries"
>> +#define SVN_RA_SVN_CAP_CHANGE_REV_PROP "change-rev-prop"
>>
>
> We shouldn't need a new capability for this. The invocation of a new
> command shouldn't abort the edit.
>
Are sure even for the pipe-lined editing case?
>> Index: subversion/include/svn_client.h
>> ===================================================================
>> --- subversion/include/svn_client.h (revision 23480)
>> +++ subversion/include/svn_client.h (working copy)
>> @@ -762,6 +762,10 @@
>> /** MIME types map.
>> * @since New in 1.5. */
>> apr_hash_t *mimetypes_map;
>> +
>> + /** Table holding the extra revision properties to be set.
>> + * @since New in 1.5. */
>> + apr_hash_t * revprop_table;
> ^ Spurious space.
>
> More importantly, the documentation needs to be extended to say
> what the types of the keys and avlues are. Also, what is the relationship
> between this table and log_msg_func2?
>
I'm confused: Why would there be a relationship with the log_msg_func2?
>
>> Index: subversion/libsvn_client/commit.c
>> ===================================================================
>> --- subversion/libsvn_client/commit.c (revision 23480)
>> +++ subversion/libsvn_client/commit.c (working copy)
>> @@ -467,6 +467,7 @@
>> apr_array_header_t *batons = NULL;
>> const char *edit_path = "";
>> import_ctx_t *import_ctx = apr_pcalloc(pool, sizeof(*import_ctx));
>> + svn_error_t *err;
>>
>> /* Get a root dir baton. We pass an invalid revnum to open_root
>> to mean "base this on the youngest revision". Should we have an
>> @@ -558,7 +559,10 @@
>> }
>> }
>>
>> - if (import_ctx->repos_changed)
>> + err = svn_client__set_rev_props(editor, edit_baton, ctx->revprop_table,
>> + pool);
>> +
>> + if (!err && import_ctx->repos_changed)
>> SVN_ERR(editor->close_edit(edit_baton, pool));
>> else
>> SVN_ERR(editor->abort_edit(edit_baton, pool));
>
> Hmmm, what's the point in setting revision properties and comitting an
> otherwise empty revision? I would expect the call to _set_rev_props to
> happen only if repos_changed is TRUE.
>
Yes. I didn't look too much to context here, so you're probably right...
>
>> @@ -406,14 +409,18 @@
>> the consumer must read and discard edit operations until writing
>> unblocks or it reads an abort-edit.
>>
>> -If edit pipelining is not negotiated, then the target-rev, open-root,
>> -delete-entry, add-dir, open-dir, close-dir, and close-file operations
>> -produce empty responses. Errors produced other operations are
>> -reported by the enclosing close-dir or close-file operation.
>> +If edit pipelining is not negotiated, then the target-rev,
>> +change-rev-prop, open-root, delete-entry, add-dir, open-dir, close-dir,
>> +and close-file operations produce empty responses. Errors produced
>> +other operations are reported by the enclosing close-dir or close-file
>> +operation.
>>
>> target-rev
>> params: ( rev:number )
>>
>> + change-rev-prop
>> + params: ( name:string [ value:string ] )
>> +
>
> When is it useful to specify no value? This is normally for deleting
> properties, but is that applicable here?
>
I chose this mainly because the lowlevel repos supported it. However,
I thought I could also prove useful to be able to set and later clear
a property.
The other comments are acknowledged and will be used in subsequent
revisions.
With regard,
Gerco.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Feb 28 01:19:02 2007