Julian Foad wrote:
> Mathias Weinert wrote:
> > Karl Fogel wrote:
> >>Mathias Weinert writes:
[...]
>
> > 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.
It seems as if you are wrong here. 'svn help ...' isn't affected by this
patch, you can still get help for multiple commands - although I only
noticed this functionality while testing my patch yesterday and have to
agree to you that this not the most important use case of 'svn help'...
>
> 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.
Thank you.
>
>
> > 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?
svnadmin, svnlook etc.
>
>
> > --- 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"
Thanks.
>
> > + 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.
So what do you propose here?
char* new_argv[2];
new_argv[0] = (char*) argv[0];
new_argv[1] = (char*) subcommand->name;
and a cast to const char* later on?
>
> > + 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"
Thanks.
>
> > + 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".
Absolutely right.
>
> > + }
> > +
> > /* Check that the subcommand wasn't passed any inappropriate options. */
> > for (i = 0; i < received_opts->nelts; i++)
>
Thank you for reviewing my patch!
Mathias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Aug 24 07:24:35 2006