Hi Jonathan,
Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>
> > Introduce three new command-line options: --continue, --abort, and
> > --skip resembling the correspoding options in "rebase -i". For now,
> > just parse the options into the replay_opts structure, making sure
> > that two of them are not specified together. They will actually be
> > implemented later in the series.
>
> I'd suggest squashing this patch with the next one. If a "git
> cherry-pick" accepting an --abort option that does not do anything
> leaked into the wild, that would not be a good outcome.
What about --continue and --skip? They're no-ops too here, and
there'll soon be patches adding the functionality. Do you think it's
alright to parse and exit immediately?
> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> > @@ -145,7 +153,47 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
> > opts->xopts_nr = xopts_nr;
> > opts->xopts_alloc = xopts_alloc;
> >
> > - if (opts->commit_argc < 2)
> > + /* Check for incompatible command line arguments */
> > + if (opts->abort_oper || opts->skip_oper || opts->continue_oper) {
> > + char *this_oper;
> > + if (opts->abort_oper) {
> > + this_oper = "--abort";
> > + die_opt_incompatible(me, this_oper,
> > + "--skip", opts->skip_oper,
> > + NULL);
> > + die_opt_incompatible(me, this_oper,
> > + "--continue", opts->continue_oper,
> > + NULL);
>
> What happened to
>
> ...(me, "--abort",
> "--skip", opts->skip,
> "--continue", opts->continue);
Huh? Why? I've caught every possible combination of two of those
options -- that already covers all three.
> ? I also wonder if there should not be a function to deal with
> mutually incompatible options:
>
> va_start(ap, commandname);
> while ((arg1 = va_arg(ap, const char *))) {
> int set = va_arg(ap, int);
> if (set)
> break;
> }
> while ((arg2 = va_arg(ap, const char *))) {
> int set = va_arg(ap, int);
> if (set)
> die(arg1 and arg2 are incompatible);
> }
> va_end(ap);
I personally think having a function is cleaner: I even like the new
API suggested by Junio. We can probably even move it to a common
place, and have others use it as well.
> > + die_opt_incompatible(me, this_oper,
> > + "--no-commit", opts->no_commit,
> [...]
>
> Seems reasonable. A part of me would want to accept such options and
> only error out if the saved state indicates that they are different
> from the options supplied before, so if a person has
>
> alias applycommits = git cherry-pick --no-commit
>
> then "applycommits --continue" could work without trouble, but
> that's probably overegineering.
Over-engineering definitely! I'm looking to get something working
first; add-on functionality like this can come as later patches.
And yes, as you pointed out in another review, the name
verify_opt_incompatible_or_die is more appropriate.
Thanks for the detailed review.
-- Ram
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html