[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Fix issue #1976: Set arbitrary revision properties (revprops) during commit.

From: Gerco Ballintijn <gerco_at_ballintijn.com>
Date: 2007-02-09 17:36:52 CET

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",
>> + }
> "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,

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

This is an archived mail posted to the Subversion Dev mailing list.