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