Re: [PATCH 0/2] pack-write,repack: prevent opening packs too early

From: Taylor Blau <me@ttaylorr.com>
To: Jeff King <peff@peff.net>
Cc: Taylor Blau <me@ttaylorr.com>, git@vger.kernel.org
Subject: Re: [PATCH 0/2] pack-write,repack: prevent opening packs too early
Date: Wed, 1 Sep 2021 00:29:54 -0400	[thread overview]
Message-ID: <YS8BwgfurPzhT4xh@nand.local> (raw)
In-Reply-To: <YS75P7r33NIBmFh2@coredump.intra.peff.net>

On Tue, Aug 31, 2021 at 11:53:35PM -0400, Jeff King wrote:
> So far so good. But the other obvious way to get a pack idx is via
> index-pack (especially "--stdin").
>
> It looks like we'd want the same re-ordering to happen in
> builtin/index-pack.c:final(), which is where we rename the temporary
> files into place.

Sure, that's easy enough.

> We _might_ also want to re-order the calls to write_idx_file() and
> write_rev_file() in its caller, given that simultaneous readers are
> happy to read our tmp_pack_* files. I guess the same might apply to the
> actual file write order pack-objects, too? I'm not sure if that's even
> possible, though; do we rely on side effects of generating the .idx when
> generating the other meta files?

These are a little trickier. write_idx_file() is also responsible for
rearranging the objects array into index (name) order on the way out,
which write_rev_file() depends on in order to build up its mapping.

So you could sort the array at the call-site before calling either
function, but it gets awkward since there are a handful of other callers
of write_idx_file() besides the two we're discussing.

It could be done, but it feels like the wrong approach, because...

> I think it might be more sensible if the reading side was taught to
> ignore ".tmp-*" and "tmp_*" (and possibly even ".*", though it's
> possible somebody is relying on that).

...this seems like the much-better way to go. Git shouldn't have to
worry about what order it writes the temporary files in, only what order
those temporary files are made non-temporary.

But I need to do some more investigation to make sure there aren't any
other implications. So I'm happy to wait on that, or send a new version
of this series with a patch to fix the race in
builtin/index-pack.c:final(), too.

(Unrelated to your suggestions above) another consideration for "stuff
to do later" would be to flip the default of pack.writeReverseIndex. I
had intentions to do that in the 2.32 cycle, but I forgot about it.

> -Peff

Thanks,
Taylor