You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Rall <dl...@collab.net> on 2006/10/02 22:36:16 UTC
'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);
> > +}
...
Re: 'svnadmin setrevprop' command and its interface (r21736)
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 03 Oct 2006, Malcolm Rowe wrote:
> On Tue, Oct 03, 2006 at 09:29:27AM -0400, Garrett Rooney wrote:
> > I prefer setrevprop. I think actually making commits to the
> > repository (other than the special case of dump/load) is outside the
> > realm of svnadmin, and should remain that way.
>
> +1. My thoughts exactly.
Thanks guys, sounds fine to me.
Re: 'svnadmin setrevprop' command and its interface (r21736)
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Tue, Oct 03, 2006 at 09:29:27AM -0400, Garrett Rooney wrote:
> I prefer setrevprop. I think actually making commits to the
> repository (other than the special case of dump/load) is outside the
> realm of svnadmin, and should remain that way.
>
+1. My thoughts exactly.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: 'svnadmin setrevprop' command and its interface (r21736)
Posted by "C. Michael Pilato" <cm...@collab.net>.
Daniel Rall wrote:
> Is this something we should consider for backport to the 1.4.x line?
Do we do new features in patch releases? My recollection is "no".
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: 'svnadmin setrevprop' command and its interface (r21736)
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 03 Oct 2006, C. Michael Pilato wrote:
> Daniel Rall wrote:
> > On Tue, 03 Oct 2006, Garrett Rooney wrote:
> >
> >> On 10/2/06, Daniel Rall <dl...@collab.net> wrote:
> >>
> >>> 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.
> >> Perhaps something like --skip-hook-scripts with a default of skipping
> >> all of them, but optionally taking a list of hooks to skip? Or the
> >> other way around with --run-hook-scripts. Honestly I'm fine with it
> >> the way it is though.
> > ...
> >
> > --skip-hook-scripts is what the existing --bypass-hooks flag does
> > (which Mike doesn't like). Running hook scripts does not strike me as
> > the desired default behavior for an administrative program like
> > 'svnadmin', nor would it be consistent with the 'load' sub-command
> > (though that difference is arguably understandable based on the
> > command core use cases). The main reason to make --bypass-hooks a
> > flag instead of a default is to help administrators unfamiliar with
> > 'svnadmin' from accidentally shooting themselves in the foot.
> >
> > The only hook scripts we run for a 'propset --revprop' are
> > pre-/post-revprop-change hooks. Do we really need a list here? That
> > interface seems less friendly than two well-described, well-named
> > flags.
>
> I think a list is overkill. And I like switching the default to be to
> skip the hooks. Let's add --use-(pre|post)-revprop-change hook. Doing
> so means we don't have to validate an option parameters (these are
> boolean options), and gives us consistency with the 'svnadmin load' hook
> interaction.
Done in r21746.
Is this something we should consider for backport to the 1.4.x line?
Re: 'svnadmin setrevprop' command and its interface (r21736)
Posted by "C. Michael Pilato" <cm...@collab.net>.
Daniel Rall wrote:
> On Tue, 03 Oct 2006, Garrett Rooney wrote:
>
>> On 10/2/06, Daniel Rall <dl...@collab.net> wrote:
>>
>>> 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.
>> Perhaps something like --skip-hook-scripts with a default of skipping
>> all of them, but optionally taking a list of hooks to skip? Or the
>> other way around with --run-hook-scripts. Honestly I'm fine with it
>> the way it is though.
> ...
>
> --skip-hook-scripts is what the existing --bypass-hooks flag does
> (which Mike doesn't like). Running hook scripts does not strike me as
> the desired default behavior for an administrative program like
> 'svnadmin', nor would it be consistent with the 'load' sub-command
> (though that difference is arguably understandable based on the
> command core use cases). The main reason to make --bypass-hooks a
> flag instead of a default is to help administrators unfamiliar with
> 'svnadmin' from accidentally shooting themselves in the foot.
>
> The only hook scripts we run for a 'propset --revprop' are
> pre-/post-revprop-change hooks. Do we really need a list here? That
> interface seems less friendly than two well-described, well-named
> flags.
I think a list is overkill. And I like switching the default to be to
skip the hooks. Let's add --use-(pre|post)-revprop-change hook. Doing
so means we don't have to validate an option parameters (these are
boolean options), and gives us consistency with the 'svnadmin load' hook
interaction.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: 'svnadmin setrevprop' command and its interface (r21736)
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 03 Oct 2006, Garrett Rooney wrote:
> On 10/2/06, Daniel Rall <dl...@collab.net> wrote:
>
> >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.
>
> Perhaps something like --skip-hook-scripts with a default of skipping
> all of them, but optionally taking a list of hooks to skip? Or the
> other way around with --run-hook-scripts. Honestly I'm fine with it
> the way it is though.
...
--skip-hook-scripts is what the existing --bypass-hooks flag does
(which Mike doesn't like). Running hook scripts does not strike me as
the desired default behavior for an administrative program like
'svnadmin', nor would it be consistent with the 'load' sub-command
(though that difference is arguably understandable based on the
command core use cases). The main reason to make --bypass-hooks a
flag instead of a default is to help administrators unfamiliar with
'svnadmin' from accidentally shooting themselves in the foot.
The only hook scripts we run for a 'propset --revprop' are
pre-/post-revprop-change hooks. Do we really need a list here? That
interface seems less friendly than two well-described, well-named
flags.
Re: 'svnadmin setrevprop' command and its interface (r21736)
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 10/2/06, Daniel Rall <dl...@collab.net> wrote:
> 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.
Perhaps something like --skip-hook-scripts with a default of skipping
all of them, but optionally taking a list of hooks to skip? Or the
other way around with --run-hook-scripts. Honestly I'm fine with it
the way it is though.
> 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.
I prefer setrevprop. I think actually making commits to the
repository (other than the special case of dump/load) is outside the
realm of svnadmin, and should remain that way.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org