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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 4, 2022, 13:32 UTC
Message-ID: <221104.86r0yiygtd.gmgdl@evledraar.gmail.com>
In-Reply-To: <081f3f7f9501012404fb9e59ab6d94f632180b53.1667561761.git.congdanhqx@gmail.com>

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

Show 5 quoted lines

> 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@gmail.com/ 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)

....

Show 17 quoted lines

>  	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@gmail.com/) 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...