This review is a picky one. All of these comments are about the help text; I
didn't find any other problems this time (but I know there are ongoing
discussions about negative changes).
Please post the log message along with each version of the patch.
Daniel Berlin wrote:
>>
>>Index: subversion/clients/cmdline/main.c
>>===================================================================
>>--- subversion/clients/cmdline/main.c (revision 17002)
>>+++ subversion/clients/cmdline/main.c (working copy)
>>@@ -69,6 +69,7 @@ const apr_getopt_option_t svn_cl__option
>> {"quiet", 'q', 0, N_("print as little as possible")},
>> {"recursive", 'R', 0, N_("descend recursively")},
>> {"non-recursive", 'N', 0, N_("operate on single directory only")},
>>+ {"change", 'c', 1, N_("Shorthand for the revision range NUMBER-1:NUMBER")},
OK, I see you changed this to indicate that the argument has to be a number;
but the help printing system calls the argument "arg", not "number". I wonder
if we can improve on this further? Maybe
"the change made by revision ARG (like -r ARG-1:ARG)"
But devising really good, concise help text is an art and we shouldn't hold off
on this patch just for this. We can always improve it later.
The descriptions generally start with a lower-case letter.
>> {"revision", 'r', 1, N_
>> ("ARG (some commands also take ARG1:ARG2 range)\n"
>> " A revision argument can be one of:\n"
(This help text for "-r" is an odd case, and is not the best-written example in
the world.)
>>@@ -276,7 +277,7 @@ const svn_opt_subcommand_desc_t svn_cl__
>>
>> { "diff", svn_cl__diff, {"di"},
>> N_("Display the differences between two paths.\n"
>>- "usage: 1. diff [-r N[:M]] [TARGET[@REV]...]\n"
>>+ "usage: 1. diff [-r N[:M] -c L] [TARGET[@REV]...]\n"
Missing "|" character.
Need to be consistent about whether its argument is called "L" or "M" or "N":
for "merge" you call it "N", and I recommend the same here.
>> " 2. diff [-r N[:M]] --old=OLD-TGT[@OLDREV] "
Why not case 2 as well?
Shouldn't the body of the description mention "-c", too, like you did for "merge"?
>> "[--new=NEW-TGT[@NEWREV]] \\\n"
>> " [PATH...]\n"
>> "usage: 1. merge sourceURL1[@N] sourceURL2[@M] [WCPATH]\n"
>> " 2. merge sourceWCPATH1@N sourceWCPATH2@M [WCPATH]\n"
>>- " 3. merge -r N:M SOURCE[@REV] [WCPATH]\n"
>>+ " 3. merge [-r N:M | -c N] SOURCE[@REV] [WCPATH]\n"
>>- " M. If REV is not specified, HEAD is assumed.\n"
>>+ " M in the case of -r, and as it existed between revisions N-1 and \n"
>>+ " N in the case of -c. If REV is not specified, HEAD is assumed.\n"
>> case 'r':
>> if (opt_state.start_revision.kind != svn_opt_revision_unspecified)
>> {
>> err = svn_error_create
>> (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
>> _("Multiple revision arguments encountered; "
>>+ "can't specifiy -r and -c, or"
"specifiy" -> "specify".
Missing space after "or".
>> "try '-r M:N' instead of '-r M -r N'"));
>> return svn_cmdline_handle_exit_error (err, pool, "svn: ");
>> }
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Oct 27 12:53:57 2005