Daniel Berlin wrote:
> This adds an option "-c" (long form --change) that is shorthand for
> "<rev -1>:<rev>", to merge.
What's good for "merge" is good for "diff", in my view. It is inconsistent not
to support it in "diff" as well.
> The only thing i'm a little iffy on is whether we should add more state
> just so we can tell the user they specified both "-r" and "-c", instead
> of just saying we found multiple revision arguments (which could pop up
> from multiple -c's, multiple -r's, or mixing -c and -r)
I wouldn't bother. Just make the error messages sufficiently general.
> The -c option was discussed a while ago, and nobody seemed to object to
> it. They only objected to allowing the same formats that svk allows,
> instead of just the single revision form.
>
> I'll wait a few days for objections or ideas or whatever before
> committing.
Yes, please do wait. This is by no means uncontroversial.
> [[[
>
> Add a -c option to merge to represent change number.
A slightly more precise comment wouldn't go amiss here, and in the subject line
of this email.
>
> * subversion/clients/cmdline/main.c
> (svn_cl__option): Add 'c'/'change' option.
That would be "svn_cl__options".
> (svn_cl__commands): Merge can take 'c'.
That would be "svn_cl__cmd_table".
> (main): Handle parsing for 'c'.
>
> ]]]
>
> --Dan
>
>
>
> ------------------------------------------------------------------------
>
> 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', 0, N_("Shorthand for the revision range ARG1-1:ARG1")},
What exactly is your propsed syntax? I would imagine "-c ARG" as an
alternative to "-r ARG-1:ARG". In that case, you need to specify that this
option takes an argument by changing that "0" to "1", and you'd be better off
saying "ARG" instead of "ARG1" in the description.
> {"revision", 'r', 1, N_
> ("ARG (some commands also take ARG1:ARG2 range)\n"
> " A revision argument can be one of:\n"
> @@ -466,7 +467,7 @@ const svn_opt_subcommand_desc_t svn_cl__
> " If WCPATH is omitted, a default value of '.' is assumed, unless\n"
> " the sources have identical basenames that match a file within '.':\n"
> " in which case, the differences will be applied to that file.\n"),
> - {'r', 'N', 'q', svn_cl__force_opt, svn_cl__dry_run_opt,
> + {'r', 'N', 'q', 'c', svn_cl__force_opt, svn_cl__dry_run_opt,
It would be nice to put 'c' next to 'r' so that the closely related options
appear together in the help.
> svn_cl__merge_cmd_opt, svn_cl__ignore_ancestry_opt,
> SVN_CL__AUTH_OPTIONS, svn_cl__config_dir_opt} },
>
> @@ -914,6 +915,38 @@ main (int argc, const char * const *argv
> opt_state.message = apr_pstrdup (pool, opt_arg);
> dash_m_arg = opt_arg;
> break;
> + case 'c':
> + {
> + char *end;
> + if (opt_state.start_revision.kind != svn_opt_revision_unspecified)
> + {
(Style: Please convert your tab characters to spaces.)
> + err = svn_error_create
> + (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("Multiple revision arguments encountered; "
> + "can't specify -c twice, or both -c and -r"));
You ought to update the "-r" error message correspondingly.
> + return svn_cmdline_handle_exit_error (err, pool, "svn: ");
> + }
> + opt_state.end_revision.value.number = strtol (opt_arg, &end, 10);
> + opt_state.end_revision.kind = svn_opt_revision_number;
> + if (end == opt_arg || *end != '\0')
> + {
> + err = svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("Non-numeric change argument given"));
So the argument must be a number? You haven't explicitly mentioned that.
> + return svn_cmdline_handle_exit_error (err, pool, "svn: ");
> + }
> + else if (opt_state.end_revision.value.number == 0)
> + {
> + err = svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("There is no change 0, changes "
> + "represent the difference from "
> + "revision <change number - 1> "
> + "to <change number>"));
(Not a big deal, but in my opinion that's overly verbose.)
> + return svn_cmdline_handle_exit_error (err, pool, "svn: ");
> + }
> + opt_state.start_revision.value.number = opt_state.end_revision.value.number - 1;
> + opt_state.start_revision.kind = svn_opt_revision_number;
> + }
> + break;
> case 'r':
> if (opt_state.start_revision.kind != svn_opt_revision_unspecified)
> {
My manual first test fails:
~/src/subversion> svn merge -c 16000
Segmentation fault
If we agree that we want this, please would you add a very basic regression
test for this, which could be a modified copy of an existing merge test?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Oct 25 21:53:52 2005