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

On Thu, Sep 09, 2021 at 05:50:47PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

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.

I don't think it's a problem in practice. We would never have
MIDX_WRITE_REV_INDEX set when executing cmd_multi_pack_index_repack(),
(and the same is true for all other subcommands besides `write`) because
the default value for flags is just MIDX_PROGRESS (if isatty(2)), and we
only add the WRITE_REV_INDEX bit from within the write handler.

More generally, that is to say "we only support writing a bitmap from
the `write` sub-command". There is no reason that we couldn't lift this
limitation and support writing a bitmap on the resulting MIDX after
`expire` or `repack` we just haven't done so.

But I don't see any problems with not getting the right flags, etc.

Thanks,
Taylor