git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: Victoria Dye <vdye@github.com>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org, jonathantanmy@google.com, gitster@pobox.com
Subject: Re: [PATCH v2 3/4] builtin/pack-objects.c: ensure included `--stdin-packs` exist
Date: Thu, 26 May 2022 16:05:50 -0400	[thread overview]
Message-ID: <Yo/dnnz8DPbUKDnX@nand.local> (raw)
In-Reply-To: <f8509b71-1951-58fe-c12d-3ced30e4ed79@github.com>

On Thu, May 26, 2022 at 12:21:48PM -0700, Victoria Dye wrote:
> > 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

  reply	other threads:[~2022-05-26 20:06 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-13 16:23 [PATCH 0/2] pack-objects: fix a pair of MIDX bitmap-related races Taylor Blau
2022-05-13 16:23 ` [PATCH 1/2] pack-bitmap: check preferred pack validity when opening MIDX bitmap Taylor Blau
2022-05-13 18:19   ` Junio C Hamano
2022-05-13 19:55     ` Taylor Blau
2022-05-13 16:23 ` [PATCH 2/2] builtin/pack-objects.c: ensure pack validity from MIDX bitmap objects Taylor Blau
2022-05-13 23:06   ` Jonathan Tan
2022-05-14 13:17     ` Taylor Blau
2022-05-16  6:07       ` Jonathan Tan
2022-05-14 13:34     ` Taylor Blau
2022-05-16  6:11       ` Jonathan Tan
2022-05-24 18:54 ` [PATCH v2 0/4] pack-objects: fix a pair of MIDX bitmap-related races Taylor Blau
2022-05-24 18:54   ` [PATCH v2 1/4] pack-bitmap.c: check preferred pack validity when opening MIDX bitmap Taylor Blau
2022-05-24 19:36     ` Ævar Arnfjörð Bjarmason
2022-05-24 21:38       ` Taylor Blau
2022-05-24 21:51         ` Ævar Arnfjörð Bjarmason
2022-05-24 18:54   ` [PATCH v2 2/4] builtin/pack-objects.c: avoid redundant NULL check Taylor Blau
2022-05-24 21:44     ` Junio C Hamano
2022-05-25  0:11       ` Taylor Blau
2022-05-24 18:54   ` [PATCH v2 3/4] builtin/pack-objects.c: ensure included `--stdin-packs` exist Taylor Blau
2022-05-24 19:46     ` Ævar Arnfjörð Bjarmason
2022-05-24 21:33       ` Taylor Blau
2022-05-24 21:49         ` Ævar Arnfjörð Bjarmason
2022-05-24 22:03     ` Junio C Hamano
2022-05-25  0:14       ` Taylor Blau
2022-05-26 19:21     ` Victoria Dye
2022-05-26 20:05       ` Taylor Blau [this message]
2022-05-24 18:54   ` [PATCH v2 4/4] builtin/pack-objects.c: ensure pack validity from MIDX bitmap objects Taylor Blau
2022-05-24 21:38   ` [PATCH v2 0/4] pack-objects: fix a pair of MIDX bitmap-related races Junio C Hamano
2022-05-25  0:16     ` Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Yo/dnnz8DPbUKDnX@nand.local \
    --to=me@ttaylorr.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jonathantanmy@google.com \
    --cc=vdye@github.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).