Mathias Weinert wrote:
> Karl Fogel wrote:
>>Mathias Weinert writes:
>>
>>>$ svn info -m "" --help
>>>Subcommand 'help' doesn't accept option '-m [--message] arg'
>>>Type 'svn help help' for usage.
>>>
>>>IMHO the second command should result in something like
>>>"Option 'help' mustn't be combined with other options"
>>>
>>>BTW, in the following, a bit different, situation the message
>>>seems to be okay:
>>>
>>>$ svn help info -m " "
>>>Subcommand 'help' doesn't accept option '-m [--message] arg'
>>>Type 'svn help help' for usage.
>>
>>No, I think these are all unintended, accidental consequences of the
>>way our help system is implemented. (Since help is both a subcommand
>>and an option, though, we'd want to be careful about how we word
>>suggestions to the user.)
>>
>>I'm not sure this is worth fixing, but if you think it is, it
>>shouldn't be too hard to make a patch...
This might be partly my fault, as I recently changed the handling of "--help"
to try to improve it. Sorry about that, if so.
> here comes a patch that changes the above mentioned problem in a way a
> bit different from what I proposed first. This patch makes svn always
> show the help for a given command when --help (or any equivalent) is set
> no matter what other (valid) options are given.
OK, I like this. But what about when invalid options are given? Oh, I see,
you mean any options that svn knows about, regardless whether they are valid
for the particular subcommand.
Your patch removes the ability to request help on multiple subcommands, and so
the help for "help" would have to change from "help [SUBCOMMAND...]" to "help
[SUBCOMMAND]". I think this change is a good thing, as I don't believe there
is any real need for multiple output, but it is incompatible so you should
probably not do that in this patch. Instead, concentrate on ignoring unknown
option switches and do not change this part of the behaviour.
Your patch changes the behaviour in respect of options in this way:
- "help", "h", "?": no change: options that make sense for "help" can be
given, e.g. "--config-dir=".
- "--help", "-h", "-?": all other options that are valid for "svn" are now
ignored. (Options with invalid syntax, e.g. "--revision=a", will still throw
an error.)
I think that is OK, but I want to point it out so that others can comment.
> I find this helpful when I sometimes can't remember a special option but
> already started typing my svn command. Now I can just append -h to what
> I already typed and will get the help text for the command.
>
> What do you think about it? (I'm sure there are more elegant ways to
> achieve this ;-) ).
> If this patch (or another version of it) will be applied I think we
> should also look at the other commands.
What do you mean? What should we look for?
> --- subversion/svn/main.c.orig 2006-07-12 19:07:21.000000000 +0200
> +++ subversion/svn/main.c 2006-08-23 14:33:37.241766500 +0200
> @@ -1191,63 +1191,69 @@
> if (err)
> return svn_cmdline_handle_exit_error(err, pool, "svn: ");
>
> - /* If the user asked for help, then the rest of the arguments are
> - the names of subcommands to get help on (if any), or else they're
> - just typos/mistakes. Whatever the case, the subcommand to
> - actually run is svn_cl__help(). */
> - if (opt_state.help)
> - subcommand = svn_opt_get_canonical_subcommand2(svn_cl__cmd_table, "help");
> -
> - /* If we're not running the `help' subcommand, then look for a
> - subcommand in the first argument. */
> - if (subcommand == NULL)
> + /* Look for a subcommand in the first argument. */
> + if (os->ind >= os->argc)
> {
> - if (os->ind >= os->argc)
> + if (opt_state.version)
> {
> - if (opt_state.version)
> - {
> - /* Use the "help" subcommand to handle the "--version" option. */
> - static const svn_opt_subcommand_desc2_t pseudo_cmd =
> - { "--version", svn_cl__help, {0}, "",
> - {svn_cl__version_opt, /* must accept its own option */
> - 'q', /* brief output */
> - svn_cl__config_dir_opt /* all commands accept this */
> - } };
> + /* Use the "help" subcommand to handle the "--version" option. */
> + static const svn_opt_subcommand_desc2_t pseudo_cmd =
> + { "--version", svn_cl__help, {0}, "",
> + {svn_cl__version_opt, /* must accept its own option */
> + 'q', /* brief output */
> + svn_cl__config_dir_opt /* all commands accept this */
> + } };
>
> - subcommand = &pseudo_cmd;
> - }
> - else
> - {
> - svn_error_clear
> - (svn_cmdline_fprintf(stderr, pool,
> - _("Subcommand argument required\n")));
> - svn_cl__help(NULL, NULL, pool);
> - svn_pool_destroy(pool);
> - return EXIT_FAILURE;
> - }
> + subcommand = &pseudo_cmd;
> }
> else
> {
> - const char *first_arg = os->argv[os->ind++];
> - subcommand = svn_opt_get_canonical_subcommand2(svn_cl__cmd_table,
> - first_arg);
> - if (subcommand == NULL)
> - {
> - const char *first_arg_utf8;
> - err = svn_utf_cstring_to_utf8(&first_arg_utf8, first_arg, pool);
> - if (err)
> - return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> - svn_error_clear
> - (svn_cmdline_fprintf(stderr, pool,
> - _("Unknown command: '%s'\n"),
> - first_arg_utf8));
> - svn_cl__help(NULL, NULL, pool);
> - svn_pool_destroy(pool);
> - return EXIT_FAILURE;
> - }
> + svn_error_clear
> + (svn_cmdline_fprintf(stderr, pool,
> + _("Subcommand argument required\n")));
> + svn_cl__help(NULL, NULL, pool);
> + svn_pool_destroy(pool);
> + return EXIT_FAILURE;
> + }
> + }
> + else
> + {
> + const char *first_arg = os->argv[os->ind++];
> + subcommand = svn_opt_get_canonical_subcommand2(svn_cl__cmd_table,
> + first_arg);
> + if (subcommand == NULL)
> + {
> + const char *first_arg_utf8;
> + err = svn_utf_cstring_to_utf8(&first_arg_utf8, first_arg, pool);
> + if (err)
> + return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> + svn_error_clear
> + (svn_cmdline_fprintf(stderr, pool,
> + _("Unknown command: '%s'\n"),
> + first_arg_utf8));
> + svn_cl__help(NULL, NULL, pool);
> + svn_pool_destroy(pool);
> + return EXIT_FAILURE;
> }
> }
>
> + /* If the user asked for by setting the --help (or equivalent) option
"asked for" -> "asked for help"
> + we define a new dummy command line to use which only specifies the
> + subcommand and the --help option.
> + With this it doesn't matter which other options are set, the user
> + will always get help. */
> + if (opt_state.help)
> + {
> + /* Define a new command line for further use */
> + const char* new_argv[] = {(char*) argv[0], (char*) subcommand->name};
We use ANSI/ISO C 89/90 in which you can't use non-constant initialisers for an
array.
> + err = svn_cmdline__getopt_init(&os, 2, new_argv, pool);
> + if (err)
> + return svn_cmdline_handle_exit_error(err, pool, "svn: ");
> + /* set 'help' as subcommand and reset all saved optiones. */
"options"
> + subcommand = svn_opt_get_canonical_subcommand2(svn_cl__cmd_table, "help");
> + received_opts = apr_array_make(pool, SVN_OPT_MAX_OPTIONS, sizeof(int));
I couldn't understand why you allocate "received_opts" here. The purpose is to
empty it, so it would be clearer if you put in a little comment like "empty the
array of options".
> + }
> +
> /* Check that the subcommand wasn't passed any inappropriate options. */
> for (i = 0; i < received_opts->nelts; i++)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Aug 24 01:46:34 2006