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

[Taylor Blau <me@ttaylorr.com>]

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