Re: [PATCH v5 07/16] bisect--helper: migrate to OPT_SUBCOMMAND()

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 16260 bytes --]

Hi Ævar,

On Mon, 29 Aug 2022, Ævar Arnfjörð Bjarmason wrote:

> 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).

> 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.

> (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.

>
> >  		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.

> > +	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.

>
> >  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.

>
> > @@ -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.

> >
> >  	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