On Tue, Jul 26 2022, Johannes Schindelin wrote: > Hi Junio, > > On Mon, 25 Jul 2022, Junio C Hamano wrote: > >> SZEDER Gábor <szeder.dev@xxxxxxxxx> writes: >> >> > - builtin/bisect.c: after the conversion/rename from 'bisect--helper', >> > cmd_bisect() doesn't use parse-options anymore. Take what's on 'seen' >> > to resolve the conflict. >> > Note that the conflicting topic should have marked cmd_bisect() with >> > the NO_PARSEOPT flag in 'git.c's command list. >> >> I was wondering about this one. Does the new "subcommand" support >> help implementing the dispatching to subcommands better? If so it >> may not be a bad idea to redo the cmd_bisect() on top of this series >> once it proves to be solid. > > The built-in `bisect` code carries around some local state, in a somewhat > futile attempt to keep things in a state that would be more amenable to > libifying. > > The `subcommand` series does not accommodate for such a local state, the > signature `typedef int parse_opt_subcommand_fn(int argc, const char > **argv, const char *prefix);` requires all pre-subcommand options to be > parsed into global (or file-local, but not function-local) variables. > > This implies that moving `cmd_bisect()` on top of the `subcommand` topic > would require the `bisect` code to become less libifyable, which is an > undesirable direction. What are you referring to here specifically? The commands in the builtin/bisect.c take a "struct bisect_terms", but as far as I can tell we could simply move setting that up into the sub-command callbacks. So cmd_bisect() only needs to parse_options() enough to figure out that the first argument is e.g. "start", then call bisect_start(), which will do its own parse_options() & setup of the rest. But I could see how we'd in general have a need to carry state from the cmd_name() to the cmd_name_subcmd(). Such a thing doesn't seem like a big change to SZEDER's patches here, we'd support functions that take a void *, similar to how we support two types of "callback" in the "struct option" itself. Or, you could have perfectly lib-ified code where your cmd_name_subcmd(int argc, char **argv, char *prefix) would be a one-line wrapper for another function taking an extra argument. You'd use a global only to ferry state between the cmd_name() and that cmd_name_subcmd(), which would be some boilerplate, but it wouldn't be prohibitive. When used as a library the API would probably want to take a struct, and not an argc/argv/prefix. I don't see how having just that part of the command callback handling needing one global would close the door on anything else...
