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
