William Baker <williamtbakeremail@xxxxxxxxx> writes: >> I also see in the code that >> handles opts.batch_size that there is a workaround for this inverted >> code structure to make sure subcommands other than repack does not >> allow --batch-size option specified. > >> we probably would want to correct the use >> of parse_options() API in the implementation of this command before >> adding any new option or subcommand. > > To confirm I understand, is the recommendation that > cmd_multi_pack_index be updated to only parse "batch-size" for the repack > subcommand (i.e. use PARSE_OPT_STOP_AT_NON_OPTION to parse all of the common > options, and then only parse "batch-size" when the repack subcommand is running)? Compare the ways how dispatching and command line option parsing of subcommands in "multi-pack-index" and "commit-graph" are implemented. When a command (e.g. "commit-graph") takes common options and also has subcommands (e.g. "read" and "write") that take different set of options, there is a common options parser in the primary entry point (e.g. "cmd_commit_graph()"), and after dispatching to a chosen subcommand, the implementation of each subcommand (e.g. "graph_read()" and "graph_write()") parses its own options. That's bog-standard way. cmd_multi_pack_index() "cheats" and does not implement proper subcommand dispatch (it directly makes underlying API calls instead). Started as an isolated experimental command whose existence as a standalone command is solely because it was easier to experiment with (as opposed to being a plumbing command to be used by scripters), it probably was an acceptable trade-off to leave the code in this shape. In the longer run, I suspect we'd rather want to get rid of "git multi-pack-index" as a standalone command and instead make "git gc" and other commands make direct calls to the internal machinery of midx code from strategic places. So in that sense, I am not sure if I should "recommend" fixing the way the subcommand dispatching works in this command, or just accept to keep the ugly technical debt and let it limp along... >> subcommands to also understand the progress output or verbosity >> option (and if the excuse given as a response to the above analysis >> is "this is just a first step, more will come later") > > Yep this was my thinking. Today "repack" and "verify" are the only subcommands > that have any progress output but as the other subcommands learn how to provide > progress the [--[no-]progress] option can be used to control it. > >> instead of adding a "unsigned flag" local variable to the function, it would >> probably make much more sense to >> >> (1) make struct opts_multi_pack_index as a part of the public API >> between cmd_multi_pack_index() and midx.c and document it in >> midx.h; >> >> (2) instead of passing opts.object_dir to existing command >> implementations, pass &opts, the pointer to the whole >> structure; >> >> (3) add a new field "unsigned progress" to the structure, and teach >> the command line parser to flip it upon seeing "--[no-]progress". >> > > Thanks for this suggestion I'll use this approach in the next patch. ...but it appears to me that you are more enthused than I am in improving this code, so I probably should actually recommend fixing the main entry point function of this command to imitate the way "commit-graph" implements subcommands and their own set of options ;-)
