On 9/13/19 1:26 PM, Junio C Hamano wrote: > 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. Thanks for the pointer to "commit-graph", looking through that code cleared up any questions I had. After taking another look through the "multi-pack-index" code my plan is to update all of the subcommands to understand [--[no-]progress]. I gave the public struct in midx.h approach a try, but after seeing how that looks I think it would be cleaner to update "write" and "expire" to display progress and have an explicit parameter. > 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... Thanks for the background here, it helped with understanding why the multi-pack-index parsing is different than the other commands. My plan is to include 3 commits in the next (v2) patch series: 1. Adding the new parameters to midx.h/c to control progress 2. Update write/expire to display progress 3. Update the multi-pack-index.c builtin to parse the [--[no-]progress] option and update the tests. I wasn't thinking that I would adjust the the subcommand dispatching in multi-pack-index.c in this patch series. By updating all of the subcommands to support [--[no-]progress] I should be able to keep the changes to multi-pack-index.c quite small. If you see any potential issues with this approach please let me know. Thanks, William
