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

From: Jeff King <peff@peff.net>
Date: Mar 29, 2021, 11:20 UTC
Message-ID: <YGG4F+TmtBC/WZmb@coredump.intra.peff.net>
In-Reply-To: <43fc0ad276406ff77283613c45188e102a6dc515.1615482270.git.me@ttaylorr.com>

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

Show 7 quoted lines

> 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