Subject: 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.

From: William Baker <[email-masked]>
Date: Mon, 16 Sep 2019 12:49:24 -0700

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