From: Jeff King <peff@peff•net>
To: Taylor Blau <me@ttaylorr•com>
Cc: Patrick Steinhardt <ps@pks•im>,
git@vger•kernel.org, Junio C Hamano <gitster@pobox•com>
Subject: Re: [PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles
Date: Thu, 22 May 2025 22:18:28 -0400 [thread overview]
Message-ID: <20250523021828.GC559000@coredump.intra.peff.net> (raw)
In-Reply-To: <aC/QBHBbmYaVUzHV@nand.local>
On Thu, May 22, 2025 at 09:31:48PM -0400, Taylor Blau wrote:
> > Yuck, yet another spot that needs to be aware of the new tri-state
> > value. One alternative is using an auxiliary array to cache the errors,
> > and then only the lookup function needs to care. Like:
>
> I like this direction, though I dislike having a separate array that we
> need to keep in sync with m->packs. It might be nice to have an array
> like:
>
> struct {
> struct packed_git *p;
> unsigned err:1;
> } *packs;
>
> , which would allow you to keep the error state next to the packed_git
> itself.
In general, yes, I think array-of-struct is better than struct-of-array.
In this particular case the latter is not too bad because the management
is all handled centrally in the midx constructor.
The downside of doing array-of-struct as you propose is that every site
that uses it will need to be modified. But that is at least a one-time
pain and not an ongoing maintenance burden (unlike the "magic" value
approach).
> I wonder if changing the signature to:
>
> int prepare_midx_pack(struct repository *r,
> struct multi_pack_index *m,
> uint32_t pack_int_id,
> struct packed_git **p_out);
>
> would be a good idea. It allows you to pass garbage input (like a
> non-existent pack_int_id) and get a useful error back. It also allows
> you to pass a pack_int_id that is valid, but cannot be loaded and get a
> useful error back via the return value.
Would the return value be a richer set of values than the current
success/fail? If not, then I think just returning the pack pointer does
that fine.
I was also tempted to suggest that it should take a "struct
multi_pack_index **", to return the matching midx as an out-parameter.
That would "just work" for callers that want to look at the surrounding
midx, too.
But maybe it gets weird for ones that are (correctly) expecting to find
the pack within the same midx. I.e., code like this:
for (i = 0; i < m->num_packs; i++) {
if (prepare_midx_pack(r, m, i + m->num_packs_in_base))
die(...);
/* do something with m->pack[i] */
}
is correct now, and we _shouldn't_ ever need to switch to a different
"m" inside prepare_midx_pack(). But if we ever did, propagating that up
to the caller would be mighty confusing.
So perhaps better to leave it as-is, and let the caller explicitly do
midx_for_pack() or whatever to find the right "m" as necessary.
> But I think without actually trying it and seeing what the fallout looks
> like, it's hard to say whether or not the above is a step in the right
> direction.
Yup. All of this can wait, too. Patrick's series is fixing its own
localized issue (however he wants to structure the extra bit of
storage). Most of what we're talking about here is future-proofing so it
can happen at our leisure. I think the only other actual bug is the
want_included_pack() one discussed in the other part of the thread.
-Peff
next prev parent reply other threads:[~2025-05-23 2:18 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-05-16 8:55 [PATCH] packfile: avoid access(3p) calls for missing packs Patrick Steinhardt
2025-05-16 18:34 ` Junio C Hamano
2025-05-19 6:52 ` Jeff King
2025-05-19 15:46 ` Junio C Hamano
2025-05-20 6:45 ` Patrick Steinhardt
2025-05-22 5:28 ` Jeff King
2025-05-23 1:02 ` Taylor Blau
2025-05-23 2:03 ` Jeff King
2025-05-20 9:53 ` [PATCH v2 0/2] " Patrick Steinhardt
2025-05-20 9:53 ` [PATCH v2 1/2] packfile: explain ordering of how we look up auxiliary pack files Patrick Steinhardt
2025-05-23 1:03 ` Taylor Blau
2025-05-20 9:53 ` [PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles Patrick Steinhardt
2025-05-22 5:32 ` Jeff King
2025-05-22 15:47 ` Junio C Hamano
2025-05-22 16:59 ` Jeff King
2025-05-22 18:44 ` Junio C Hamano
2025-05-23 1:22 ` Taylor Blau
2025-05-23 2:08 ` Jeff King
2025-05-23 17:46 ` Taylor Blau
2025-05-25 18:41 ` [PATCH 0/5] midx: improve prepare_midx_pack() ergonomics Taylor Blau
2025-05-25 18:41 ` [PATCH 1/5] pack-bitmap.c: fix broken warning() when missing MIDX'd pack Taylor Blau
2025-05-26 7:23 ` Patrick Steinhardt
2025-05-28 2:00 ` Taylor Blau
2025-05-25 18:41 ` [PATCH 2/5] midx-write.c: guard against incremental MIDXs in want_included_pack() Taylor Blau
2025-05-26 7:23 ` Patrick Steinhardt
2025-05-28 2:08 ` Taylor Blau
2025-05-25 18:41 ` [PATCH 3/5] midx-write.c: simplify fill_packs_from_midx() Taylor Blau
2025-05-26 7:23 ` Patrick Steinhardt
2025-05-28 2:15 ` Taylor Blau
2025-05-25 18:42 ` [PATCH 4/5] midx-write.c: extract inner loop from fill_packs_from_midx() Taylor Blau
2025-05-25 18:42 ` [PATCH 5/5] midx: return a `packed_git` pointer from `prepare_midx_pack()` Taylor Blau
2025-05-26 7:24 ` Patrick Steinhardt
2025-05-28 2:18 ` Taylor Blau
2025-05-28 11:53 ` Patrick Steinhardt
2025-05-28 22:58 ` [PATCH v2 0/4] midx: improve prepare_midx_pack() ergonomics Taylor Blau
2025-05-28 22:59 ` [PATCH v2 1/4] midx: access pack names through `nth_midxed_pack_name()` Taylor Blau
2025-05-29 20:47 ` Junio C Hamano
2025-06-03 22:22 ` Taylor Blau
2025-05-29 20:51 ` Junio C Hamano
2025-06-03 22:23 ` Taylor Blau
2025-05-28 22:59 ` [PATCH v2 2/4] midx-write.c: guard against incremental MIDXs in want_included_pack() Taylor Blau
2025-05-28 22:59 ` [PATCH v2 3/4] midx-write.c: extract inner loop from fill_packs_from_midx() Taylor Blau
2025-05-28 22:59 ` [PATCH v2 4/4] midx: return a `packed_git` pointer from `prepare_midx_pack()` Taylor Blau
2025-05-30 6:50 ` Jeff King
2025-06-03 22:27 ` Taylor Blau
2025-08-28 23:25 ` Junio C Hamano
2025-05-23 1:31 ` [PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles Taylor Blau
2025-05-23 2:18 ` Jeff King [this message]
2025-05-21 13:24 ` [PATCH v2 0/2] packfile: avoid access(3p) calls for missing packs Junio C Hamano
2025-05-28 12:24 ` [PATCH v3 " Patrick Steinhardt
2025-05-28 12:24 ` [PATCH v3 1/2] packfile: explain ordering of how we look up auxiliary pack files Patrick Steinhardt
2025-05-28 12:24 ` [PATCH v3 2/2] midx: stop repeatedly looking up nonexistent packfiles Patrick Steinhardt
2025-05-30 6:27 ` [PATCH v3 0/2] packfile: avoid access(3p) calls for missing packs Jeff King
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250523021828.GC559000@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
--cc=ps@pks$(echo .)im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox