Re: [PATCH 7/8] revert: Implement parsing --continue, --abort and --skip
- From
Ramkumar Ramachandra <artagnon@gmail.com>
- Date
- May 13, 2011, 09:16 UTC
- Message-ID
- <20110513091619.GC14272@ramkum.desktop.amazon.com>
- In-Reply-To
- <20110511125900.GH2676@elie>
Hi Jonathan,
Jonathan Nieder writes:
Show 11 quoted lines
> 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?
Show 24 quoted lines
> > --- 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.
Show 15 quoted lines
> ? 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.
Show 12 quoted lines
> > + 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