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@xxxxxx> > > > > 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@xxxxxxxxxxxxxxxxxxx/: > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx> > > --- > > 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
