On 2/6/07, Gerco Ballintijn <gerco@ballintijn.com> wrote:
> Apologies when this email arrives twice...
>
> [[[
> Fix issue #1976: Set arbitrary revision properties (revprops) during
> commit.
>
> This patch adds a "--with-revprop" option to the various URL-based
> operations (e.g., commit, mkdir, delete). This option can be used
> multiple times to set multiple revision properties. The argument of
> the --with-revprop option is of the form "name=value". The fix also
> extends the svn protocol to transfer the revision properties.
Hi Gerco! Thanks for this patch. I've kept meaning to get around to
fixing this, but have been too busy. I gave it a quick skim; I hope
I'll find time for a real review later. (Thanks for including a test
suite as well!)
(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.)
[ ... ]
> * 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.
(As a little note, the indentation in your log message isn't quite our
standard.)
> * subversion/tests/cmdline/commit_tests.py
> (mkdir_with_revprop, delete_with_revprop, commit_with_revprop,
> import_with_revprop, copy_R2R_with_revprop, copy_WC2R_with_revprop,
> move_R2R_with_revprop, propedit_with_revprop,
> set_multiple_props_with_revprop, set_std_props_with_revprop,
> use_empty_string_as_revprop_pair, use_empty_value_in_revprop_pair,
> no_equals_in_revprop_pair)
> New test functions.
Probably should mention here that propedit_with_revprop is XFail.
[...]
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 23331)
> +++ subversion/include/svn_client.h (working copy)
> @@ -762,6 +762,9 @@
> /** MIME types map.
> * @since New in 1.5. */
> apr_hash_t *mimetypes_map;
> +
> + /* TODO: Add comment */
> + apr_hash_t * revprop_table;
Sounds like a TODO worth doing :-)
[ ... ]
> @@ -872,6 +877,35 @@
> }
>
>
> +static svn_error_t *
> +add_revprop_from_string(apr_hash_t **revprop_table_p,
> + const char *revprop_pair,
> + apr_pool_t *pool)
> +{
> + const char *sep, *propname;
> + svn_string_t *propval;
> +
> + if (! *revprop_pair)
> + return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("Revision property pair is empty"));
> +
> + if (! *revprop_table_p)
> + *revprop_table_p = apr_hash_make(pool);
> +
> + sep = strchr(revprop_pair, '=');
> + if (! sep)
> + return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("Revision property pair contains no '='"));
> +
> + propname = apr_pstrndup(pool, revprop_pair, sep - revprop_pair);
> + propval = svn_string_create(sep + 1, pool);
> +
> + apr_hash_set(*revprop_table_p, propname, APR_HASH_KEY_STRING, propval);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
The indentation on this function seems a little wacky.
[...]
> Index: subversion/svn/propedit-cmd.c
> ===================================================================
> --- subversion/svn/propedit-cmd.c (revision 23331)
> +++ subversion/svn/propedit-cmd.c (working copy)
> @@ -193,11 +193,12 @@
> }
> else
> {
> - if (opt_state->message || opt_state->filedata)
> + if (opt_state->message || opt_state->filedata || opt_state->revprop_table)
> {
> return svn_error_create
> (SVN_ERR_CL_UNNECESSARY_LOG_MESSAGE, NULL,
> - _("Local, non-commit operations do not take a log message"));
> + _("Local, non-commit operations do not take a log message "
> + "or revision properties"));
> }
>
> /* Split the path if it is a file path. */
> @@ -245,6 +246,8 @@
> opt_state, NULL, ctx->config,
> subpool));
>
> + ctx->revprop_table = opt_state->revprop_table;
> +
> err = svn_client_propset3(&commit_info,
> pname_utf8, edited_propval, target,
> FALSE, opt_state->force,
... hmm, you've got propset3, so that should be trunk which supports
propedit-URL. Unfortunately I can't test the patch right now; what's
not working about this feature with propedit?
(As an unrelated note, it looks like "svn propedit --revprop -m" fails
to report the "you don't need a log message" error, like with "svn mv
-m" as you separately reported.)
[...]
> 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.
[...]
Sorry for only giving such a superficial review; I hope I'll have time to
actually test out the next version of your patch!
--dave
--
David Glasser | glasser_at_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
Received on Fri Feb 9 16:14:03 2007