You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Christian Stork <cs...@ics.uci.edu> on 2006/02/15 19:26:46 UTC
[Patch] subcommand-specific option descriptors [Was: "svn add" acts non-recursively without -N given]
On Wed, Feb 15, 2006 at 10:54:07AM -0600, Ben Collins-Sussman wrote:
> On 2/15/06, Molle Bestefich <mo...@gmail.com> wrote:
> > "force operation to run" is like saying that "force" means "force"...
> The problem is that we have a bunch of global flags which can mean
> slightly different things when used with different subcommands. But
> the way our commandline client is designed... well, it only allows a
> single generic description of each global flag. If you'd like to
> redesign that bit of code, it would be nice to have context-specific
> descriptions of the flags. :-)
I send the attached patch to the list a while back to get the ball
rolling on this. Back then I was concerned with documenting the
subcommand-specific meanings of -F.
The only response I got was from Max saying that he'd put it on his todo
list.
This was my first real patch so please be gentle. ;-)
[[[
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.
* trunk/subversion/include/svn_opt.h (svn_opt_subcommand_desc_t): Add
options_table field for subcommand-specific option info.
* trunk/subversion/libsvn_subr/opt.c (print_command_info): Print
subcommand-specific option descriptions.
* 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.
]]]
I know that changing the public type svn_opt_subcommand_desc_t needs a
new name, etc. Let's discuss interface revving if the design of this
patch is accepted.
Note that this patch is duplicating a little bit of information in the
new entries, which might not be a problem since this information
(option name and has_arg flag) should be very static.
Please check if the description for the lock command are correct. I
just assumed so.
Patch against trunk attached. It passes the test suite.
--
Chris Stork <> Support eff.org! <> http://www.ics.uci.edu/~cstork/
OpenPGP fingerprint: B08B 602C C806 C492 D069 021E 41F3 8C8D 50F9 CA2F
Re: [Patch] subcommand-specific option descriptors
Posted by Christian Stork <cs...@ics.uci.edu>.
Thanks Max,
your commit is indeed much improved over my little patch.
-Chris
PS: I didn't know how much code churn you guys are willing to let this
little improvement cost you. So I started with the least invasive
design (my first patch) and the second one was just a quick hack to see
if you guys like the approach (otherwise I would have at least compiled
it, be assured). Anyway I see, you're willing the take all the churn.
;-)
--
Chris Stork <> Support eff.org! <> http://www.ics.uci.edu/~cstork/
OpenPGP fingerprint: B08B 602C C806 C492 D069 021E 41F3 8C8D 50F9 CA2F
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] subcommand-specific option descriptors
Posted by Max Bowsher <ma...@ukf.net>.
-----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
Re: [Patch] subcommand-specific option descriptors
Posted by Christian Stork <cs...@ics.uci.edu>.
On Wed, Feb 15, 2006 at 09:11:26PM +0000, Julian Foad wrote:
> Christian Stork wrote:
> >Note that this patch is duplicating a little bit of information in the
> >new entries, which might not be a problem since this information
> >(option name and has_arg flag) should be very static.
> >@@ -407,7 +425,11 @@
> > " Use --force to steal the lock from another user or working
> > copy.\n"),
> > { svn_cl__targets_opt, 'm', 'F', svn_cl__force_log_opt,
> > svn_cl__encoding_opt, SVN_CL__AUTH_OPTIONS, svn_cl__config_dir_opt,
> >- svn_cl__force_opt } },
> >+ svn_cl__force_opt},
> >+ {{"file", 'F', 1, N_("read lock message from file ARG")},
> >+ {"message", 'm', 1, N_("specify lock message ARG")},
> >+ {0, 0, 0, 0}} },
> Little though it is, I think this is too much duplication of information,
> and would prefer a design that avoids it, perhaps just using the
> single-letter option name to identify the message.
Ok, here's a different version of the patch using your approach. The
downside is that it needs a bit more boilerplate.
[[[
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.
* 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.
* trunk/subversion/libsvn_subr/opt.c (print_command_info,
svn_opt_get_opt_desc_from_code): Print subcommand-specific option
descriptions.
* 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.
]]]
I know that changing the public type svn_opt_subcommand_desc_t needs a
new name, etc. Let's discuss interface revving if the design of this
patch is accepted.
--
Chris Stork <> Support eff.org! <> http://www.ics.uci.edu/~cstork/
OpenPGP fingerprint: B08B 602C C806 C492 D069 021E 41F3 8C8D 50F9 CA2F
Re: [Patch] subcommand-specific option descriptors [Was: "svn add"
acts non-recursively without -N given]
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Wed, 15 Feb 2006, Julian Foad wrote:
> svn add (Also recurse into versioned folders.)
> svn delete Also remove unversioned and modified items.
> svn diff ()
Show diff even if content-type says the file is binary.
> svn export ()
Overwrite existing files or directories.
> svn lock Steal the lock from another user or working copy.
> svn merge ()
Delete locally modified or unversioned files.
> svn move ()
Allow moving of unversioned/modified files.
> svn propedit ()
Skip some validity checking on the new value.
> svn propset ()
Dito.
> svn unlock Break the lock.
>
Extracted from the API documentation.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [Patch] subcommand-specific option descriptors [Was: "svn add"
acts non-recursively without -N given]
Posted by Julian Foad <ju...@btopenworld.com>.
Christian Stork wrote:
> Note that this patch is duplicating a little bit of information in the
> new entries, which might not be a problem since this information
> (option name and has_arg flag) should be very static.
> @@ -407,7 +425,11 @@
> " Use --force to steal the lock from another user or working copy.\n"),
> { svn_cl__targets_opt, 'm', 'F', svn_cl__force_log_opt,
> svn_cl__encoding_opt, SVN_CL__AUTH_OPTIONS, svn_cl__config_dir_opt,
> - svn_cl__force_opt } },
> + svn_cl__force_opt},
> + {{"file", 'F', 1, N_("read lock message from file ARG")},
> + {"message", 'm', 1, N_("specify lock message ARG")},
> + {0, 0, 0, 0}} },
Little though it is, I think this is too much duplication of information, and
would prefer a design that avoids it, perhaps just using the single-letter
option name to identify the message.
As for the particular text that you added, it's fine, but, for the "--file"
option, we should also change the default text to mention that it's for a log
message, and add special text for the only other place that uses it, "propset".
I think the approach of adding specialised versions of the help for certain
commands is OK if we do it with restraint. There is a danger that we might
begin to abuse it by assigning completely different meanings for the same
option. Or maybe we will decide later that doing so is OK in some cases.
"-m/--message" and "-F/--file" seem fine sharing the two meanings (log message
and lock message), with the third meaning of "-F/--file" for the property value
in "propset" being a bit of an oddity.
"--force" is a nasty beast with many disparate meanings, but it's a handy
short-cut for command-line users looking for a "quick fix" to overcome some
restriction that they have just encountered. As long is has a single, precise
meaning for each command, it's also usable by scripts. (If we start needing
more than one meaning per command, we should add options that are more specific.)
Here's a review of the current uses of "--force". Parentheses indicate that
the meaning is currently undocumented in the help text.
svn add (Also recurse into versioned folders.)
svn delete Also remove unversioned and modified items.
svn diff ()
svn export ()
svn lock Steal the lock from another user or working copy.
svn merge ()
svn move ()
svn propedit ()
svn propset ()
svn unlock Break the lock.
We cerainly should document these properly, either in the body of each
command's help text or in your new command-specific option help.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org