'svnadmin setrevprop' command and its interface (r21736)

[ Dropping commits list from CC list. ]

On Mon, 02 Oct 2006, C. Michael Pilato wrote:

> Dan, I was just Friday thinking that we needed to add such a thing.
> Thanks for adding it!

I'm happy to hear that I'm not the only one who thought it's something
we need. (We probably thought about it for the same reason.)

> However, I'm not thrilled about the all-or-nothing approach you
> inherited from 'svnadmin setlog' in the hook bypassing stuff. There are
> many times where an admin would like to bypass the pre-revprop-change
> hook (because, as an admin, he or she has a right to not get bounced by
> any rules present in that hook) but still wants to have the
> post-revprop-change hook fire. The same reasoning explains why I added
> both '--use-pre-commit-hook' and '--use-post-commit-hook' as individual
> options to 'svnadmin load'. I want that level of control in this case, too.
>
> Have you an opinion on the matter, or were you simply shooting for
> low-hanging expansion of 'setlog' to something more generic?

Mike, I'm completely open to accomodating any reasonable behavioral
refinements, and I really like your suggestion of finer-grained
control over the hooks (and even more so because it'll make us more
consistent with 'svnadmin load').

'svnadmin setlog' seems like a one-off to me -- I was only using it as
a starting point, and was really hoping to get some discussion going
around this by kicking things off with a commit. ;-)

My desired behavior for 'svnadmin setrevprop' is be:

o Bypass revprop change hook scripts by default, as this is a program
  intended for repository administration. This behavior differs from
  that of 'svnadmin setlog'.

o Allow hook scripts to be invoked using command-line flags, a la
  'svnadmin load':
    --use-pre-revprop-change-hook
    --use-post-revprop-change-hook

I'd be happy to implement the above.

I'd also like to float whether anyone thinks that we should name the
command 'setprop' or 'propset' instead of 'setrevprop' (it currently
can set only revprops, and setting normal properties requires that a
full commit be made). I'm not volunteering to implement setting of
normal properties, nor am I even sure that would be a good idea, but
neither do I want to release an interface which is not extension-able.

Thoughts?

> dlr@tigris.org wrote:
> > Author: dlr
> > Date: Mon Oct 2 14:33:25 2006
> > New Revision: 21736
> >
> > Log:
> > Add an 'svnadmin setrevprop' command. This new generic command could
> > theoretically replace 'svnadmin setlog'.

--- additional tidbit subsequently added to the log msg ---
Use case: To enable repository administrators (e.g. those with shell
access to the repository) to change revision properties in a manner
which would normally be prohibited by the repository configuration
without temporarily re-configuring the repos. This situation could
arise either because there's no pre-revprop-change hook script, or
because the hook script prevents modification of certain properties
(e.g. "svn:author").
--- end tidbit --

> > Alternately, we could rename this command 'setprop', if there's a
> > desire that 'svnadmin' could be used eventually to set normal
> > properties (meaning that it would be making commits).
> >
> >
> > * subversion/svnadmin/main.c
> > (set_revprop): A helper function which contains the guts of
> > 'setrevprop' and 'setlog', factored out of the previous 'setlog'
> > implementation.
> >
> > (subcommand_setlog): Delegate to set_revprop().
> >
> > (subcommand_setrevprop): Add new sub-command function, and to
> > svn_opt_subcommand_t list.
> >
> > (cmd_table): Add help for "setrevprop".
> >
> > * subversion/tests/cmdline/svnadmin_tests.py
> > (setrevprop): A new test for 'setlog' and 'setrevprop', which
> > bypasses hook scripts.
> >
> > (test_list): Add setrevprop to the list.
> >
> >
> > Modified:
> > trunk/subversion/svnadmin/main.c
> > trunk/subversion/tests/cmdline/svnadmin_tests.py
> >
> > Modified: trunk/subversion/svnadmin/main.c
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/svnadmin/main.c?pathrev=21736&r1=21735&r2=21736
> > ==============================================================================
> > --- trunk/subversion/svnadmin/main.c (original)
> > +++ trunk/subversion/svnadmin/main.c Mon Oct 2 14:33:25 2006
> > @@ -194,6 +194,7 @@
> > subcommand_recover,
> > subcommand_rmlocks,
> > subcommand_rmtxns,
> > + subcommand_setrevprop,
> > subcommand_setlog,
> > subcommand_verify;
> >
> > @@ -386,6 +387,19 @@
> > "Delete the named transaction(s).\n"),
> > {'q'} },
> >
> > + {"setrevprop", subcommand_setrevprop, {0}, N_
> > + ("usage: svnadmin setrevprop REPOS_PATH -r REVISION NAME FILE\n\n"
> > + "Set the property NAME on revision REVISION to the contents of FILE. Use\n"
> > + "--bypass-hooks to avoid triggering the revision-property-related hooks\n"
> > + "(for example, if you do not want an email notification sent\n"
> > + "from your post-revprop-change hook, or because the modification of\n"
> > + "revision properties has not been enabled in the pre-revprop-change\n"
> > + "hook).\n\n"
> > + "NOTE: Revision properties are not versioned (e.g. no revision history\n"
> > + "is maintained), so this command will permanently overwrite the previous\n"
> > + "value for the property.\n"),
> > + {'r', svnadmin__bypass_hooks} },
> > +
> > {"setlog", subcommand_setlog, {0}, N_
> > ("usage: svnadmin setlog REPOS_PATH -r REVISION FILE\n\n"
> > "Set the log-message on revision REVISION to the contents of FILE. Use\n"
> > @@ -911,41 +925,25 @@
> > }
> >
> >
> > -/* This implements `svn_opt_subcommand_t'. */
> > +/* A helper for the 'setrevprop' and 'setlog' commands. */
> > static svn_error_t *
> > -subcommand_setlog(apr_getopt_t *os, void *baton, apr_pool_t *pool)
> > +set_revprop(const char *prop_name, const char *filename,
> > + struct svnadmin_opt_state *opt_state, apr_pool_t *pool)
> > {
> > - struct svnadmin_opt_state *opt_state = baton;
> > svn_repos_t *repos;
> > + svn_string_t *prop_value = svn_string_create("", pool);
> > svn_stringbuf_t *file_contents;
> > const char *filename_utf8;
> > - apr_array_header_t *args;
> > - svn_string_t *log_contents = svn_string_create("", pool);
> >
> > - if (opt_state->start_revision.kind != svn_opt_revision_number)
> > - return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > - _("Missing revision"));
> > - else if (opt_state->end_revision.kind != svn_opt_revision_unspecified)
> > - return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > - _("Only one revision allowed"));
> > -
> > - SVN_ERR(svn_opt_parse_all_args(&args, os, pool));
> > -
> > - if (args->nelts != 1)
> > - return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > - _("Exactly one file argument required"));
> > -
> > - SVN_ERR(svn_utf_cstring_to_utf8(&filename_utf8,
> > - APR_ARRAY_IDX(args, 0, const char *),
> > - pool));
> > + SVN_ERR(svn_utf_cstring_to_utf8(&filename_utf8, filename, pool));
> > filename_utf8 = svn_path_internal_style(filename_utf8, pool);
> > +
> > SVN_ERR(svn_stringbuf_from_file(&file_contents, filename_utf8, pool));
> >
> > - log_contents->data = file_contents->data;
> > - log_contents->len = file_contents->len;
> > + prop_value->data = file_contents->data;
> > + prop_value->len = file_contents->len;
> >
> > - SVN_ERR(svn_subst_translate_string(&log_contents, log_contents,
> > - NULL, pool));
> > + SVN_ERR(svn_subst_translate_string(&prop_value, prop_value, NULL, pool));
> >
> > /* Open the filesystem */
> > SVN_ERR(open_repos(&repos, opt_state->repository_path, pool));
> > @@ -956,14 +954,14 @@
> > {
> > svn_fs_t *fs = svn_repos_fs(repos);
> > SVN_ERR(svn_fs_change_rev_prop
> > - (fs, opt_state->start_revision.value.number,
> > - SVN_PROP_REVISION_LOG, log_contents, pool));
> > + (fs, opt_state->start_revision.value.number,
> > + prop_name, prop_value, pool));
> > }
> > else
> > {
> > SVN_ERR(svn_repos_fs_change_rev_prop2
> > (repos, opt_state->start_revision.value.number,
> > - NULL, SVN_PROP_REVISION_LOG, log_contents, NULL, NULL, pool));
> > + NULL, prop_name, prop_value, NULL, NULL, pool));
> > }
> >
> > return SVN_NO_ERROR;
> > @@ -972,6 +970,59 @@
> >
> > /* This implements `svn_opt_subcommand_t'. */
> > static svn_error_t *
> > +subcommand_setrevprop(apr_getopt_t *os, void *baton, apr_pool_t *pool)
> > +{
> > + struct svnadmin_opt_state *opt_state = baton;
> > + apr_array_header_t *args;
> > +
> > + if (opt_state->start_revision.kind != svn_opt_revision_number)
> > + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > + _("Missing revision"));
> > + else if (opt_state->end_revision.kind != svn_opt_revision_unspecified)
> > + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > + _("Only one revision allowed"));
> > +
> > + SVN_ERR(svn_opt_parse_all_args(&args, os, pool));
> > +
> > + if (args->nelts != 2)
> > + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > + _("Exactly one property name and one file "
> > + "argument required"));
> > +
> > + return set_revprop(APR_ARRAY_IDX(args, 0, const char *),
> > + APR_ARRAY_IDX(args, 1, const char *),
> > + opt_state, pool);
> > +}
> > +
> > +
> > +/* This implements `svn_opt_subcommand_t'. */
> > +static svn_error_t *
> > +subcommand_setlog(apr_getopt_t *os, void *baton, apr_pool_t *pool)
> > +{
> > + struct svnadmin_opt_state *opt_state = baton;
> > + apr_array_header_t *args;
> > +
> > + if (opt_state->start_revision.kind != svn_opt_revision_number)
> > + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > + _("Missing revision"));
> > + else if (opt_state->end_revision.kind != svn_opt_revision_unspecified)
> > + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > + _("Only one revision allowed"));
> > +
> > + SVN_ERR(svn_opt_parse_all_args(&args, os, pool));
> > +
> > + if (args->nelts != 1)
> > + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > + _("Exactly one file argument required"));
> > +
> > + return set_revprop(SVN_PROP_REVISION_LOG,
> > + APR_ARRAY_IDX(args, 0, const char *),
> > + opt_state, pool);
> > +}
...