Re: [PATCH v2 3/4] builtin/pack-objects.c: ensure included `--stdin-packs` exist

From: Taylor Blau <me@ttaylorr.com>
Date: May 26, 2022, 20:05 UTC
Message-ID: <Yo/dnnz8DPbUKDnX@nand.local>
In-Reply-To: <f8509b71-1951-58fe-c12d-3ced30e4ed79@github.com>

On Thu, May 26, 2022 at 12:21:48PM -0700, Victoria Dye wrote:

Show 12 quoted lines

> > pack does not pass the `is_pack_valid()` check.
> >
> > The `--stdin-packs` mode of `pack-objects` is not quite prepared to
> > handle this. To prepare it for this change, do the following two things:
> >
> >   - Ensure provided packs pass the `is_pack_valid()` check when
> >     collecting the caller-provided packs into the "included" and
> >     "excluded" lists.
> >
>
> Is the 'is_pack_valid()' check happening for the "excluded" packs? It looks
> like you only added it for the packs in the "included" list in this patch.

You're right that we don't do it explicitly. That's OK, since we won't use any objects in excluded packs, and thus don't need to eagerly grab an descriptor on it to prevent against the race we're handling here.

(In practice, we do end up calling is_pack_valid() on excluded packs later on, via

  - want_found_object() (or one of its many callers), which itself calls
  - has_object_kept_pack(), which calls
  - find_kept_pack_entry(), which calls
  - fill_pack_entry(), which calls
  - is_pack_valid(), which calls

but that's a side-effect that doesn't help or hurt us.)

We _do_ need to be able to open the .idx (which happens in `fill_pack_entry() -> find_pack_entry_one() -> open_pack_index()` , but we'll fail appropriately when the index cannot be located.

> The rest of this makes sense and (as far as I can tell) lines up with the
> implementation below.

Thanks for taking a look!

Thanks, Taylor