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

On Thu, Sep 09 2021, Taylor Blau wrote:

On Thu, Sep 09, 2021 at 11:34:16AM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

In similar spirit as my
https://lore.kernel.org/git/87v93bidhn.fsf@evledraar.gmail.com/ (local) 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....

quoted

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.