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

["Đoàn Trần Công Danh" <congdanhqx@gmail.com>]

On 2022-11-04 14:32:34+0100, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> 
> 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@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)
> ....
> 
> >  	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.

Yes, I saw your patches, but I think adding a whole new
parse-options.c API is a lot (taking a side the type-safety the new
API introduced). If I were doing the new parse-options API without the
type safety, I probably make an API like:

	int (*subcommand_fn)(int argc, const char **argv, const char *prefix, void *ctx)

We would still have a non-safe 4th argument, but we won't need a new
typedef and picking_fn in every source file.

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

Sure. I'll see which direction is favourable by other people.
Yes, the free_terms is the one that increase the size of this patch,


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

However, if we're doing this, aren't we getting back to step 1:
strcmp to a list of subcommand instead of using OPT_SUBCOMMAND?

>                 [...];
> 	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...

-- 
Danh