Hi David,
Thanks for your comments. I have incorporated them ..
David Glasser wrote:
>
> (For what it's worth, I still think that an API that lets you set
> revprops after running through the editor tree would be better, since
> you could do signatures and the like, but one could still do that with
> a preliminary "dry run" anyway.)
>
Do you mean pass them as apart of the svn_delta_editor_t::close_edit()
call? I guess moving the parameter from the get_commit_editor() calls
to the close_edit() is relatively straightforward; however, I have no
idea on the impact of that move on the ra_dav implementation. I have
not spent much time there (the code was somewhat frightening :-)
>
>> * subversion/svn/propedit-cmd.c
>> (svn_cl__propedit): Check whether the revprop_table field is set
>> only when actually commiting, and pass on the revprop_table
>> field. Note: This doesn't work (yet) because propedit-cmd
>> is not implemented for URLs (yet?)! :-(
>
> Hmm, are you working against trunk or 1.4? Trunk should have propedit
> for URLs since around October.
>
Yes, please ignore that comment; it works. I didn't complete get the Python
test code the first time.
>
>> Index: subversion/libsvn_ra_dav/commit.c
>> ===================================================================
>> --- subversion/libsvn_ra_dav/commit.c (revision 23331)
>> +++ subversion/libsvn_ra_dav/commit.c (working copy)
>> @@ -1276,8 +1276,9 @@
>> }
>>
>>
>> -static svn_error_t * apply_log_message(commit_ctx_t *cc,
>> +static svn_error_t * apply_revprops(commit_ctx_t *cc,
>> const char *log_msg,
>> + apr_hash_t *revprop_table,
>> apr_pool_t *pool)
>> {
>> const svn_string_t *vcc;
>> @@ -1336,6 +1337,13 @@
>>
>> apr_hash_set(prop_changes, SVN_PROP_PREFIX "log",
>> APR_HASH_KEY_STRING, log_str);
>> + if (revprop_table)
>> + {
>> + prop_changes = apr_hash_overlay(pool, prop_changes,
>> revprop_table);
>> + /* The svn:author property is already set somewhere else. */
>> + apr_hash_set(prop_changes, SVN_PROP_PREFIX "author",
>> + APR_HASH_KEY_STRING, NULL);
>> + }
>
> "somewhere else" is vague; might as well be specific.
>
I would have been more specific I could have. I didn't examine the
DAV code that much, so I really don't know... :-)
>
> Sorry for only giving such a superficial review; I hope I'll have time to
> actually test out the next version of your patch!
>
My main concerns focused on whether the command line interface was good
enough and on the impact of change on the general architecture. I chose
the most direct approach I could see. However, I'm now wondering if moving
the revision properties to the close_edit() call is worthwhile. Maybe
somebody else can comment?
A separate issue was (is) whether to ignore, to filter, or to flag the
standard revision properties as errors when passed in the table.
With regards,
Gerco.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Feb 9 17:37:12 2007