Re: [PATCH 2/3] bisect--helper: move all subcommands into their own functions

[Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>]

On Fri, Nov 04 2022, Đoàn Trần Công Danh wrote:

> In a later change, we will use OPT_SUBCOMMAND to parse sub-commands to
> avoid consuming non-option opts.
>
> Since OPT_SUBCOMMAND needs a function pointer to operate,
> let's move it now.

As shown in
https://lore.kernel.org/git/patch-11.13-d261c32ddd7-20221104T132117Z-avarab@xxxxxxxxx/
this can be much nicer in terms of avoiding these wrappers if we jsut
teach parse-options.c to take our custom signature'd callback, but...

> +static int cmd_bisect__reset(int argc, const char **argv, const char *prefix UNUSED)
> +static int cmd_bisect__terms(int argc, const char **argv, const char *prefix UNUSED)
> +static int cmd_bisect__start(int argc, const char **argv, const char *prefix UNUSED)
....

>  	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);
> +		res = cmd_bisect__reset(argc, argv, prefix);
>  		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);
> +		res = cmd_bisect__terms(argc, argv, prefix);
>  		break;
>  	case BISECT_START:
> -		set_terms(&terms, "bad", "good");
> -		res = bisect_start(&terms, argv, argc);
> +		res = cmd_bisect__start(argc, argv, prefix);

If we're not going to do that this isn't too bad actually. s noted in my
CL
(https://lore.kernel.org/git/cover-00.13-00000000000-20221104T132117Z-avarab@xxxxxxxxx/)
I started seeing if I could cut Johannes's built-in-bisect series down
to size so we could have it merged sooner than later.

It ended up refactoring every single user of "terms" to take the
file-global instead of the variable on the stack, but this shows that
that's not something we need, even without a new parse-options.c API.

B.t.w. you can cut down more on the verbosity by doing:

	struct bisect_terms terms = { 0 };

Which is the same as "{ .term_good = NULL, .term_bad = NULL }". I left
it in place in my version because I'm explicitly trying to avoid
touching anything we don't need to for a bisect built-in, but if we're
refactoring this anyway...

I also think this could be further reduced in size a lot if we go for
your approach, i.e. make a helper function that these call, like:

	if (have_err)
		return error(_(error_msg));
        if (set_terms)
		set_terms(&terms, "bad", "good");
	if (get_terms)
		get_terms(&terms);
	res = !strcmp(iam, "terms") ? bisect_terms(&terms, argc == 1 ? argv[0] : NULL) :
        	!strcmp(iam, "start") ? bisect_start(&terms, argv, argc) :
                [...];
	free_terms(&terms);
	return res;

Then e.g. the body of "terms" is just:

	return that_helper(argc, argv, prefix, "terms", /* iam */
        		  argc > 1, /* have err */ N_("--bisect-terms requires 0 or 1 argument"), /* err msg */
                          0, 0, /* get terms and set terms */);

Etc., I think that might be worth it, as almost all of the contents of
these functions is just copy/pasted...