Re: [Patch] subcommand-specific option descriptors

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Christian Stork wrote:
> [[[
>
> Add subcommand-specific option descriptors in a new field in
> svn_cl__cmd_table entries and use them for more sensible help
> messages.
> To illustrate, entries for -m and -F are added.

Thanks for the patch (and sorry for taking so long to get back to this
topic).

I've committed (r18522) changes that accomplish this.

Here are my comments on your patch, to explain why I didn't use it directly.

> * trunk/subversion/include/svn_opt.h (svn_opt_option_desc_t):
> Add opt_desc_table field for subcommand-specific option description array.
> (svn_opt_subcommand_desc_t, svn_opt_get_opt_desc_from_code): New.

Message garbled a bit, you've got svn_opt_option_desc_t and
svn_opt_subcommand_desc_t swapped.

In any case, I think we can use an anonymous struct definition here.

> * trunk/subversion/libsvn_subr/opt.c (print_command_info,
> svn_opt_get_opt_desc_from_code): Print subcommand-specific option
> descriptions.

I feel it is better to make svn_opt_get_option_from_code return the
proper description, rather than force a two-stage get operation on callers.

> * trunk/subversion/clients/cmdline/main.c (svn_cl__cmd_table): Fill
> subcommand-specific options_table entries. For now, I only considered
> the -m and -F options.

We can use the C compiler's 'static initializer zero-fill' behaviour to
eliminate the empty lists. (Also, contrary to the log message, you
didn't actually include any overridden descriptions in the patch).

> I know that changing the public type svn_opt_subcommand_desc_t needs a
> new name, etc.

Indeed. It's the bulk of the changes, even.

> + apr_getopt_option_t *option;
> + option = apr_palloc(pool, sizeof(option));
> + memcpy(option,
> + svn_opt_get_option_from_code(code, options_table),
> + sizeof(option));

This is broken - I suspect it would have printed uninitialized data. To
make it work, you would need sizeof(*option) [the struct] rather than
sizeof(option) [a pointer, much fewer bytes].

Also, to assign to a struct, it's a lot clearer to just do a direct
assignment, than a memcpy():

*option = *svn_opt_get_option_from_code(code, options_table);

Max.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (Cygwin)

iD8DBQFD93HPfFNSmcDyxYARAjHQAJ4w8UdjkDYf2BPKTRA9xGkQskKvqQCeOFHu
XkU/1gBiC9yPq9t1SM0vZCQ=
=PQwA
-----END PGP SIGNATURE-----

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Feb 18 20:13:43 2006