On Wed, Sep 01, 2021 at 02:08:48AM -0400, Jeff King wrote: > On Wed, Sep 01, 2021 at 01:12:41AM -0400, Taylor Blau wrote: > > > On Wed, Sep 01, 2021 at 12:59:23AM -0400, Jeff King wrote: > > > So anyway. I think we definitely want the index-pack.c change. We > > > _could_ stop there and change the read side as a separate series, but I > > > think that until we do, the ordering changes on the write side aren't > > > really doing that much. They're shrinking the race a little, I guess, > > > but it's still very much there. > > > > Yeah, now that I've had a chance to look at it it seems pretty > > straightforward. Probably limited to checks in prepare_pack(), and > > add_pack_to_midx(), which are the lone two callbacks of > > for_each_file_in_pack_dir(). > > Yes, that matches what I'd expect. Hmm. As I was wondering about about, this is more complicated than meets the eye. Consider t5616.36, which tests that repacking does not loosen promisor objects. In builtin/repack.c:repack_promisor_object(), the repack builtin tells pack-objects about the pack that it just wrote with `--keep-pack` (and we rely on that working in order to not loosen all of the objects that we just wrote). Except when we iterate through `get_all_packs()`, we don't see the keep pack yet, because it is still prefixed with .tmp. So, this does get kind of tricky. There are some internal callers that do want to know about .tmp packs and a whole host of other callers that don't or shouldn't. Maybe that should point us towards "we should be more careful about the order we write packs in, even temporary ones". But alternatively, perhaps it should point us to "this must not actually be happening very often, otherwise we would have heard more complaints." So, ultimately I'm not sure of the best way forward. I'll have to think on it some more... Thanks, Taylor
