On Thu, Sep 09, 2021 at 05:50:47PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> 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
