Re: [PATCH v5 07/16] bisect--helper: migrate to OPT_SUBCOMMAND()
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Aug 30, 2022, 15:42 UTC
- Message-ID
- <61rq5601-o016-3006-3p67-o56307109qq5@tzk.qr>
- In-Reply-To
- <220829.86wnar2wus.gmgdl@evledraar.gmail.com>
Hi Ævar,
On Mon, 29 Aug 2022, Ævar Arnfjörð Bjarmason wrote:
Show 36 quoted lines
> On Sat, Aug 27 2022, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> > -static const char * const git_bisect_helper_usage[] = {
> > - N_("git bisect--helper --bisect-reset [<commit>]"),
> > - "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>...]"),
> > - "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>)...]"),
> > - "git bisect--helper --bisect-visualize",
> > - N_("git bisect--helper --bisect-run <cmd>..."),
> > +static const char * const bisect_usage[] = {
> > + N_("git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>]"
> > + " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--]"
> > + " [<pathspec>...]"),
> > + N_("git bisect (bad|new) [<rev>]"),
> > + N_("git bisect (good|old) [<rev>...]"),
> > + N_("git bisect terms [--term-good | --term-bad]"),
> > + N_("git bisect skip [(<rev>|<range>)...]"),
> > + N_("git bisect next"),
> > + N_("git bisect reset [<commit>]"),
> > + N_("git bisect (visualize|view)"),
> > + N_("git bisect replay <logfile>"),
> > + N_("git bisect log"),
> > + N_("git bisect run <cmd>..."),
> > + NULL
> > +};
>
> Parts of this are erroneously reverting bits of 959d670d1a4 (i18n:
> remove from i18n strings that do not hold translatable parts,
> 2022-01-31).That is misreading what this patch does. I did not turn the pre-image into the post-image.
As the commit message states, we now use the subcommand API with the intention to replace the `git-bisect.sh` script altogether (that's written in the first paragraph of the proposed commit message).
This means that the command modes are replaced by subcommands, and the usage of the subcommands is copied from the script.
And that is the reason why you see a usage about `view` and `log`, as I assume you noticed.
By the way, this here distraction makes a really fine case why it would have been better to leave the migration to the subcommand API to a follow-up patch series. You will recall that I favored that, and I would have loved for you to agree.
Having said that, it is a good point that we do not want to reintroduce the problem that was fixed by 959d670d1a4 (i18n: remove from i18n strings that do not hold translatable parts, 2022-01-31).
Show 13 quoted lines
> I.e. the N_() should not be there in cases like "git bisect next", only
> commands that have placeholders like "<rev>".
>
> > +static const char * const bisect_start_usage[] = {
> > + N_("git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>]"
> > + " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--]"
> > + " [<pathspec>...]"),
> > + NULL
> > +};
>
> Rather than copy/pasting these we can define them once & re-use that
> definition with a macro. See builtin/commit-graph.c and 8757b35d443
> (commit-graph: define common usage with a macro, 2021-08-23).I would do this if I found it elegant.
Show 6 quoted lines
> (Ditto elided below) > > > + argc = parse_options(argc, argv, prefix, options, > > + bisect_reset_usage, PARSE_OPT_STOP_AT_NON_OPTION); > > Nit: Stray " " (double space) here in the argument list.
Good find! Will fix. And obviously not only the `reset` case.
Show 10 quoted lines
>
> > printf(_("Your current terms are %s for the old state\n"
> > "and %s for the new state.\n"),
> > terms.term_good, terms.term_bad);
> > return 0;
>
> Using '%s'-quoting here for the interpolated terms would help
> translators & normalize this with other bessages. It was (partially)
> like this in the pre-image, but since we're changing the whole string
> anyway...It is a bad practice to draw out code contributions by asking contributors to add unrelated changes. This here mail thread presents a fine example why. Let's avoid doing that from now on.
Show 7 quoted lines
> > + if (argc > 0)
> > + usage_msg_opt(_("requires 0 argument"),
> > + bisect_next_usage, options);
>
> Earlier a message says "requires either no argument[...]", but here we
> say "requires 0 argument", which sounds like it expects a literal "0" in
> the argument list.As you undoubtedly noticed, this message was moved from the `cmd_bisect__helper()` function, with its `--bisect-next` part deleted (because it is incorrect now that we no longer have command modes).
> But what we actually mean is "requires no arguments", let's change the > string to that.
No, let's not. For the same reason as noted above.
> > - } > > + > > Here's a "whitespace change while at it..."...
This removes a curly bracket and adds an empty line. You are referring to the empty line?
It is a bit hard to understand what you mean, not only because of the lack of clarity but also because too much context was clipped to easily find the part of the patch that you were referring to. Please be more considerate in the future.
I have to assume that you are referring to a tiny part of one hunk that describes how `bisect_start()` became `cmd_bisect_start()`. If that is indeed what you were referring to, then you are talking about a hunk that seemingly replaces a `for (i = 0; i < argc; i++)` loop that parses the command-line with a loop that validates that only revision parameters are specified before the double dash, if any. In other words, the "introduced" white-space is part of a completely new loop.
Show 13 quoted lines
> > > finish: > > + strvec_clear(&orig_argv); > > + > > Ditto here, why make this clear/release visually separate from the > cleanup below?: > > > string_list_clear(&revs, 0); > > string_list_clear(&states, 0); > > strbuf_release(&start_head); > > ^^^ i.e. this.
Because the latter statements clean up data structures that were used for the core functionality of `bisect start`, while the `orig_argv` purely were a copy for the sake of the bisect log. Therefore, they are logically separate, and the empty line helps readers such as myself.
Show 141 quoted lines
>
> > @@ -1004,8 +1135,10 @@ static int process_replay_line(struct strbuf *line)
> > if (!strcmp(p, "start")) {
> > struct strvec argv = STRVEC_INIT;
> > int res;
> > +
>
> Ditto whitespace change here...
>
> > + strvec_push(&argv, p);
> > sq_dequote_to_strvec(rev, &argv);
> > [...]
> > struct strvec argv = STRVEC_INIT;
> > int res;
> > +
>
> ...and here.., I think these are good, but they add up, perhaps worth
> splitting up various whitespace-prettification in this series for
> readability into its own commit.
>
> > [...]
> > @@ -1272,105 +1445,26 @@ static int bisect_run(int argc, const char **argv)
> >
> > int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> > {
> > - enum {
> > - BISECT_RESET = 1,
> > - BISECT_TERMS,
> > - BISECT_START,
> > - BISECT_NEXT,
> > - BISECT_STATE,
> > - BISECT_LOG,
> > - BISECT_REPLAY,
> > - BISECT_SKIP,
> > - BISECT_VISUALIZE,
> > - BISECT_RUN,
> > - } cmdmode = 0;
> > + parse_opt_subcommand_fn *fn = NULL;
> > int res = 0;
> > struct option options[] = {
> > - OPT_CMDMODE(0, "bisect-reset", &cmdmode,
> > - N_("reset the bisection state"), BISECT_RESET),
> > - OPT_CMDMODE(0, "bisect-terms", &cmdmode,
> > - N_("print out the bisect terms"), BISECT_TERMS),
> > - OPT_CMDMODE(0, "bisect-start", &cmdmode,
> > - N_("start the bisect session"), BISECT_START),
> > - OPT_CMDMODE(0, "bisect-next", &cmdmode,
> > - N_("find the next bisection commit"), BISECT_NEXT),
> > - OPT_CMDMODE(0, "bisect-state", &cmdmode,
> > - N_("mark the state of ref (or refs)"), BISECT_STATE),
> > - OPT_CMDMODE(0, "bisect-log", &cmdmode,
> > - N_("list the bisection steps so far"), BISECT_LOG),
> > - OPT_CMDMODE(0, "bisect-replay", &cmdmode,
> > - N_("replay the bisection process from the given file"), BISECT_REPLAY),
> > - OPT_CMDMODE(0, "bisect-skip", &cmdmode,
> > - N_("skip some commits for checkout"), BISECT_SKIP),
> > - OPT_CMDMODE(0, "bisect-visualize", &cmdmode,
> > - N_("visualize the bisection"), BISECT_VISUALIZE),
> > - OPT_CMDMODE(0, "bisect-run", &cmdmode,
> > - N_("use <cmd>... to automatically bisect"), BISECT_RUN),
> > + OPT_SUBCOMMAND("reset", &fn, cmd_bisect_reset),
> > + OPT_SUBCOMMAND("terms", &fn, cmd_bisect_terms),
> > + OPT_SUBCOMMAND("start", &fn, cmd_bisect_start),
> > + OPT_SUBCOMMAND("next", &fn, cmd_bisect_next),
> > + OPT_SUBCOMMAND("state", &fn, cmd_bisect_state),
> > + OPT_SUBCOMMAND("log", &fn, cmd_bisect_log),
> > + OPT_SUBCOMMAND("replay", &fn, cmd_bisect_replay),
> > + OPT_SUBCOMMAND("skip", &fn, cmd_bisect_skip),
> > + OPT_SUBCOMMAND("visualize", &fn, cmd_bisect_visualize),
> > + OPT_SUBCOMMAND("run", &fn, cmd_bisect_run),
> > OPT_END()
> > };
> >
> > argc = parse_options(argc, argv, prefix, options,
> > - git_bisect_helper_usage,
> > - PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN_OPT);
> > -
> > - if (!cmdmode)
> > - usage_with_options(git_bisect_helper_usage, options);
> > + bisect_usage, 0);
> >
> > - switch (cmdmode) {
> > - 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_TERMS:
> > - if (argc > 1)
> > - return error(_("--bisect-terms requires 0 or 1 argument"));
> > - res = bisect_terms(argc == 1 ? argv[0] : NULL);
> > - break;
> > - case BISECT_START:
> > - set_terms("bad", "good");
> > - res = bisect_start(argc, argv);
> > - break;
> > - case BISECT_NEXT:
> > - if (argc)
> > - return error(_("--bisect-next requires 0 arguments"));
> > - get_terms();
> > - res = bisect_next(prefix);
> > - break;
> > - case BISECT_STATE:
> > - set_terms("bad", "good");
> > - get_terms();
> > - res = bisect_state(argc, argv);
> > - break;
> > - case BISECT_LOG:
> > - if (argc)
> > - return error(_("--bisect-log requires 0 arguments"));
> > - res = bisect_log();
> > - break;
> > - case BISECT_REPLAY:
> > - if (argc != 1)
> > - return error(_("no logfile given"));
> > - set_terms("bad", "good");
> > - res = bisect_replay(argv[0]);
> > - break;
> > - case BISECT_SKIP:
> > - set_terms("bad", "good");
> > - get_terms();
> > - res = bisect_skip(argc, argv);
> > - break;
> > - case BISECT_VISUALIZE:
> > - get_terms();
> > - res = bisect_visualize(argc, argv);
> > - break;
> > - case BISECT_RUN:
> > - if (!argc)
> > - return error(_("bisect run failed: no command provided."));
> > - get_terms();
> > - res = bisect_run(argc, argv);
> > - break;
> > - default:
> > - BUG("unknown subcommand %d", cmdmode);
> > - }
> > + res = fn(argc, argv, prefix);
> > free_terms();
>
> The real meat of this is here, and looks much better on top of SZEDER's
> series in "next" ...Beauty is no quality in things themselves: It exists merely in the mind which contemplates them; and each mind perceives a different beauty.
Plus, this hunk cannot be taken out of context: It requires all the lengthy hunks that come before it, and that are relatively hard to parse as you found out.
Show 58 quoted lines
> > > > return is_bisect_success(res) ? 0 : -res; > > diff --git a/git-bisect.sh b/git-bisect.sh > > index 405cf76f2a3..e19847eba0d 100755 > > --- a/git-bisect.sh > > +++ b/git-bisect.sh > > @@ -58,26 +58,26 @@ case "$#" in > > help) > > git bisect -h ;; > > start) > > - git bisect--helper --bisect-start "$@" ;; > > + git bisect--helper start "$@" ;; > > bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD") > > - git bisect--helper --bisect-state "$cmd" "$@" ;; > > + git bisect--helper state "$cmd" "$@" ;; > > skip) > > - git bisect--helper --bisect-skip "$@" || exit;; > > + git bisect--helper skip "$@" || exit;; > > next) > > # Not sure we want "next" at the UI level anymore. > > - git bisect--helper --bisect-next "$@" || exit ;; > > + git bisect--helper next "$@" || exit ;; > > visualize|view) > > - git bisect--helper --bisect-visualize "$@" || exit;; > > + git bisect--helper visualize "$@" || exit;; > > reset) > > - git bisect--helper --bisect-reset "$@" ;; > > + git bisect--helper reset "$@" ;; > > replay) > > - git bisect--helper --bisect-replay "$@" || exit;; > > + git bisect--helper replay "$@" || exit;; > > log) > > - git bisect--helper --bisect-log || exit ;; > > + git bisect--helper log || exit ;; > > run) > > - git bisect--helper --bisect-run "$@" || exit;; > > + git bisect--helper run "$@" || exit;; > > terms) > > - git bisect--helper --bisect-terms "$@" || exit;; > > + git bisect--helper terms "$@" || exit;; > > *) > > usage ;; > > esac > > ...I did wonder if OPT_SUBCOMMAND() supported subcommands "--like-this" > as opposed to "this", if so the renaming of all of these could be split > up into its own change. > > Maybe not worth it, but it's a very large commit, which does at least: > > * Migration to that new API > * Stray whitespace changes while at it > * Migrating the sub-command functions themselves to parse_options() > (good!), but not strictly required for OPT_SUBCOMMAND() (an > orthagonal change). > * Renaming the "verbs" for the internal helper from > e.g. "--bisect-skip" to "skip", which is what we want eventually, but > does inflate the almost 500-line change.
These suggestions could very easily be construed to be intentionally drawing out a patch contribution that pretty much all other participants in the mail thread have expressed a desire to be done with.
I do understand that you care a lot about these details and about white-space and the parse_options API.
Please do understand that I care about the correctness of the code, and wish I could see more suggestions between all those suggestions that would be conducive to further that goal.
This is a complex patch, to be sure, and it is pretty much solely due to having had to include the move from the command-mode paradigm to the subcommand paradigm in the patch series, which I would very much have preferred to do in a separate patch series.
Could that move have been done more incrementally? Maybe. I tried to split it up as much as I could _without_ losing focus of the actual goal of this patch series: the conversion from a script that implemented the `bisect` operation to a pure built-in that is supposed to perform the same operation without introducing regressions. If it were a follow-up patch series whose sole intention is to migrate `git bisect` to the subcommand API, I would have been eager to spent more time on making it more incremental. However, the goal of this patch series was, and is, to finish the conversion of `git bisect` from a script to a built-in.
That goal was unfortuntely made a lot harder than I wanted to by the feedback I received, and I really wish that it would have been easier to convert `git-bisect.sh` to `builtin/bisect.c`. I would see the feedback in more favorable light if I could see how that feedback furthered that goal, but I have a hard time finding enough evidence that I could consider to merit 5 iterations and 7 months of work.
I want to be clear: any feedback you provide that helps me improve the quality and correctness of my patches is appreciated. Your feedback where you pointed out that my v5 would regress on 959d670d1a4 (i18n: remove from i18n strings that do not hold translatable parts, 2022-01-31) is such a feedback. I want to see more of that kind of feedback from you, and will not hold it against you if you reduce tangential feedback in order to do that.
Thank you, Johannes