Re: [PATCH] fixup! midx: implement writing incremental MIDX bitmaps

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 29, 2024, 18:57 UTC
Message-ID: <ZtDEhNRfXth63SJs@nand.local>
In-Reply-To: <20240828183356.GA4043247@coredump.intra.peff.net>

On Wed, Aug 28, 2024 at 02:33:56PM -0400, Jeff King wrote:

Show 10 quoted lines

> Is that right, though? It looks like the caller might pass in a
> tempfile name like .../pack/multi-pack-index.d/tmp_midx_XXXXXX,
> if we're in incremental mode. But we'll write directly to
> "multi-pack-index-$hash.bitmap" in the same directory. I'm not sure to
> what degree it matters, since that's the name we want in the long run.
> But would we possibly overwrite an active-in-use file rather than doing
> the atomic rename-into-place if we happened to generate the same midx?
>
> It feels like we should still respect the name the caller is using for
> tempfiles, and then rename it into the correct spot at the end.

In either case, we're going to write to a temporary file initialized by the pack-bitmap machinery and then rename() it into place at the end of bitmap_writer_finish().

On the caller side, in the non-incremental mode, we'll pass $GIT_DIR/objects/pack/multi-pack-index-$hash.bitmap as the location, write its contents into a temporary file, and then rename() it there.

But in the incremental mode this series introduces, I think it would be a bug to pass a tmp_midx_XXXXXX file path there, since nobody would move it from tmp_midx_XXXXX-$HASH.bitmap into its final location.

So I think what's written here with the fixup! patch is right (and should be squashed into 13/13 in the next round), but let me know if I'm missing something.

Thanks, Taylor