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 <williamtbakeremail@gmail.com>
Date: Sep 16, 2019, 19:49 UTC
Message-ID: <ec5e29f4-9aa1-b179-c71e-2693e4247b01@gmail.com>
In-Reply-To: <xmqqo8zoc57y.fsf@gitster-ct.c.googlers.com>

On 9/13/19 1:26 PM, Junio C Hamano wrote:

Show 9 quoted lines

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

Show 11 quoted lines

> 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