| Message ID | dc04b06206bbb833ce3a7fa893d724d00fe58a74.1645547423.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Finish converting git bisect into a built-in | expand |
On Tue, Feb 22 2022, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > On our journey to a fully built-in `git bisect`, this is the > second-to-last step. > > Side note: The `if (!strcmp(...)) ... else if (!strcmp(...)) ... else if > (!strcmp(...)) ...` chain seen in this patch was not actually the first > idea how to convert the command modes to sub-commands. Since the > `bisect--helper` command already used the `parse-opions` API with neatly > set-up command modes, naturally the idea was to use `PARSE_OPT_NODASH` > to support proper sub-commands instead. However, the `parse-options` API > is not set up for that, and even after making that option work with long > options, it turned out that `STOP_AT_NON_OPTION` and `KEEP_UNKNOWN` > would have to be used but these options were not designed to work > together. So it would appear as if a lot of work would have to be done > just to be able to use `parse_options()` just to parse the sub-command, > instead of a simple `if...else if` chain, the latter being a > dramatically simpler implementation. As I noted in https://lore.kernel.org/git/220129.86pmobauyt.gmgdl@evledraar.gmail.com/: > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > builtin/bisect--helper.c | 133 ++++++++++++++++----------------------- > git-bisect.sh | 49 +-------------- > 2 files changed, 56 insertions(+), 126 deletions(-) > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 5228964937d..ef0b06d594b 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -20,18 +20,34 @@ static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES") > static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") > static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN") > > -static const char * const git_bisect_helper_usage[] = { > - N_("git bisect--helper --bisect-reset [<commit>]"), > - N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"), > - N_("git bisect--helper --bisect-start [--term-{new,bad}=<term> --term-{old,good}=<term>]" > - " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"), > - N_("git bisect--helper --bisect-next"), > - N_("git bisect--helper [--bisect-state] (bad|new) [<rev>]"), > - N_("git bisect--helper [--bisect-state] (good|old) [<rev>...]"), > - N_("git bisect--helper --bisect-replay <filename>"), > - N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"), > - N_("git bisect--helper --bisect-visualize"), > - N_("git bisect--helper --bisect-run <cmd>..."), > +static const char * const git_bisect_usage[] = { > + N_("git bisect help\n" > + "\tprint this long help message."), > + N_("git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>]\n" > + "\t\t [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<pathspec>...]\n" > + "\treset bisect state and start bisection."), > + N_("git bisect (bad|new) [<rev>]\n" > + "\tmark <rev> a known-bad revision/\n" > + "\t\ta revision after change in a given property."), > + N_("git bisect (good|old) [<rev>...]\n" > + "\tmark <rev>... known-good revisions/\n" > + "\t\trevisions before change in a given property."), > + N_("git bisect terms [--term-good | --term-bad]\n" > + "\tshow the terms used for old and new commits (default: bad, good)"), > + N_("git bisect skip [(<rev>|<range>)...]\n" > + "\tmark <rev>... untestable revisions."), > + N_("git bisect next\n" > + "\tfind next bisection to test and check it out."), > + N_("git bisect reset [<commit>]\n" > + "\tfinish bisection search and go back to commit."), > + N_("git bisect (visualize|view)\n" > + "\tshow bisect status in gitk."), > + N_("git bisect replay <logfile>\n" > + "\treplay bisection log."), > + N_("git bisect log\n" > + "\tshow bisect log."), > + N_("git bisect run <cmd>...\n" > + "\tuse <cmd>... to automatically bisect."), > NULL > }; Even that doesn't explain why this needs to be changed as well. I.e. this could just be: diff --git a/builtin/bisect.c b/builtin/bisect.c index e8a346fa516..d27b80ddaf3 100644 --- a/builtin/bisect.c +++ b/builtin/bisect.c @@ -20,33 +20,18 @@ static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN") static const char * const builtin_bisect_usage[] = { - N_("git bisect help\n" - "\tprint this long help message."), + N_("git bisect reset [<commit>]"), + N_("git bisect terms [--term-good | --term-old | --term-bad | --term-new]"), N_("git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>]\n" - "\t\t [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<pathspec>...]\n" - "\treset bisect state and start bisection."), - N_("git bisect (bad|new) [<rev>]\n" - "\tmark <rev> a known-bad revision/\n" - "\t\ta revision after change in a given property."), - N_("git bisect (good|old) [<rev>...]\n" - "\tmark <rev>... known-good revisions/\n" - "\t\trevisions before change in a given property."), - N_("git bisect terms [--term-good | --term-bad]\n" - "\tshow the terms used for old and new commits (default: bad, good)"), - N_("git bisect skip [(<rev>|<range>)...]\n" - "\tmark <rev>... untestable revisions."), - N_("git bisect next\n" - "\tfind next bisection to test and check it out."), - N_("git bisect reset [<commit>]\n" - "\tfinish bisection search and go back to commit."), - N_("git bisect (visualize|view)\n" - "\tshow bisect status in gitk."), - N_("git bisect replay <logfile>\n" - "\treplay bisection log."), - N_("git bisect log\n" - "\tshow bisect log."), - N_("git bisect run <cmd>...\n" - "\tuse <cmd>... to automatically bisect."), + " [--no-checkout] [--first-parent] [<bad> [<good>...]]\n" + " [--] [<paths>...]"), + N_("git bisect next"), + N_("git bisect state (bad|new) [<rev>]"), + N_("git bisect state (good|old) [<rev>...]"), + N_("git bisect replay <filename>"), + N_("git bisect skip [(<rev>|<range>)...]"), + N_("git bisect visualize"), + N_("git bisect run <cmd>..."), NULL }; Which turns the help output into: $ ./git bisect -h usage: git bisect reset [<commit>] or: git bisect terms [--term-good | --term-old | --term-bad | --term-new] or: git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>] [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...] or: git bisect next or: git bisect state (bad|new) [<rev>] or: git bisect state (good|old) [<rev>...] or: git bisect replay <filename> or: git bisect skip [(<rev>|<range>)...] or: git bisect visualize or: git bisect run <cmd>... Instead of: $ ./git bisect -h usage: git bisect help print this long help message. or: git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>] [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<pathspec>...] reset bisect state and start bisection. or: git bisect (bad|new) [<rev>] mark <rev> a known-bad revision/ a revision after change in a given property. or: git bisect (good|old) [<rev>...] mark <rev>... known-good revisions/ revisions before change in a given property. or: git bisect terms [--term-good | --term-bad] show the terms used for old and new commits (default: bad, good) or: git bisect skip [(<rev>|<range>)...] mark <rev>... untestable revisions. or: git bisect next find next bisection to test and check it out. or: git bisect reset [<commit>] finish bisection search and go back to commit. or: git bisect (visualize|view) show bisect status in gitk. or: git bisect replay <logfile> replay bisection log. or: git bisect log show bisect log. or: git bisect run <cmd>... use <cmd>... to automatically bisect. I.e. parse_options() != the usage_with_options() formatting function in parse-options.c. You can use one without using the other. The commit message only claims (wrongly I think, but let's leave that aside for the moment) that we can't use parse_options(), but doesn't say why we *also* need to move to doing our own formatting of the usage output, those are two different things. As I noted in the previous round I think you were trying to retain the OPT_CMDMODE help messages. We could use the "" parse_options() usage feature to emit output similar to "git bisect--helper -h", but I think just having it by the same as current built-ins is fine. I.e. for "stash" etc. we're not emitting human readable help explanations along with every subcommand, and could just do the same for "git bisect". But on to parse_options() usage: > @@ -1168,108 +1184,69 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc) > > int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > { > - enum { > - BISECT_START = 1, > - BISECT_STATE, > - BISECT_TERMS, > - BISECT_SKIP, > - BISECT_NEXT, > - BISECT_RESET, > - BISECT_VISUALIZE, > - BISECT_REPLAY, > - BISECT_LOG, > - BISECT_RUN, > - } cmdmode = 0; > int res = 0; > struct option options[] = { > - OPT_CMDMODE(0, "bisect-start", &cmdmode, > - N_("start the bisect session"), BISECT_START), > - OPT_CMDMODE(0, "bisect-state", &cmdmode, > - N_("mark the state of ref (or refs)"), BISECT_STATE), > - OPT_CMDMODE(0, "bisect-terms", &cmdmode, > - N_("print out the bisect terms"), BISECT_TERMS), > - OPT_CMDMODE(0, "bisect-skip", &cmdmode, > - N_("skip some commits for checkout"), BISECT_SKIP), > - OPT_CMDMODE(0, "bisect-next", &cmdmode, > - N_("find the next bisection commit"), BISECT_NEXT), > - OPT_CMDMODE(0, "bisect-reset", &cmdmode, > - N_("reset the bisection state"), BISECT_RESET), > - OPT_CMDMODE(0, "bisect-visualize", &cmdmode, > - N_("visualize the bisection"), BISECT_VISUALIZE), > - OPT_CMDMODE(0, "bisect-replay", &cmdmode, > - N_("replay the bisection process from the given file"), BISECT_REPLAY), > - OPT_CMDMODE(0, "bisect-log", &cmdmode, > - N_("list the bisection steps so far"), BISECT_LOG), > - OPT_CMDMODE(0, "bisect-run", &cmdmode, > - N_("use <cmd>... to automatically bisect."), BISECT_RUN), > OPT_END() > }; > struct bisect_terms terms = { .term_good = NULL, .term_bad = NULL }; > + 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); Because of thinking that we need to get rid of parse_options() here we... > + if (!strcmp("-h", command) || !strcmp("help", command)) > + usage_with_options(git_bisect_usage, options); ...end up duplicating some of its behavior here... > > - switch (cmdmode ? cmdmode : BISECT_STATE) { > - case BISECT_START: > + argc -= 2; > + argv += 2; > + > + if (!strcmp("start", command)) { > set_terms(&terms, "bad", "good"); > res = bisect_start(&terms, argv, argc); > - break; > - case BISECT_TERMS: > + } else if (!strcmp("terms", command)) { > if (argc > 1) > - die(_("--bisect-terms requires 0 or 1 argument")); > + die(_("'terms' requires 0 or 1 argument")); > res = bisect_terms(&terms, argc == 1 ? argv[0] : NULL); > - break; > - case BISECT_SKIP: > + } else if (!strcmp("skip", command)) { > set_terms(&terms, "bad", "good"); > get_terms(&terms); > res = bisect_skip(&terms, argv, argc); > - break; > - case BISECT_NEXT: > + } else if (!strcmp("next", command)) { > if (argc) > - die(_("--bisect-next requires 0 arguments")); > + die(_("'next' requires 0 arguments")); > get_terms(&terms); > res = bisect_next(&terms, prefix); > - break; > - case BISECT_RESET: > + } else if (!strcmp("reset", command)) { > if (argc > 1) > - die(_("--bisect-reset requires either no argument or a commit")); > + die(_("'reset' requires either no argument or a commit")); > res = bisect_reset(argc ? argv[0] : NULL); > - break; > - case BISECT_VISUALIZE: > + } else if (one_of(command, "visualize", "view", NULL)) { > get_terms(&terms); > res = bisect_visualize(&terms, argv, argc); > - break; > - case BISECT_REPLAY: > + } else if (!strcmp("replay", command)) { > if (argc != 1) > die(_("no logfile given")); > set_terms(&terms, "bad", "good"); > res = bisect_replay(&terms, argv[0]); > - break; > - case BISECT_LOG: > + } else if (!strcmp("log", command)) { > if (argc) > - die(_("--bisect-log requires 0 arguments")); > + die(_("'log' requires 0 arguments")); > res = bisect_log(); > - break; > - case BISECT_RUN: > + } else if (!strcmp("run", command)) { > if (!argc) > die(_("bisect run failed: no command provided.")); > get_terms(&terms); > res = bisect_run(&terms, argv, argc); > - break; > - case BISECT_STATE: > + } else { > set_terms(&terms, "bad", "good"); > get_terms(&terms); > - if (!cmdmode && > - (!argc || check_and_set_terms(&terms, argv[0]))) { > - char *msg = xstrfmt(_("unknown command: '%s'"), argv[0]); > - usage_msg_opt(msg, git_bisect_helper_usage, options); > + if (check_and_set_terms(&terms, command)) { > + char *msg = xstrfmt(_("unknown command: '%s'"), command); > + usage_msg_opt(msg, git_bisect_usage, options); [Change this usage_msg_opt() to a usage_msg_optf() and drop the xstrfmt()] > } > + /* shift the `command` back in */ > + argc++; > + argv--; > res = bisect_state(&terms, argv, argc); > - break; > - default: > - BUG("unknown subcommand %d", cmdmode); > } ..and introducing bugs here, e.g. "git bisect --blah" is now a valid way to start a bisect", we no longer understand "git bisect <subcommand> -h", but did before etc. Is the reason for further extending the custom command parser here because of e.g. the "die(..requires N arguments"? For all of those this could follow the pattern that builtin/commit-graph.c etc. use. I.e. just define a usage for say "log", and empty options, then pass argc/argv to that subcommand, and have it call parse_options(). Then not only will the user get an error, they'll get the subset of "git bisect -h" output appropriate for whatever "git bisect subcommand <bad usage>" they invoked.
Hi Ævar, On Wed, 23 Feb 2022, Ævar Arnfjörð Bjarmason wrote: > On Tue, Feb 22 2022, Johannes Schindelin via GitGitGadget wrote: > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > On our journey to a fully built-in `git bisect`, this is the > > second-to-last step. > > > > Side note: The `if (!strcmp(...)) ... else if (!strcmp(...)) ... else if > > (!strcmp(...)) ...` chain seen in this patch was not actually the first > > idea how to convert the command modes to sub-commands. Since the > > `bisect--helper` command already used the `parse-opions` API with neatly > > set-up command modes, naturally the idea was to use `PARSE_OPT_NODASH` > > to support proper sub-commands instead. However, the `parse-options` API > > is not set up for that, and even after making that option work with long > > options, it turned out that `STOP_AT_NON_OPTION` and `KEEP_UNKNOWN` > > would have to be used but these options were not designed to work > > together. So it would appear as if a lot of work would have to be done > > just to be able to use `parse_options()` just to parse the sub-command, > > instead of a simple `if...else if` chain, the latter being a > > dramatically simpler implementation. > > As I noted in > https://lore.kernel.org/git/220129.86pmobauyt.gmgdl@evledraar.gmail.com/: > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > builtin/bisect--helper.c | 133 ++++++++++++++++----------------------- > > git-bisect.sh | 49 +-------------- > > 2 files changed, 56 insertions(+), 126 deletions(-) > > > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > > index 5228964937d..ef0b06d594b 100644 > > --- a/builtin/bisect--helper.c > > +++ b/builtin/bisect--helper.c > > @@ -20,18 +20,34 @@ static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES") > > static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") > > static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN") > > > > -static const char * const git_bisect_helper_usage[] = { > > - N_("git bisect--helper --bisect-reset [<commit>]"), > > - N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"), > > - N_("git bisect--helper --bisect-start [--term-{new,bad}=<term> --term-{old,good}=<term>]" > > - " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"), > > - N_("git bisect--helper --bisect-next"), > > - N_("git bisect--helper [--bisect-state] (bad|new) [<rev>]"), > > - N_("git bisect--helper [--bisect-state] (good|old) [<rev>...]"), > > - N_("git bisect--helper --bisect-replay <filename>"), > > - N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"), > > - N_("git bisect--helper --bisect-visualize"), > > - N_("git bisect--helper --bisect-run <cmd>..."), > > +static const char * const git_bisect_usage[] = { > > + N_("git bisect help\n" > > + "\tprint this long help message."), > > + N_("git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>]\n" > > + "\t\t [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<pathspec>...]\n" > > + "\treset bisect state and start bisection."), > > + N_("git bisect (bad|new) [<rev>]\n" > > + "\tmark <rev> a known-bad revision/\n" > > + "\t\ta revision after change in a given property."), > > + N_("git bisect (good|old) [<rev>...]\n" > > + "\tmark <rev>... known-good revisions/\n" > > + "\t\trevisions before change in a given property."), > > + N_("git bisect terms [--term-good | --term-bad]\n" > > + "\tshow the terms used for old and new commits (default: bad, good)"), > > + N_("git bisect skip [(<rev>|<range>)...]\n" > > + "\tmark <rev>... untestable revisions."), > > + N_("git bisect next\n" > > + "\tfind next bisection to test and check it out."), > > + N_("git bisect reset [<commit>]\n" > > + "\tfinish bisection search and go back to commit."), > > + N_("git bisect (visualize|view)\n" > > + "\tshow bisect status in gitk."), > > + N_("git bisect replay <logfile>\n" > > + "\treplay bisection log."), > > + N_("git bisect log\n" > > + "\tshow bisect log."), > > + N_("git bisect run <cmd>...\n" > > + "\tuse <cmd>... to automatically bisect."), > > NULL > > }; > > Even that doesn't explain why this needs to be changed as > well. True. The explanation is easy: I am not in the business of changing the usage shown by `git bisect -h`. > we no longer understand "git bisect <subcommand> -h", but did before > etc. ... for some definition of "understand" ;-) See for yourself: $ git bisect visualize -h usage: git bisect--helper --bisect-reset [<commit>] or: git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new] or: git bisect--helper --bisect-start [--term-{new,bad}=<term> --term-{old,good}=<term>] [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...] or: git bisect--helper --bisect-next or: git bisect--helper --bisect-state (bad|new) [<rev>] or: git bisect--helper --bisect-state (good|old) [<rev>...] or: git bisect--helper --bisect-replay <filename> or: git bisect--helper --bisect-skip [(<rev>|<range>)...] or: git bisect--helper --bisect-visualize or: git bisect--helper --bisect-run <cmd>... --bisect-reset reset the bisection state --bisect-next-check check whether bad or good terms exist --bisect-terms print out the bisect terms --bisect-start start the bisect session --bisect-next find the next bisection commit --bisect-state mark the state of ref (or refs) --bisect-log list the bisection steps so far --bisect-replay replay the bisection process from the given file --bisect-skip skip some commits for checkout --bisect-visualize visualize the bisection --bisect-run use <cmd>... to automatically bisect. --no-log no log for BISECT_WRITE That's not a helpful usage for `git bisect visualize`. It's better to leave it to a future patch for `bisect_visualize()` to handle `-h`. In other words, what you point out as a bug is actually fixing one. I find the other comments on not using the `parse_options()` machinery similar, and your feedback seems to flatly dismiss the side note in the commit message as irrelevant. To be clear: I spent a substantial amount of time trying to extend the `parse_options()` machinery to support dash-less subcommands. The end result was neither elegant nor particularly useful. So, with all due respect, I disagree with your disagreeing. Ciao, Johannes
On Fri, Feb 25 2022, Johannes Schindelin wrote: > Hi Ævar, > > On Wed, 23 Feb 2022, Ævar Arnfjörð Bjarmason wrote: > >> On Tue, Feb 22 2022, Johannes Schindelin via GitGitGadget wrote: >> >> > From: Johannes Schindelin <johannes.schindelin@gmx.de> >> > >> > On our journey to a fully built-in `git bisect`, this is the >> > second-to-last step. >> > >> > Side note: The `if (!strcmp(...)) ... else if (!strcmp(...)) ... else if >> > (!strcmp(...)) ...` chain seen in this patch was not actually the first >> > idea how to convert the command modes to sub-commands. Since the >> > `bisect--helper` command already used the `parse-opions` API with neatly >> > set-up command modes, naturally the idea was to use `PARSE_OPT_NODASH` >> > to support proper sub-commands instead. However, the `parse-options` API >> > is not set up for that, and even after making that option work with long >> > options, it turned out that `STOP_AT_NON_OPTION` and `KEEP_UNKNOWN` >> > would have to be used but these options were not designed to work >> > together. So it would appear as if a lot of work would have to be done >> > just to be able to use `parse_options()` just to parse the sub-command, >> > instead of a simple `if...else if` chain, the latter being a >> > dramatically simpler implementation. >> >> As I noted in >> https://lore.kernel.org/git/220129.86pmobauyt.gmgdl@evledraar.gmail.com/: >> >> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> >> > --- >> > builtin/bisect--helper.c | 133 ++++++++++++++++----------------------- >> > git-bisect.sh | 49 +-------------- >> > 2 files changed, 56 insertions(+), 126 deletions(-) >> > >> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c >> > index 5228964937d..ef0b06d594b 100644 >> > --- a/builtin/bisect--helper.c >> > +++ b/builtin/bisect--helper.c >> > @@ -20,18 +20,34 @@ static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES") >> > static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") >> > static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN") >> > >> > -static const char * const git_bisect_helper_usage[] = { >> > - N_("git bisect--helper --bisect-reset [<commit>]"), >> > - N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"), >> > - N_("git bisect--helper --bisect-start [--term-{new,bad}=<term> --term-{old,good}=<term>]" >> > - " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"), >> > - N_("git bisect--helper --bisect-next"), >> > - N_("git bisect--helper [--bisect-state] (bad|new) [<rev>]"), >> > - N_("git bisect--helper [--bisect-state] (good|old) [<rev>...]"), >> > - N_("git bisect--helper --bisect-replay <filename>"), >> > - N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"), >> > - N_("git bisect--helper --bisect-visualize"), >> > - N_("git bisect--helper --bisect-run <cmd>..."), >> > +static const char * const git_bisect_usage[] = { >> > + N_("git bisect help\n" >> > + "\tprint this long help message."), >> > + N_("git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>]\n" >> > + "\t\t [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<pathspec>...]\n" >> > + "\treset bisect state and start bisection."), >> > + N_("git bisect (bad|new) [<rev>]\n" >> > + "\tmark <rev> a known-bad revision/\n" >> > + "\t\ta revision after change in a given property."), >> > + N_("git bisect (good|old) [<rev>...]\n" >> > + "\tmark <rev>... known-good revisions/\n" >> > + "\t\trevisions before change in a given property."), >> > + N_("git bisect terms [--term-good | --term-bad]\n" >> > + "\tshow the terms used for old and new commits (default: bad, good)"), >> > + N_("git bisect skip [(<rev>|<range>)...]\n" >> > + "\tmark <rev>... untestable revisions."), >> > + N_("git bisect next\n" >> > + "\tfind next bisection to test and check it out."), >> > + N_("git bisect reset [<commit>]\n" >> > + "\tfinish bisection search and go back to commit."), >> > + N_("git bisect (visualize|view)\n" >> > + "\tshow bisect status in gitk."), >> > + N_("git bisect replay <logfile>\n" >> > + "\treplay bisection log."), >> > + N_("git bisect log\n" >> > + "\tshow bisect log."), >> > + N_("git bisect run <cmd>...\n" >> > + "\tuse <cmd>... to automatically bisect."), >> > NULL >> > }; >> >> Even that doesn't explain why this needs to be changed as >> well. > > True. The explanation is easy: I am not in the business of changing the > usage shown by `git bisect -h`. You are changing it, here's the diff between "master" and "seen": --- b 2022-02-25 17:49:07.264273673 +0100 +++ a 2022-02-25 17:48:46.076460197 +0100 @@ -1,31 +1,28 @@ -usage: git bisect [help|start|bad|good|new|old|terms|skip|next|reset|visualize|view|replay|log|run] +usage: git bisect help + print this long help message. + or: git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>] + [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<pathspec>...] + reset bisect state and start bisection. + or: git bisect (bad|new) [<rev>] + mark <rev> a known-bad revision/ + a revision after change in a given property. + or: git bisect (good|old) [<rev>...] + mark <rev>... known-good revisions/ + revisions before change in a given property. + or: git bisect terms [--term-good | --term-bad] + show the terms used for old and new commits (default: bad, good) + or: git bisect skip [(<rev>|<range>)...] + mark <rev>... untestable revisions. + or: git bisect next + find next bisection to test and check it out. + or: git bisect reset [<commit>] + finish bisection search and go back to commit. + or: git bisect (visualize|view) + show bisect status in gitk. + or: git bisect replay <logfile> + replay bisection log. + or: git bisect log + show bisect log. + or: git bisect run <cmd>... + use <cmd>... to automatically bisect. -git bisect help - print this long help message. -git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>] - [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<pathspec>...] - reset bisect state and start bisection. -git bisect (bad|new) [<rev>] - mark <rev> a known-bad revision/ - a revision after change in a given property. -git bisect (good|old) [<rev>...] - mark <rev>... known-good revisions/ - revisions before change in a given property. -git bisect terms [--term-good | --term-bad] - show the terms used for old and new commits (default: bad, good) -git bisect skip [(<rev>|<range>)...] - mark <rev>... untestable revisions. -git bisect next - find next bisection to test and check it out. -git bisect reset [<commit>] - finish bisection search and go back to commit. -git bisect (visualize|view) - show bisect status in gitk. -git bisect replay <logfile> - replay bisection log. -git bisect log - show bisect log. -git bisect run <cmd>... - use <cmd>... to automatically bisect. - -Please use "git help bisect" to get the full man page. I think such changes are fine, but if you don't then actually parse_options() would make that easier to emit with the "" delimiter, i.e. to mark up the rest as free-form text, as it is now. But I don't see why you'd view that as a goal, for the rest of built-in migrations we've changed this human-readable output in various ways while we were at it. >> we no longer understand "git bisect <subcommand> -h", but did before >> etc. > > ... for some definition of "understand" ;-) See for yourself: > > $ git bisect visualize -h > usage: git bisect--helper --bisect-reset [<commit>] > or: git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new] > or: git bisect--helper --bisect-start [--term-{new,bad}=<term> --term-{old,good}=<term>] [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...] > or: git bisect--helper --bisect-next > or: git bisect--helper --bisect-state (bad|new) [<rev>] > or: git bisect--helper --bisect-state (good|old) [<rev>...] > or: git bisect--helper --bisect-replay <filename> > or: git bisect--helper --bisect-skip [(<rev>|<range>)...] > or: git bisect--helper --bisect-visualize > or: git bisect--helper --bisect-run <cmd>... > > --bisect-reset reset the bisection state > --bisect-next-check check whether bad or good terms exist > --bisect-terms print out the bisect terms > --bisect-start start the bisect session > --bisect-next find the next bisection commit > --bisect-state mark the state of ref (or refs) > --bisect-log list the bisection steps so far > --bisect-replay replay the bisection process from the given file > --bisect-skip skip some commits for checkout > --bisect-visualize visualize the bisection > --bisect-run use <cmd>... to automatically bisect. > --no-log no log for BISECT_WRITE > > That's not a helpful usage for `git bisect visualize`. It's better to > leave it to a future patch for `bisect_visualize()` to handle `-h`. Yes, it could be better, but with your changes: git bisect start -h Or whatever will start a bisection session. > In other words, what you point out as a bug is actually fixing one. ... > I find the other comments on not using the `parse_options()` machinery > similar, and your feedback seems to flatly dismiss the side note in the > commit message as irrelevant. > > To be clear: I spent a substantial amount of time trying to extend the > `parse_options()` machinery to support dash-less subcommands. The end > result was neither elegant nor particularly useful. > > So, with all due respect, I disagree with your disagreeing. Yes I agree that trying to get parse_option() to consider --bisect-reset implicitly as "reset" is a dead-end. What I'm pointing out is that we could do it exactly as bisect/stash etc. do it. I don't see how you ended up concluding that your conversion needed to do anything different, and failing that that you couldn't use parse_options() at all. Just use it like those other commands use it.
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 5228964937d..ef0b06d594b 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -20,18 +20,34 @@ static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES") static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT") static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN") -static const char * const git_bisect_helper_usage[] = { - N_("git bisect--helper --bisect-reset [<commit>]"), - N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"), - N_("git bisect--helper --bisect-start [--term-{new,bad}=<term> --term-{old,good}=<term>]" - " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"), - N_("git bisect--helper --bisect-next"), - N_("git bisect--helper [--bisect-state] (bad|new) [<rev>]"), - N_("git bisect--helper [--bisect-state] (good|old) [<rev>...]"), - N_("git bisect--helper --bisect-replay <filename>"), - N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"), - N_("git bisect--helper --bisect-visualize"), - N_("git bisect--helper --bisect-run <cmd>..."), +static const char * const git_bisect_usage[] = { + N_("git bisect help\n" + "\tprint this long help message."), + N_("git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>]\n" + "\t\t [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<pathspec>...]\n" + "\treset bisect state and start bisection."), + N_("git bisect (bad|new) [<rev>]\n" + "\tmark <rev> a known-bad revision/\n" + "\t\ta revision after change in a given property."), + N_("git bisect (good|old) [<rev>...]\n" + "\tmark <rev>... known-good revisions/\n" + "\t\trevisions before change in a given property."), + N_("git bisect terms [--term-good | --term-bad]\n" + "\tshow the terms used for old and new commits (default: bad, good)"), + N_("git bisect skip [(<rev>|<range>)...]\n" + "\tmark <rev>... untestable revisions."), + N_("git bisect next\n" + "\tfind next bisection to test and check it out."), + N_("git bisect reset [<commit>]\n" + "\tfinish bisection search and go back to commit."), + N_("git bisect (visualize|view)\n" + "\tshow bisect status in gitk."), + N_("git bisect replay <logfile>\n" + "\treplay bisection log."), + N_("git bisect log\n" + "\tshow bisect log."), + N_("git bisect run <cmd>...\n" + "\tuse <cmd>... to automatically bisect."), NULL }; @@ -1168,108 +1184,69 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc) int cmd_bisect__helper(int argc, const char **argv, const char *prefix) { - enum { - BISECT_START = 1, - BISECT_STATE, - BISECT_TERMS, - BISECT_SKIP, - BISECT_NEXT, - BISECT_RESET, - BISECT_VISUALIZE, - BISECT_REPLAY, - BISECT_LOG, - BISECT_RUN, - } cmdmode = 0; int res = 0; struct option options[] = { - OPT_CMDMODE(0, "bisect-start", &cmdmode, - N_("start the bisect session"), BISECT_START), - OPT_CMDMODE(0, "bisect-state", &cmdmode, - N_("mark the state of ref (or refs)"), BISECT_STATE), - OPT_CMDMODE(0, "bisect-terms", &cmdmode, - N_("print out the bisect terms"), BISECT_TERMS), - OPT_CMDMODE(0, "bisect-skip", &cmdmode, - N_("skip some commits for checkout"), BISECT_SKIP), - OPT_CMDMODE(0, "bisect-next", &cmdmode, - N_("find the next bisection commit"), BISECT_NEXT), - OPT_CMDMODE(0, "bisect-reset", &cmdmode, - N_("reset the bisection state"), BISECT_RESET), - OPT_CMDMODE(0, "bisect-visualize", &cmdmode, - N_("visualize the bisection"), BISECT_VISUALIZE), - OPT_CMDMODE(0, "bisect-replay", &cmdmode, - N_("replay the bisection process from the given file"), BISECT_REPLAY), - OPT_CMDMODE(0, "bisect-log", &cmdmode, - N_("list the bisection steps so far"), BISECT_LOG), - OPT_CMDMODE(0, "bisect-run", &cmdmode, - N_("use <cmd>... to automatically bisect."), BISECT_RUN), OPT_END() }; struct bisect_terms terms = { .term_good = NULL, .term_bad = NULL }; + 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); + if (!strcmp("-h", command) || !strcmp("help", command)) + usage_with_options(git_bisect_usage, options); - switch (cmdmode ? cmdmode : BISECT_STATE) { - case BISECT_START: + argc -= 2; + argv += 2; + + if (!strcmp("start", command)) { set_terms(&terms, "bad", "good"); res = bisect_start(&terms, argv, argc); - break; - case BISECT_TERMS: + } else if (!strcmp("terms", command)) { if (argc > 1) - die(_("--bisect-terms requires 0 or 1 argument")); + die(_("'terms' requires 0 or 1 argument")); res = bisect_terms(&terms, argc == 1 ? argv[0] : NULL); - break; - case BISECT_SKIP: + } else if (!strcmp("skip", command)) { set_terms(&terms, "bad", "good"); get_terms(&terms); res = bisect_skip(&terms, argv, argc); - break; - case BISECT_NEXT: + } else if (!strcmp("next", command)) { if (argc) - die(_("--bisect-next requires 0 arguments")); + die(_("'next' requires 0 arguments")); get_terms(&terms); res = bisect_next(&terms, prefix); - break; - case BISECT_RESET: + } else if (!strcmp("reset", command)) { if (argc > 1) - die(_("--bisect-reset requires either no argument or a commit")); + die(_("'reset' requires either no argument or a commit")); res = bisect_reset(argc ? argv[0] : NULL); - break; - case BISECT_VISUALIZE: + } else if (one_of(command, "visualize", "view", NULL)) { get_terms(&terms); res = bisect_visualize(&terms, argv, argc); - break; - case BISECT_REPLAY: + } else if (!strcmp("replay", command)) { if (argc != 1) die(_("no logfile given")); set_terms(&terms, "bad", "good"); res = bisect_replay(&terms, argv[0]); - break; - case BISECT_LOG: + } else if (!strcmp("log", command)) { if (argc) - die(_("--bisect-log requires 0 arguments")); + die(_("'log' requires 0 arguments")); res = bisect_log(); - break; - case BISECT_RUN: + } else if (!strcmp("run", command)) { if (!argc) die(_("bisect run failed: no command provided.")); get_terms(&terms); res = bisect_run(&terms, argv, argc); - break; - case BISECT_STATE: + } else { set_terms(&terms, "bad", "good"); get_terms(&terms); - if (!cmdmode && - (!argc || check_and_set_terms(&terms, argv[0]))) { - char *msg = xstrfmt(_("unknown command: '%s'"), argv[0]); - usage_msg_opt(msg, git_bisect_helper_usage, options); + if (check_and_set_terms(&terms, command)) { + char *msg = xstrfmt(_("unknown command: '%s'"), command); + usage_msg_opt(msg, git_bisect_usage, options); } + /* shift the `command` back in */ + argc++; + argv--; res = bisect_state(&terms, argv, argc); - break; - default: - BUG("unknown subcommand %d", cmdmode); } + free_terms(&terms); /* diff --git a/git-bisect.sh b/git-bisect.sh index fbf56649d7d..028d39cd9ce 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -34,51 +34,4 @@ Please use "git help bisect" to get the full man page.' OPTIONS_SPEC= . git-sh-setup -TERM_BAD=bad -TERM_GOOD=good - -get_terms () { - if test -s "$GIT_DIR/BISECT_TERMS" - then - { - read TERM_BAD - read TERM_GOOD - } <"$GIT_DIR/BISECT_TERMS" - fi -} - -case "$#" in -0) - usage ;; -*) - cmd="$1" - get_terms - shift - case "$cmd" in - help) - git bisect -h ;; - start) - git bisect--helper --bisect-start "$@" ;; - bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD") - git bisect--helper "$cmd" "$@" ;; - skip) - git bisect--helper --bisect-skip "$@" || exit;; - next) - # Not sure we want "next" at the UI level anymore. - git bisect--helper --bisect-next "$@" || exit ;; - visualize|view) - git bisect--helper --bisect-visualize "$@" || exit;; - reset) - git bisect--helper --bisect-reset "$@" ;; - replay) - git bisect--helper --bisect-replay "$@" || exit;; - log) - git bisect--helper --bisect-log || exit ;; - run) - git bisect--helper --bisect-run "$@" || exit;; - terms) - git bisect--helper --bisect-terms "$@" || exit;; - *) - usage ;; - esac -esac +exec git bisect--helper "$@"