Hi,
I've taken a closer look at your patch except for the editor part
which I commented on two days ago. There are some other things that could
be addressed while we make a decision on that one.
I've included some style nits in my comments below, which obviously
can be addressed by the one committing the final version of this
patch. However, since a new version of this patch will be required
anyway, and if you are going to submit more patches in the future,
they might be useful.
Thanks a lot for your work so far. Patches spanning all RA layers aren't
the easiest thing.
//Peter
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.
> 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?
>
> Index: subversion/libsvn_client/client.h
> ===================================================================
> --- subversion/libsvn_client/client.h (revision 23480)
> +++ subversion/libsvn_client/client.h (working copy)
> @@ -779,7 +779,6 @@
> apr_pool_t *pool);
>
>
> -
> /* Retrieve log messages using the first provided (non-NULL) callback
> in the set of *CTX->log_msg_func3, CTX->log_msg_func2, or
> CTX->log_msg_func. Other arguments same as
Please avoid unrelated spurious whitespace changes, because they
make merging harder later on.
> @@ -790,6 +789,15 @@
> const apr_array_header_t *commit_items,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
> +
> +
> +/* Set the revision properties for this edit by calling the change_rev_prop()
> + * editor function of editor for all (key, value)-pairs in revprop_table.
> + */
> +svn_error_t *
> +svn_client__set_rev_props(const svn_delta_editor_t *editor, void *edit_baton,
> + apr_hash_t *revprop_table, apr_pool_t *pool);
> +
In internal function documentation, parameter names should be capitalized.
The use of POOL should be documented and the types of the keys/values of
REVPROP_TABLE should be mentioned.
>
> #ifdef __cplusplus
> }
> Index: subversion/libsvn_client/prop_commands.c
> ===================================================================
> --- subversion/libsvn_client/prop_commands.c (revision 23480)
> +++ subversion/libsvn_client/prop_commands.c (working copy)
> @@ -284,6 +284,15 @@
> return err;
> }
>
> + err = svn_client__set_rev_props(editor, edit_baton, ctx->revprop_table,
> + pool);
> + if (err)
> + {
> + /* At least try to abort the edit (and fs txn) before throwing err. */
> + svn_error_clear(editor->abort_edit(edit_baton, pool));
> + return err;
> + }
> +
Instead of copying the abort-edit-on-error part, when we have more than
one call that needs this, we can do something like:
err = call1();
if (! err)
err = call2();
if (! err)
err = call3();
...
if (err)
... /* Abort edit and return. */
/* Close edit */
For two calls, it might not be that significant, but this approach scales
better IMO. We have the same in other places, which I'm not going to mention.
> 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.
> Index: subversion/tests/cmdline/commit_tests.py
> ===================================================================
> --- subversion/tests/cmdline/commit_tests.py (revision 23480)
> +++ subversion/tests/cmdline/commit_tests.py (working copy)
> @@ -2004,6 +2004,280 @@
> 'commit', '-m', 'log message',
> wc_dir)
>
> +
> +def mkdir_with_revprop(sbox):
> + "set revision props during remote mkdir"
> +
> + sbox.build()
> + remote_dir = sbox.repo_url + "/dir"
> + outlines, errlines = svntest.main.run_svn(None, 'mkdir', '-m', 'msg',
> + '--with-revprop', 'bug=42',
> + remote_dir)
> + if errlines != []:
> + raise svntest.Failure
> +
Using run_and_verify_svn here gives better output in case of a failure.
(You can pass None for EXPECTED_STDOUT if you don't want to check it).
> + expected = ['Unversioned properties on revision 2:\n',
> + ' svn:log\n', ' svn:author\n',
> + ' bug\n', ' svn:date\n']
Here, you depend on the order of the listed properties, which is not
guaranteed. Maybe we need an run_and_verify_proplist?
> +def commit_with_revprop(sbox):
> + "set revision props during commit"
> +
> + sbox.build()
> + local_file = os.path.join(sbox.wc_dir, 'file')
> + svntest.main.file_write(local_file, "xxxx")
> + svntest.actions.run_and_verify_svn(sbox.wc_dir, None, [], 'add', local_file)
> +
> + was_cwd = os.getcwd()
> + os.chdir(sbox.wc_dir)
> + outlines, errlines = svntest.main.run_svn(None, 'commit', '-m', 'msg',
> + '--with-revprop', 'bug=62')
> + os.chdir(was_cwd)
If you temporarily need to change the current directory, you need to do that
in a try/finally statement so the remaining tests don't start failing
if this one fails. But why not use run_and_verify_commit and actually verify
that the commit actually worked as expected?
> @@ -2044,8 +2318,21 @@
> local_mods_are_not_commits,
> post_commit_hook_test,
> commit_same_folder_in_targets,
> - commit_inconsistent_eol
> - ]
> + commit_inconsistent_eol,
> + 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
> + ]
>
A very minor nit: you can add a comma in the last line making the diff look
better the next time;)
Anyway, I think the tests have good coverage (except for unchecked commits).
Maybe they could be refactored a bit to extract common functionality?
> @@ -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?
> Index: subversion/svn/cl.h
> ===================================================================
> --- subversion/svn/cl.h (revision 23480)
> +++ subversion/svn/cl.h (working copy)
> @@ -81,7 +81,8 @@
> svn_cl__targets_opt,
> svn_cl__version_opt,
> svn_cl__xml_opt,
> - svn_cl__keep_local_opt
> + svn_cl__keep_local_opt,
> + svn_cl__with_revprop_opt,
> } svn_cl__longopt_t;
However, here you can't add a comma after the last field, because that's C99
and we are still stuck in C90. ;)
>
>
> Index: subversion/svn/main.c
> ===================================================================
> --- subversion/svn/main.c (revision 23480)
> +++ subversion/svn/main.c (working copy)
> @@ -872,6 +877,39 @@
> }
>
>
> +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"));
> +
> + 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);
> + if (svn_prop_is_svn_prop(propname))
> + return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("Cannot set standard (revision) properties "
> + "using --with-revprop"));
> +
This has to be enforced on the server side to be effective.
> Index: subversion/svn/propedit-cmd.c
> ===================================================================
> --- subversion/svn/propedit-cmd.c (revision 23480)
> +++ 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)
Please keep lines under 80 characters in length.
> Index: subversion/libsvn_ra_dav/commit.c
> ===================================================================
> --- subversion/libsvn_ra_dav/commit.c (revision 23480)
> +++ subversion/libsvn_ra_dav/commit.c (working copy)
> @@ -580,7 +580,93 @@
> }
>
>
> +static svn_error_t * apply_rev_prop(commit_ctx_t *cc,
> + const char *name,
> + const svn_string_t *value,
> + apr_pool_t *pool)
I know that ra_dav is not known for its extensive documentation, but it is
always good to document new functions;)
> + /* To set the log message, we must checkout the latest baseline
> + and get back a mutable "working" baseline. */
Shouldn't this comment be expressed more generally now that we deal with
other revprops?
> @@ -1280,69 +1366,12 @@
> const char *log_msg,
> apr_pool_t *pool)
> {
> - const svn_string_t *vcc;
> - const svn_string_t *baseline_url;
> - version_rsrc_t baseline_rsrc = { SVN_INVALID_REVNUM };
> - svn_error_t *err = NULL;
> - int retry_count = 5;
> + svn_string_t *log_str = svn_string_create(log_msg, pool);
>
> - /* ### this whole sequence can/should be replaced with an expand-property
> - ### REPORT when that is available on the server. */
> + return apply_rev_prop(cc, SVN_PROP_PREFIX "log", log_str, pool);
> +}
>
This wrapper (apply_log) seems a bit unnecessary now.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Feb 26 10:09:17 2007