Re: [PATCH 3/4] midx.c: respect 'pack.writeBitmapHashcache' when writing bitmaps

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 9, 2021, 15:50 UTC
Message-ID: <8735qdeoa0.fsf@evledraar.gmail.com>
In-Reply-To: <YTogUpdtjsFlbqYM@nand.local>

On Thu, Sep 09 2021, Taylor Blau wrote:

Show 18 quoted lines

> On Thu, Sep 09, 2021 at 11:34:16AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> In similar spirit as my
>> https://lore.kernel.org/git/87v93bidhn.fsf@evledraar.gmail.com/ I
>> started seeing if not doing the flags via getopt but instead variables &
>> setting the flags later was better, and came up with this on top. Not
>> for this series, more to muse on how we can write these subcommands in a
>> simpler manner (or not).
>
> Sure, I think that everything you wrote here is correct. I don't have a
> strong opinion, really, but one benefit of not manipulating a single
> 'flags' int is that we don't have to do stuff like:
>
>   if (git_config_bool(var, value))
>     opts.flags |= FLAG_FOO;
>   else
>     opts.flags &= ~FLAG_FOO;
>
> and instead can write `opts.foo = git_config_bool(var, value)`.

*nod*

> Of course, the trade-off is that you later have to turn `opts.foo` into
> flags at some point (or pass each option as an individual parameter). So
> nothing's free, and I see it as a toss-up between which is easier to
> read and write.

I think that trade-off is usually a benefit, also in the pack-write.c etc. case, i.e. you enforce a clear boundary between the built-in and the underlying API, and don't have to e.g. wonder if some write flag is handled by verify() (which will just care about the progress flag), as you pass some moral equivalent of a "struct all_the_options_for_all_the_things" around between all of them.

I realize trying to solve that problem from a different angle may be how you ended up going for per-subcommand config reading....

Show 9 quoted lines

>> I may have discovered a subtle bug in the process, in
>> cmd_multi_pack_index_repack() we end up calling write_midx_internal(),
>> which cares about MIDX_WRITE_REV_INDEX, but only
>> cmd_multi_pack_index_write() will set that flag, both before & after my
>> patch. Are we using the wrong flags during repack as a result?
>
> Only the `write` sub-command would ever want to set that flag, since we
> don't support writing a bitmap after `expire`. So that part seems right,
> but perhaps there is a another problem you're seeing?

In midx_repack() we'll call write_midx_internal(). That function gets the "flags" we pass to midx_repack() and will check MIDX_WRITE_REV_INDEX. I haven't checked whether we actually reach that, but that's what I was wondering, i.e. whether the repack routine would "write" when repacking, and we missed the flag option there.