Hi Elijah,
On Tue, 8 Feb 2022, Elijah Newren wrote:
> On Fri, Jan 28, 2022 at 3:27 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@xxxxxxxxx> 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.
Makes sense.
While at it, I also explicitly handle the `-h` case and no longer bother
with the `parse_options()` call at all anymore.
>
> > 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?
Right, we can just "shift the command back in".
Thank you,
Dscho
