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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 9, 2021, 16:23 UTC
Message-ID: <YTo0+FzYjpYG7B5I@nand.local>
In-Reply-To: <8735qdeoa0.fsf@evledraar.gmail.com>

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

Show 15 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.

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