Re: [PATCH v3 01/16] builtin/multi-pack-index.c: inline 'flags' with options

On Thu, Mar 11, 2021 at 12:04:36PM -0500, Taylor Blau wrote:

Subcommands of the 'git multi-pack-index' command (e.g., 'write',
'verify', etc.) will want to optionally change a set of shared flags
that are eventually passed to the MIDX libraries.

Right now, options and flags are handled separately. Inline them into
the same structure so that sub-commands can more easily share the
'flags' data.

This "opts" struct is kind of funny. It is used to collect the options
in cmd_multi_pack_index(), but nobody ever passes it anywhere! Instead,
we pass individual components of it around.

So I'm not sure I buy "...so that sub-commands can more easily share the
flags data", since either way they are all receiving the individual
flags field already. And your patch 2 could just as easily do the same
simplification by modifying the function-local "flags" variable.

But. I think things get more interesting when you later introduce
common_opts, because now those options have to refer back to actuals
storage for each item. Which means that "flags" would have to become a
global variable. And there it's nicer to have all of the options stuffed
into a struct, even if it is a single global struct.

So I think this is the right direction, but it took me a minute to
realize quite why.

-Peff