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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 9, 2021, 14:55 UTC
Message-ID: <YTogUpdtjsFlbqYM@nand.local>
In-Reply-To: <87v93adr8r.fsf@evledraar.gmail.com>

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

Show 6 quoted lines

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

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.

Show 5 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?

Thanks, Taylor