Re: [PATCH 07/11] bisect: move even the option parsing to `bisect--helper`

> On Fri, Jan 28, 2022 at 3:27 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>
> > [...]
> > +       const char *command = argc > 1 ? argv[1] : "help";
> >
> > -       argc = parse_options(argc, argv, prefix, options,
> > -                            git_bisect_helper_usage,
> > -                            PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN);
> > +       /* Handle -h and invalid options */
> > +       parse_options(argc - 1, argv + 1, prefix, options,
> > +                     git_bisect_usage,
> > +                     PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN |
> > +                     PARSE_OPT_ONE_SHOT | PARSE_OPT_STOP_AT_NON_OPTION);
> >
> > -       if (!cmdmode && argc > 0) {
> > +       if (!strcmp("help", command))
> > +               usage_with_options(git_bisect_usage, options);
>
> I think the rest of the changes in this file would be much easier to
> read if you did a
>    argc -= 2
>    argv += 2
> here...
>
> > +       else if (!strcmp("start", command)) {
>
> ...and dropped the else here, just starting a new 'if' instead.

>
> >                 set_terms(&terms, "bad", "good");
> > -               get_terms(&terms);
> > -               if (!check_and_set_terms(&terms, argv[0]))
> > -                       cmdmode = BISECT_STATE;
> > -       }
> > -
> > -       if (!cmdmode)
> > -               usage_with_options(git_bisect_helper_usage, options);
> > -
> > -       switch (cmdmode) {
> > -       case BISECT_START:
> > -               set_terms(&terms, "bad", "good");
> > -               res = bisect_start(&terms, argv, argc);
> > -               break;
> > -       case BISECT_TERMS:
> > -               if (argc > 1)
> > -                       return error(_("--bisect-terms requires 0 or 1 argument"));
> > -               res = bisect_terms(&terms, argc == 1 ? argv[0] : NULL);
> > -               break;
> > -       case BISECT_SKIP:
> > +               res = bisect_start(&terms, argv + 2, argc - 2);
> > +       } else if (!strcmp("terms", command)) {
> > +               if (argc > 3)
> > +                       return error(_("'terms' requires 0 or 1 argument"));
> > +               res = bisect_terms(&terms, argc == 3 ? argv[2] : NULL);
> > +       } else if (!strcmp("skip", command)) {
> >                 set_terms(&terms, "bad", "good");
> >                 get_terms(&terms);
> > -               res = bisect_skip(&terms, argv, argc);
> > -               break;
> > -       case BISECT_NEXT:
> > -               if (argc)
> > -                       return error(_("--bisect-next requires 0 arguments"));
> > +               res = bisect_skip(&terms, argv + 2, argc - 2);
> > +       } else if (!strcmp("next", command)) {
> > +               if (argc != 2)
> > +                       return error(_("'next' requires 0 arguments"));
> >                 get_terms(&terms);
> >                 res = bisect_next(&terms, prefix);
> > -               break;
> > -       case BISECT_RESET:
> > -               if (argc > 1)
> > -                       return error(_("--bisect-reset requires either no argument or a commit"));
> > -               res = bisect_reset(argc ? argv[0] : NULL);
> > -               break;
> > -       case BISECT_VISUALIZE:
> > +       } else if (!strcmp("reset", command)) {
> > +               if (argc > 3)
> > +                       return error(_("'reset' requires either no argument or a commit"));
> > +               res = bisect_reset(argc > 2 ? argv[2] : NULL);
> > +       } else if (one_of(command, "visualize", "view", NULL)) {
> >                 get_terms(&terms);
> > -               res = bisect_visualize(&terms, argv, argc);
> > -               break;
> > -       case BISECT_REPLAY:
> > -               if (argc != 1)
> > +               res = bisect_visualize(&terms, argv + 2, argc - 2);
> > +       } else if (!strcmp("replay", command)) {
> > +               if (argc != 3)
> >                         return error(_("no logfile given"));
> >                 set_terms(&terms, "bad", "good");
> > -               res = bisect_replay(&terms, argv[0]);
> > -               break;
> > -       case BISECT_LOG:
> > -               if (argc)
> > -                       return error(_("--bisect-log requires 0 arguments"));
> > +               res = bisect_replay(&terms, argv[2]);
> > +       } else if (!strcmp("log", command)) {
> > +               if (argc > 2)
> > +                       return error(_("'log' requires 0 arguments"));
> >                 res = bisect_log();
> > -               break;
> > -       case BISECT_RUN:
> > -               if (!argc)
> > +       } else if (!strcmp("run", command)) {
> > +               if (argc < 3)
> >                         return error(_("bisect run failed: no command provided."));
> >                 get_terms(&terms);
> > -               res = bisect_run(&terms, argv, argc);
> > -               break;
> > -       case BISECT_STATE:
> > -               if (!terms.term_good) {
> > -                       set_terms(&terms, "bad", "good");
> > -                       get_terms(&terms);
> > +               res = bisect_run(&terms, argv + 2, argc - 2);
> > +       } else {
> > +               set_terms(&terms, "bad", "good");
> > +               get_terms(&terms);
> > +               if (!check_and_set_terms(&terms, command))
> > +                       res = bisect_state(&terms, argv + 1, argc - 1);
>
> Oh, hmm... this one is +1,-1 while all others are +2,-2, because there
> is not a separate "command"; the command was implicit and so that
> reserved argument is actually part of the terms.  Still if we did the
> offset by two trick above, I guess we'd just add a comment here and do
> argv-1, argc+1?