Re: [PATCH 1/1] multi-pack-index: add --no-progress Add --no-progress option to git multi-pack-index. The progress feature was added in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but the ability to opt-out was overlooked.

[Junio C Hamano <gitster@pobox.com>]

William Baker <williamtbakeremail@gmail.com> 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
;-)