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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 1, 2021, 04:29 UTC
Message-ID: <YS8BwgfurPzhT4xh@nand.local>
In-Reply-To: <YS75P7r33NIBmFh2@coredump.intra.peff.net>

On Tue, Aug 31, 2021 at 11:53:35PM -0400, Jeff King wrote:

Show 6 quoted lines

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

Show 6 quoted lines

> 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