{"thread":{"id":"63469","subject":"[PATCH] packfile: avoid access(3p) calls for missing packs","startedAt":"2025-05-16T08:55:14Z","lastAt":"2025-08-28T23:25:20Z","messageCount":53,"participants":["Patrick Steinhardt","Junio C Hamano","Jeff King","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"518218","messageId":"20250516-pks-pack-avoid-stats-on-missing-v1-1-e2ef4d8798a3@pks.im","threadId":"63469","inReplyTo":null,"subject":"[PATCH] packfile: avoid access(3p) calls for missing packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-16T08:55:10Z","receivedAt":"2025-05-16T08:55:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `add_packed_git()` adds a packfile to the known list of\npacks in case it exists. In case the packfile doesn't look like a pack\nthough, or in case it doesn't exist, the function will simply return a\n`NULL` pointer.\n\nThe ordering in which those checks are done is somewhat curious though:\nwe first perform a couple of access(3p) syscalls for auxiliary files\nlike \".keep\" before we determine whether the \".pack\" file itself exists.\nAnd if we see that the \".pack\" file does not exist, we bail out and thus\neffectively discard all the information. This means that the access(3p)\ncalls were a complete waste of time.\n\nThe reason why we do this is likely because we reuse `p->pack_name` to\nderive the other files' names, as well, so that we only have to allocate\nthis buffer once. As such, we have to compute the packfile name last so\nthat it doesn't get overwritten by the other names.\n\nAll of this likely doesn't matter in the general case: Git shouldn't end\nup looking too many nonexistent packfiles anyway. But there are edge\ncases where it _does_ matter. One such edge case that we have hit in our\nproduction systems was a stale \"multi-pack-index\" file that contained\nreferences to nonexistent packfiles. As there is no negative lookup\ncache this causes us to repeatedly look up the same packfiles via the\nmulti-pack index, only to realize that they don't exist. This translated\ninto hundreds of thousands of syscalls to access(3p) and stat(3p).\n\nWhile the issue was entirely self-made because the multi-pack index\nshould have been regenerated, we can still reduce the number of syscalls\nby 75% in the case of nonexistent packfiles by reordering these calls.\nThis requires us to restore the final packfile name after the access(3p)\ncalls. But given that this is a mere memory copy it is very unlikely to\nmatter in comparison to the four syscalls we performed beforehand.\n\nAnother mitigation would be to introduce a negative lookup cache so that\nwe don't try to add missing packfiles repeatedly. But that refactoring\nwould be a bit more involved for dubious gains, so for now the author\nhas decided against it.\n\nSigned-off-by: Patrick Steinhardt \n---\nHi,\n\nthis patch addresses an issue we have recently seen in our production\nsystems due to a stale MIDX. The MIDX contained entries for packfiles\nthat didn't exist anymore, which caused Git to repeatedly look up those\npackfiles. Each missing packfile resulted in four repeated syscalls:\nthree access(3p) calls to check for supporting data structures, and one\ncall to stat(3p) to check for the packfile itself. The first three calls\nare essentially wasted though when the stat(3p) call itself fails, which\nis being fixed by this patch.\n\nI doubt that the patch matters in almost any repository, but given that\nthe refactoring is trivial I thought to submit the patch regardless of\nthat. Another step would be to introduce a negative lookup cache -- but\nthat would be a bit more involved, so I decided against it for now as I\ndon't want to introduce complexity for dubious gains.\n\nThanks!\n\nPatrick\n---\n packfile.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex d91016f1c7f..870d48bd949 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -737,6 +737,12 @@ struct packed_git *add_packed_git(struct repository *r, const char *path,\n \tp = alloc_packed_git(r, alloc);\n \tmemcpy(p->pack_name, path, path_len);\n \n+\txsnprintf(p->pack_name + path_len, alloc - path_len, \".pack\");\n+\tif (stat(p->pack_name, &st) || !S_ISREG(st.st_mode)) {\n+\t\tfree(p);\n+\t\treturn NULL;\n+\t}\n+\n \txsnprintf(p->pack_name + path_len, alloc - path_len, \".keep\");\n \tif (!access(p->pack_name, F_OK))\n \t\tp->pack_keep = 1;\n@@ -749,11 +755,8 @@ struct packed_git *add_packed_git(struct repository *r, const char *path,\n \tif (!access(p->pack_name, F_OK))\n \t\tp->is_cruft = 1;\n \n+\t/* Restore the final packfile name. */\n \txsnprintf(p->pack_name + path_len, alloc - path_len, \".pack\");\n-\tif (stat(p->pack_name, &st) || !S_ISREG(st.st_mode)) {\n-\t\tfree(p);\n-\t\treturn NULL;\n-\t}\n \n \t/* ok, it looks sane as far as we can check without\n \t * actually mapping the pack file.\n\n---\nbase-commit: 1a8a4971cc6c179c4dd711f4a7f5d7178f4b3ab7\nchange-id: 20250516-pks-pack-avoid-stats-on-missing-8e3b75755cf0\n\n"},{"id":"518292","messageId":"xmqq7c2gv1zx.fsf@gitster.g","threadId":"63469","inReplyTo":"20250516-pks-pack-avoid-stats-on-missing-v1-1-e2ef4d8798a3@pks.im","subject":"Re: [PATCH] packfile: avoid access(3p) calls for missing packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-16T18:34:10Z","receivedAt":"2025-05-16T18:34:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> The function `add_packed_git()` adds a packfile to the known list of\n> packs in case it exists. In case the packfile doesn't look like a pack\n> though, or in case it doesn't exist, the function will simply return a\n> `NULL` pointer.\n>\n> The ordering in which those checks are done is somewhat curious though:\n> we first perform a couple of access(3p) syscalls for auxiliary files\n> like \".keep\" before we determine whether the \".pack\" file itself exists.\n> And if we see that the \".pack\" file does not exist, we bail out and thus\n> effectively discard all the information. This means that the access(3p)\n> calls were a complete waste of time.\n>\n> The reason why we do this is likely because we reuse `p->pack_name` to\n> derive the other files' names, as well, so that we only have to allocate\n> this buffer once. As such, we have to compute the packfile name last so\n> that it doesn't get overwritten by the other names.\n\nI vaguely recall that the order of checks between .idx and .pack\nwere deliberately chosen, as we do not want to add and attempt to\nuse .pack file before its associated .idx file is ready since\nwithout the latter the former is unusable at runtime. \n\n Side note: It may have been more important back when the name of\n the packfile was a hash of names of the objects in the pack,\n which meant that when you repack a quiescent and fully repacked\n repository twice with different parameters, you could have ended\n up with a packfile whose name is the same but the contents as a\n bytestream (hence the object offset) are different, making a\n stale .idx not pointing into the correct position in the new\n .pack file. These days the name of the packfile is based on the\n contents of the pack as a bytestream, so we no longer suffer\n in such a scenario.\n\nBut I wouldn't be surprised if checks for other auxiliary files were\nadded later to come next to .idx not .pack, exactly because .pack is\nthe primary thing.\n\n> All of this likely doesn't matter in the general case: Git shouldn't end\n> up looking too many nonexistent packfiles anyway.\n\nIn any case, we shouldn't attempt to use .pack when its associated\nfiles are missing. It is not about nonexistent but about incomplete\n(including \"in the process of being written\").\n\n> But there are edge\n> cases where it _does_ matter. One such edge case that we have hit in our\n> production systems was a stale \"multi-pack-index\" file that contained\n> references to nonexistent packfiles. As there is no negative lookup\n> cache this causes us to repeatedly look up the same packfiles via the\n> multi-pack index, only to realize that they don't exist. This translated\n> into hundreds of thousands of syscalls to access(3p) and stat(3p).\n\nOuch. Multi-pack index is a more recent invention, and I am not\nsurprised if such bugs still lurk. And as you hinted, a solution\nmay be a negative \"known-not-to-exist\" list, but given one such\nstale midx, would we try to open the same missing packfiles over and\nover within the same process?\n\n> while the issue was entirely self-made because the multi-pack index\n> should have been regenerated, we can still reduce the number of syscalls\n> by 75% in the case of nonexistent packfiles by reordering these calls.\n\nThat sounds more like a band-aid, if we still do the remaining 25%\nthat we somehow know would be unnecessary.\n\nNote that I do not know if the ordering that I think was originally\nintroduced for correctness still matters for correctness. The first\nparagraph of this message was written without consulting \"git log\",\nlore archive, or even the current state of the source, but from\nvague memory.\n\n"},{"id":"518381","messageId":"20250519065221.GC102701@coredump.intra.peff.net","threadId":"63469","inReplyTo":"xmqq7c2gv1zx.fsf@gitster.g","subject":"Re: [PATCH] packfile: avoid access(3p) calls for missing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-19T06:52:21Z","receivedAt":"2025-05-19T06:52:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 16, 2025 at 11:34:10AM -0700, Junio C Hamano wrote:\n\n> > The reason why we do this is likely because we reuse `p->pack_name` to\n> > derive the other files' names, as well, so that we only have to allocate\n> > this buffer once. As such, we have to compute the packfile name last so\n> > that it doesn't get overwritten by the other names.\n> \n> I vaguely recall that the order of checks between .idx and .pack\n> were deliberately chosen, as we do not want to add and attempt to\n> use .pack file before its associated .idx file is ready since\n> without the latter the former is unusable at runtime. \n\nIt is definitely intentional and important that we discover packs by\ntheir .idx file first. The writing side uses the creation of the .idx\nfile (via atomic rename) as the final step in making a pack accessible.\nBefore that, a reader could see a .pack without its various accessory\nfiles (and so not realize it has a .keep file, for example).\n\n> Side note: It may have been more important back when the name of\n> the packfile was a hash of names of the objects in the pack,\n> which meant that when you repack a quiescent and fully repacked\n> repository twice with different parameters, you could have ended\n> up with a packfile whose name is the same but the contents as a\n> bytestream (hence the object offset) are different, making a\n> stale .idx not pointing into the correct position in the new\n> .pack file. These days the name of the packfile is based on the\n> contents of the pack as a bytestream, so we no longer suffer\n> in such a scenario.\n\nI don't think reading the .idx file first was sufficient to protect us\nthere, since the .pack could be replaced racily after the reader opened\nthe .idx. I don't recall if we were subject to that race back then (and\nit just didn't come up enough to worry about), or if repack would throw\naway an identically-named file. Anyway, as you note, it's no longer an\nissue.\n\nBut I think that is mostly orthogonal to how the auxiliary files are\nhandled. I think Patrick's guess is correct that the order we have is\nsimply because it was most convenient to end up with \".pack\" in the\nname.\n\n> > All of this likely doesn't matter in the general case: Git shouldn't end\n> > up looking too many nonexistent packfiles anyway.\n> \n> In any case, we shouldn't attempt to use .pack when its associated\n> files are missing. It is not about nonexistent but about incomplete\n> (including \"in the process of being written\").\n\nThis shouldn't be a race for files being written. The .idx is written\nlast, so the .pack should always be there. And the midx is written after\nthat, so again, the pack should always be there. At least for newly\nwritten packs.\n\nWe might see the opposite: packs going away because they were repacked\n(and perhaps even a new midx generated). In that case you might see\na race like:\n\n - process R opens the existing midx, which mentions pack P (but does\n not yet open P)\n\n - process W repacks, deleting P and generating a new midx with P'\n\n - process R wants object X; the midx claims it is in pack P, so it\n tries to open that but fails\n\nWhat should happen here is that R will fail to find the object at all,\nshould hit reprepare_packed_git(), and will then pick up the new pack.\n\nI don't recall offhand whether reprepare_packed_git() will open the new\nmidx. If it doesn't, we'd still find the object via the actual pack\n.idx, but we may end up consulting the now-stale midx over and over (so\nwe'll get the right answer, but there may be some room for\noptimization).\n\n> > while the issue was entirely self-made because the multi-pack index\n> > should have been regenerated, we can still reduce the number of syscalls\n> > by 75% in the case of nonexistent packfiles by reordering these calls.\n> \n> That sounds more like a band-aid, if we still do the remaining 25%\n> that we somehow know would be unnecessary.\n\nYeah, that was my gut reaction, too. add_packed_git() is not\noptimized[1] at all. But it shouldn't have to be, because it's meant to\nbe called once per process. Even with the re-ordering, we'd still make a\nbunch of pointless stat() calls for the .pack file we know is not there.\n\nThe code in prepare_midx_pack() converts a numeric pack id (referenced\ninside the midx) into a \"struct packed_git\" pointer, caching the results\nin multi_pack_index->packs. That field holds NULL for \"we have not\nlooked it up yet\" or a valid pointer to a packed_git. It probably needs\nto hold a third state: \"we tried and failed\".\n\nSomething like this (large untested) patch:\n\ndiff --git a/midx.c b/midx.c\nindex 3d0015f782..354b1f886c 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -405,7 +405,7 @@ void close_midx(struct multi_pack_index *m)\n \tmunmap((unsigned char *)m->data, m->data_len);\n \n \tfor (i = 0; i < m->num_packs; i++) {\n-\t\tif (m->packs[i])\n+\t\tif (m->packs[i] && m->packs[i] != (void *)(intptr_t)-1)\n \t\t\tm->packs[i]->multi_pack_index = 0;\n \t}\n \tFREE_AND_NULL(m->packs);\n@@ -458,6 +458,8 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n \n \tpack_int_id = midx_for_pack(&m, pack_int_id);\n \n+\tif (m->packs[pack_int_id] == (void *)(intptr_t)-1)\n+\t\treturn 1;\n \tif (m->packs[pack_int_id])\n \t\treturn 0;\n \n@@ -482,8 +484,10 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n \tstrbuf_release(&pack_name);\n \tstrbuf_release(&key);\n \n-\tif (!p)\n+\tif (!p) {\n+\t\tm->packs[pack_int_id] = (void *)(intptr_t)-1;\n \t\treturn 1;\n+\t}\n \n \tp->multi_pack_index = 1;\n \tm->packs[pack_int_id] = p;\n\n-Peff\n\n[1] There probably are optimization opportunities in add_packed_git(). I\n don't think re-ordering will help much in the common case that we\n actually do have the pack. But really, most callers do not care\n about these auxiliary files at all! We could simply skip them during\n the initial setup and lazy-load them via accessor functions.\n\n I _think_ that should be OK with respect to races. For a newly added\n pack, we know they will always be in place before the matching .idx\n file (per the logic I outlined above). For a pack that goes away, we\n might racily fail to see its auxiliary file. But that is mostly true\n now (we might see its .idx, and then the .promisor file is deleted\n before we call access()). It does increase the size of that window,\n though (and in particular lets it happen even if the pack has\n actually been opened).\n\n I'm not sure how much that would matter in practice. OTOH, I'm not\n sure that saving a few access() calls would be all that meaningful,\n unless you have a zillion packs. And if you have a zillion packs,\n you're likely to get much bigger speedups in other areas by\n repacking them anyway. So it hasn't really been an area I've\n pursued before.\n"},{"id":"518428","messageId":"xmqqsel0ob72.fsf@gitster.g","threadId":"63469","inReplyTo":"20250519065221.GC102701@coredump.intra.peff.net","subject":"Re: [PATCH] packfile: avoid access(3p) calls for missing packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-19T15:46:25Z","receivedAt":"2025-05-19T15:46:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The code in prepare_midx_pack() converts a numeric pack id (referenced\n> inside the midx) into a \"struct packed_git\" pointer, caching the results\n> in multi_pack_index->packs. That field holds NULL for \"we have not\n> looked it up yet\" or a valid pointer to a packed_git. It probably needs\n> to hold a third state: \"we tried and failed\".\n\nYeah, this was exactly why I asked \"are we having repeated failures\nin the same process\", and I am happy with this direction.\n\n> Something like this (large untested) patch:\n>\n> diff --git a/midx.c b/midx.c\n> index 3d0015f782..354b1f886c 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -405,7 +405,7 @@ void close_midx(struct multi_pack_index *m)\n> \tmunmap((unsigned char *)m->data, m->data_len);\n> \n> \tfor (i = 0; i < m->num_packs; i++) {\n> -\t\tif (m->packs[i])\n> +\t\tif (m->packs[i] && m->packs[i] != (void *)(intptr_t)-1)\n> \t\t\tm->packs[i]->multi_pack_index = 0;\n> \t}\n> \tFREE_AND_NULL(m->packs);\n> @@ -458,6 +458,8 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n> \n> \tpack_int_id = midx_for_pack(&m, pack_int_id);\n> \n> +\tif (m->packs[pack_int_id] == (void *)(intptr_t)-1)\n> +\t\treturn 1;\n> \tif (m->packs[pack_int_id])\n> \t\treturn 0;\n> \n> @@ -482,8 +484,10 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n> \tstrbuf_release(&pack_name);\n> \tstrbuf_release(&key);\n> \n> -\tif (!p)\n> +\tif (!p) {\n> +\t\tm->packs[pack_int_id] = (void *)(intptr_t)-1;\n> \t\treturn 1;\n> +\t}\n> \n> \tp->multi_pack_index = 1;\n> \tm->packs[pack_int_id] = p;\n"},{"id":"518468","messageId":"aCwk8vnK-QCtp9Bo@pks.im","threadId":"63469","inReplyTo":"20250519065221.GC102701@coredump.intra.peff.net","subject":"Re: [PATCH] packfile: avoid access(3p) calls for missing packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-20T06:45:06Z","receivedAt":"2025-05-20T06:45:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 19, 2025 at 02:52:21AM -0400, Jeff King wrote:\n> On Fri, May 16, 2025 at 11:34:10AM -0700, Junio C Hamano wrote:\n> \n> > > The reason why we do this is likely because we reuse `p->pack_name` to\n> > > derive the other files' names, as well, so that we only have to allocate\n> > > this buffer once. As such, we have to compute the packfile name last so\n> > > that it doesn't get overwritten by the other names.\n> > \n> > I vaguely recall that the order of checks between .idx and .pack\n> > were deliberately chosen, as we do not want to add and attempt to\n> > use .pack file before its associated .idx file is ready since\n> > without the latter the former is unusable at runtime. \n> \n> It is definitely intentional and important that we discover packs by\n> their .idx file first. The writing side uses the creation of the .idx\n> file (via atomic rename) as the final step in making a pack accessible.\n> Before that, a reader could see a .pack without its various accessory\n> files (and so not realize it has a .keep file, for example).\n\nYup. But that order doesn't change anyway in this patch. What changes is\nonly the order for auxiliary and non-mandatory \".keep\", \".promisor\" and\n\".mtimes\" files.\n\n> > Side note: It may have been more important back when the name of\n> > the packfile was a hash of names of the objects in the pack,\n> > which meant that when you repack a quiescent and fully repacked\n> > repository twice with different parameters, you could have ended\n> > up with a packfile whose name is the same but the contents as a\n> > bytestream (hence the object offset) are different, making a\n> > stale .idx not pointing into the correct position in the new\n> > .pack file. These days the name of the packfile is based on the\n> > contents of the pack as a bytestream, so we no longer suffer\n> > in such a scenario.\n> \n> I don't think reading the .idx file first was sufficient to protect us\n> there, since the .pack could be replaced racily after the reader opened\n> the .idx. I don't recall if we were subject to that race back then (and\n> it just didn't come up enough to worry about), or if repack would throw\n> away an identically-named file. Anyway, as you note, it's no longer an\n> issue.\n> \n> But I think that is mostly orthogonal to how the auxiliary files are\n> handled. I think Patrick's guess is correct that the order we have is\n> simply because it was most convenient to end up with \".pack\" in the\n> name.\n\nTaking a step back though: do we always ensure the order in which we\nthose auxiliary files are created and deleted? If we know that those are\nalways:\n\n - Moved into place before their \".idx\" file.\n\n - Deleted after deleting their \".pack\" file.\n\nThen reordering may cause us to race indeed so that we see the packfile,\nbut miss the auxiliary data structures. `unlink_pack_path()` does seem\nto ensure the latter property, as both \".idx\" and \".pack\" are deleted\nfirst. I'm not quite sure about the former, but it seems like we also do\nthis.\n\nSo I think that the proposed patch is wrong. There should definitely be\na comment in that function though to say that this order is intentional\nand not merely an optimization.\n\n[snip]\n> > > while the issue was entirely self-made because the multi-pack index\n> > > should have been regenerated, we can still reduce the number of syscalls\n> > > by 75% in the case of nonexistent packfiles by reordering these calls.\n> > \n> > That sounds more like a band-aid, if we still do the remaining 25%\n> > that we somehow know would be unnecessary.\n> \n> Yeah, that was my gut reaction, too. add_packed_git() is not\n> optimized[1] at all. But it shouldn't have to be, because it's meant to\n> be called once per process. Even with the re-ordering, we'd still make a\n> bunch of pointless stat() calls for the .pack file we know is not there.\n\nIt certainly is, as mentioned in the cover letter. It was a supposedly\ncheap win as I didn't want to introduce a negative lookup cache for an\nedge case like this. But the patch below looks simple enough -- I feared\nit would be a lot more involved.\n\n> The code in prepare_midx_pack() converts a numeric pack id (referenced\n> inside the midx) into a \"struct packed_git\" pointer, caching the results\n> in multi_pack_index->packs. That field holds NULL for \"we have not\n> looked it up yet\" or a valid pointer to a packed_git. It probably needs\n> to hold a third state: \"we tried and failed\".\n> \n> Something like this (large untested) patch:\n> \n> diff --git a/midx.c b/midx.c\n> index 3d0015f782..354b1f886c 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -405,7 +405,7 @@ void close_midx(struct multi_pack_index *m)\n> \tmunmap((unsigned char *)m->data, m->data_len);\n> \n> \tfor (i = 0; i < m->num_packs; i++) {\n> -\t\tif (m->packs[i])\n> +\t\tif (m->packs[i] && m->packs[i] != (void *)(intptr_t)-1)\n> \t\t\tm->packs[i]->multi_pack_index = 0;\n> \t}\n> \tFREE_AND_NULL(m->packs);\n> @@ -458,6 +458,8 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n> \n> \tpack_int_id = midx_for_pack(&m, pack_int_id);\n> \n> +\tif (m->packs[pack_int_id] == (void *)(intptr_t)-1)\n> +\t\treturn 1;\n> \tif (m->packs[pack_int_id])\n> \t\treturn 0;\n> \n> @@ -482,8 +484,10 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n> \tstrbuf_release(&pack_name);\n> \tstrbuf_release(&key);\n> \n> -\tif (!p)\n> +\tif (!p) {\n> +\t\tm->packs[pack_int_id] = (void *)(intptr_t)-1;\n> \t\treturn 1;\n> +\t}\n> \n> \tp->multi_pack_index = 1;\n> \tm->packs[pack_int_id] = p;\n> \n> -Peff\n> \n> [1] There probably are optimization opportunities in add_packed_git(). I\n> don't think re-ordering will help much in the common case that we\n> actually do have the pack. But really, most callers do not care\n> about these auxiliary files at all! We could simply skip them during\n> the initial setup and lazy-load them via accessor functions.\n> \n> I _think_ that should be OK with respect to races. For a newly added\n> pack, we know they will always be in place before the matching .idx\n> file (per the logic I outlined above). For a pack that goes away, we\n> might racily fail to see its auxiliary file. But that is mostly true\n> now (we might see its .idx, and then the .promisor file is deleted\n> before we call access()). It does increase the size of that window,\n> though (and in particular lets it happen even if the pack has\n> actually been opened).\n\nI'm not sure it would be okay, as mentioned above. The current ordering\nensures that we always see auxiliary data structures in case the \".pack\"\nfile exists. If we started to cache then that wouldn't be the case\nanymore.\n\n> I'm not sure how much that would matter in practice. OTOH, I'm not\n> sure that saving a few access() calls would be all that meaningful,\n> unless you have a zillion packs. And if you have a zillion packs,\n> you're likely to get much bigger speedups in other areas by\n> repacking them anyway. So it hasn't really been an area I've\n> pursued before.\n\nIt probably doesn't in the general case at all.\n\nPatrick\n"},{"id":"518482","messageId":"20250520-pks-pack-avoid-stats-on-missing-v2-0-333c5217fb05@pks.im","threadId":"63469","inReplyTo":"20250516-pks-pack-avoid-stats-on-missing-v1-1-e2ef4d8798a3@pks.im","subject":"[PATCH v2 0/2] packfile: avoid access(3p) calls for missing packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-20T09:53:08Z","receivedAt":"2025-05-20T09:53:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch addresses an issue we have recently seen in our production\nsystems due to a stale MIDX. The MIDX contained entries for packfiles\nthat didn't exist anymore, which caused Git to repeatedly look up those\npackfiles. Each missing packfile resulted in four repeated syscalls:\nthree access(3p) calls to check for supporting data structures, and one\ncall to stat(3p) to check for the packfile itself. The first three calls\nare essentially wasted though when the stat(3p) call itself fails, which\nis being fixed by this patch.\n\nI doubt that the patch matters in almost any repository, but given that\nthe refactoring is trivial I thought to submit the patch regardless of\nthat. Another step would be to introduce a negative lookup cache -- but\nthat would be a bit more involved, so I decided against it for now as I\ndon't want to introduce complexity for dubious gains.\n\nChanges in v2:\n - Drop the patch that reorders syscalls and add a comment explaining\n why the order is important.\n - Add a negative lookup cache for indexed packfiles.\n - Link to v1: https://lore.kernel.org/r/20250516-pks-pack-avoid-stats-on-missing-v1-1-e2ef4d8798a3@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (2):\n packfile: explain ordering of how we look up auxiliary pack files\n midx: stop repeatedly looking up nonexistent packfiles\n\n midx.c | 10 ++++++++--\n packfile.c | 11 +++++++++++\n 2 files changed, 19 insertions(+), 2 deletions(-)\n\nRange-diff versus v1:\n\n1: 31ffb3712ca < -: ----------- packfile: avoid access(3p) calls for missing packs\n-: ----------- > 1: 6125b84389d packfile: explain ordering of how we look up auxiliary pack files\n-: ----------- > 2: 8cb82a771c0 midx: stop repeatedly looking up nonexistent packfiles\n\n---\nbase-commit: 1a8a4971cc6c179c4dd711f4a7f5d7178f4b3ab7\nchange-id: 20250516-pks-pack-avoid-stats-on-missing-8e3b75755cf0\n\n"},{"id":"518483","messageId":"20250520-pks-pack-avoid-stats-on-missing-v2-1-333c5217fb05@pks.im","threadId":"63469","inReplyTo":"20250520-pks-pack-avoid-stats-on-missing-v2-0-333c5217fb05@pks.im","subject":"[PATCH v2 1/2] packfile: explain ordering of how we look up auxiliary pack files","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-20T09:53:09Z","receivedAt":"2025-05-20T09:53:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When adding a packfile to an object database we perform four syscalls:\n\n - Three calls to access(3p) are done to check for auxiliary data\n structures.\n\n - One call to stat(3p) is done to check for the \".pack\" itself.\n\nOne curious bit is that we perform the access(3p) calls before checking\nfor the packfile itself, but if the packfile doesn't exist we discard\nall results. The access(3p) calls are thus essentially wasted, so one\nmay be triggered to reorder those calls so that we can short-circuit the\nother syscalls in case the packfile does not exist.\n\nThe order in which we look up files is quite important though to help\navoid races:\n\n - When installing a packfile we move auxiliary data structures into\n place before we install the \".idx\" file.\n\n - When deleting a packfile we first delete the \".idx\" and \".pack\"\n files before deleting auxiliary data structures.\n\nAs such, to avoid any races with concurrently created or deleted packs\nwe need to make sure that we _first_ read auxiliary data structures\nbefore we read the corresponding \".idx\" or \".pack\" file. Otherwise it\nmay easily happen that we return a populated but misclassified pack.\n\nAdd a comment to `add_packed_git()` to make future readers aware of this\nordering requirement.\n\nSigned-off-by: Patrick Steinhardt \n---\n packfile.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/packfile.c b/packfile.c\nindex d91016f1c7f..933036e2606 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -737,6 +737,17 @@ struct packed_git *add_packed_git(struct repository *r, const char *path,\n \tp = alloc_packed_git(r, alloc);\n \tmemcpy(p->pack_name, path, path_len);\n \n+\t/*\n+\t * Note that we have to check auxiliary data structures before we check\n+\t * for the \".pack\" file to exist to avoid races with a packfile that is\n+\t * in the process of being deleted. The \".pack\" file is unlinked before\n+\t * its auxiliary data structures, so we know that we either get a\n+\t * consistent snapshot of all data structures or that we'll fail to\n+\t * stat(3p) the packfile itself and thus return `NULL`.\n+\t *\n+\t * As such, we cannot bail out before the access(3p) calls in case the\n+\t * packfile doesn't exist without doing two stat(3p) calls for it.\n+\t */\n \txsnprintf(p->pack_name + path_len, alloc - path_len, \".keep\");\n \tif (!access(p->pack_name, F_OK))\n \t\tp->pack_keep = 1;\n\n-- \n2.49.0.1151.ga128411c76.dirty\n\n"},{"id":"518484","messageId":"20250520-pks-pack-avoid-stats-on-missing-v2-2-333c5217fb05@pks.im","threadId":"63469","inReplyTo":"20250520-pks-pack-avoid-stats-on-missing-v2-0-333c5217fb05@pks.im","subject":"[PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-20T09:53:10Z","receivedAt":"2025-05-20T09:53:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The multi-pack index acts as a cache across a set of packfiles so that\nwe can quickly look up which of those packfiles contains a given object.\nAs such, the multi-pack index naturally needs to be updated every time\none of the packfiles goes away, or otherwise the multi-pack index has\ngrown stale.\n\nA stale multi-pack index should be handled gracefully by Git though, and\nin fact it is: if the indexed pack cannot be found we simply ignore it\nand eventually we fall back to doing the object lookup by just iterating\nthrough all packs, even if those aren't indexed.\n\nBut while this fallback works, it has one significant downside: we don't\ncache the fact that a pack has vanished. This leads to us repeatedly\ntrying to look up the same pack only to realize that it (still) doesn't\nexist.\n\nThis issue can be easily demonstrated by creating a repository with a\nstale multi-pack index and a couple of objects. We do so by creating a\nrepository with two packfiles, both of which are indexed by the\nmulti-pack index, and then repack those two packfiles. Note that we have\nto move the multi-pack-index before doing the final repack, as Git knows\nto delete it otherwise.\n\n $ git init repo\n $ cd repo/\n $ git config set maintenance.auto false\n $ for i in $(seq 1000); do printf \"%d-original\" $i >file-$i; done\n $ git add .\n $ git commit -moriginal\n $ git repack -dl\n $ for i in $(seq 1000); do printf \"%d-modified\" $i >file-$i; done\n $ git commit -a -mmodified\n $ git repack -dl\n $ git multi-pack-index write\n $ mv .git/objects/pack/multi-pack-index .\n $ git repack -Adl\n $ mv multi-pack-index .git/objects/pack/\n\nCommands that cause a lot of objects lookups will now repeatedly invoke\n`add_packed_git()`, which leads to three failed access(3p) calls as well\nas one failed stat(3p) call. The following strace for example is done\nfor `git log --patch` in the above repository:\n\n % time seconds usecs/call calls errors syscall\n ------ ----------- ----------- --------- --------- ----------------\n 74.67 0.024693 1 18038 18031 access\n 25.33 0.008378 1 6045 6017 newfstatat\n ------ ----------- ----------- --------- --------- ----------------\n 100.00 0.033071 1 24083 24048 total\n\nFix the issue by introducing a negative lookup cache for indexed packs.\nThis cache works by simply storing an invalid pointer for a missing pack\nwhen `prepare_midx_pack()` fails to look up the pack. Most users of the\n`packs` array don't need to be adjusted, either, as they all know to\ncall `prepare_midx_pack()` before accessing the array.\n\nWith this change in place we can now see a significantly reduced number\nof syscalls:\n\n % time seconds usecs/call calls errors syscall\n ------ ----------- ----------- --------- --------- ----------------\n 73.58 0.000323 5 60 28 newfstatat\n 26.42 0.000116 5 23 16 access\n ------ ----------- ----------- --------- --------- ----------------\n 100.00 0.000439 5 83 44 total\n\nFurthermore, this change also results in a speedup:\n\n Benchmark 1: git log --patch (revision = HEAD~)\n Time (mean ± σ): 50.4 ms ± 2.5 ms [User: 22.0 ms, System: 24.4 ms]\n Range (min … max): 45.4 ms … 54.9 ms 53 runs\n\n Benchmark 2: git log --patch (revision = HEAD)\n Time (mean ± σ): 12.7 ms ± 0.4 ms [User: 11.1 ms, System: 1.6 ms]\n Range (min … max): 12.4 ms … 15.0 ms 191 runs\n\n Summary\n git log --patch (revision = HEAD) ran\n 3.96 ± 0.22 times faster than git log --patch (revision = HEAD~)\n\nIn the end, it should in theory never be necessary to have this negative\nlookup cache given that we know to update the multi-pack index together\nwith repacks. But as the change is quite contained and as the speedup\ncan be significant as demonstrated above, it does feel sensible to have\nthe negative lookup cache regardless.\n\nBased-on-patch-by: Jeff King \nSigned-off-by: Patrick Steinhardt \n---\n midx.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 3d0015f7828..fbce88bd463 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -405,7 +405,7 @@ void close_midx(struct multi_pack_index *m)\n \tmunmap((unsigned char *)m->data, m->data_len);\n \n \tfor (i = 0; i < m->num_packs; i++) {\n-\t\tif (m->packs[i])\n+\t\tif (m->packs[i] && m->packs[i] != (void *)(intptr_t)-1)\n \t\t\tm->packs[i]->multi_pack_index = 0;\n \t}\n \tFREE_AND_NULL(m->packs);\n@@ -458,6 +458,8 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n \n \tpack_int_id = midx_for_pack(&m, pack_int_id);\n \n+\tif (m->packs[pack_int_id] == (void *)(intptr_t)-1)\n+\t\treturn 1;\n \tif (m->packs[pack_int_id])\n \t\treturn 0;\n \n@@ -482,8 +484,10 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n \tstrbuf_release(&pack_name);\n \tstrbuf_release(&key);\n \n-\tif (!p)\n+\tif (!p) {\n+\t\tm->packs[pack_int_id] = (void *)(intptr_t)-1;\n \t\treturn 1;\n+\t}\n \n \tp->multi_pack_index = 1;\n \tm->packs[pack_int_id] = p;\n@@ -495,6 +499,8 @@ struct packed_git *nth_midxed_pack(struct multi_pack_index *m,\n \t\t\t\t uint32_t pack_int_id)\n {\n \tuint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id);\n+\tif (m->packs[local_pack_int_id] == (void *)(intptr_t)-1)\n+\t\treturn NULL;\n \treturn m->packs[local_pack_int_id];\n }\n \n\n-- \n2.49.0.1151.ga128411c76.dirty\n\n"},{"id":"518570","messageId":"xmqqh61edrlq.fsf@gitster.g","threadId":"63469","inReplyTo":"20250520-pks-pack-avoid-stats-on-missing-v2-0-333c5217fb05@pks.im","subject":"Re: [PATCH v2 0/2] packfile: avoid access(3p) calls for missing packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-21T13:24:17Z","receivedAt":"2025-05-21T13:24:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Changes in v2:\n> - Drop the patch that reorders syscalls and add a comment explaining\n> why the order is important.\n\nMuch nicer.\n\n> - Add a negative lookup cache for indexed packfiles.\n\nLooking very good.\n\nThanks, queued.\n\n\n> - Link to v1: https://lore.kernel.org/r/20250516-pks-pack-avoid-stats-on-missing-v1-1-e2ef4d8798a3@pks.im\n>\n> Thanks!\n>\n> Patrick\n>\n> ---\n> Patrick Steinhardt (2):\n> packfile: explain ordering of how we look up auxiliary pack files\n> midx: stop repeatedly looking up nonexistent packfiles\n>\n> midx.c | 10 ++++++++--\n> packfile.c | 11 +++++++++++\n> 2 files changed, 19 insertions(+), 2 deletions(-)\n>\n> Range-diff versus v1:\n>\n> 1: 31ffb3712ca < -: ----------- packfile: avoid access(3p) calls for missing packs\n> -: ----------- > 1: 6125b84389d packfile: explain ordering of how we look up auxiliary pack files\n> -: ----------- > 2: 8cb82a771c0 midx: stop repeatedly looking up nonexistent packfiles\n>\n> ---\n> base-commit: 1a8a4971cc6c179c4dd711f4a7f5d7178f4b3ab7\n> change-id: 20250516-pks-pack-avoid-stats-on-missing-8e3b75755cf0\n"},{"id":"518641","messageId":"20250522052821.GA1134267@coredump.intra.peff.net","threadId":"63469","inReplyTo":"aCwk8vnK-QCtp9Bo@pks.im","subject":"Re: [PATCH] packfile: avoid access(3p) calls for missing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-22T05:28:21Z","receivedAt":"2025-05-22T05:28:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 20, 2025 at 08:45:06AM +0200, Patrick Steinhardt wrote:\n\n> Taking a step back though: do we always ensure the order in which we\n> those auxiliary files are created and deleted? If we know that those are\n> always:\n> \n> - Moved into place before their \".idx\" file.\n> \n> - Deleted after deleting their \".pack\" file.\n> \n> Then reordering may cause us to race indeed so that we see the packfile,\n> but miss the auxiliary data structures. `unlink_pack_path()` does seem\n> to ensure the latter property, as both \".idx\" and \".pack\" are deleted\n> first. I'm not quite sure about the former, but it seems like we also do\n> this.\n> \n> So I think that the proposed patch is wrong. There should definitely be\n> a comment in that function though to say that this order is intentional\n> and not merely an optimization.\n\nHmm, interesting. Yes, I agree that deleting the .pack before any other\nauxiliary files should catch the race, given the ordering on the read\nside in add_packed_git().\n\nBut I don't think we ever discussed that ordering or its implications.\nSo I assumed all bets were off with respect to racing.\n\nHowever, it seems we lucked into doing the correct thing by virtue of\nadding to the end of the list in unlink_pack_path() repeatedly (though I\nthink if you dig all the way back into git-repack.sh, when there were\njust .keep files, there were periods where we did it in the wrong\norder).\n\nBut if we are doing it in the best order, whether correct or not, I\nagree we should not disrupt that.\n\n> > [1] There probably are optimization opportunities in add_packed_git(). I\n> > don't think re-ordering will help much in the common case that we\n> > actually do have the pack. But really, most callers do not care\n> > about these auxiliary files at all! We could simply skip them during\n> > the initial setup and lazy-load them via accessor functions.\n> > \n> > I _think_ that should be OK with respect to races. For a newly added\n> > pack, we know they will always be in place before the matching .idx\n> > file (per the logic I outlined above). For a pack that goes away, we\n> > might racily fail to see its auxiliary file. But that is mostly true\n> > now (we might see its .idx, and then the .promisor file is deleted\n> > before we call access()). It does increase the size of that window,\n> > though (and in particular lets it happen even if the pack has\n> > actually been opened).\n> \n> I'm not sure it would be okay, as mentioned above. The current ordering\n> ensures that we always see auxiliary data structures in case the \".pack\"\n> file exists. If we started to cache then that wouldn't be the case\n> anymore.\n\nYeah, you're right. I thought it was already racy, but it luckily is\nnot. It does still feel unfortunate that we have to spend syscalls\nloading information that won't be used by most commands, but it may not\nbe a measurable performance issue. I think if we did want to lazy-load\nthat information, we'd probably need to do an extra stat() of .pack\nafterwards to check for the deletion race (so repack and pack-objects\nwould pay the penalty, but most other commands would not).\n\nAnyway, that can be explored another day.\n\n-Peff\n"},{"id":"518642","messageId":"20250522053235.GB1134267@coredump.intra.peff.net","threadId":"63469","inReplyTo":"20250520-pks-pack-avoid-stats-on-missing-v2-2-333c5217fb05@pks.im","subject":"Re: [PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-22T05:32:35Z","receivedAt":"2025-05-22T05:32:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 20, 2025 at 11:53:10AM +0200, Patrick Steinhardt wrote:\n\n> @@ -458,6 +458,8 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n> \n> \tpack_int_id = midx_for_pack(&m, pack_int_id);\n> \n> +\tif (m->packs[pack_int_id] == (void *)(intptr_t)-1)\n> +\t\treturn 1;\n> \tif (m->packs[pack_int_id])\n> \t\treturn 0;\n\nI did wonder while writing this if we might be able to hide the magic\nnumber and gross casting inside a constant or macro. I think just:\n\n #define MIDX_PACK_ERROR ((void *)(intptr_t)-1)\n\nwould be enough?\n\nThough...\n\n> @@ -495,6 +499,8 @@ struct packed_git *nth_midxed_pack(struct multi_pack_index *m,\n> \t\t\t\t uint32_t pack_int_id)\n> {\n> \tuint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id);\n> +\tif (m->packs[local_pack_int_id] == (void *)(intptr_t)-1)\n> +\t\treturn NULL;\n> \treturn m->packs[local_pack_int_id];\n\nYuck, yet another spot that needs to be aware of the new tri-state\nvalue. One alternative is using an auxiliary array to cache the errors,\nand then only the lookup function needs to care. Like:\n\ndiff --git a/midx.c b/midx.c\nindex c1adff4404..df71ead50b 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -186,6 +186,7 @@ static struct multi_pack_index *load_multi_pack_index_one(struct repository *r,\n \n \tCALLOC_ARRAY(m->pack_names, m->num_packs);\n \tCALLOC_ARRAY(m->packs, m->num_packs);\n+\tCALLOC_ARRAY(m->pack_err, m->num_packs);\n \n \tcur_pack_name = (const char *)m->chunk_pack_names;\n \tfor (i = 0; i < m->num_packs; i++) {\n@@ -408,6 +409,7 @@ void close_midx(struct multi_pack_index *m)\n \t\tif (m->packs[i])\n \t\t\tm->packs[i]->multi_pack_index = 0;\n \t}\n+\tFREE_AND_NULL(m->pack_errs);\n \tFREE_AND_NULL(m->packs);\n \tFREE_AND_NULL(m->pack_names);\n \tfree(m);\n@@ -460,6 +462,8 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n \n \tif (m->packs[pack_int_id])\n \t\treturn 0;\n+\tif (m->pack_errs[pack_int_id])\n+\t\treturn 1;\n \n \tstrbuf_addf(&pack_name, \"%s/pack/%s\", m->object_dir,\n \t\t m->pack_names[pack_int_id]);\n@@ -482,8 +486,10 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n \tstrbuf_release(&pack_name);\n \tstrbuf_release(&key);\n \n-\tif (!p)\n+\tif (!p) {\n+\t\tm->pack_errs[pack_int_id] = 1;\n \t\treturn 1;\n+\t}\n \n \tp->multi_pack_index = 1;\n \tm->packs[pack_int_id] = p;\n\nYou could even lazy-malloc the extra array if you wanted to optimize the\ncommon no-errors case, but I'm not sure it's a big deal.\n\n-Peff\n"},{"id":"518662","messageId":"xmqqbjrk7ikx.fsf@gitster.g","threadId":"63469","inReplyTo":"20250522053235.GB1134267@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-22T15:47:58Z","receivedAt":"2025-05-22T15:48:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> @@ -460,6 +462,8 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n> \n> \tif (m->packs[pack_int_id])\n> \t\treturn 0;\n> +\tif (m->pack_errs[pack_int_id])\n> +\t\treturn 1;\n> \n> \tstrbuf_addf(&pack_name, \"%s/pack/%s\", m->object_dir,\n> \t\t m->pack_names[pack_int_id]);\n> @@ -482,8 +486,10 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n> \tstrbuf_release(&pack_name);\n> \tstrbuf_release(&key);\n> \n> -\tif (!p)\n> +\tif (!p) {\n> +\t\tm->pack_errs[pack_int_id] = 1;\n> \t\treturn 1;\n> +\t}\n> \n> \tp->multi_pack_index = 1;\n> \tm->packs[pack_int_id] = p;\n>\n> You could even lazy-malloc the extra array if you wanted to optimize the\n> common no-errors case, but I'm not sure it's a big deal.\n\nSo the idea is to leave m->packs[] for unused pack NULL instead of\nmagic value, so that users of that array do not need to check?\n\nI think that is a lot safer than the magic \"we know this fails\"\nvalue that existing callers that have long trusted that a non-NULL\n.packs[] element is a valid pack.\n\nBy the way, I suspect I am not reading the code correctly, but I am\nnot sure what fill_midx_entry() does with a failed case.\n\n\tmidx_for_object(&m, pos);\n\tpack_int_id = nth_midxed_pack_int_id(m, pos);\n\n\tif (prepare_midx_pack(r, m, pack_int_id))\n\t\treturn 0;\n\nWith or without cached failure, this should return 0 when and only\nwhen m->packs[pack_int_id] is a usable pack. But what about the\naccess on the next line?\n\n\tp = m->packs[pack_int_id - m->num_packs_in_base];\n\nDo we have any guarantee that we called prepare_midx_pack() for \nthe pack at (pack_int_id - m->num_packs_in_base)th slot? Can p\nbe NULL here? And with the magic \"we know this fails\" value, can p\nbe that magic value?\n\n\t/*\n\t* We are about to tell the caller where they can locate the\n\t* requested object. We better make sure the packfile is\n\t* still here and can be accessed before supplying that\n\t* answer, as it may have been deleted since the MIDX was\n\t* loaded!\n\t*/\n\tif (!is_pack_valid(p))\n\t\treturn 0;\n\nThat value is passed to is_pack_valid(), which I do not think even\ntakes NULL.\n\nPuzzled.\n"},{"id":"518672","messageId":"20250522165924.GA1613@coredump.intra.peff.net","threadId":"63469","inReplyTo":"xmqqbjrk7ikx.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-22T16:59:24Z","receivedAt":"2025-05-22T16:59:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 22, 2025 at 08:47:58AM -0700, Junio C Hamano wrote:\n\n> So the idea is to leave m->packs[] for unused pack NULL instead of\n> magic value, so that users of that array do not need to check?\n> \n> I think that is a lot safer than the magic \"we know this fails\"\n> value that existing callers that have long trusted that a non-NULL\n> .packs[] element is a valid pack.\n\nRight. It would protect any further users of that assumption that we\nmissed.\n\n> By the way, I suspect I am not reading the code correctly, but I am\n> not sure what fill_midx_entry() does with a failed case.\n> \n> \tmidx_for_object(&m, pos);\n> \tpack_int_id = nth_midxed_pack_int_id(m, pos);\n> \n> \tif (prepare_midx_pack(r, m, pack_int_id))\n> \t\treturn 0;\n> \n> With or without cached failure, this should return 0 when and only\n> when m->packs[pack_int_id] is a usable pack. But what about the\n> access on the next line?\n\nI think there's a subtlety here with incremental midx's, in that a pack\nid can be \"global\" within the whole midx chain, or a local index into a\nspecific chain element's list of packs.\n\nIn fill_midx_entry(), I think we get such a global id back from\nnth_midxed_pack_int_id(). And then when we hand that to\nprepare_midx_pack(), it is converted into a local midx/id pair with:\n\n pack_int_id = midx_for_pack(&m, pack_int_id);\n\nwhere the end of that function is the same:\n\n return pack_int_id - m->num_packs_in_base;\n\nyou saw elsewhere. So at that point we have a local midx and index,\nwhich is what prepare_midx_pack() fills via the m->packs[pack_int_id]\nfield.\n\nSo when you say \"only when m->packs[pack_int_id] is a usable pack\", you\nare talking about the local m/pack_int_id within that function.\n\nBut back in the caller...\n\n> \tp = m->packs[pack_int_id - m->num_packs_in_base];\n> \n> Do we have any guarantee that we called prepare_midx_pack() for \n> the pack at (pack_int_id - m->num_packs_in_base)th slot? Can p\n> be NULL here? And with the magic \"we know this fails\" value, can p\n> be that magic value?\n\nOur pack_int_id is the global one, so it needs to be adjusted. But this\npack pointer we access is the same one that was filled (or not) by\nprepare_midx_pack(). So it cannot be NULL or the magic \"fails\" value,\nbecause prepare_midx_pack() returned 0.\n\nSo I think this code is fine.\n\n\nOne thing that did puzzle me: in prepare_midx_pack() we not only adjust\nthe pack_int_id, but we may walk back through the midx chain to find the\ncorrect multi_pack_index struct. Wouldn't the caller need to do the\nsame?\n\nThe answer is that it does. The midx_for_object() call in\nfill_midx_entry() does that same walk, storing the result in its local\n\"m\" variable. So the walk backwards in prepare_midx_pack() is\nsuperfluous for this particular caller, who we know is already handing\nus the desired multi_pack_index struct, and it could just do:\n\n pack_int_id -= m->num_packs_in_base;\n\nrather than calling midx_for_pack(). But the same is not necessarily\ntrue for other callers, so we should continue calling that function.\n\nI suspect this would all be a bit more obvious if prepare_midx_pack()\nsimply returned the pack pointer, avoiding the need for callers to look\nat m->packs at all (and making it a true cache, internally only to\nprepare_midx_pack()).\n\nLooking at other callers of prepare_midx_pack():\n\n - in fill_packs_from_midx(), we do not adjust our \"m\" to match the\n index. But that is OK, because we adjust our local index (which we\n get by iterating from 0 to m->num_packs) to a global index when\n calling the function:\n\n if (prepare_midx_pack(ctx->repo, m, m->num_packs_in_base + i))\n\t...\n open_pack_index(m->packs[i]);\n\n which is fine.\n\n - I'm less sure of the call in expire_midx_packs(). It iterates over\n num_packs in the same way, but does:\n\n if (prepare_midx_pack(r, m, i))\n continue;\n\n and then looks at m->packs[i]. That would be wrong if \"m\" is not the\n first item in the chain. Ah, I see. Earlier we do:\n\n if (m->base_midx)\n die(_(\"cannot expire packs from an incremental multi-pack-index\"));\n\n so we know that the global and local ids are equivalent in this\n instance (since the \"base\" midx . Still seems a bit fragile.\n\n - There's a similar case in midx-write.c:want_included_pack(). That\n one seems to have the same local/global confusion, but I do not\n obviously see anything preventing it from being fed a non-base midx.\n So it might possibly be buggy?\n\n Likewise fill_included_packs_batch() in the same file.\n\n In both cases I think if prepare_midx_pack() returned a pointer, we\n could just use it directly.\n\n - In nth_bitmapped_pack(), we call midx_for_pack() ourselves to get\n the right midx. That's good, but getting a pack pointer from\n prepare_midx_pack() wouldn't help us, because we still look at the\n midx struct for other reasons.\n\nSo I dunno. It might be a useful refactor, but it doesn't make the\nproblem go away entirely. I'm suspicious of those calls in midx-write.c,\nbut Taylor can probably say more about whether they're wrong or if\nthere's some less-obvious reason we'd only see a base midx there.\n\n-Peff\n"},{"id":"518690","messageId":"xmqqy0uo5vtw.fsf@gitster.g","threadId":"63469","inReplyTo":"20250522165924.GA1613@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-22T18:44:43Z","receivedAt":"2025-05-22T18:44:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But back in the caller...\n>\n>> \tp = m->packs[pack_int_id - m->num_packs_in_base];\n>> ...\n>\n> Our pack_int_id is the global one, so it needs to be adjusted. But this\n> pack pointer we access is the same one that was filled (or not) by\n> prepare_midx_pack(). So it cannot be NULL or the magic \"fails\" value,\n> because prepare_midx_pack() returned 0.\n>\n> So I think this code is fine.\n\nAhh, I missed that call to midx_for_pack() in prepare_midx_pack()\nthat modifies pack_int_id variable. Of course, it did not help that\nthe implementation detail of adjusting by m->num_packs_in_base which\nis done in midx_for_pack() is not abstracted out and the caller needs\nto do the same as above. That confused me.\n\nThanks for an explanation.\n\n\n"},{"id":"518737","messageId":"aC/JDrF5uJh9XxXj@nand.local","threadId":"63469","inReplyTo":"20250519065221.GC102701@coredump.intra.peff.net","subject":"Re: [PATCH] packfile: avoid access(3p) calls for missing packs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-23T01:02:06Z","receivedAt":"2025-05-23T01:02:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, May 19, 2025 at 02:52:21AM -0400, Jeff King wrote:\n> On Fri, May 16, 2025 at 11:34:10AM -0700, Junio C Hamano wrote:\n>\n> > > The reason why we do this is likely because we reuse `p->pack_name` to\n> > > derive the other files' names, as well, so that we only have to allocate\n> > > this buffer once. As such, we have to compute the packfile name last so\n> > > that it doesn't get overwritten by the other names.\n> >\n> > I vaguely recall that the order of checks between .idx and .pack\n> > were deliberately chosen, as we do not want to add and attempt to\n> > use .pack file before its associated .idx file is ready since\n> > without the latter the former is unusable at runtime.\n>\n> It is definitely intentional and important that we discover packs by\n> their .idx file first. The writing side uses the creation of the .idx\n> file (via atomic rename) as the final step in making a pack accessible.\n> Before that, a reader could see a .pack without its various accessory\n> files (and so not realize it has a .keep file, for example).\n\nYup. Or seeing a pack before its .bitmap is written, in which case any\nclones or fetches unfortunate enough to take place after the pack was\nread but before the .bitmap was written would be significantly slower\nthan they otherwise would have been with bitmaps.\n\n> > > All of this likely doesn't matter in the general case: Git shouldn't end\n> > > up looking too many nonexistent packfiles anyway.\n> >\n> > In any case, we shouldn't attempt to use .pack when its associated\n> > files are missing. It is not about nonexistent but about incomplete\n> > (including \"in the process of being written\").\n>\n> This shouldn't be a race for files being written. The .idx is written\n> last, so the .pack should always be there. And the midx is written after\n> that, so again, the pack should always be there. At least for newly\n> written packs.\n>\n> We might see the opposite: packs going away because they were repacked\n> (and perhaps even a new midx generated). In that case you might see\n> a race like:\n>\n> - process R opens the existing midx, which mentions pack P (but does\n> not yet open P)\n>\n> - process W repacks, deleting P and generating a new midx with P'\n>\n> - process R wants object X; the midx claims it is in pack P, so it\n> tries to open that but fails\n>\n> What should happen here is that R will fail to find the object at all,\n> should hit reprepare_packed_git(), and will then pick up the new pack.\n>\n> I don't recall offhand whether reprepare_packed_git() will open the new\n> midx. If it doesn't, we'd still find the object via the actual pack\n> .idx, but we may end up consulting the now-stale midx over and over (so\n> we'll get the right answer, but there may be some room for\n> optimization).\n\nThis all sounds right to me. And we do end up loading a new MIDX via\nreprepare_packed_git(): that function calls prepare_packed_git() (which\ndoesn't immediately return, since we just zero'd out the\nr->objects->packed_git_initialized flag). We then enumerate the ODBs,\ncalling prepare_multi_pack_index_one() and prepare_packed_git_one() for\neach.\n\nFrom there we end up in load_multi_pack_index(), which gets a freshened\nview of the MIDX file. If all goes well there, then\nprepare_multi_pack_index_one() assigns it to\nr->objects->multi_pack_index as expected.\n\n> The code in prepare_midx_pack() converts a numeric pack id (referenced\n> inside the midx) into a \"struct packed_git\" pointer, caching the results\n> in multi_pack_index->packs. That field holds NULL for \"we have not\n> looked it up yet\" or a valid pointer to a packed_git. It probably needs\n> to hold a third state: \"we tried and failed\".\n\nYup, that seems like a good direction to me.\n\n> [1] There probably are optimization opportunities in add_packed_git(). I\n> don't think re-ordering will help much in the common case that we\n> actually do have the pack. But really, most callers do not care\n> about these auxiliary files at all! We could simply skip them during\n> the initial setup and lazy-load them via accessor functions.\n\nNice idea. I initially though, \"that's not right, we definitely need to\npopulate the flag bits, e.g., p->is_cruft\". But that was before I read\n\"and lazy-load them via accessor functions\". That turns those fields\ninto a tri-state, which is a nice way to avoid this altogether.\n\nThanks,\nTaylor\n"},{"id":"518738","messageId":"aC/Jf06HdCxTJwnI@nand.local","threadId":"63469","inReplyTo":"20250520-pks-pack-avoid-stats-on-missing-v2-1-333c5217fb05@pks.im","subject":"Re: [PATCH v2 1/2] packfile: explain ordering of how we look up auxiliary pack files","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-23T01:03:59Z","receivedAt":"2025-05-23T01:04:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, May 20, 2025 at 11:53:09AM +0200, Patrick Steinhardt wrote:\n> Add a comment to `add_packed_git()` to make future readers aware of this\n> ordering requirement.\n\nThanks, I am really glad that you added this comment to aide future\nreaders.\n\nThanks,\nTaylor\n"},{"id":"518739","messageId":"aC/Nz2wQgncXzFvs@nand.local","threadId":"63469","inReplyTo":"20250522165924.GA1613@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-23T01:22:23Z","receivedAt":"2025-05-23T01:22:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 22, 2025 at 12:59:24PM -0400, Jeff King wrote:\n> > By the way, I suspect I am not reading the code correctly, but I am\n> > not sure what fill_midx_entry() does with a failed case.\n> >\n> > \tmidx_for_object(&m, pos);\n> > \tpack_int_id = nth_midxed_pack_int_id(m, pos);\n> >\n> > \tif (prepare_midx_pack(r, m, pack_int_id))\n> > \t\treturn 0;\n> >\n> > With or without cached failure, this should return 0 when and only\n> > when m->packs[pack_int_id] is a usable pack. But what about the\n> > access on the next line?\n>\n> I think there's a subtlety here with incremental midx's, in that a pack\n> id can be \"global\" within the whole midx chain, or a local index into a\n> specific chain element's list of packs.\n\nIf you'll indulge me in a bit of pedantry for a moment, I would add that\nthe pack_int_id is *always* global within the whole MIDX chain. It only\nhappens to additionally be the correct local index when\nm->num_packs_in_base is zero.\n\nThe thing here:\n\n m->packs[pack_int_id - m->num_packs_in_base];\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n\nis indexing into the current layer's pack array, which only contains the\npacks at that layer, so the index there is no longer really a\npack_int_id at all.\n\nBut...\n\n> In fill_midx_entry(), I think we get such a global id back from\n> nth_midxed_pack_int_id(). And then when we hand that to\n> prepare_midx_pack(), it is converted into a local midx/id pair with:\n>\n> pack_int_id = midx_for_pack(&m, pack_int_id);\n\n...this confusion is really my fault, since the thing midx_for_pack() is\nreturning isn't really a pack_int_id in the classic sense at all. I\nthink I chose this name to avoid making the patch noisy, and to avoid\nthe possibility of accidentally using the global identifier when you\nmeant to use the local index.\n\n(As an aside, I think I recall you suggesting a while ago that it might\nbe interesting to define \"global\" things with a different type than\n\"local\" ones to prevent this sort of confusion. That would allow us to\nkeep both \"pack_int_id\" and the return value of \"midx_for_pack()\" in\nscope at the same time, without the possibility of using one when you\nmeant to use the other.)\n\n> where the end of that function is the same:\n>\n> return pack_int_id - m->num_packs_in_base;\n>\n> you saw elsewhere. So at that point we have a local midx and index,\n> which is what prepare_midx_pack() fills via the m->packs[pack_int_id]\n> field.\n>\n> So when you say \"only when m->packs[pack_int_id] is a usable pack\", you\n> are talking about the local m/pack_int_id within that function.\n>\n> But back in the caller...\n>\n> > \tp = m->packs[pack_int_id - m->num_packs_in_base];\n> >\n> > Do we have any guarantee that we called prepare_midx_pack() for\n> > the pack at (pack_int_id - m->num_packs_in_base)th slot? Can p\n> > be NULL here? And with the magic \"we know this fails\" value, can p\n> > be that magic value?\n>\n> Our pack_int_id is the global one, so it needs to be adjusted. But this\n> pack pointer we access is the same one that was filled (or not) by\n> prepare_midx_pack(). So it cannot be NULL or the magic \"fails\" value,\n> because prepare_midx_pack() returned 0.\n>\n> So I think this code is fine.\n\nThanks for the analysis.\n\n> One thing that did puzzle me: in prepare_midx_pack() we not only adjust\n> the pack_int_id, but we may walk back through the midx chain to find the\n> correct multi_pack_index struct. Wouldn't the caller need to do the\n> same?\n>\n> The answer is that it does. The midx_for_object() call in\n> fill_midx_entry() does that same walk, storing the result in its local\n> \"m\" variable. So the walk backwards in prepare_midx_pack() is\n> superfluous for this particular caller, who we know is already handing\n> us the desired multi_pack_index struct, and it could just do:\n>\n> pack_int_id -= m->num_packs_in_base;\n\nRight. You could imagine that after getting the pack_int_id back from\nnth_midxed_pack_int_id(), we could do:\n\n midx_for_pack(&m, pack_int_id);\n\n, which would be a noop because of the chain invariant that we never\nduplicate objects or packs anywhere in the chain. (IOW, if you know\nobject X is in pack Y, and you move your MIDX pointer to the layer\ncontaining X, you are guaranteed to be able to find Y in that layer's\narray of packs.)\n\n> rather than calling midx_for_pack(). But the same is not necessarily\n> true for other callers, so we should continue calling that function.\n\n...exactly ;-).\n\n> I suspect this would all be a bit more obvious if prepare_midx_pack()\n> simply returned the pack pointer, avoiding the need for callers to look\n> at m->packs at all (and making it a true cache, internally only to\n> prepare_midx_pack()).\n>\n> Looking at other callers of prepare_midx_pack():\n>\n> - in fill_packs_from_midx(), we do not adjust our \"m\" to match the\n> index. But that is OK, because we adjust our local index (which we\n> get by iterating from 0 to m->num_packs) to a global index when\n> calling the function:\n>\n> if (prepare_midx_pack(ctx->repo, m, m->num_packs_in_base + i))\n> \t...\n> open_pack_index(m->packs[i]);\n>\n> which is fine.\n>\n> - I'm less sure of the call in expire_midx_packs(). It iterates over\n> num_packs in the same way, but does:\n>\n> if (prepare_midx_pack(r, m, i))\n> continue;\n>\n> and then looks at m->packs[i]. That would be wrong if \"m\" is not the\n> first item in the chain. Ah, I see. Earlier we do:\n>\n> if (m->base_midx)\n> die(_(\"cannot expire packs from an incremental multi-pack-index\"));\n>\n> so we know that the global and local ids are equivalent in this\n> instance (since the \"base\" midx . Still seems a bit fragile.\n\nYeah, I think you can only meaningfully expire packs from the most\nrecent layer, which is why I added that guard. I agree it is still\nfragile, though.\n\n> - There's a similar case in midx-write.c:want_included_pack(). That\n> one seems to have the same local/global confusion, but I do not\n> obviously see anything preventing it from being fed a non-base midx.\n> So it might possibly be buggy?\n\nYeah, this spot is definitely broken. At minimum it would need something\nlike:\n\n--- 8< ---\ndiff --git a/midx-write.c b/midx-write.c\nindex 0897cbd829..54a04f7b75 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1634,9 +1634,10 @@ static int want_included_pack(struct repository *r,\n \t\t\t uint32_t pack_int_id)\n {\n \tstruct packed_git *p;\n+\tmidx_for_pack(&m, pack_int_id);\n \tif (prepare_midx_pack(r, m, pack_int_id))\n \t\treturn 0;\n-\tp = m->packs[pack_int_id];\n+\tp = m->packs[pack_int_id - m->num_packs_in_base];\n \tif (!pack_kept_objects && p->pack_keep)\n \t\treturn 0;\n \tif (p->is_cruft)\n--- >8 ---\n\n> Likewise fill_included_packs_batch() in the same file.\n\nI think this one is actually OK for the same reason as the\nexpire_midx_packs() case. Its sole caller in midx_repack() has:\n\n if (m->base_midx)\n die(_(\"cannot repack an incremental multi-pack-index\"));\n\n, so we are OK there. We might want to add an ASSERT() in\nfill_included_packs_batch() to make it clearer, though.\n\n> In both cases I think if prepare_midx_pack() returned a pointer, we\n> could just use it directly.\n\nAgreed.\n\nThanks,\nTaylor\n"},{"id":"518740","messageId":"aC/QBHBbmYaVUzHV@nand.local","threadId":"63469","inReplyTo":"20250522053235.GB1134267@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-23T01:31:48Z","receivedAt":"2025-05-23T01:31:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 22, 2025 at 01:32:35AM -0400, Jeff King wrote:\n> On Tue, May 20, 2025 at 11:53:10AM +0200, Patrick Steinhardt wrote:\n>\n> > @@ -458,6 +458,8 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n> >\n> > \tpack_int_id = midx_for_pack(&m, pack_int_id);\n> >\n> > +\tif (m->packs[pack_int_id] == (void *)(intptr_t)-1)\n> > +\t\treturn 1;\n> > \tif (m->packs[pack_int_id])\n> > \t\treturn 0;\n>\n> I did wonder while writing this if we might be able to hide the magic\n> number and gross casting inside a constant or macro. I think just:\n>\n> #define MIDX_PACK_ERROR ((void *)(intptr_t)-1)\n>\n> would be enough?\n>\n> Though...\n\nI agree with the longer-term goal of having prepare_midx_pack() just\nreturn a pointer to a struct packed_git. But in the meantime, I do think\nhaving a #define for the \"oops, I tried to load this packfile and it was\nbroken\" case is a good idea.\n\n> > @@ -495,6 +499,8 @@ struct packed_git *nth_midxed_pack(struct multi_pack_index *m,\n> > \t\t\t\t uint32_t pack_int_id)\n> > {\n> > \tuint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id);\n> > +\tif (m->packs[local_pack_int_id] == (void *)(intptr_t)-1)\n> > +\t\treturn NULL;\n> > \treturn m->packs[local_pack_int_id];\n>\n> Yuck, yet another spot that needs to be aware of the new tri-state\n> value. One alternative is using an auxiliary array to cache the errors,\n> and then only the lookup function needs to care. Like:\n\nI like this direction, though I dislike having a separate array that we\nneed to keep in sync with m->packs. It might be nice to have an array\nlike:\n\n struct {\n struct packed_git *p;\n unsigned err:1;\n } *packs;\n\n, which would allow you to keep the error state next to the packed_git\nitself.\n\nI wonder if changing the signature to:\n\n int prepare_midx_pack(struct repository *r,\n struct multi_pack_index *m,\n uint32_t pack_int_id,\n struct packed_git **p_out);\n\nwould be a good idea. It allows you to pass garbage input (like a\nnon-existent pack_int_id) and get a useful error back. It also allows\nyou to pass a pack_int_id that is valid, but cannot be loaded and get a\nuseful error back via the return value.\n\nBut I think without actually trying it and seeing what the fallout looks\nlike, it's hard to say whether or not the above is a step in the right\ndirection.\n\nThanks,\nTaylor\n"},{"id":"518741","messageId":"20250523020308.GA559000@coredump.intra.peff.net","threadId":"63469","inReplyTo":"aC/JDrF5uJh9XxXj@nand.local","subject":"Re: [PATCH] packfile: avoid access(3p) calls for missing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-23T02:03:08Z","receivedAt":"2025-05-23T02:03:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 22, 2025 at 09:02:06PM -0400, Taylor Blau wrote:\n\n> > I don't recall offhand whether reprepare_packed_git() will open the new\n> > midx. If it doesn't, we'd still find the object via the actual pack\n> > .idx, but we may end up consulting the now-stale midx over and over (so\n> > we'll get the right answer, but there may be some room for\n> > optimization).\n> \n> This all sounds right to me. And we do end up loading a new MIDX via\n> reprepare_packed_git(): that function calls prepare_packed_git() (which\n> doesn't immediately return, since we just zero'd out the\n> r->objects->packed_git_initialized flag). We then enumerate the ODBs,\n> calling prepare_multi_pack_index_one() and prepare_packed_git_one() for\n> each.\n> \n> From there we end up in load_multi_pack_index(), which gets a freshened\n> view of the MIDX file. If all goes well there, then\n> prepare_multi_pack_index_one() assigns it to\n> r->objects->multi_pack_index as expected.\n\nOne thing that puzzled me here is how prepare_multi_pack_index_one()\ndeals with an existing r->objects->multi_pack_index:\n\n m = load_multi_pack_index(r, object_dir, local);\n \n if (m) {\n struct multi_pack_index *mp = r->objects->multi_pack_index;\n if (mp) {\n m->next = mp->next;\n mp->next = m;\n } else\n r->objects->multi_pack_index = m;\n return 1;\n }\n\nI don't see reprepare_packed_git() zero-ing out the existing midx\npointer. So would we hit that \"if (mp)\" block, in which case we seem to\ncreate a loop of next pointers? But the old midx would still be the\nfirst one in the loop. Would we still consult it?\n\nOr am I just reading this totally wrong?\n\n-Peff\n"},{"id":"518742","messageId":"20250523020820.GB559000@coredump.intra.peff.net","threadId":"63469","inReplyTo":"aC/Nz2wQgncXzFvs@nand.local","subject":"Re: [PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-23T02:08:20Z","receivedAt":"2025-05-23T02:08:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 22, 2025 at 09:22:23PM -0400, Taylor Blau wrote:\n\n> > I think there's a subtlety here with incremental midx's, in that a pack\n> > id can be \"global\" within the whole midx chain, or a local index into a\n> > specific chain element's list of packs.\n> \n> If you'll indulge me in a bit of pedantry for a moment, I would add that\n> the pack_int_id is *always* global within the whole MIDX chain. It only\n> happens to additionally be the correct local index when\n> m->num_packs_in_base is zero.\n\nYeah, I agree with that...\n\n> The thing here:\n> \n> m->packs[pack_int_id - m->num_packs_in_base];\n> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> \n> is indexing into the current layer's pack array, which only contains the\n> packs at that layer, so the index there is no longer really a\n> pack_int_id at all.\n\n...and this...\n\n> > In fill_midx_entry(), I think we get such a global id back from\n> > nth_midxed_pack_int_id(). And then when we hand that to\n> > prepare_midx_pack(), it is converted into a local midx/id pair with:\n> >\n> > pack_int_id = midx_for_pack(&m, pack_int_id);\n> \n> ...this confusion is really my fault, since the thing midx_for_pack() is\n> returning isn't really a pack_int_id in the classic sense at all. I\n> think I chose this name to avoid making the patch noisy, and to avoid\n> the possibility of accidentally using the global identifier when you\n> meant to use the local index.\n\n...but yeah, this is the part that got me. ;) I think elsewhere you\ncalled it local_pack_int_id, in nth_midxed_pack(). Which made things\nmore clear (though still calls it a pack_int_id ;) ).\n\n> (As an aside, I think I recall you suggesting a while ago that it might\n> be interesting to define \"global\" things with a different type than\n> \"local\" ones to prevent this sort of confusion. That would allow us to\n> keep both \"pack_int_id\" and the return value of \"midx_for_pack()\" in\n> scope at the same time, without the possibility of using one when you\n> meant to use the other.)\n\nYeah. The trouble is that it becomes awkward in C, since the language\nwill happily intermix two integer typedefs. So you have to wrap them in\na struct and access the struct fields everywhere.\n\n> > - There's a similar case in midx-write.c:want_included_pack(). That\n> > one seems to have the same local/global confusion, but I do not\n> > obviously see anything preventing it from being fed a non-base midx.\n> > So it might possibly be buggy?\n> \n> Yeah, this spot is definitely broken. At minimum it would need something\n> like:\n> \n> --- 8< ---\n> diff --git a/midx-write.c b/midx-write.c\n> index 0897cbd829..54a04f7b75 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -1634,9 +1634,10 @@ static int want_included_pack(struct repository *r,\n> \t\t\t uint32_t pack_int_id)\n> {\n> \tstruct packed_git *p;\n> +\tmidx_for_pack(&m, pack_int_id);\n> \tif (prepare_midx_pack(r, m, pack_int_id))\n> \t\treturn 0;\n> -\tp = m->packs[pack_int_id];\n> +\tp = m->packs[pack_int_id - m->num_packs_in_base];\n> \tif (!pack_kept_objects && p->pack_keep)\n> \t\treturn 0;\n> \tif (p->is_cruft)\n> --- >8 ---\n\nYep, that's exactly what I was envisioning. I guess it's probably\npossible to trigger in practice by writing a new midx based on an\nexisting incremental state. I'll let you figure that part out. :)\n\n> > Likewise fill_included_packs_batch() in the same file.\n> \n> I think this one is actually OK for the same reason as the\n> expire_midx_packs() case. Its sole caller in midx_repack() has:\n> \n> if (m->base_midx)\n> die(_(\"cannot repack an incremental multi-pack-index\"));\n> \n> , so we are OK there. We might want to add an ASSERT() in\n> fill_included_packs_batch() to make it clearer, though.\n\nAh, good. Yes, I agree that an assertion would be a nice bit of\ndocumentation/safety. Though for these cases I think they only care\nabout the pack (not the containing midx), so changing the return type of\nprepare_midx_pack() might be an even easier way to get that safety.\n\n-Peff\n"},{"id":"518743","messageId":"20250523021828.GC559000@coredump.intra.peff.net","threadId":"63469","inReplyTo":"aC/QBHBbmYaVUzHV@nand.local","subject":"Re: [PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-23T02:18:28Z","receivedAt":"2025-05-23T02:18:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 22, 2025 at 09:31:48PM -0400, Taylor Blau wrote:\n\n> > Yuck, yet another spot that needs to be aware of the new tri-state\n> > value. One alternative is using an auxiliary array to cache the errors,\n> > and then only the lookup function needs to care. Like:\n> \n> I like this direction, though I dislike having a separate array that we\n> need to keep in sync with m->packs. It might be nice to have an array\n> like:\n> \n> struct {\n> struct packed_git *p;\n> unsigned err:1;\n> } *packs;\n> \n> , which would allow you to keep the error state next to the packed_git\n> itself.\n\nIn general, yes, I think array-of-struct is better than struct-of-array.\nIn this particular case the latter is not too bad because the management\nis all handled centrally in the midx constructor.\n\nThe downside of doing array-of-struct as you propose is that every site\nthat uses it will need to be modified. But that is at least a one-time\npain and not an ongoing maintenance burden (unlike the \"magic\" value\napproach).\n\n> I wonder if changing the signature to:\n> \n> int prepare_midx_pack(struct repository *r,\n> struct multi_pack_index *m,\n> uint32_t pack_int_id,\n> struct packed_git **p_out);\n> \n> would be a good idea. It allows you to pass garbage input (like a\n> non-existent pack_int_id) and get a useful error back. It also allows\n> you to pass a pack_int_id that is valid, but cannot be loaded and get a\n> useful error back via the return value.\n\nWould the return value be a richer set of values than the current\nsuccess/fail? If not, then I think just returning the pack pointer does\nthat fine.\n\nI was also tempted to suggest that it should take a \"struct\nmulti_pack_index **\", to return the matching midx as an out-parameter.\nThat would \"just work\" for callers that want to look at the surrounding\nmidx, too.\n\nBut maybe it gets weird for ones that are (correctly) expecting to find\nthe pack within the same midx. I.e., code like this:\n\n for (i = 0; i < m->num_packs; i++) {\n\tif (prepare_midx_pack(r, m, i + m->num_packs_in_base))\n\t\tdie(...);\n\t/* do something with m->pack[i] */\n }\n\nis correct now, and we _shouldn't_ ever need to switch to a different\n\"m\" inside prepare_midx_pack(). But if we ever did, propagating that up\nto the caller would be mighty confusing.\n\nSo perhaps better to leave it as-is, and let the caller explicitly do\nmidx_for_pack() or whatever to find the right \"m\" as necessary.\n\n> But I think without actually trying it and seeing what the fallout looks\n> like, it's hard to say whether or not the above is a step in the right\n> direction.\n\nYup. All of this can wait, too. Patrick's series is fixing its own\nlocalized issue (however he wants to structure the extra bit of\nstorage). Most of what we're talking about here is future-proofing so it\ncan happen at our leisure. I think the only other actual bug is the\nwant_included_pack() one discussed in the other part of the thread.\n\n-Peff\n"},{"id":"518786","messageId":"aDC0bK+NOuuVvQtb@nand.local","threadId":"63469","inReplyTo":"20250523020820.GB559000@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] midx: stop repeatedly looking up nonexistent packfiles","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-23T17:46:20Z","receivedAt":"2025-05-23T17:46:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 22, 2025 at 10:08:20PM -0400, Jeff King wrote:\n> > (As an aside, I think I recall you suggesting a while ago that it might\n> > be interesting to define \"global\" things with a different type than\n> > \"local\" ones to prevent this sort of confusion. That would allow us to\n> > keep both \"pack_int_id\" and the return value of \"midx_for_pack()\" in\n> > scope at the same time, without the possibility of using one when you\n> > meant to use the other.)\n>\n> Yeah. The trouble is that it becomes awkward in C, since the language\n> will happily intermix two integer typedefs. So you have to wrap them in\n> a struct and access the struct fields everywhere.\n\nYup :-<.\n\n> > > - There's a similar case in midx-write.c:want_included_pack(). That\n> > > one seems to have the same local/global confusion, but I do not\n> > > obviously see anything preventing it from being fed a non-base midx.\n> > > So it might possibly be buggy?\n> >\n> > Yeah, this spot is definitely broken. At minimum it would need something\n> > like:\n> >\n> > --- 8< ---\n> > diff --git a/midx-write.c b/midx-write.c\n> > index 0897cbd829..54a04f7b75 100644\n> > --- a/midx-write.c\n> > +++ b/midx-write.c\n> > @@ -1634,9 +1634,10 @@ static int want_included_pack(struct repository *r,\n> > \t\t\t uint32_t pack_int_id)\n> > {\n> > \tstruct packed_git *p;\n> > +\tmidx_for_pack(&m, pack_int_id);\n> > \tif (prepare_midx_pack(r, m, pack_int_id))\n> > \t\treturn 0;\n> > -\tp = m->packs[pack_int_id];\n> > +\tp = m->packs[pack_int_id - m->num_packs_in_base];\n> > \tif (!pack_kept_objects && p->pack_keep)\n> > \t\treturn 0;\n> > \tif (p->is_cruft)\n> > --- >8 ---\n>\n> Yep, that's exactly what I was envisioning. I guess it's probably\n> possible to trigger in practice by writing a new midx based on an\n> existing incremental state. I'll let you figure that part out. :)\n\nHmm. I thought that this spot was broken last night, but looking at it\nagain today I think that it is actually OK. I started writing an\nanalysis of why in response to your email, and then decided to throw it\naway since those details really should live in the patch message. Here's\nwhat I came up with:\n\n--- 8< ---\nSubject: [PATCH] midx-write.c: guard against incremental MIDXs in\n want_included_pack()\n\nThe function want_included_pack() is used to determine whether or not a\nthe packfile corresponding to some given pack_int_id should be included\nin a 'git multi-pack-index repack' operation.\n\nThis spot looks like it would be broken, particularly in:\n\n struct packed_git *p;\n if (prepare_midx_pack(r, m, pack_int_id))\n return 0;\n p = m->packs[pack_int_id];\n\n, when pack_int_id is greater than m->num_pack_in_base (supposing that\nm->num_packs_in_base is non-zero, or equivalently that m->base_midx is\nnon-NULL).\n\nSuppose we have two MIDXs in an incremental MIDX chain, each having two\npacks:\n\n - M0 = {packs: [P0, P1], base_midx: NULL}\n - M1 = {packs: [P2, P3], base_midx: M0}\n\nnoting that each pack is identified by its global pack_int_id within the\nchain.\n\nSo if you do something like:\n\n want_included_pack(the_repository, M1, pack_kept_objects, 2);\n\nthat function will call prepare_midx_pack(), which is smart enough to\nrealize that the pack of interest is in the current layer (M1), and\nknows how to adjust its global pack_int_id into an index into the\ncurrent layer's 'packs' array.\n\nBut the following line:\n\n p = m->packs[pack_int_id]; /* m is M1, pack_int_id is 2 */\n\nlooks broken, since each layer of the MIDX only maintains an array of\nthe packs stored within that layer, and 'm' wasn't adjusted back to\npoint at M1->base_midx (M0).\n\nThe right thing to do would be:\n\n struct packed_git *p;\n if (prepare_midx_pack(r, m, pack_int_id))\n return 0;\n\n /* open-code midx.c::midx_for_pack(), which is private */\n while (m && pack_int_id < m->num_packs_in_base)\n m = m->base_midx;\n\n if (!m)\n BUG(\"broken midx?\");\n if (pack_int_id >= m->num_packs + m->num_packs_in_base)\n BUG(\"broken pack_int_id?\");\n\n p = m->packs[pack_int_id - m->num_packs_in_base];\n\nBut that would be overkill, since this function never deals with\nincremental MIDXs having more than one layer! To see why, observe that\nwant_included_pack() has two callers:\n\n - midx-write.c::fill_included_packs_all()\n - midx-write.c::fill_included_packs_batch()\n\nand those functions' sole caller is in midx-write.c::midx_repack(),\nwhich dispatches a call to one or the other depending on whether or not\nthe batch_size is non-zero.\n\nAnd at the very top of midx_repack(), we have a guard against\nnon-trivial incremental MIDX chains:\n\n if (m->base_midx)\n die(_(\"cannot repack an incremental multi-pack-index\"));\n\nSo want_included_pack() is OK becuase it will never encounter a\nsituation where it has to chase backwards through the '->base_midx'\npointer. But that is not immediately clear from reading the code, and is\ntoo fragile for my comfort. Make this more clear by adding an ASSERT()\nto the above effect.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 0897cbd829..991c42d1dc 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1634,6 +1634,9 @@ static int want_included_pack(struct repository *r,\n \t\t\t uint32_t pack_int_id)\n {\n \tstruct packed_git *p;\n+\n+\tASSERT(m && !m->base_midx);\n+\n \tif (prepare_midx_pack(r, m, pack_int_id))\n \t\treturn 0;\n \tp = m->packs[pack_int_id];\n@@ -1653,6 +1656,8 @@ static void fill_included_packs_all(struct repository *r,\n \tuint32_t i;\n \tint pack_kept_objects = 0;\n\n+\tASSERT(m && !m->base_midx);\n+\n \trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n\n \tfor (i = 0; i < m->num_packs; i++) {\n@@ -1673,6 +1678,8 @@ static void fill_included_packs_batch(struct repository *r,\n \tstruct repack_info *pack_info;\n \tint pack_kept_objects = 0;\n\n+\tASSERT(m && !m->base_midx);\n+\n \tCALLOC_ARRAY(pack_info, m->num_packs);\n\n \trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n--\n2.49.0.221.g485f5f8636.dirty\n--- >8 ---\n\n> > > Likewise fill_included_packs_batch() in the same file.\n> >\n> > I think this one is actually OK for the same reason as the\n> > expire_midx_packs() case. Its sole caller in midx_repack() has:\n> >\n> > if (m->base_midx)\n> > die(_(\"cannot repack an incremental multi-pack-index\"));\n> >\n> > , so we are OK there. We might want to add an ASSERT() in\n> > fill_included_packs_batch() to make it clearer, though.\n>\n> Ah, good. Yes, I agree that an assertion would be a nice bit of\n> documentation/safety. Though for these cases I think they only care\n> about the pack (not the containing midx), so changing the return type of\n> prepare_midx_pack() might be an even easier way to get that safety.\n\nIronically, the same argument applies for this function (and the _all()\nvariant) as well ;-).\n\nThanks,\nTaylor\n"},{"id":"518864","messageId":"cover.1748198489.git.me@ttaylorr.com","threadId":"63469","inReplyTo":"aDC0bK+NOuuVvQtb@nand.local","subject":"[PATCH 0/5] midx: improve prepare_midx_pack() ergonomics","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-25T18:41:40Z","receivedAt":"2025-05-25T18:41:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"[This brief series is based on ps/midx-negative-packfile-cache].\n\nThis series implements a suggestion from this thread to have\nprepare_midx_pack() return a pointer to packed_git structure identified\nby the given pack_int_id.\n\nThis convention makes converting a pack_int_id into a bona-fide pack\nsignificantly less error-prone, particularly when dealing with\nincremental MIDXs.\n\nAlong the way, I noticed a couple of opportunities for clean-up and\nlight refactorings, which make up the first four patches.\n\nIn the future, I think we could take this further by:\n\n - applying the same pattern to the 'pack_names' array, and\n\n - renaming both arrays something like 'packs_' or similar to indicate\n that they are \"private\" and should not be accessed outside of\n midx.c.\n\nWe might be able to go even further than that by making the entire\nstructure opaque to external callers by defining it within midx.c\nitself. We don't get there in this series, but it sets us on the right\ndirection.\n\nThanks in advance for your review!\n\nTaylor Blau (5):\n pack-bitmap.c: fix broken warning() when missing MIDX'd pack\n midx-write.c: guard against incremental MIDXs in want_included_pack()\n midx-write.c: simplify fill_packs_from_midx()\n midx-write.c: extract inner loop from fill_packs_from_midx()\n midx: return a `packed_git` pointer from `prepare_midx_pack()`\n\n midx-write.c | 114 +++++++++++++++++++++++++++++++-------------------\n midx.c | 81 ++++++++++++++++++-----------------\n midx.h | 4 +-\n pack-bitmap.c | 4 +-\n 4 files changed, 115 insertions(+), 88 deletions(-)\n\n\nbase-commit: 73fe0882eec06f7b9ea021fe062a32cd1731e574\n-- \n2.49.0.641.gb9c9c4c3bd\n"},{"id":"518865","messageId":"ad7295b11b2e2c48d859623bf1bcfd92ef15ef9d.1748198489.git.me@ttaylorr.com","threadId":"63469","inReplyTo":"cover.1748198489.git.me@ttaylorr.com","subject":"[PATCH 1/5] pack-bitmap.c: fix broken warning() when missing MIDX'd pack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-25T18:41:51Z","receivedAt":"2025-05-25T18:41:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In commit 44f9fd6496 (pack-bitmap.c: check preferred pack validity when\nopening MIDX bitmap, 2022-05-24) we started opening all packs contained\nwithin a MIDX when loading its corresponding bitmap.\n\nHowever, if a pack is missing, then we will emit a warning like:\n\n warning: could not open pack pack-$HASH.pack\n\nLater on commit f31a17cea5 (pack-bitmap.c: open and store incremental\nbitmap layers, 2025-03-20) updated this code to work with incremental\nMIDX bitmaps, but did not adjust the index into the 'pack_names' field.\n\nSo if there is a pack in an incremental MIDX chain with a pack in a MIDX\nlayer with a non-zero number of packs in its base layer(s) (in other\nwords, any MIDX layer outside of the first one) that cannot be loaded,\nwe will do an out-of-bounds lookup.\n\nAdjust the lookup into the 'pack_names' array by the number of packs in\nthe base to prevent a potential SIGSEGV here.\n\nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex b9f1d86604..99c4927e9c 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -490,7 +490,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n \tfor (i = 0; i < bitmap_git->midx->num_packs + bitmap_git->midx->num_packs_in_base; i++) {\n \t\tif (prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {\n \t\t\twarning(_(\"could not open pack %s\"),\n-\t\t\t\tbitmap_git->midx->pack_names[i]);\n+\t\t\t\tbitmap_git->midx->pack_names[i - bitmap_git->midx->num_packs_in_base]);\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n-- \n2.49.0.641.gb9c9c4c3bd\n\n"},{"id":"518866","messageId":"d2f9645aa15b58824531fe5d981d2a73372b9482.1748198489.git.me@ttaylorr.com","threadId":"63469","inReplyTo":"cover.1748198489.git.me@ttaylorr.com","subject":"[PATCH 2/5] midx-write.c: guard against incremental MIDXs in want_included_pack()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-25T18:41:54Z","receivedAt":"2025-05-25T18:41:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function want_included_pack() is used to determine whether or not a\nthe packfile corresponding to some given pack_int_id should be included\nin a 'git multi-pack-index repack' operation.\n\nThis spot looks like it would be broken, particularly in:\n\n struct packed_git *p;\n if (prepare_midx_pack(r, m, pack_int_id))\n return 0;\n p = m->packs[pack_int_id];\n\n, when pack_int_id is greater than m->num_pack_in_base (supposing that\nm->num_packs_in_base is non-zero, or equivalently that m->base_midx is\nnon-NULL).\n\nSuppose we have two MIDXs in an incremental MIDX chain, each having two\npacks:\n\n - M0 = {packs: [P0, P1], base_midx: NULL}\n - M1 = {packs: [P2, P3], base_midx: M0}\n\nnoting that each pack is identified by its global pack_int_id within the\nchain.\n\nSo if you do something like:\n\n want_included_pack(the_repository, M1, pack_kept_objects, 2);\n\nthat function will call prepare_midx_pack(), which is smart enough to\nrealize that the pack of interest is in the current layer (M1), and\nknows how to adjust its global pack_int_id into an index into the\ncurrent layer's 'packs' array.\n\nBut the following line:\n\n p = m->packs[pack_int_id]; /* m is M1, pack_int_id is 2 */\n\nlooks broken, since each layer of the MIDX only maintains an array of\nthe packs stored within that layer, and 'm' wasn't adjusted back to\npoint at M1->base_midx (M0).\n\nThe right thing to do would be:\n\n struct packed_git *p;\n if (prepare_midx_pack(r, m, pack_int_id))\n return 0;\n\n /* open-code midx.c::midx_for_pack(), which is private */\n while (m && pack_int_id < m->num_packs_in_base)\n m = m->base_midx;\n\n if (!m)\n BUG(\"broken midx?\");\n if (pack_int_id >= m->num_packs + m->num_packs_in_base)\n BUG(\"broken pack_int_id?\");\n\n p = m->packs[pack_int_id - m->num_packs_in_base];\n\nBut that would be overkill, since this function never deals with\nincremental MIDXs having more than one layer! To see why, observe that\nwant_included_pack() has two callers:\n\n - midx-write.c::fill_included_packs_all()\n - midx-write.c::fill_included_packs_batch()\n\nand those functions' sole caller is in midx-write.c::midx_repack(),\nwhich dispatches a call to one or the other depending on whether or not\nthe batch_size is non-zero.\n\nAnd at the very top of midx_repack(), we have a guard against\nnon-trivial incremental MIDX chains:\n\n if (m->base_midx)\n die(_(\"cannot repack an incremental multi-pack-index\"));\n\nSo want_included_pack() is OK becuase it will never encounter a\nsituation where it has to chase backwards through the '->base_midx'\npointer. But that is not immediately clear from reading the code, and is\ntoo fragile for my comfort. Make this more clear by adding an ASSERT()\nto the above effect.\n\nApply the same treatment to each of the fill_included_packs-related\nfunctions as well, since those are deceptively OK by the same reasoning.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex dd3b3070e5..e4a3830d45 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1636,6 +1636,9 @@ static int want_included_pack(struct repository *r,\n \t\t\t uint32_t pack_int_id)\n {\n \tstruct packed_git *p;\n+\n+\tASSERT(m && !m->base_midx);\n+\n \tif (prepare_midx_pack(r, m, pack_int_id))\n \t\treturn 0;\n \tp = m->packs[pack_int_id];\n@@ -1655,6 +1658,8 @@ static void fill_included_packs_all(struct repository *r,\n \tuint32_t i;\n \tint pack_kept_objects = 0;\n \n+\tASSERT(m && !m->base_midx);\n+\n \trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n \n \tfor (i = 0; i < m->num_packs; i++) {\n@@ -1675,6 +1680,8 @@ static void fill_included_packs_batch(struct repository *r,\n \tstruct repack_info *pack_info;\n \tint pack_kept_objects = 0;\n \n+\tASSERT(m && !m->base_midx);\n+\n \tCALLOC_ARRAY(pack_info, m->num_packs);\n \n \trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n-- \n2.49.0.641.gb9c9c4c3bd\n\n"},{"id":"518867","messageId":"5d97b706e17117217bf6e90e5cbca9e6e408c5b4.1748198489.git.me@ttaylorr.com","threadId":"63469","inReplyTo":"cover.1748198489.git.me@ttaylorr.com","subject":"[PATCH 3/5] midx-write.c: simplify fill_packs_from_midx()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-25T18:41:57Z","receivedAt":"2025-05-25T18:41:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When enumerating packs within fill_packs_from_midx(), we open the pack's\nindex iff we are either writing a reverse index, have a preferred pack\n(by name) or both.\n\nLet's simplify this into a single case by setting the\nMIDX_WRITE_REV_INDEX flag bit when we have a preferred_pack_name. This\nis a little bit of a shortcut to reduce the line length in the loop\nbelow. But it sets us up nicely to extract the inner loop of this\nfunction out into its own function, where we will no longer have to pass\nthe preferred_pack_name.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex e4a3830d45..7802a4b694 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -943,6 +943,19 @@ static int fill_packs_from_midx(struct write_midx_context *ctx,\n {\n \tstruct multi_pack_index *m;\n \n+\tif (preferred_pack_name) {\n+\t\t/*\n+\t\t * If a preferred pack is specified, need to have\n+\t\t * packed_git's loaded to ensure the chosen preferred\n+\t\t * pack has a non-zero object count.\n+\t\t *\n+\t\t * Trick ourselves into thinking that we're writing a\n+\t\t * reverse index in this case in order to open up the\n+\t\t * pack index file.\n+\t\t */\n+\t\tflags |= MIDX_WRITE_REV_INDEX;\n+\t}\n+\n \tfor (m = ctx->m; m; m = m->base_midx) {\n \t\tuint32_t i;\n \n@@ -953,13 +966,8 @@ static int fill_packs_from_midx(struct write_midx_context *ctx,\n \t\t\t * If generating a reverse index, need to have\n \t\t\t * packed_git's loaded to compare their\n \t\t\t * mtimes and object count.\n-\t\t\t *\n-\t\t\t * If a preferred pack is specified, need to\n-\t\t\t * have packed_git's loaded to ensure the chosen\n-\t\t\t * preferred pack has a non-zero object count.\n \t\t\t */\n-\t\t\tif (flags & MIDX_WRITE_REV_INDEX ||\n-\t\t\t preferred_pack_name) {\n+\t\t\tif (flags & MIDX_WRITE_REV_INDEX) {\n \t\t\t\tif (prepare_midx_pack(ctx->repo, m,\n \t\t\t\t\t\t m->num_packs_in_base + i)) {\n \t\t\t\t\terror(_(\"could not load pack\"));\n-- \n2.49.0.641.gb9c9c4c3bd\n\n"},{"id":"518868","messageId":"a4080c12dfb971cff1f029502b13da01dc6f0929.1748198489.git.me@ttaylorr.com","threadId":"63469","inReplyTo":"cover.1748198489.git.me@ttaylorr.com","subject":"[PATCH 4/5] midx-write.c: extract inner loop from fill_packs_from_midx()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-25T18:42:00Z","receivedAt":"2025-05-25T18:42:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function fill_packs_from_midx() does relatively little, but ends up\nin a doubly-nested loop because we're enumerating each pack within each\nlayer of the incremental MIDX chain.\n\nLet's de-dent the inner loop of fill_packs_from_midx() by extracting its\ncontents into a separate function, and calling that.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 62 ++++++++++++++++++++++++++++++----------------------\n 1 file changed, 36 insertions(+), 26 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 7802a4b694..2afb67d728 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -938,6 +938,38 @@ static struct multi_pack_index *lookup_multi_pack_index(struct repository *r,\n \treturn result;\n }\n \n+static int fill_packs_from_midx_1(struct write_midx_context *ctx,\n+\t\t\t\t struct multi_pack_index *m,\n+\t\t\t\t uint32_t flags)\n+{\n+\tfor (uint32_t i = 0; i < m->num_packs; i++) {\n+\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n+\n+\t\t/*\n+\t\t * If generating a reverse index, need to have\n+\t\t * packed_git's loaded to compare their\n+\t\t * mtimes and object count.\n+\t\t */\n+\t\tif (flags & MIDX_WRITE_REV_INDEX) {\n+\t\t\tif (prepare_midx_pack(ctx->repo, m,\n+\t\t\t\t\t m->num_packs_in_base + i)) {\n+\t\t\t\terror(_(\"could not load pack\"));\n+\t\t\t\treturn 1;\n+\t\t\t}\n+\n+\t\t\tif (open_pack_index(m->packs[i]))\n+\t\t\t\tdie(_(\"could not open index for %s\"),\n+\t\t\t\t m->packs[i]->pack_name);\n+\t\t}\n+\n+\t\tfill_pack_info(&ctx->info[ctx->nr++], m->packs[i],\n+\t\t\t m->pack_names[i],\n+\t\t\t m->num_packs_in_base + i);\n+\t}\n+\n+\treturn 0;\n+}\n+\n static int fill_packs_from_midx(struct write_midx_context *ctx,\n \t\t\t\tconst char *preferred_pack_name, uint32_t flags)\n {\n@@ -957,33 +989,11 @@ static int fill_packs_from_midx(struct write_midx_context *ctx,\n \t}\n \n \tfor (m = ctx->m; m; m = m->base_midx) {\n-\t\tuint32_t i;\n-\n-\t\tfor (i = 0; i < m->num_packs; i++) {\n-\t\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n-\n-\t\t\t/*\n-\t\t\t * If generating a reverse index, need to have\n-\t\t\t * packed_git's loaded to compare their\n-\t\t\t * mtimes and object count.\n-\t\t\t */\n-\t\t\tif (flags & MIDX_WRITE_REV_INDEX) {\n-\t\t\t\tif (prepare_midx_pack(ctx->repo, m,\n-\t\t\t\t\t\t m->num_packs_in_base + i)) {\n-\t\t\t\t\terror(_(\"could not load pack\"));\n-\t\t\t\t\treturn 1;\n-\t\t\t\t}\n-\n-\t\t\t\tif (open_pack_index(m->packs[i]))\n-\t\t\t\t\tdie(_(\"could not open index for %s\"),\n-\t\t\t\t\t m->packs[i]->pack_name);\n-\t\t\t}\n-\n-\t\t\tfill_pack_info(&ctx->info[ctx->nr++], m->packs[i],\n-\t\t\t\t m->pack_names[i],\n-\t\t\t\t m->num_packs_in_base + i);\n-\t\t}\n+\t\tint ret = fill_packs_from_midx_1(ctx, m, flags);\n+\t\tif (ret)\n+\t\t\treturn ret;\n \t}\n+\n \treturn 0;\n }\n \n-- \n2.49.0.641.gb9c9c4c3bd\n\n"},{"id":"518869","messageId":"80699bb3ee35f8c4728e7efe8dc53d17134a5fe5.1748198489.git.me@ttaylorr.com","threadId":"63469","inReplyTo":"cover.1748198489.git.me@ttaylorr.com","subject":"[PATCH 5/5] midx: return a `packed_git` pointer from `prepare_midx_pack()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-25T18:42:03Z","receivedAt":"2025-05-25T18:42:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The prepare_midx_pack() function is designed to convert a MIDX-specific\npack_int_id for a given pack into a pointer into an actual `packed_git`\nstructure.\n\nIn general, these calls look something like:\n\n struct packed_git *p;\n if (prepare_midx_pack(the_repository, some_midx, some_pack_int_id))\n die(\"could not load pack xyz\");\n p = some_midx->packs[some_pack_int_id];\n\n, and in a pre-incremental MIDX world, this pattern works well. However,\nin a post-incremental MIDX world, this pattern is a little more prone to\nerrors.\n\nThese errors can happen when the given 'pack_int_id' is not usable as an\nindex into the 'm->packs' array. And this happens in all layers but the\nbottom-most one in an incremental MIDX chain. Each layer stores only the\npacks that are local to that layer of the chain, and offsets them by the\ntotal number of packs in the base MIDX(s).\n\nBut there is other awkwardness here. Thinking back to the above snippet,\nsuppose that the pack with ID 'some_pack_int_id' is in a layer in the\nmiddle of the MIDX chain. Then it is still invalid to do:\n\n p = some_midx->packs[some_pack_int_id - some_midx->num_packs_in_base];\n\n, becuase the top-most layer (here 'some_midx') may not even have that\npack! So we would have to chase down the '->base_midx' pointer in order\nto get the correct result. midx.c has a helper to do this (called\n'midx_for_pack()'), but it is meant only for internal use.\n\nThat means that a full, working version of the above adjusted to handle\nincremental MIDXs would look something like:\n\n struct packed_git *p;\n if (prepare_midx_pack(the_repository, some_midx, some_pack_int_id))\n die(\"could not load pack xyz\");\n\n while (m && pack_int_id < m->num_packs_in_base)\n m = m->base_midx;\n\n if (!m)\n BUG(\"broken midx?\");\n if (pack_int_id >= m->num_packs + m->num_packs_in_base)\n BUG(\"broken pack_int_id?\");\n\n p = m->packs[pack_int_id - m->num_packs_in_base];\n\n, which is far too verbose to access a single pack by its pack_int_id in\na MIDX chain.\n\nLet's instead have prepare_midx_pack() return a pointer to the\npacked_git structure itself, hiding the above as an implementation\ndetail of prepare_midx_pack(). This patch turns the above snippet into:\n\n struct packed_git *p = prepare_midx_pack(the_repository, some_midx,\n some_pack_int_id);\n if (!p)\n die(\"could not load pack xyz\");\n\nmaking it far easier and less error-prone to access packs by their\npack_int_id in a MIDX chain.\n\n(In the future, we may want to consider similar treatment for, e.g., the\npack_names array. Likewise, it might make sense to rename the \"packs\"\nmember of the MIDX structure to suggest that it shouldn't be accessed\ndirectly outside of midx.c.)\n\nSuggested-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n midx-write.c | 37 +++++++++++------------\n midx.c | 81 +++++++++++++++++++++++++--------------------------\n midx.h | 4 ++-\n pack-bitmap.c | 2 +-\n 4 files changed, 63 insertions(+), 61 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex 2afb67d728..e8dc771665 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -943,6 +943,8 @@ static int fill_packs_from_midx_1(struct write_midx_context *ctx,\n \t\t\t\t uint32_t flags)\n {\n \tfor (uint32_t i = 0; i < m->num_packs; i++) {\n+\t\tstruct packed_git *p = NULL;\n+\n \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n \n \t\t/*\n@@ -951,18 +953,19 @@ static int fill_packs_from_midx_1(struct write_midx_context *ctx,\n \t\t * mtimes and object count.\n \t\t */\n \t\tif (flags & MIDX_WRITE_REV_INDEX) {\n-\t\t\tif (prepare_midx_pack(ctx->repo, m,\n-\t\t\t\t\t m->num_packs_in_base + i)) {\n+\t\t\tp = prepare_midx_pack(ctx->repo, m,\n+\t\t\t\t\t m->num_packs_in_base + i);\n+\t\t\tif (!p) {\n \t\t\t\terror(_(\"could not load pack\"));\n \t\t\t\treturn 1;\n \t\t\t}\n \n-\t\t\tif (open_pack_index(m->packs[i]))\n+\t\t\tif (open_pack_index(p))\n \t\t\t\tdie(_(\"could not open index for %s\"),\n-\t\t\t\t m->packs[i]->pack_name);\n+\t\t\t\t p->pack_name);\n \t\t}\n \n-\t\tfill_pack_info(&ctx->info[ctx->nr++], m->packs[i],\n+\t\tfill_pack_info(&ctx->info[ctx->nr++], p,\n \t\t\t m->pack_names[i],\n \t\t\t m->num_packs_in_base + i);\n \t}\n@@ -1596,20 +1599,19 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla\n \t\t\t\t\t _(\"Finding and deleting unreferenced packfiles\"),\n \t\t\t\t\t m->num_packs);\n \tfor (i = 0; i < m->num_packs; i++) {\n+\t\tstruct packed_git *p;\n \t\tchar *pack_name;\n \t\tdisplay_progress(progress, i + 1);\n \n \t\tif (count[i])\n \t\t\tcontinue;\n \n-\t\tif (prepare_midx_pack(r, m, i))\n+\t\tp = prepare_midx_pack(r, m, i);\n+\t\tif (!p || p->pack_keep || p->is_cruft)\n \t\t\tcontinue;\n \n-\t\tif (m->packs[i]->pack_keep || m->packs[i]->is_cruft)\n-\t\t\tcontinue;\n-\n-\t\tpack_name = xstrdup(m->packs[i]->pack_name);\n-\t\tclose_pack(m->packs[i]);\n+\t\tpack_name = xstrdup(p->pack_name);\n+\t\tclose_pack(p);\n \n \t\tstring_list_insert(&packs_to_drop, m->pack_names[i]);\n \t\tunlink_pack_path(pack_name, 0);\n@@ -1657,9 +1659,9 @@ static int want_included_pack(struct repository *r,\n \n \tASSERT(m && !m->base_midx);\n \n-\tif (prepare_midx_pack(r, m, pack_int_id))\n+\tp = prepare_midx_pack(r, m, pack_int_id);\n+\tif (!p)\n \t\treturn 0;\n-\tp = m->packs[pack_int_id];\n \tif (!pack_kept_objects && p->pack_keep)\n \t\treturn 0;\n \tif (p->is_cruft)\n@@ -1705,12 +1707,11 @@ static void fill_included_packs_batch(struct repository *r,\n \trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n \n \tfor (i = 0; i < m->num_packs; i++) {\n+\t\tstruct packed_git *p = prepare_midx_pack(r, m, i);\n+\n \t\tpack_info[i].pack_int_id = i;\n-\n-\t\tif (prepare_midx_pack(r, m, i))\n-\t\t\tcontinue;\n-\n-\t\tpack_info[i].mtime = m->packs[i]->mtime;\n+\t\tif (p)\n+\t\t\tpack_info[i].mtime = p->mtime;\n \t}\n \n \tfor (i = 0; i < m->num_objects; i++) {\ndiff --git a/midx.c b/midx.c\nindex fbce88bd46..f7f509cf46 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -449,50 +449,48 @@ static uint32_t midx_for_pack(struct multi_pack_index **_m,\n \treturn pack_int_id - m->num_packs_in_base;\n }\n \n-int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n-\t\t uint32_t pack_int_id)\n+struct packed_git *prepare_midx_pack(struct repository *r,\n+\t\t\t\t struct multi_pack_index *m,\n+\t\t\t\t uint32_t pack_int_id)\n {\n-\tstruct strbuf pack_name = STRBUF_INIT;\n-\tstruct strbuf key = STRBUF_INIT;\n-\tstruct packed_git *p;\n+\tuint32_t pack_pos = midx_for_pack(&m, pack_int_id);\n \n-\tpack_int_id = midx_for_pack(&m, pack_int_id);\n+\tif (!m->packs[pack_pos]) {\n+\t\tstruct strbuf pack_name = STRBUF_INIT;\n+\t\tstruct strbuf key = STRBUF_INIT;\n+\t\tstruct packed_git *p;\n \n-\tif (m->packs[pack_int_id] == (void *)(intptr_t)-1)\n-\t\treturn 1;\n-\tif (m->packs[pack_int_id])\n-\t\treturn 0;\n+\t\tstrbuf_addf(&pack_name, \"%s/pack/%s\", m->object_dir,\n+\t\t\t m->pack_names[pack_pos]);\n \n-\tstrbuf_addf(&pack_name, \"%s/pack/%s\", m->object_dir,\n-\t\t m->pack_names[pack_int_id]);\n-\n-\t/* pack_map holds the \".pack\" name, but we have the .idx */\n-\tstrbuf_addbuf(&key, &pack_name);\n-\tstrbuf_strip_suffix(&key, \".idx\");\n-\tstrbuf_addstr(&key, \".pack\");\n-\tp = hashmap_get_entry_from_hash(&r->objects->pack_map,\n-\t\t\t\t\tstrhash(key.buf), key.buf,\n-\t\t\t\t\tstruct packed_git, packmap_ent);\n-\tif (!p) {\n-\t\tp = add_packed_git(r, pack_name.buf, pack_name.len, m->local);\n-\t\tif (p) {\n-\t\t\tinstall_packed_git(r, p);\n-\t\t\tlist_add_tail(&p->mru, &r->objects->packed_git_mru);\n+\t\t/* pack_map holds the \".pack\" name, but we have the .idx */\n+\t\tstrbuf_addbuf(&key, &pack_name);\n+\t\tstrbuf_strip_suffix(&key, \".idx\");\n+\t\tstrbuf_addstr(&key, \".pack\");\n+\t\tp = hashmap_get_entry_from_hash(&r->objects->pack_map,\n+\t\t\t\t\t\tstrhash(key.buf), key.buf,\n+\t\t\t\t\t\tstruct packed_git, packmap_ent);\n+\t\tif (!p) {\n+\t\t\tp = add_packed_git(r, pack_name.buf, pack_name.len,\n+\t\t\t\t\t m->local);\n+\t\t\tif (p) {\n+\t\t\t\tinstall_packed_git(r, p);\n+\t\t\t\tlist_add_tail(&p->mru,\n+\t\t\t\t\t &r->objects->packed_git_mru);\n+\t\t\t}\n \t\t}\n-\t}\n \n-\tstrbuf_release(&pack_name);\n-\tstrbuf_release(&key);\n+\t\tstrbuf_release(&pack_name);\n+\t\tstrbuf_release(&key);\n \n-\tif (!p) {\n-\t\tm->packs[pack_int_id] = (void *)(intptr_t)-1;\n-\t\treturn 1;\n+\t\tm->packs[pack_pos] = p ? p : (void *)(intptr_t)-1;\n+\t\tif (p)\n+\t\t\tp->multi_pack_index = 1;\n \t}\n \n-\tp->multi_pack_index = 1;\n-\tm->packs[pack_int_id] = p;\n-\n-\treturn 0;\n+\tif (m->packs[pack_pos] == (void *)(intptr_t)-1)\n+\t\treturn NULL;\n+\treturn m->packs[pack_pos];\n }\n \n struct packed_git *nth_midxed_pack(struct multi_pack_index *m,\n@@ -514,10 +512,11 @@ int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,\n \tif (!m->chunk_bitmapped_packs)\n \t\treturn error(_(\"MIDX does not contain the BTMP chunk\"));\n \n-\tif (prepare_midx_pack(r, m, pack_int_id))\n-\t\treturn error(_(\"could not load bitmapped pack %\"PRIu32), pack_int_id);\n+\tbp->p = prepare_midx_pack(r, m, pack_int_id);\n+\tif (!bp->p)\n+\t\treturn error(_(\"could not load bitmapped pack %\"PRIu32),\n+\t\t\t pack_int_id);\n \n-\tbp->p = m->packs[local_pack_int_id];\n \tbp->bitmap_pos = get_be32((char *)m->chunk_bitmapped_packs +\n \t\t\t\t MIDX_CHUNK_BITMAPPED_PACKS_WIDTH * local_pack_int_id);\n \tbp->bitmap_nr = get_be32((char *)m->chunk_bitmapped_packs +\n@@ -614,9 +613,9 @@ int fill_midx_entry(struct repository *r,\n \tmidx_for_object(&m, pos);\n \tpack_int_id = nth_midxed_pack_int_id(m, pos);\n \n-\tif (prepare_midx_pack(r, m, pack_int_id))\n+\tp = prepare_midx_pack(r, m, pack_int_id);\n+\tif (!p)\n \t\treturn 0;\n-\tp = m->packs[pack_int_id - m->num_packs_in_base];\n \n \t/*\n \t* We are about to tell the caller where they can locate the\n@@ -917,7 +916,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t\t\t\t\t\t _(\"Looking for referenced packfiles\"),\n \t\t\t\t\t\t m->num_packs + m->num_packs_in_base);\n \tfor (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n-\t\tif (prepare_midx_pack(r, m, i))\n+\t\tif (!prepare_midx_pack(r, m, i))\n \t\t\tmidx_report(\"failed to load pack in position %d\", i);\n \n \t\tdisplay_progress(progress, i + 1);\ndiff --git a/midx.h b/midx.h\nindex 9d1374cbd5..612999fe6a 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -104,7 +104,9 @@ void get_split_midx_filename_ext(const struct git_hash_algo *hash_algo,\n struct multi_pack_index *load_multi_pack_index(struct repository *r,\n \t\t\t\t\t const char *object_dir,\n \t\t\t\t\t int local);\n-int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id);\n+struct packed_git *prepare_midx_pack(struct repository *r,\n+\t\t\t\t struct multi_pack_index *m,\n+\t\t\t\t uint32_t pack_int_id);\n struct packed_git *nth_midxed_pack(struct multi_pack_index *m,\n \t\t\t\t uint32_t pack_int_id);\n int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 99c4927e9c..648b9c0212 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -488,7 +488,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n \t}\n \n \tfor (i = 0; i < bitmap_git->midx->num_packs + bitmap_git->midx->num_packs_in_base; i++) {\n-\t\tif (prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {\n+\t\tif (!prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {\n \t\t\twarning(_(\"could not open pack %s\"),\n \t\t\t\tbitmap_git->midx->pack_names[i - bitmap_git->midx->num_packs_in_base]);\n \t\t\tgoto cleanup;\n-- \n2.49.0.641.gb9c9c4c3bd\n"},{"id":"518905","messageId":"aDQXBBrL2Ni69gwE@pks.im","threadId":"63469","inReplyTo":"ad7295b11b2e2c48d859623bf1bcfd92ef15ef9d.1748198489.git.me@ttaylorr.com","subject":"Re: [PATCH 1/5] pack-bitmap.c: fix broken warning() when missing MIDX'd pack","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-26T07:23:48Z","receivedAt":"2025-05-26T07:23:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 25, 2025 at 02:41:51PM -0400, Taylor Blau wrote:\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index b9f1d86604..99c4927e9c 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -490,7 +490,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n> \tfor (i = 0; i < bitmap_git->midx->num_packs + bitmap_git->midx->num_packs_in_base; i++) {\n> \t\tif (prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {\n> \t\t\twarning(_(\"could not open pack %s\"),\n> -\t\t\t\tbitmap_git->midx->pack_names[i]);\n> +\t\t\t\tbitmap_git->midx->pack_names[i - bitmap_git->midx->num_packs_in_base]);\n\nDoesn't this cause a negative array index though in the case where\n`prepare_midx_pack()` returns an error for any `i` smaller than the\nnumber of packs in base?\n\nPatrick\n"},{"id":"518906","messageId":"aDQXDOelx3fc8W2n@pks.im","threadId":"63469","inReplyTo":"d2f9645aa15b58824531fe5d981d2a73372b9482.1748198489.git.me@ttaylorr.com","subject":"Re: [PATCH 2/5] midx-write.c: guard against incremental MIDXs in want_included_pack()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-26T07:23:56Z","receivedAt":"2025-05-26T07:24:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 25, 2025 at 02:41:54PM -0400, Taylor Blau wrote:\n[snip]\n> So want_included_pack() is OK becuase it will never encounter a\n\ns/becuase/because/\n\n> situation where it has to chase backwards through the '->base_midx'\n> pointer. But that is not immediately clear from reading the code, and is\n> too fragile for my comfort. Make this more clear by adding an ASSERT()\n> to the above effect.\n> \n> Apply the same treatment to each of the fill_included_packs-related\n> functions as well, since those are deceptively OK by the same reasoning.\n\nOk.\n\nPatrick\n"},{"id":"518907","messageId":"aDQXD8EHD163Mijo@pks.im","threadId":"63469","inReplyTo":"5d97b706e17117217bf6e90e5cbca9e6e408c5b4.1748198489.git.me@ttaylorr.com","subject":"Re: [PATCH 3/5] midx-write.c: simplify fill_packs_from_midx()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-26T07:23:59Z","receivedAt":"2025-05-26T07:24:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 25, 2025 at 02:41:57PM -0400, Taylor Blau wrote:\n> diff --git a/midx-write.c b/midx-write.c\n> index e4a3830d45..7802a4b694 100644\n> --- a/midx-write.c\n> +++ b/midx-write.c\n> @@ -943,6 +943,19 @@ static int fill_packs_from_midx(struct write_midx_context *ctx,\n> {\n> \tstruct multi_pack_index *m;\n> \n> +\tif (preferred_pack_name) {\n> +\t\t/*\n> +\t\t * If a preferred pack is specified, need to have\n> +\t\t * packed_git's loaded to ensure the chosen preferred\n> +\t\t * pack has a non-zero object count.\n> +\t\t *\n> +\t\t * Trick ourselves into thinking that we're writing a\n> +\t\t * reverse index in this case in order to open up the\n> +\t\t * pack index file.\n> +\t\t */\n> +\t\tflags |= MIDX_WRITE_REV_INDEX;\n> +\t}\n\nThis change feels a bit weird to me. Sure, it does allow us to pull out\nthe loop in the subsequent patch. But honestly, that makes this\nworkaround even weirder in that we now set unrelated flags in some\nfunction and expect a different function to only honor it in order to\nopen the packfile.\n\nShouldn't we instead have a separate flag for the new function that\ntells it whether or not it is supposed to prepare the pack?\n\nPatrick\n"},{"id":"518908","messageId":"aDQXEz-RtrcUAwic@pks.im","threadId":"63469","inReplyTo":"80699bb3ee35f8c4728e7efe8dc53d17134a5fe5.1748198489.git.me@ttaylorr.com","subject":"Re: [PATCH 5/5] midx: return a `packed_git` pointer from `prepare_midx_pack()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-26T07:24:03Z","receivedAt":"2025-05-26T07:24:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 25, 2025 at 02:42:03PM -0400, Taylor Blau wrote:\n> diff --git a/midx.c b/midx.c\n> index fbce88bd46..f7f509cf46 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -449,50 +449,48 @@ static uint32_t midx_for_pack(struct multi_pack_index **_m,\n> \treturn pack_int_id - m->num_packs_in_base;\n> }\n> \n> -int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n> -\t\t uint32_t pack_int_id)\n> +struct packed_git *prepare_midx_pack(struct repository *r,\n> +\t\t\t\t struct multi_pack_index *m,\n> +\t\t\t\t uint32_t pack_int_id)\n> {\n> -\tstruct strbuf pack_name = STRBUF_INIT;\n> -\tstruct strbuf key = STRBUF_INIT;\n> -\tstruct packed_git *p;\n> +\tuint32_t pack_pos = midx_for_pack(&m, pack_int_id);\n> \n> -\tpack_int_id = midx_for_pack(&m, pack_int_id);\n> +\tif (!m->packs[pack_pos]) {\n> +\t\tstruct strbuf pack_name = STRBUF_INIT;\n> +\t\tstruct strbuf key = STRBUF_INIT;\n> +\t\tstruct packed_git *p;\n> \n> -\tif (m->packs[pack_int_id] == (void *)(intptr_t)-1)\n> -\t\treturn 1;\n\nAh, so this series builds on top of my patch that introduces the\nnegative lookup cache? That wasn't quite clear to me and makes it a bit\nhard to iterate on my patch now. Could I suggest that you maybe include\nthat patch as part of this series so that those can be iterated on in\ntandem?\n\nOverall I think that this change is quite sensible and hides away at\nleast some of the complexity. Thanks!\n\nPatrick\n"},{"id":"519035","messageId":"aDZuMciIqrP2aPq8@nand.local","threadId":"63469","inReplyTo":"aDQXBBrL2Ni69gwE@pks.im","subject":"Re: [PATCH 1/5] pack-bitmap.c: fix broken warning() when missing MIDX'd pack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-28T02:00:17Z","receivedAt":"2025-05-28T02:00:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, May 26, 2025 at 09:23:48AM +0200, Patrick Steinhardt wrote:\n> On Sun, May 25, 2025 at 02:41:51PM -0400, Taylor Blau wrote:\n> > diff --git a/pack-bitmap.c b/pack-bitmap.c\n> > index b9f1d86604..99c4927e9c 100644\n> > --- a/pack-bitmap.c\n> > +++ b/pack-bitmap.c\n> > @@ -490,7 +490,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n> > \tfor (i = 0; i < bitmap_git->midx->num_packs + bitmap_git->midx->num_packs_in_base; i++) {\n> > \t\tif (prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {\n> > \t\t\twarning(_(\"could not open pack %s\"),\n> > -\t\t\t\tbitmap_git->midx->pack_names[i]);\n> > +\t\t\t\tbitmap_git->midx->pack_names[i - bitmap_git->midx->num_packs_in_base]);\n>\n> Doesn't this cause a negative array index though in the case where\n> `prepare_midx_pack()` returns an error for any `i` smaller than the\n> number of packs in base?\n\nNice catch, yeah, this is definitely broken when the value i is smaller\nthan m->num_packs_in_base.\n\nThis is another spot that would benefit from similar treatment where\ncallers (outside of midx.c and *maybe* midx-write.c) access the\npack_names array through a function liked nth_midxed_pack_name() or\nsimilar.\n\nI'll adjust this and send a new round.\n\nThanks,\nTaylor\n"},{"id":"519036","messageId":"aDZwGsBpTjTwe6BJ@nand.local","threadId":"63469","inReplyTo":"aDQXDOelx3fc8W2n@pks.im","subject":"Re: [PATCH 2/5] midx-write.c: guard against incremental MIDXs in want_included_pack()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-28T02:08:26Z","receivedAt":"2025-05-28T02:08:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, May 26, 2025 at 09:23:56AM +0200, Patrick Steinhardt wrote:\n> On Sun, May 25, 2025 at 02:41:54PM -0400, Taylor Blau wrote:\n> [snip]\n> > So want_included_pack() is OK becuase it will never encounter a\n>\n> s/becuase/because/\n\nThanks for spotting.\n\nThanks,\nTaylor\n"},{"id":"519037","messageId":"aDZxtK+I0sWNCUVl@nand.local","threadId":"63469","inReplyTo":"aDQXD8EHD163Mijo@pks.im","subject":"Re: [PATCH 3/5] midx-write.c: simplify fill_packs_from_midx()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-28T02:15:16Z","receivedAt":"2025-05-28T02:15:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, May 26, 2025 at 09:23:59AM +0200, Patrick Steinhardt wrote:\n> On Sun, May 25, 2025 at 02:41:57PM -0400, Taylor Blau wrote:\n> > diff --git a/midx-write.c b/midx-write.c\n> > index e4a3830d45..7802a4b694 100644\n> > --- a/midx-write.c\n> > +++ b/midx-write.c\n> > @@ -943,6 +943,19 @@ static int fill_packs_from_midx(struct write_midx_context *ctx,\n> > {\n> > \tstruct multi_pack_index *m;\n> >\n> > +\tif (preferred_pack_name) {\n> > +\t\t/*\n> > +\t\t * If a preferred pack is specified, need to have\n> > +\t\t * packed_git's loaded to ensure the chosen preferred\n> > +\t\t * pack has a non-zero object count.\n> > +\t\t *\n> > +\t\t * Trick ourselves into thinking that we're writing a\n> > +\t\t * reverse index in this case in order to open up the\n> > +\t\t * pack index file.\n> > +\t\t */\n> > +\t\tflags |= MIDX_WRITE_REV_INDEX;\n> > +\t}\n>\n> This change feels a bit weird to me. Sure, it does allow us to pull out\n> the loop in the subsequent patch. But honestly, that makes this\n> workaround even weirder in that we now set unrelated flags in some\n> function and expect a different function to only honor it in order to\n> open the packfile.\n>\n> Shouldn't we instead have a separate flag for the new function that\n> tells it whether or not it is supposed to prepare the pack?\n\nYeah, I like that much better, thanks!\n\nThanks,\nTaylor\n"},{"id":"519038","messageId":"aDZyXlPojqxxxjl6@nand.local","threadId":"63469","inReplyTo":"aDQXEz-RtrcUAwic@pks.im","subject":"Re: [PATCH 5/5] midx: return a `packed_git` pointer from `prepare_midx_pack()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-28T02:18:06Z","receivedAt":"2025-05-28T02:18:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, May 26, 2025 at 09:24:03AM +0200, Patrick Steinhardt wrote:\n> On Sun, May 25, 2025 at 02:42:03PM -0400, Taylor Blau wrote:\n> > diff --git a/midx.c b/midx.c\n> > index fbce88bd46..f7f509cf46 100644\n> > --- a/midx.c\n> > +++ b/midx.c\n> > @@ -449,50 +449,48 @@ static uint32_t midx_for_pack(struct multi_pack_index **_m,\n> > \treturn pack_int_id - m->num_packs_in_base;\n> > }\n> >\n> > -int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n> > -\t\t uint32_t pack_int_id)\n> > +struct packed_git *prepare_midx_pack(struct repository *r,\n> > +\t\t\t\t struct multi_pack_index *m,\n> > +\t\t\t\t uint32_t pack_int_id)\n> > {\n> > -\tstruct strbuf pack_name = STRBUF_INIT;\n> > -\tstruct strbuf key = STRBUF_INIT;\n> > -\tstruct packed_git *p;\n> > +\tuint32_t pack_pos = midx_for_pack(&m, pack_int_id);\n> >\n> > -\tpack_int_id = midx_for_pack(&m, pack_int_id);\n> > +\tif (!m->packs[pack_pos]) {\n> > +\t\tstruct strbuf pack_name = STRBUF_INIT;\n> > +\t\tstruct strbuf key = STRBUF_INIT;\n> > +\t\tstruct packed_git *p;\n> >\n> > -\tif (m->packs[pack_int_id] == (void *)(intptr_t)-1)\n> > -\t\treturn 1;\n>\n> Ah, so this series builds on top of my patch that introduces the\n> negative lookup cache? That wasn't quite clear to me and makes it a bit\n> hard to iterate on my patch now.\n\nEek, sorry for the confusion. I mentioned it in the beginning of my\ncover letter as well as the (less visible) \"base-commit\" identifier. Is\nthere another spot where I could have highlighted the dependency more\nclearly?\n\n> Could I suggest that you maybe include that patch as part of this\n> series so that those can be iterated on in tandem?\n\nI could, however your branch is already tentatively marked for 'next',\nand Junio applied this topic to a branch based on yours as I had\nsuggested. So we could change it, but I think it would be more of a\nhassle for Junio, so I'd rather avoid doing so.\n\nAre there changes that you want to make on top of your patch?\n\n> Overall I think that this change is quite sensible and hides away at\n> least some of the complexity. Thanks!\n\nMuch appreciated :-).\n\nThanks,\nTaylor\n"},{"id":"519072","messageId":"aDb5QP3l8PaFNwCB@pks.im","threadId":"63469","inReplyTo":"aDZyXlPojqxxxjl6@nand.local","subject":"Re: [PATCH 5/5] midx: return a `packed_git` pointer from `prepare_midx_pack()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-28T11:53:36Z","receivedAt":"2025-05-28T11:53:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, May 27, 2025 at 10:18:06PM -0400, Taylor Blau wrote:\n> On Mon, May 26, 2025 at 09:24:03AM +0200, Patrick Steinhardt wrote:\n> > On Sun, May 25, 2025 at 02:42:03PM -0400, Taylor Blau wrote:\n> > > diff --git a/midx.c b/midx.c\n> > > index fbce88bd46..f7f509cf46 100644\n> > > --- a/midx.c\n> > > +++ b/midx.c\n> > > @@ -449,50 +449,48 @@ static uint32_t midx_for_pack(struct multi_pack_index **_m,\n> > > \treturn pack_int_id - m->num_packs_in_base;\n> > > }\n> > >\n> > > -int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n> > > -\t\t uint32_t pack_int_id)\n> > > +struct packed_git *prepare_midx_pack(struct repository *r,\n> > > +\t\t\t\t struct multi_pack_index *m,\n> > > +\t\t\t\t uint32_t pack_int_id)\n> > > {\n> > > -\tstruct strbuf pack_name = STRBUF_INIT;\n> > > -\tstruct strbuf key = STRBUF_INIT;\n> > > -\tstruct packed_git *p;\n> > > +\tuint32_t pack_pos = midx_for_pack(&m, pack_int_id);\n> > >\n> > > -\tpack_int_id = midx_for_pack(&m, pack_int_id);\n> > > +\tif (!m->packs[pack_pos]) {\n> > > +\t\tstruct strbuf pack_name = STRBUF_INIT;\n> > > +\t\tstruct strbuf key = STRBUF_INIT;\n> > > +\t\tstruct packed_git *p;\n> > >\n> > > -\tif (m->packs[pack_int_id] == (void *)(intptr_t)-1)\n> > > -\t\treturn 1;\n> >\n> > Ah, so this series builds on top of my patch that introduces the\n> > negative lookup cache? That wasn't quite clear to me and makes it a bit\n> > hard to iterate on my patch now.\n> \n> Eek, sorry for the confusion. I mentioned it in the beginning of my\n> cover letter as well as the (less visible) \"base-commit\" identifier. Is\n> there another spot where I could have highlighted the dependency more\n> clearly?\n\nOh, it was right at the top of that mail. Maybe the square brackets have\nmade me skip it? No idea. In any case it was present, I just happened to\nread over it.\n\n> > Could I suggest that you maybe include that patch as part of this\n> > series so that those can be iterated on in tandem?\n> \n> I could, however your branch is already tentatively marked for 'next',\n> and Junio applied this topic to a branch based on yours as I had\n> suggested. So we could change it, but I think it would be more of a\n> hassle for Junio, so I'd rather avoid doing so.\n> \n> Are there changes that you want to make on top of your patch?\n\nThe only change was that I wanted to add is the `(void *)(intptr_t)-1`\npart with a macro, as proposed by Peff. You know, it should be trivial\nto adapt to that change, so I'll just send out a new version now, which\nshould then probably be merged down to `next` soonish anyway.\n\nPatrick\n"},{"id":"519073","messageId":"20250528-pks-pack-avoid-stats-on-missing-v3-0-4cbd0e14bed8@pks.im","threadId":"63469","inReplyTo":"20250516-pks-pack-avoid-stats-on-missing-v1-1-e2ef4d8798a3@pks.im","subject":"[PATCH v3 0/2] packfile: avoid access(3p) calls for missing packs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-28T12:24:09Z","receivedAt":"2025-05-28T12:24:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch addresses an issue we have recently seen in our production\nsystems due to a stale MIDX. The MIDX contained entries for packfiles\nthat didn't exist anymore, which caused Git to repeatedly look up those\npackfiles. Each missing packfile resulted in four repeated syscalls:\nthree access(3p) calls to check for supporting data structures, and one\ncall to stat(3p) to check for the packfile itself. The first three calls\nare essentially wasted though when the stat(3p) call itself fails, which\nis being fixed by this patch.\n\nI doubt that the patch matters in almost any repository, but given that\nthe refactoring is trivial I thought to submit the patch regardless of\nthat. Another step would be to introduce a negative lookup cache -- but\nthat would be a bit more involved, so I decided against it for now as I\ndon't want to introduce complexity for dubious gains.\n\nChanges in v2:\n - Drop the patch that reorders syscalls and add a comment explaining\n why the order is important.\n - Add a negative lookup cache for indexed packfiles.\n - Link to v1: https://lore.kernel.org/r/20250516-pks-pack-avoid-stats-on-missing-v1-1-e2ef4d8798a3@pks.im\n\nChanges in v3:\n - Use a macro to hide away the `(void *)(intptr_t)-1` magic.\n - Link to v2: https://lore.kernel.org/r/20250520-pks-pack-avoid-stats-on-missing-v2-0-333c5217fb05@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (2):\n packfile: explain ordering of how we look up auxiliary pack files\n midx: stop repeatedly looking up nonexistent packfiles\n\n midx.c | 12 ++++++++++--\n packfile.c | 11 +++++++++++\n 2 files changed, 21 insertions(+), 2 deletions(-)\n\nRange-diff versus v2:\n\n1: cf601a63e98 = 1: c4087674967 packfile: explain ordering of how we look up auxiliary pack files\n2: e3108f7ce48 ! 2: 2fa98231464 midx: stop repeatedly looking up nonexistent packfiles\n @@ Commit message\n Signed-off-by: Patrick Steinhardt \n \n ## midx.c ##\n +@@\n + #include \"pack-bitmap.h\"\n + #include \"pack-revindex.h\"\n + \n ++#define MIDX_PACK_ERROR ((void *)(intptr_t)-1)\n ++\n + int midx_checksum_valid(struct multi_pack_index *m);\n + void clear_midx_files_ext(const char *object_dir, const char *ext,\n + \t\t\t const char *keep_hash);\n @@ midx.c: void close_midx(struct multi_pack_index *m)\n \tmunmap((unsigned char *)m->data, m->data_len);\n \n \tfor (i = 0; i < m->num_packs; i++) {\n -\t\tif (m->packs[i])\n -+\t\tif (m->packs[i] && m->packs[i] != (void *)(intptr_t)-1)\n ++\t\tif (m->packs[i] && m->packs[i] != MIDX_PACK_ERROR)\n \t\t\tm->packs[i]->multi_pack_index = 0;\n \t}\n \tFREE_AND_NULL(m->packs);\n @@ midx.c: int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n \n \tpack_int_id = midx_for_pack(&m, pack_int_id);\n \n -+\tif (m->packs[pack_int_id] == (void *)(intptr_t)-1)\n ++\tif (m->packs[pack_int_id] == MIDX_PACK_ERROR)\n +\t\treturn 1;\n \tif (m->packs[pack_int_id])\n \t\treturn 0;\n @@ midx.c: int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n \n -\tif (!p)\n +\tif (!p) {\n -+\t\tm->packs[pack_int_id] = (void *)(intptr_t)-1;\n ++\t\tm->packs[pack_int_id] = MIDX_PACK_ERROR;\n \t\treturn 1;\n +\t}\n \n @@ midx.c: struct packed_git *nth_midxed_pack(struct multi_pack_index *m,\n \t\t\t\t uint32_t pack_int_id)\n {\n \tuint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id);\n -+\tif (m->packs[local_pack_int_id] == (void *)(intptr_t)-1)\n ++\tif (m->packs[local_pack_int_id] == MIDX_PACK_ERROR)\n +\t\treturn NULL;\n \treturn m->packs[local_pack_int_id];\n }\n\n---\nbase-commit: 1a8a4971cc6c179c4dd711f4a7f5d7178f4b3ab7\nchange-id: 20250516-pks-pack-avoid-stats-on-missing-8e3b75755cf0\n\n"},{"id":"519074","messageId":"20250528-pks-pack-avoid-stats-on-missing-v3-2-4cbd0e14bed8@pks.im","threadId":"63469","inReplyTo":"20250528-pks-pack-avoid-stats-on-missing-v3-0-4cbd0e14bed8@pks.im","subject":"[PATCH v3 2/2] midx: stop repeatedly looking up nonexistent packfiles","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-28T12:24:11Z","receivedAt":"2025-05-28T12:24:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The multi-pack index acts as a cache across a set of packfiles so that\nwe can quickly look up which of those packfiles contains a given object.\nAs such, the multi-pack index naturally needs to be updated every time\none of the packfiles goes away, or otherwise the multi-pack index has\ngrown stale.\n\nA stale multi-pack index should be handled gracefully by Git though, and\nin fact it is: if the indexed pack cannot be found we simply ignore it\nand eventually we fall back to doing the object lookup by just iterating\nthrough all packs, even if those aren't indexed.\n\nBut while this fallback works, it has one significant downside: we don't\ncache the fact that a pack has vanished. This leads to us repeatedly\ntrying to look up the same pack only to realize that it (still) doesn't\nexist.\n\nThis issue can be easily demonstrated by creating a repository with a\nstale multi-pack index and a couple of objects. We do so by creating a\nrepository with two packfiles, both of which are indexed by the\nmulti-pack index, and then repack those two packfiles. Note that we have\nto move the multi-pack-index before doing the final repack, as Git knows\nto delete it otherwise.\n\n $ git init repo\n $ cd repo/\n $ git config set maintenance.auto false\n $ for i in $(seq 1000); do printf \"%d-original\" $i >file-$i; done\n $ git add .\n $ git commit -moriginal\n $ git repack -dl\n $ for i in $(seq 1000); do printf \"%d-modified\" $i >file-$i; done\n $ git commit -a -mmodified\n $ git repack -dl\n $ git multi-pack-index write\n $ mv .git/objects/pack/multi-pack-index .\n $ git repack -Adl\n $ mv multi-pack-index .git/objects/pack/\n\nCommands that cause a lot of objects lookups will now repeatedly invoke\n`add_packed_git()`, which leads to three failed access(3p) calls as well\nas one failed stat(3p) call. The following strace for example is done\nfor `git log --patch` in the above repository:\n\n % time seconds usecs/call calls errors syscall\n ------ ----------- ----------- --------- --------- ----------------\n 74.67 0.024693 1 18038 18031 access\n 25.33 0.008378 1 6045 6017 newfstatat\n ------ ----------- ----------- --------- --------- ----------------\n 100.00 0.033071 1 24083 24048 total\n\nFix the issue by introducing a negative lookup cache for indexed packs.\nThis cache works by simply storing an invalid pointer for a missing pack\nwhen `prepare_midx_pack()` fails to look up the pack. Most users of the\n`packs` array don't need to be adjusted, either, as they all know to\ncall `prepare_midx_pack()` before accessing the array.\n\nWith this change in place we can now see a significantly reduced number\nof syscalls:\n\n % time seconds usecs/call calls errors syscall\n ------ ----------- ----------- --------- --------- ----------------\n 73.58 0.000323 5 60 28 newfstatat\n 26.42 0.000116 5 23 16 access\n ------ ----------- ----------- --------- --------- ----------------\n 100.00 0.000439 5 83 44 total\n\nFurthermore, this change also results in a speedup:\n\n Benchmark 1: git log --patch (revision = HEAD~)\n Time (mean ± σ): 50.4 ms ± 2.5 ms [User: 22.0 ms, System: 24.4 ms]\n Range (min … max): 45.4 ms … 54.9 ms 53 runs\n\n Benchmark 2: git log --patch (revision = HEAD)\n Time (mean ± σ): 12.7 ms ± 0.4 ms [User: 11.1 ms, System: 1.6 ms]\n Range (min … max): 12.4 ms … 15.0 ms 191 runs\n\n Summary\n git log --patch (revision = HEAD) ran\n 3.96 ± 0.22 times faster than git log --patch (revision = HEAD~)\n\nIn the end, it should in theory never be necessary to have this negative\nlookup cache given that we know to update the multi-pack index together\nwith repacks. But as the change is quite contained and as the speedup\ncan be significant as demonstrated above, it does feel sensible to have\nthe negative lookup cache regardless.\n\nBased-on-patch-by: Jeff King \nSigned-off-by: Patrick Steinhardt \n---\n midx.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 3d0015f7828..cd6e766ce2b 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -13,6 +13,8 @@\n #include \"pack-bitmap.h\"\n #include \"pack-revindex.h\"\n \n+#define MIDX_PACK_ERROR ((void *)(intptr_t)-1)\n+\n int midx_checksum_valid(struct multi_pack_index *m);\n void clear_midx_files_ext(const char *object_dir, const char *ext,\n \t\t\t const char *keep_hash);\n@@ -405,7 +407,7 @@ void close_midx(struct multi_pack_index *m)\n \tmunmap((unsigned char *)m->data, m->data_len);\n \n \tfor (i = 0; i < m->num_packs; i++) {\n-\t\tif (m->packs[i])\n+\t\tif (m->packs[i] && m->packs[i] != MIDX_PACK_ERROR)\n \t\t\tm->packs[i]->multi_pack_index = 0;\n \t}\n \tFREE_AND_NULL(m->packs);\n@@ -458,6 +460,8 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n \n \tpack_int_id = midx_for_pack(&m, pack_int_id);\n \n+\tif (m->packs[pack_int_id] == MIDX_PACK_ERROR)\n+\t\treturn 1;\n \tif (m->packs[pack_int_id])\n \t\treturn 0;\n \n@@ -482,8 +486,10 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n \tstrbuf_release(&pack_name);\n \tstrbuf_release(&key);\n \n-\tif (!p)\n+\tif (!p) {\n+\t\tm->packs[pack_int_id] = MIDX_PACK_ERROR;\n \t\treturn 1;\n+\t}\n \n \tp->multi_pack_index = 1;\n \tm->packs[pack_int_id] = p;\n@@ -495,6 +501,8 @@ struct packed_git *nth_midxed_pack(struct multi_pack_index *m,\n \t\t\t\t uint32_t pack_int_id)\n {\n \tuint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id);\n+\tif (m->packs[local_pack_int_id] == MIDX_PACK_ERROR)\n+\t\treturn NULL;\n \treturn m->packs[local_pack_int_id];\n }\n \n\n-- \n2.49.0.1266.g31b7d2e469.dirty\n\n"},{"id":"519075","messageId":"20250528-pks-pack-avoid-stats-on-missing-v3-1-4cbd0e14bed8@pks.im","threadId":"63469","inReplyTo":"20250528-pks-pack-avoid-stats-on-missing-v3-0-4cbd0e14bed8@pks.im","subject":"[PATCH v3 1/2] packfile: explain ordering of how we look up auxiliary pack files","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-28T12:24:10Z","receivedAt":"2025-05-28T12:24:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When adding a packfile to an object database we perform four syscalls:\n\n - Three calls to access(3p) are done to check for auxiliary data\n structures.\n\n - One call to stat(3p) is done to check for the \".pack\" itself.\n\nOne curious bit is that we perform the access(3p) calls before checking\nfor the packfile itself, but if the packfile doesn't exist we discard\nall results. The access(3p) calls are thus essentially wasted, so one\nmay be triggered to reorder those calls so that we can short-circuit the\nother syscalls in case the packfile does not exist.\n\nThe order in which we look up files is quite important though to help\navoid races:\n\n - When installing a packfile we move auxiliary data structures into\n place before we install the \".idx\" file.\n\n - When deleting a packfile we first delete the \".idx\" and \".pack\"\n files before deleting auxiliary data structures.\n\nAs such, to avoid any races with concurrently created or deleted packs\nwe need to make sure that we _first_ read auxiliary data structures\nbefore we read the corresponding \".idx\" or \".pack\" file. Otherwise it\nmay easily happen that we return a populated but misclassified pack.\n\nAdd a comment to `add_packed_git()` to make future readers aware of this\nordering requirement.\n\nSigned-off-by: Patrick Steinhardt \n---\n packfile.c | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/packfile.c b/packfile.c\nindex d91016f1c7f..933036e2606 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -737,6 +737,17 @@ struct packed_git *add_packed_git(struct repository *r, const char *path,\n \tp = alloc_packed_git(r, alloc);\n \tmemcpy(p->pack_name, path, path_len);\n \n+\t/*\n+\t * Note that we have to check auxiliary data structures before we check\n+\t * for the \".pack\" file to exist to avoid races with a packfile that is\n+\t * in the process of being deleted. The \".pack\" file is unlinked before\n+\t * its auxiliary data structures, so we know that we either get a\n+\t * consistent snapshot of all data structures or that we'll fail to\n+\t * stat(3p) the packfile itself and thus return `NULL`.\n+\t *\n+\t * As such, we cannot bail out before the access(3p) calls in case the\n+\t * packfile doesn't exist without doing two stat(3p) calls for it.\n+\t */\n \txsnprintf(p->pack_name + path_len, alloc - path_len, \".keep\");\n \tif (!access(p->pack_name, F_OK))\n \t\tp->pack_keep = 1;\n\n-- \n2.49.0.1266.g31b7d2e469.dirty\n\n"},{"id":"519107","messageId":"cover.1748473122.git.me@ttaylorr.com","threadId":"63469","inReplyTo":"cover.1748198489.git.me@ttaylorr.com","subject":"[PATCH v2 0/4] midx: improve prepare_midx_pack() ergonomics","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-28T22:58:57Z","receivedAt":"2025-05-28T22:58:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series has been updated to be based on v3 of\nps/midx-negative-packfile-cache, which is 1f34bf3e08 (midx: stop\nrepeatedly looking up nonexistent packfiles, 2025-05-28) at the time of\nwriting.\n\nThere are a couple of minor changes from last time, including:\n\n - using the new MIDX_PACK_ERROR macro instead of casting '-1' to a void\n pointer\n\n - converting some direct m->pack_names lookups (outside of midx.c and\n midx-write.c with one exception) to use a similar pattern to access\n the nth pack name\n\nAs usual, a range-diff against v1 is below for convenience.\n\nThanks again for your review!\n\nTaylor Blau (4):\n midx: access pack names through `nth_midxed_pack_name()`\n midx-write.c: guard against incremental MIDXs in want_included_pack()\n midx-write.c: extract inner loop from fill_packs_from_midx()\n midx: return a `packed_git` pointer from `prepare_midx_pack()`\n\n midx-write.c | 101 +++++++++++++++++++++-----------------\n midx.c | 88 +++++++++++++++++----------------\n midx.h | 6 ++-\n pack-bitmap.c | 6 +--\n t/helper/test-read-midx.c | 7 +--\n 5 files changed, 116 insertions(+), 92 deletions(-)\n\nRange-diff against v1:\n1: a7082dc7f0 < -: ---------- packfile: explain ordering of how we look up auxiliary pack files\n2: 73fe0882ee < -: ---------- midx: stop repeatedly looking up nonexistent packfiles\n3: ad7295b11b < -: ---------- pack-bitmap.c: fix broken warning() when missing MIDX'd pack\n5: 5d97b706e1 ! 1: d3508d3cfb midx-write.c: simplify fill_packs_from_midx()\n @@ Metadata\n Author: Taylor Blau \n \n ## Commit message ##\n - midx-write.c: simplify fill_packs_from_midx()\n + midx: access pack names through `nth_midxed_pack_name()`\n \n - When enumerating packs within fill_packs_from_midx(), we open the pack's\n - index iff we are either writing a reverse index, have a preferred pack\n - (by name) or both.\n + Accessing a MIDX's 'pack_names' array is somewhat error-prone when\n + dealing with incremental MIDX chains, where the (global) pack_int_id for\n + some pack may differ from the containing layer's index for that pack.\n \n - Let's simplify this into a single case by setting the\n - MIDX_WRITE_REV_INDEX flag bit when we have a preferred_pack_name. This\n - is a little bit of a shortcut to reduce the line length in the loop\n - below. But it sets us up nicely to extract the inner loop of this\n - function out into its own function, where we will no longer have to pass\n - the preferred_pack_name.\n + Introduce `nth_midxed_pack_name()` in an effort to reduce a common\n + source of errors by discouraging external callers from accessing a\n + layer's `pack_names` array directly.\n \n Signed-off-by: Taylor Blau \n \n - ## midx-write.c ##\n -@@ midx-write.c: static int fill_packs_from_midx(struct write_midx_context *ctx,\n - {\n - \tstruct multi_pack_index *m;\n + ## midx.c ##\n +@@ midx.c: struct packed_git *nth_midxed_pack(struct multi_pack_index *m,\n + \treturn m->packs[local_pack_int_id];\n + }\n \n -+\tif (preferred_pack_name) {\n -+\t\t/*\n -+\t\t * If a preferred pack is specified, need to have\n -+\t\t * packed_git's loaded to ensure the chosen preferred\n -+\t\t * pack has a non-zero object count.\n -+\t\t *\n -+\t\t * Trick ourselves into thinking that we're writing a\n -+\t\t * reverse index in this case in order to open up the\n -+\t\t * pack index file.\n -+\t\t */\n -+\t\tflags |= MIDX_WRITE_REV_INDEX;\n -+\t}\n ++const char *nth_midxed_pack_name(struct multi_pack_index *m,\n ++\t\t\t\t uint32_t pack_int_id)\n ++{\n ++\tuint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id);\n ++\treturn m->pack_names[local_pack_int_id];\n ++}\n +\n - \tfor (m = ctx->m; m; m = m->base_midx) {\n - \t\tuint32_t i;\n + #define MIDX_CHUNK_BITMAPPED_PACKS_WIDTH (2 * sizeof(uint32_t))\n \n -@@ midx-write.c: static int fill_packs_from_midx(struct write_midx_context *ctx,\n - \t\t\t * If generating a reverse index, need to have\n - \t\t\t * packed_git's loaded to compare their\n - \t\t\t * mtimes and object count.\n --\t\t\t *\n --\t\t\t * If a preferred pack is specified, need to\n --\t\t\t * have packed_git's loaded to ensure the chosen\n --\t\t\t * preferred pack has a non-zero object count.\n - \t\t\t */\n --\t\t\tif (flags & MIDX_WRITE_REV_INDEX ||\n --\t\t\t preferred_pack_name) {\n -+\t\t\tif (flags & MIDX_WRITE_REV_INDEX) {\n - \t\t\t\tif (prepare_midx_pack(ctx->repo, m,\n - \t\t\t\t\t\t m->num_packs_in_base + i)) {\n - \t\t\t\t\terror(_(\"could not load pack\"));\n + int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,\n +\n + ## midx.h ##\n +@@ midx.h: struct multi_pack_index *load_multi_pack_index(struct repository *r,\n + int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id);\n + struct packed_git *nth_midxed_pack(struct multi_pack_index *m,\n + \t\t\t\t uint32_t pack_int_id);\n ++const char *nth_midxed_pack_name(struct multi_pack_index *m,\n ++\t\t\t\t uint32_t pack_int_id);\n + int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,\n + \t\t struct bitmapped_pack *bp, uint32_t pack_int_id);\n + int bsearch_one_midx(const struct object_id *oid, struct multi_pack_index *m,\n +\n + ## pack-bitmap.c ##\n +@@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n + \tfor (i = 0; i < bitmap_git->midx->num_packs + bitmap_git->midx->num_packs_in_base; i++) {\n + \t\tif (prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {\n + \t\t\twarning(_(\"could not open pack %s\"),\n +-\t\t\t\tbitmap_git->midx->pack_names[i]);\n ++\t\t\t\tnth_midxed_pack_name(bitmap_git->midx, i));\n + \t\t\tgoto cleanup;\n + \t\t}\n + \t}\n +@@ pack-bitmap.c: void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n + \t\t\tstruct bitmapped_pack pack;\n + \t\t\tif (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {\n + \t\t\t\twarning(_(\"unable to load pack: '%s', disabling pack-reuse\"),\n +-\t\t\t\t\tbitmap_git->midx->pack_names[i]);\n ++\t\t\t\t\tnth_midxed_pack_name(bitmap_git->midx, i));\n + \t\t\t\tfree(packs);\n + \t\t\t\treturn;\n + \t\t\t}\n +\n + ## t/helper/test-read-midx.c ##\n +@@ t/helper/test-read-midx.c: static int read_midx_file(const char *object_dir, const char *checksum,\n + \tprintf(\"\\nnum_objects: %d\\n\", m->num_objects);\n + \n + \tprintf(\"packs:\\n\");\n +-\tfor (i = 0; i < m->num_packs; i++)\n +-\t\tprintf(\"%s\\n\", m->pack_names[i]);\n ++\tfor (i = m->num_packs_in_base; i < m->num_packs + m->num_packs_in_base;\n ++\t i++)\n ++\t\tprintf(\"%s\\n\", nth_midxed_pack_name(m, i));\n + \n + \tprintf(\"object-dir: %s\\n\", m->object_dir);\n + \n +@@ t/helper/test-read-midx.c: static int read_midx_preferred_pack(const char *object_dir)\n + \t\treturn 1;\n + \t}\n + \n +-\tprintf(\"%s\\n\", midx->pack_names[preferred_pack]);\n ++\tprintf(\"%s\\n\", nth_midxed_pack_name(midx, preferred_pack));\n + \tclose_midx(midx);\n + \treturn 0;\n + }\n4: d2f9645aa1 ! 2: cf3ab81a08 midx-write.c: guard against incremental MIDXs in want_included_pack()\n @@ Commit message\n if (m->base_midx)\n die(_(\"cannot repack an incremental multi-pack-index\"));\n \n - So want_included_pack() is OK becuase it will never encounter a\n + So want_included_pack() is OK because it will never encounter a\n situation where it has to chase backwards through the '->base_midx'\n pointer. But that is not immediately clear from reading the code, and is\n too fragile for my comfort. Make this more clear by adding an ASSERT()\n6: a4080c12df ! 3: e54988bfea midx-write.c: extract inner loop from fill_packs_from_midx()\n @@ midx-write.c: static struct multi_pack_index *lookup_multi_pack_index(struct rep\n \n +static int fill_packs_from_midx_1(struct write_midx_context *ctx,\n +\t\t\t\t struct multi_pack_index *m,\n -+\t\t\t\t uint32_t flags)\n ++\t\t\t\t int prepare_packs)\n +{\n +\tfor (uint32_t i = 0; i < m->num_packs; i++) {\n -+\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n -+\n +\t\t/*\n +\t\t * If generating a reverse index, need to have\n +\t\t * packed_git's loaded to compare their\n +\t\t * mtimes and object count.\n +\t\t */\n -+\t\tif (flags & MIDX_WRITE_REV_INDEX) {\n ++\t\tif (prepare_packs) {\n +\t\t\tif (prepare_midx_pack(ctx->repo, m,\n +\t\t\t\t\t m->num_packs_in_base + i)) {\n +\t\t\t\terror(_(\"could not load pack\"));\n @@ midx-write.c: static struct multi_pack_index *lookup_multi_pack_index(struct rep\n static int fill_packs_from_midx(struct write_midx_context *ctx,\n \t\t\t\tconst char *preferred_pack_name, uint32_t flags)\n {\n -@@ midx-write.c: static int fill_packs_from_midx(struct write_midx_context *ctx,\n - \t}\n + \tstruct multi_pack_index *m;\n ++\tint prepare_packs;\n ++\n ++\t/*\n ++\t * If generating a reverse index, need to have packed_git's\n ++\t * loaded to compare their mtimes and object count.\n ++\t */\n ++\tprepare_packs = !!(flags & MIDX_WRITE_REV_INDEX || preferred_pack_name);\n \n \tfor (m = ctx->m; m; m = m->base_midx) {\n -\t\tuint32_t i;\n @@ midx-write.c: static int fill_packs_from_midx(struct write_midx_context *ctx,\n -\t\t\t * If generating a reverse index, need to have\n -\t\t\t * packed_git's loaded to compare their\n -\t\t\t * mtimes and object count.\n +-\t\t\t *\n +-\t\t\t * If a preferred pack is specified, need to\n +-\t\t\t * have packed_git's loaded to ensure the chosen\n +-\t\t\t * preferred pack has a non-zero object count.\n -\t\t\t */\n --\t\t\tif (flags & MIDX_WRITE_REV_INDEX) {\n +-\t\t\tif (flags & MIDX_WRITE_REV_INDEX ||\n +-\t\t\t preferred_pack_name) {\n -\t\t\t\tif (prepare_midx_pack(ctx->repo, m,\n -\t\t\t\t\t\t m->num_packs_in_base + i)) {\n -\t\t\t\t\terror(_(\"could not load pack\"));\n @@ midx-write.c: static int fill_packs_from_midx(struct write_midx_context *ctx,\n -\t\t\t\t m->pack_names[i],\n -\t\t\t\t m->num_packs_in_base + i);\n -\t\t}\n -+\t\tint ret = fill_packs_from_midx_1(ctx, m, flags);\n ++\t\tint ret = fill_packs_from_midx_1(ctx, m, prepare_packs);\n +\t\tif (ret)\n +\t\t\treturn ret;\n \t}\n7: 80699bb3ee ! 4: e3e21db673 midx: return a `packed_git` pointer from `prepare_midx_pack()`\n @@ Commit message\n \n ## midx-write.c ##\n @@ midx-write.c: static int fill_packs_from_midx_1(struct write_midx_context *ctx,\n - \t\t\t\t uint32_t flags)\n + \t\t\t\t int prepare_packs)\n {\n \tfor (uint32_t i = 0; i < m->num_packs; i++) {\n +-\t\t/*\n +-\t\t * If generating a reverse index, need to have\n +-\t\t * packed_git's loaded to compare their\n +-\t\t * mtimes and object count.\n +-\t\t */\n +\t\tstruct packed_git *p = NULL;\n +\n - \t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n - \n - \t\t/*\n -@@ midx-write.c: static int fill_packs_from_midx_1(struct write_midx_context *ctx,\n - \t\t * mtimes and object count.\n - \t\t */\n - \t\tif (flags & MIDX_WRITE_REV_INDEX) {\n ++\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n + \t\tif (prepare_packs) {\n -\t\t\tif (prepare_midx_pack(ctx->repo, m,\n -\t\t\t\t\t m->num_packs_in_base + i)) {\n +\t\t\tp = prepare_midx_pack(ctx->repo, m,\n @@ midx-write.c: static int fill_packs_from_midx_1(struct write_midx_context *ctx,\n \t\t}\n \n -\t\tfill_pack_info(&ctx->info[ctx->nr++], m->packs[i],\n -+\t\tfill_pack_info(&ctx->info[ctx->nr++], p,\n - \t\t\t m->pack_names[i],\n +-\t\t\t m->pack_names[i],\n ++\t\tfill_pack_info(&ctx->info[ctx->nr++], p, m->pack_names[i],\n \t\t\t m->num_packs_in_base + i);\n \t}\n + \n @@ midx-write.c: int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla\n \t\t\t\t\t _(\"Finding and deleting unreferenced packfiles\"),\n \t\t\t\t\t m->num_packs);\n @@ midx.c: static uint32_t midx_for_pack(struct multi_pack_index **_m,\n +\t\tstruct strbuf key = STRBUF_INIT;\n +\t\tstruct packed_git *p;\n \n --\tif (m->packs[pack_int_id] == (void *)(intptr_t)-1)\n +-\tif (m->packs[pack_int_id] == MIDX_PACK_ERROR)\n -\t\treturn 1;\n -\tif (m->packs[pack_int_id])\n -\t\treturn 0;\n @@ midx.c: static uint32_t midx_for_pack(struct multi_pack_index **_m,\n +\t\tstrbuf_release(&key);\n \n -\tif (!p) {\n --\t\tm->packs[pack_int_id] = (void *)(intptr_t)-1;\n +-\t\tm->packs[pack_int_id] = MIDX_PACK_ERROR;\n -\t\treturn 1;\n -+\t\tm->packs[pack_pos] = p ? p : (void *)(intptr_t)-1;\n ++\t\tm->packs[pack_pos] = p ? p : MIDX_PACK_ERROR;\n +\t\tif (p)\n +\t\t\tp->multi_pack_index = 1;\n \t}\n @@ midx.c: static uint32_t midx_for_pack(struct multi_pack_index **_m,\n -\tm->packs[pack_int_id] = p;\n -\n -\treturn 0;\n -+\tif (m->packs[pack_pos] == (void *)(intptr_t)-1)\n ++\tif (m->packs[pack_pos] == MIDX_PACK_ERROR)\n +\t\treturn NULL;\n +\treturn m->packs[pack_pos];\n }\n @@ midx.h: void get_split_midx_filename_ext(const struct git_hash_algo *hash_algo,\n +\t\t\t\t uint32_t pack_int_id);\n struct packed_git *nth_midxed_pack(struct multi_pack_index *m,\n \t\t\t\t uint32_t pack_int_id);\n - int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,\n + const char *nth_midxed_pack_name(struct multi_pack_index *m,\n \n ## pack-bitmap.c ##\n @@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n @@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n -\t\tif (prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {\n +\t\tif (!prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {\n \t\t\twarning(_(\"could not open pack %s\"),\n - \t\t\t\tbitmap_git->midx->pack_names[i - bitmap_git->midx->num_packs_in_base]);\n + \t\t\t\tnth_midxed_pack_name(bitmap_git->midx, i));\n \t\t\tgoto cleanup;\n\nbase-commit: 1f34bf3e082741e053d25b76a0ffe31d9d967594\n-- \n2.49.0.640.ga4de40e6a8\n"},{"id":"519108","messageId":"d3508d3cfbddb512dbca4c2177731fffb5827084.1748473122.git.me@ttaylorr.com","threadId":"63469","inReplyTo":"cover.1748473122.git.me@ttaylorr.com","subject":"[PATCH v2 1/4] midx: access pack names through `nth_midxed_pack_name()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-28T22:59:00Z","receivedAt":"2025-05-28T22:59:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Accessing a MIDX's 'pack_names' array is somewhat error-prone when\ndealing with incremental MIDX chains, where the (global) pack_int_id for\nsome pack may differ from the containing layer's index for that pack.\n\nIntroduce `nth_midxed_pack_name()` in an effort to reduce a common\nsource of errors by discouraging external callers from accessing a\nlayer's `pack_names` array directly.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 7 +++++++\n midx.h | 2 ++\n pack-bitmap.c | 4 ++--\n t/helper/test-read-midx.c | 7 ++++---\n 4 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex cd6e766ce2..6705e77881 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -506,6 +506,13 @@ struct packed_git *nth_midxed_pack(struct multi_pack_index *m,\n \treturn m->packs[local_pack_int_id];\n }\n \n+const char *nth_midxed_pack_name(struct multi_pack_index *m,\n+\t\t\t\t uint32_t pack_int_id)\n+{\n+\tuint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id);\n+\treturn m->pack_names[local_pack_int_id];\n+}\n+\n #define MIDX_CHUNK_BITMAPPED_PACKS_WIDTH (2 * sizeof(uint32_t))\n \n int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,\ndiff --git a/midx.h b/midx.h\nindex 9d1374cbd5..0fb490f4d4 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -107,6 +107,8 @@ struct multi_pack_index *load_multi_pack_index(struct repository *r,\n int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id);\n struct packed_git *nth_midxed_pack(struct multi_pack_index *m,\n \t\t\t\t uint32_t pack_int_id);\n+const char *nth_midxed_pack_name(struct multi_pack_index *m,\n+\t\t\t\t uint32_t pack_int_id);\n int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,\n \t\t struct bitmapped_pack *bp, uint32_t pack_int_id);\n int bsearch_one_midx(const struct object_id *oid, struct multi_pack_index *m,\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex b9f1d86604..8ddc150778 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -490,7 +490,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n \tfor (i = 0; i < bitmap_git->midx->num_packs + bitmap_git->midx->num_packs_in_base; i++) {\n \t\tif (prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {\n \t\t\twarning(_(\"could not open pack %s\"),\n-\t\t\t\tbitmap_git->midx->pack_names[i]);\n+\t\t\t\tnth_midxed_pack_name(bitmap_git->midx, i));\n \t\t\tgoto cleanup;\n \t\t}\n \t}\n@@ -2469,7 +2469,7 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \t\t\tstruct bitmapped_pack pack;\n \t\t\tif (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {\n \t\t\t\twarning(_(\"unable to load pack: '%s', disabling pack-reuse\"),\n-\t\t\t\t\tbitmap_git->midx->pack_names[i]);\n+\t\t\t\t\tnth_midxed_pack_name(bitmap_git->midx, i));\n \t\t\t\tfree(packs);\n \t\t\t\treturn;\n \t\t\t}\ndiff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c\nindex ac81390899..fbed0f6919 100644\n--- a/t/helper/test-read-midx.c\n+++ b/t/helper/test-read-midx.c\n@@ -53,8 +53,9 @@ static int read_midx_file(const char *object_dir, const char *checksum,\n \tprintf(\"\\nnum_objects: %d\\n\", m->num_objects);\n \n \tprintf(\"packs:\\n\");\n-\tfor (i = 0; i < m->num_packs; i++)\n-\t\tprintf(\"%s\\n\", m->pack_names[i]);\n+\tfor (i = m->num_packs_in_base; i < m->num_packs + m->num_packs_in_base;\n+\t i++)\n+\t\tprintf(\"%s\\n\", nth_midxed_pack_name(m, i));\n \n \tprintf(\"object-dir: %s\\n\", m->object_dir);\n \n@@ -108,7 +109,7 @@ static int read_midx_preferred_pack(const char *object_dir)\n \t\treturn 1;\n \t}\n \n-\tprintf(\"%s\\n\", midx->pack_names[preferred_pack]);\n+\tprintf(\"%s\\n\", nth_midxed_pack_name(midx, preferred_pack));\n \tclose_midx(midx);\n \treturn 0;\n }\n-- \n2.49.0.640.ga4de40e6a8\n\n"},{"id":"519109","messageId":"cf3ab81a0808ace5a7d81aab6227730fa899350f.1748473122.git.me@ttaylorr.com","threadId":"63469","inReplyTo":"cover.1748473122.git.me@ttaylorr.com","subject":"[PATCH v2 2/4] midx-write.c: guard against incremental MIDXs in want_included_pack()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-28T22:59:03Z","receivedAt":"2025-05-28T22:59:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function want_included_pack() is used to determine whether or not a\nthe packfile corresponding to some given pack_int_id should be included\nin a 'git multi-pack-index repack' operation.\n\nThis spot looks like it would be broken, particularly in:\n\n struct packed_git *p;\n if (prepare_midx_pack(r, m, pack_int_id))\n return 0;\n p = m->packs[pack_int_id];\n\n, when pack_int_id is greater than m->num_pack_in_base (supposing that\nm->num_packs_in_base is non-zero, or equivalently that m->base_midx is\nnon-NULL).\n\nSuppose we have two MIDXs in an incremental MIDX chain, each having two\npacks:\n\n - M0 = {packs: [P0, P1], base_midx: NULL}\n - M1 = {packs: [P2, P3], base_midx: M0}\n\nnoting that each pack is identified by its global pack_int_id within the\nchain.\n\nSo if you do something like:\n\n want_included_pack(the_repository, M1, pack_kept_objects, 2);\n\nthat function will call prepare_midx_pack(), which is smart enough to\nrealize that the pack of interest is in the current layer (M1), and\nknows how to adjust its global pack_int_id into an index into the\ncurrent layer's 'packs' array.\n\nBut the following line:\n\n p = m->packs[pack_int_id]; /* m is M1, pack_int_id is 2 */\n\nlooks broken, since each layer of the MIDX only maintains an array of\nthe packs stored within that layer, and 'm' wasn't adjusted back to\npoint at M1->base_midx (M0).\n\nThe right thing to do would be:\n\n struct packed_git *p;\n if (prepare_midx_pack(r, m, pack_int_id))\n return 0;\n\n /* open-code midx.c::midx_for_pack(), which is private */\n while (m && pack_int_id < m->num_packs_in_base)\n m = m->base_midx;\n\n if (!m)\n BUG(\"broken midx?\");\n if (pack_int_id >= m->num_packs + m->num_packs_in_base)\n BUG(\"broken pack_int_id?\");\n\n p = m->packs[pack_int_id - m->num_packs_in_base];\n\nBut that would be overkill, since this function never deals with\nincremental MIDXs having more than one layer! To see why, observe that\nwant_included_pack() has two callers:\n\n - midx-write.c::fill_included_packs_all()\n - midx-write.c::fill_included_packs_batch()\n\nand those functions' sole caller is in midx-write.c::midx_repack(),\nwhich dispatches a call to one or the other depending on whether or not\nthe batch_size is non-zero.\n\nAnd at the very top of midx_repack(), we have a guard against\nnon-trivial incremental MIDX chains:\n\n if (m->base_midx)\n die(_(\"cannot repack an incremental multi-pack-index\"));\n\nSo want_included_pack() is OK because it will never encounter a\nsituation where it has to chase backwards through the '->base_midx'\npointer. But that is not immediately clear from reading the code, and is\ntoo fragile for my comfort. Make this more clear by adding an ASSERT()\nto the above effect.\n\nApply the same treatment to each of the fill_included_packs-related\nfunctions as well, since those are deceptively OK by the same reasoning.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex dd3b3070e5..e4a3830d45 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -1636,6 +1636,9 @@ static int want_included_pack(struct repository *r,\n \t\t\t uint32_t pack_int_id)\n {\n \tstruct packed_git *p;\n+\n+\tASSERT(m && !m->base_midx);\n+\n \tif (prepare_midx_pack(r, m, pack_int_id))\n \t\treturn 0;\n \tp = m->packs[pack_int_id];\n@@ -1655,6 +1658,8 @@ static void fill_included_packs_all(struct repository *r,\n \tuint32_t i;\n \tint pack_kept_objects = 0;\n \n+\tASSERT(m && !m->base_midx);\n+\n \trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n \n \tfor (i = 0; i < m->num_packs; i++) {\n@@ -1675,6 +1680,8 @@ static void fill_included_packs_batch(struct repository *r,\n \tstruct repack_info *pack_info;\n \tint pack_kept_objects = 0;\n \n+\tASSERT(m && !m->base_midx);\n+\n \tCALLOC_ARRAY(pack_info, m->num_packs);\n \n \trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n-- \n2.49.0.640.ga4de40e6a8\n\n"},{"id":"519110","messageId":"e54988bfeaef6d6e886899b30d07b58c84ddea94.1748473122.git.me@ttaylorr.com","threadId":"63469","inReplyTo":"cover.1748473122.git.me@ttaylorr.com","subject":"[PATCH v2 3/4] midx-write.c: extract inner loop from fill_packs_from_midx()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-28T22:59:06Z","receivedAt":"2025-05-28T22:59:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function fill_packs_from_midx() does relatively little, but ends up\nin a doubly-nested loop because we're enumerating each pack within each\nlayer of the incremental MIDX chain.\n\nLet's de-dent the inner loop of fill_packs_from_midx() by extracting its\ncontents into a separate function, and calling that.\n\nSigned-off-by: Taylor Blau \n---\n midx-write.c | 72 ++++++++++++++++++++++++++++++----------------------\n 1 file changed, 41 insertions(+), 31 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex e4a3830d45..ca2384e291 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -938,44 +938,54 @@ static struct multi_pack_index *lookup_multi_pack_index(struct repository *r,\n \treturn result;\n }\n \n+static int fill_packs_from_midx_1(struct write_midx_context *ctx,\n+\t\t\t\t struct multi_pack_index *m,\n+\t\t\t\t int prepare_packs)\n+{\n+\tfor (uint32_t i = 0; i < m->num_packs; i++) {\n+\t\t/*\n+\t\t * If generating a reverse index, need to have\n+\t\t * packed_git's loaded to compare their\n+\t\t * mtimes and object count.\n+\t\t */\n+\t\tif (prepare_packs) {\n+\t\t\tif (prepare_midx_pack(ctx->repo, m,\n+\t\t\t\t\t m->num_packs_in_base + i)) {\n+\t\t\t\terror(_(\"could not load pack\"));\n+\t\t\t\treturn 1;\n+\t\t\t}\n+\n+\t\t\tif (open_pack_index(m->packs[i]))\n+\t\t\t\tdie(_(\"could not open index for %s\"),\n+\t\t\t\t m->packs[i]->pack_name);\n+\t\t}\n+\n+\t\tfill_pack_info(&ctx->info[ctx->nr++], m->packs[i],\n+\t\t\t m->pack_names[i],\n+\t\t\t m->num_packs_in_base + i);\n+\t}\n+\n+\treturn 0;\n+}\n+\n static int fill_packs_from_midx(struct write_midx_context *ctx,\n \t\t\t\tconst char *preferred_pack_name, uint32_t flags)\n {\n \tstruct multi_pack_index *m;\n+\tint prepare_packs;\n+\n+\t/*\n+\t * If generating a reverse index, need to have packed_git's\n+\t * loaded to compare their mtimes and object count.\n+\t */\n+\tprepare_packs = !!(flags & MIDX_WRITE_REV_INDEX || preferred_pack_name);\n \n \tfor (m = ctx->m; m; m = m->base_midx) {\n-\t\tuint32_t i;\n-\n-\t\tfor (i = 0; i < m->num_packs; i++) {\n-\t\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n-\n-\t\t\t/*\n-\t\t\t * If generating a reverse index, need to have\n-\t\t\t * packed_git's loaded to compare their\n-\t\t\t * mtimes and object count.\n-\t\t\t *\n-\t\t\t * If a preferred pack is specified, need to\n-\t\t\t * have packed_git's loaded to ensure the chosen\n-\t\t\t * preferred pack has a non-zero object count.\n-\t\t\t */\n-\t\t\tif (flags & MIDX_WRITE_REV_INDEX ||\n-\t\t\t preferred_pack_name) {\n-\t\t\t\tif (prepare_midx_pack(ctx->repo, m,\n-\t\t\t\t\t\t m->num_packs_in_base + i)) {\n-\t\t\t\t\terror(_(\"could not load pack\"));\n-\t\t\t\t\treturn 1;\n-\t\t\t\t}\n-\n-\t\t\t\tif (open_pack_index(m->packs[i]))\n-\t\t\t\t\tdie(_(\"could not open index for %s\"),\n-\t\t\t\t\t m->packs[i]->pack_name);\n-\t\t\t}\n-\n-\t\t\tfill_pack_info(&ctx->info[ctx->nr++], m->packs[i],\n-\t\t\t\t m->pack_names[i],\n-\t\t\t\t m->num_packs_in_base + i);\n-\t\t}\n+\t\tint ret = fill_packs_from_midx_1(ctx, m, prepare_packs);\n+\t\tif (ret)\n+\t\t\treturn ret;\n \t}\n+\n \treturn 0;\n }\n \n-- \n2.49.0.640.ga4de40e6a8\n\n"},{"id":"519111","messageId":"e3e21db673f3ae6e574333d4678a0450fa44fb9c.1748473122.git.me@ttaylorr.com","threadId":"63469","inReplyTo":"cover.1748473122.git.me@ttaylorr.com","subject":"[PATCH v2 4/4] midx: return a `packed_git` pointer from `prepare_midx_pack()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-28T22:59:09Z","receivedAt":"2025-05-28T22:59:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The prepare_midx_pack() function is designed to convert a MIDX-specific\npack_int_id for a given pack into a pointer into an actual `packed_git`\nstructure.\n\nIn general, these calls look something like:\n\n struct packed_git *p;\n if (prepare_midx_pack(the_repository, some_midx, some_pack_int_id))\n die(\"could not load pack xyz\");\n p = some_midx->packs[some_pack_int_id];\n\n, and in a pre-incremental MIDX world, this pattern works well. However,\nin a post-incremental MIDX world, this pattern is a little more prone to\nerrors.\n\nThese errors can happen when the given 'pack_int_id' is not usable as an\nindex into the 'm->packs' array. And this happens in all layers but the\nbottom-most one in an incremental MIDX chain. Each layer stores only the\npacks that are local to that layer of the chain, and offsets them by the\ntotal number of packs in the base MIDX(s).\n\nBut there is other awkwardness here. Thinking back to the above snippet,\nsuppose that the pack with ID 'some_pack_int_id' is in a layer in the\nmiddle of the MIDX chain. Then it is still invalid to do:\n\n p = some_midx->packs[some_pack_int_id - some_midx->num_packs_in_base];\n\n, becuase the top-most layer (here 'some_midx') may not even have that\npack! So we would have to chase down the '->base_midx' pointer in order\nto get the correct result. midx.c has a helper to do this (called\n'midx_for_pack()'), but it is meant only for internal use.\n\nThat means that a full, working version of the above adjusted to handle\nincremental MIDXs would look something like:\n\n struct packed_git *p;\n if (prepare_midx_pack(the_repository, some_midx, some_pack_int_id))\n die(\"could not load pack xyz\");\n\n while (m && pack_int_id < m->num_packs_in_base)\n m = m->base_midx;\n\n if (!m)\n BUG(\"broken midx?\");\n if (pack_int_id >= m->num_packs + m->num_packs_in_base)\n BUG(\"broken pack_int_id?\");\n\n p = m->packs[pack_int_id - m->num_packs_in_base];\n\n, which is far too verbose to access a single pack by its pack_int_id in\na MIDX chain.\n\nLet's instead have prepare_midx_pack() return a pointer to the\npacked_git structure itself, hiding the above as an implementation\ndetail of prepare_midx_pack(). This patch turns the above snippet into:\n\n struct packed_git *p = prepare_midx_pack(the_repository, some_midx,\n some_pack_int_id);\n if (!p)\n die(\"could not load pack xyz\");\n\nmaking it far easier and less error-prone to access packs by their\npack_int_id in a MIDX chain.\n\n(In the future, we may want to consider similar treatment for, e.g., the\npack_names array. Likewise, it might make sense to rename the \"packs\"\nmember of the MIDX structure to suggest that it shouldn't be accessed\ndirectly outside of midx.c.)\n\nSuggested-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n midx-write.c | 44 +++++++++++++---------------\n midx.c | 81 +++++++++++++++++++++++++--------------------------\n midx.h | 4 ++-\n pack-bitmap.c | 2 +-\n 4 files changed, 64 insertions(+), 67 deletions(-)\n\ndiff --git a/midx-write.c b/midx-write.c\nindex ca2384e291..fc74be813d 100644\n--- a/midx-write.c\n+++ b/midx-write.c\n@@ -943,25 +943,23 @@ static int fill_packs_from_midx_1(struct write_midx_context *ctx,\n \t\t\t\t int prepare_packs)\n {\n \tfor (uint32_t i = 0; i < m->num_packs; i++) {\n-\t\t/*\n-\t\t * If generating a reverse index, need to have\n-\t\t * packed_git's loaded to compare their\n-\t\t * mtimes and object count.\n-\t\t */\n+\t\tstruct packed_git *p = NULL;\n+\n+\t\tALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);\n \t\tif (prepare_packs) {\n-\t\t\tif (prepare_midx_pack(ctx->repo, m,\n-\t\t\t\t\t m->num_packs_in_base + i)) {\n+\t\t\tp = prepare_midx_pack(ctx->repo, m,\n+\t\t\t\t\t m->num_packs_in_base + i);\n+\t\t\tif (!p) {\n \t\t\t\terror(_(\"could not load pack\"));\n \t\t\t\treturn 1;\n \t\t\t}\n \n-\t\t\tif (open_pack_index(m->packs[i]))\n+\t\t\tif (open_pack_index(p))\n \t\t\t\tdie(_(\"could not open index for %s\"),\n-\t\t\t\t m->packs[i]->pack_name);\n+\t\t\t\t p->pack_name);\n \t\t}\n \n-\t\tfill_pack_info(&ctx->info[ctx->nr++], m->packs[i],\n-\t\t\t m->pack_names[i],\n+\t\tfill_pack_info(&ctx->info[ctx->nr++], p, m->pack_names[i],\n \t\t\t m->num_packs_in_base + i);\n \t}\n \n@@ -1588,20 +1586,19 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla\n \t\t\t\t\t _(\"Finding and deleting unreferenced packfiles\"),\n \t\t\t\t\t m->num_packs);\n \tfor (i = 0; i < m->num_packs; i++) {\n+\t\tstruct packed_git *p;\n \t\tchar *pack_name;\n \t\tdisplay_progress(progress, i + 1);\n \n \t\tif (count[i])\n \t\t\tcontinue;\n \n-\t\tif (prepare_midx_pack(r, m, i))\n+\t\tp = prepare_midx_pack(r, m, i);\n+\t\tif (!p || p->pack_keep || p->is_cruft)\n \t\t\tcontinue;\n \n-\t\tif (m->packs[i]->pack_keep || m->packs[i]->is_cruft)\n-\t\t\tcontinue;\n-\n-\t\tpack_name = xstrdup(m->packs[i]->pack_name);\n-\t\tclose_pack(m->packs[i]);\n+\t\tpack_name = xstrdup(p->pack_name);\n+\t\tclose_pack(p);\n \n \t\tstring_list_insert(&packs_to_drop, m->pack_names[i]);\n \t\tunlink_pack_path(pack_name, 0);\n@@ -1649,9 +1646,9 @@ static int want_included_pack(struct repository *r,\n \n \tASSERT(m && !m->base_midx);\n \n-\tif (prepare_midx_pack(r, m, pack_int_id))\n+\tp = prepare_midx_pack(r, m, pack_int_id);\n+\tif (!p)\n \t\treturn 0;\n-\tp = m->packs[pack_int_id];\n \tif (!pack_kept_objects && p->pack_keep)\n \t\treturn 0;\n \tif (p->is_cruft)\n@@ -1697,12 +1694,11 @@ static void fill_included_packs_batch(struct repository *r,\n \trepo_config_get_bool(r, \"repack.packkeptobjects\", &pack_kept_objects);\n \n \tfor (i = 0; i < m->num_packs; i++) {\n+\t\tstruct packed_git *p = prepare_midx_pack(r, m, i);\n+\n \t\tpack_info[i].pack_int_id = i;\n-\n-\t\tif (prepare_midx_pack(r, m, i))\n-\t\t\tcontinue;\n-\n-\t\tpack_info[i].mtime = m->packs[i]->mtime;\n+\t\tif (p)\n+\t\t\tpack_info[i].mtime = p->mtime;\n \t}\n \n \tfor (i = 0; i < m->num_objects; i++) {\ndiff --git a/midx.c b/midx.c\nindex 6705e77881..a2e7a3ec0e 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -451,50 +451,48 @@ static uint32_t midx_for_pack(struct multi_pack_index **_m,\n \treturn pack_int_id - m->num_packs_in_base;\n }\n \n-int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n-\t\t uint32_t pack_int_id)\n+struct packed_git *prepare_midx_pack(struct repository *r,\n+\t\t\t\t struct multi_pack_index *m,\n+\t\t\t\t uint32_t pack_int_id)\n {\n-\tstruct strbuf pack_name = STRBUF_INIT;\n-\tstruct strbuf key = STRBUF_INIT;\n-\tstruct packed_git *p;\n+\tuint32_t pack_pos = midx_for_pack(&m, pack_int_id);\n \n-\tpack_int_id = midx_for_pack(&m, pack_int_id);\n+\tif (!m->packs[pack_pos]) {\n+\t\tstruct strbuf pack_name = STRBUF_INIT;\n+\t\tstruct strbuf key = STRBUF_INIT;\n+\t\tstruct packed_git *p;\n \n-\tif (m->packs[pack_int_id] == MIDX_PACK_ERROR)\n-\t\treturn 1;\n-\tif (m->packs[pack_int_id])\n-\t\treturn 0;\n+\t\tstrbuf_addf(&pack_name, \"%s/pack/%s\", m->object_dir,\n+\t\t\t m->pack_names[pack_pos]);\n \n-\tstrbuf_addf(&pack_name, \"%s/pack/%s\", m->object_dir,\n-\t\t m->pack_names[pack_int_id]);\n-\n-\t/* pack_map holds the \".pack\" name, but we have the .idx */\n-\tstrbuf_addbuf(&key, &pack_name);\n-\tstrbuf_strip_suffix(&key, \".idx\");\n-\tstrbuf_addstr(&key, \".pack\");\n-\tp = hashmap_get_entry_from_hash(&r->objects->pack_map,\n-\t\t\t\t\tstrhash(key.buf), key.buf,\n-\t\t\t\t\tstruct packed_git, packmap_ent);\n-\tif (!p) {\n-\t\tp = add_packed_git(r, pack_name.buf, pack_name.len, m->local);\n-\t\tif (p) {\n-\t\t\tinstall_packed_git(r, p);\n-\t\t\tlist_add_tail(&p->mru, &r->objects->packed_git_mru);\n+\t\t/* pack_map holds the \".pack\" name, but we have the .idx */\n+\t\tstrbuf_addbuf(&key, &pack_name);\n+\t\tstrbuf_strip_suffix(&key, \".idx\");\n+\t\tstrbuf_addstr(&key, \".pack\");\n+\t\tp = hashmap_get_entry_from_hash(&r->objects->pack_map,\n+\t\t\t\t\t\tstrhash(key.buf), key.buf,\n+\t\t\t\t\t\tstruct packed_git, packmap_ent);\n+\t\tif (!p) {\n+\t\t\tp = add_packed_git(r, pack_name.buf, pack_name.len,\n+\t\t\t\t\t m->local);\n+\t\t\tif (p) {\n+\t\t\t\tinstall_packed_git(r, p);\n+\t\t\t\tlist_add_tail(&p->mru,\n+\t\t\t\t\t &r->objects->packed_git_mru);\n+\t\t\t}\n \t\t}\n-\t}\n \n-\tstrbuf_release(&pack_name);\n-\tstrbuf_release(&key);\n+\t\tstrbuf_release(&pack_name);\n+\t\tstrbuf_release(&key);\n \n-\tif (!p) {\n-\t\tm->packs[pack_int_id] = MIDX_PACK_ERROR;\n-\t\treturn 1;\n+\t\tm->packs[pack_pos] = p ? p : MIDX_PACK_ERROR;\n+\t\tif (p)\n+\t\t\tp->multi_pack_index = 1;\n \t}\n \n-\tp->multi_pack_index = 1;\n-\tm->packs[pack_int_id] = p;\n-\n-\treturn 0;\n+\tif (m->packs[pack_pos] == MIDX_PACK_ERROR)\n+\t\treturn NULL;\n+\treturn m->packs[pack_pos];\n }\n \n struct packed_git *nth_midxed_pack(struct multi_pack_index *m,\n@@ -523,10 +521,11 @@ int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,\n \tif (!m->chunk_bitmapped_packs)\n \t\treturn error(_(\"MIDX does not contain the BTMP chunk\"));\n \n-\tif (prepare_midx_pack(r, m, pack_int_id))\n-\t\treturn error(_(\"could not load bitmapped pack %\"PRIu32), pack_int_id);\n+\tbp->p = prepare_midx_pack(r, m, pack_int_id);\n+\tif (!bp->p)\n+\t\treturn error(_(\"could not load bitmapped pack %\"PRIu32),\n+\t\t\t pack_int_id);\n \n-\tbp->p = m->packs[local_pack_int_id];\n \tbp->bitmap_pos = get_be32((char *)m->chunk_bitmapped_packs +\n \t\t\t\t MIDX_CHUNK_BITMAPPED_PACKS_WIDTH * local_pack_int_id);\n \tbp->bitmap_nr = get_be32((char *)m->chunk_bitmapped_packs +\n@@ -623,9 +622,9 @@ int fill_midx_entry(struct repository *r,\n \tmidx_for_object(&m, pos);\n \tpack_int_id = nth_midxed_pack_int_id(m, pos);\n \n-\tif (prepare_midx_pack(r, m, pack_int_id))\n+\tp = prepare_midx_pack(r, m, pack_int_id);\n+\tif (!p)\n \t\treturn 0;\n-\tp = m->packs[pack_int_id - m->num_packs_in_base];\n \n \t/*\n \t* We are about to tell the caller where they can locate the\n@@ -926,7 +925,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t\t\t\t\t\t _(\"Looking for referenced packfiles\"),\n \t\t\t\t\t\t m->num_packs + m->num_packs_in_base);\n \tfor (i = 0; i < m->num_packs + m->num_packs_in_base; i++) {\n-\t\tif (prepare_midx_pack(r, m, i))\n+\t\tif (!prepare_midx_pack(r, m, i))\n \t\t\tmidx_report(\"failed to load pack in position %d\", i);\n \n \t\tdisplay_progress(progress, i + 1);\ndiff --git a/midx.h b/midx.h\nindex 0fb490f4d4..4ac05b8234 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -104,7 +104,9 @@ void get_split_midx_filename_ext(const struct git_hash_algo *hash_algo,\n struct multi_pack_index *load_multi_pack_index(struct repository *r,\n \t\t\t\t\t const char *object_dir,\n \t\t\t\t\t int local);\n-int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id);\n+struct packed_git *prepare_midx_pack(struct repository *r,\n+\t\t\t\t struct multi_pack_index *m,\n+\t\t\t\t uint32_t pack_int_id);\n struct packed_git *nth_midxed_pack(struct multi_pack_index *m,\n \t\t\t\t uint32_t pack_int_id);\n const char *nth_midxed_pack_name(struct multi_pack_index *m,\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 8ddc150778..1f72e605d4 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -488,7 +488,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n \t}\n \n \tfor (i = 0; i < bitmap_git->midx->num_packs + bitmap_git->midx->num_packs_in_base; i++) {\n-\t\tif (prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {\n+\t\tif (!prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {\n \t\t\twarning(_(\"could not open pack %s\"),\n \t\t\t\tnth_midxed_pack_name(bitmap_git->midx, i));\n \t\t\tgoto cleanup;\n-- \n2.49.0.640.ga4de40e6a8\n"},{"id":"519177","messageId":"xmqqr007jg9b.fsf@gitster.g","threadId":"63469","inReplyTo":"d3508d3cfbddb512dbca4c2177731fffb5827084.1748473122.git.me@ttaylorr.com","subject":"Re: [PATCH v2 1/4] midx: access pack names through `nth_midxed_pack_name()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-29T20:47:44Z","receivedAt":"2025-05-29T20:47:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Accessing a MIDX's 'pack_names' array is somewhat error-prone when\n> dealing with incremental MIDX chains, where the (global) pack_int_id for\n> some pack may differ from the containing layer's index for that pack.\n>\n> Introduce `nth_midxed_pack_name()` in an effort to reduce a common\n> source of errors by discouraging external callers from accessing a\n> layer's `pack_names` array directly.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> midx.c | 7 +++++++\n> midx.h | 2 ++\n> pack-bitmap.c | 4 ++--\n> t/helper/test-read-midx.c | 7 ++++---\n> 4 files changed, 15 insertions(+), 5 deletions(-)\n\nHmph, I am not sure if an accessor really makes it harder to make\nmistakes, but I'd expect it to be mechanical rewrite from a[n] to\nfn(a, n)?\n\n> +const char *nth_midxed_pack_name(struct multi_pack_index *m,\n> +\t\t\t\t uint32_t pack_int_id)\n> +{\n> +\tuint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id);\n> +\treturn m->pack_names[local_pack_int_id];\n> +}\n\nOK, midx_for_pack() takes a pack_int_id, finds the midx that\ncontains the pack (by updating the 'm' via its pointer arg), and\nturns pack_int_id into local offset into m->pack_names[] array,\nand returns that string.\n\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index b9f1d86604..8ddc150778 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -490,7 +490,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n> \tfor (i = 0; i < bitmap_git->midx->num_packs + bitmap_git->midx->num_packs_in_base; i++) {\n> \t\tif (prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {\n> \t\t\twarning(_(\"could not open pack %s\"),\n> -\t\t\t\tbitmap_git->midx->pack_names[i]);\n> +\t\t\t\tnth_midxed_pack_name(bitmap_git->midx, i));\n\nThis loop runs from 0 to (num_packs + num_packs_in_base). I\nunderstand if it runs from num_packs_in_base to (num_packs +\nnum_packs_in_base), iterating only on this layer, but probably this\njust tries to open everything (i.e. in addition to num_packs we\nhave, we know num_packs_in_base packs are there in our base layer(s),\nso we iterate from 0 to that number).\n\nThe updated code converts the global 'i', which runs from 0 to\n\"everything under us\" num_packs + num_packs_in_base, to\ncorresponding layer midx plus offset in it, so it looks good, but\nthen, is the original reference to bitmap_git->midx->pack_names[i]\neven correct? If we have a base, i can run larger than\nbitmap_git->midx->num_packs, which is the size of the array\nbitmap_git->midx->pack_names[].\n\nOr, unlike how the proposed log message portrayed this change as\n(i.e. code clean up), does this patch fix real bugs that manifest\nonly when midx files are chained?\n\n> @@ -2469,7 +2469,7 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n> \t\t\tstruct bitmapped_pack pack;\n> \t\t\tif (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {\n> \t\t\t\twarning(_(\"unable to load pack: '%s', disabling pack-reuse\"),\n> -\t\t\t\t\tbitmap_git->midx->pack_names[i]);\n> +\t\t\t\t\tnth_midxed_pack_name(bitmap_git->midx, i));\n> \t\t\t\tfree(packs);\n> \t\t\t\treturn;\n> \t\t\t}\n\nSimilar to the above, this is also in a loop that runs from 0 to\nnum_packs+num_packs_in_base. Is the array access to find the name\nfor the error message in the original even correct when midx are\nchained?\n\n> diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c\n> index ac81390899..fbed0f6919 100644\n> --- a/t/helper/test-read-midx.c\n> +++ b/t/helper/test-read-midx.c\n> @@ -53,8 +53,9 @@ static int read_midx_file(const char *object_dir, const char *checksum,\n> \tprintf(\"\\nnum_objects: %d\\n\", m->num_objects);\n> \n> \tprintf(\"packs:\\n\");\n> -\tfor (i = 0; i < m->num_packs; i++)\n> -\t\tprintf(\"%s\\n\", m->pack_names[i]);\n> +\tfor (i = m->num_packs_in_base; i < m->num_packs + m->num_packs_in_base;\n> +\t i++)\n> +\t\tprintf(\"%s\\n\", nth_midxed_pack_name(m, i));\n\nOK. This used to iterate from 0 to num_packs using the local\noffset. Now it iterates from num_packs_in_base to num_packs_in_base+num_packs,\nmeaning we iterate over packs in the given midx. No change in\nbehaviour, as accesses to m->pack_names[i] using the local offset in\nthe original was correct, and the updated code iterates using the\nglobal offset. This is not a bugfix but is a code cleanup.\n\n> @@ -108,7 +109,7 @@ static int read_midx_preferred_pack(const char *object_dir)\n> \t\treturn 1;\n> \t}\n> \n> -\tprintf(\"%s\\n\", midx->pack_names[preferred_pack]);\n> +\tprintf(\"%s\\n\", nth_midxed_pack_name(midx, preferred_pack));\n\nAgain, is the original buggy when midx are chained?\n\n> \tclose_midx(midx);\n> \treturn 0;\n> }\n"},{"id":"519178","messageId":"xmqqmsavjg3s.fsf@gitster.g","threadId":"63469","inReplyTo":"d3508d3cfbddb512dbca4c2177731fffb5827084.1748473122.git.me@ttaylorr.com","subject":"Re: [PATCH v2 1/4] midx: access pack names through `nth_midxed_pack_name()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-29T20:51:03Z","receivedAt":"2025-05-29T20:51:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Accessing a MIDX's 'pack_names' array is somewhat error-prone when\n> dealing with incremental MIDX chains, where the (global) pack_int_id for\n> some pack may differ from the containing layer's index for that pack.\n>\n> Introduce `nth_midxed_pack_name()` in an effort to reduce a common\n> source of errors by discouraging external callers from accessing a\n> layer's `pack_names` array directly.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> midx.c | 7 +++++++\n> midx.h | 2 ++\n> pack-bitmap.c | 4 ++--\n> t/helper/test-read-midx.c | 7 ++++---\n> 4 files changed, 15 insertions(+), 5 deletions(-)\n\nOne thing I forgot to ask. Should we expect that\n\n\t$ git grep -E -e '(\\.|->)pack_names\\['\n\nto give hits only from the implementation of nth_midxed_pack_name()?\n\n> +const char *nth_midxed_pack_name(struct multi_pack_index *m,\n> +\t\t\t\t uint32_t pack_int_id)\n> +{\n> +\tuint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id);\n> +\treturn m->pack_names[local_pack_int_id];\n> +}\n"},{"id":"519201","messageId":"20250530062733.GB1321283@coredump.intra.peff.net","threadId":"63469","inReplyTo":"20250528-pks-pack-avoid-stats-on-missing-v3-0-4cbd0e14bed8@pks.im","subject":"Re: [PATCH v3 0/2] packfile: avoid access(3p) calls for missing packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-30T06:27:33Z","receivedAt":"2025-05-30T06:27:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 28, 2025 at 02:24:09PM +0200, Patrick Steinhardt wrote:\n\n> Changes in v3:\n> - Use a macro to hide away the `(void *)(intptr_t)-1` magic.\n> - Link to v2: https://lore.kernel.org/r/20250520-pks-pack-avoid-stats-on-missing-v2-0-333c5217fb05@pks.im\n\nThanks, this looks good to me.\n\n-Peff\n"},{"id":"519203","messageId":"20250530065034.GC1321283@coredump.intra.peff.net","threadId":"63469","inReplyTo":"e3e21db673f3ae6e574333d4678a0450fa44fb9c.1748473122.git.me@ttaylorr.com","subject":"Re: [PATCH v2 4/4] midx: return a `packed_git` pointer from `prepare_midx_pack()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-30T06:50:34Z","receivedAt":"2025-05-30T06:50:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 28, 2025 at 06:59:09PM -0400, Taylor Blau wrote:\n\n> Let's instead have prepare_midx_pack() return a pointer to the\n> packed_git structure itself, hiding the above as an implementation\n> detail of prepare_midx_pack(). This patch turns the above snippet into:\n> \n> struct packed_git *p = prepare_midx_pack(the_repository, some_midx,\n> some_pack_int_id);\n> if (!p)\n> die(\"could not load pack xyz\");\n> \n> making it far easier and less error-prone to access packs by their\n> pack_int_id in a MIDX chain.\n\nSo obviously I like this direction, but a few small comments:\n\n> (In the future, we may want to consider similar treatment for, e.g., the\n> pack_names array. Likewise, it might make sense to rename the \"packs\"\n> member of the MIDX structure to suggest that it shouldn't be accessed\n> directly outside of midx.c.)\n\nIs this note still valid for v2? It looks like patch 1 adds\nnth_midxed_pack_name() and tries to use it everywhere.\n\n> @@ -1649,9 +1646,9 @@ static int want_included_pack(struct repository *r,\n> \n> \tASSERT(m && !m->base_midx);\n> \n> -\tif (prepare_midx_pack(r, m, pack_int_id))\n> +\tp = prepare_midx_pack(r, m, pack_int_id);\n> +\tif (!p)\n> \t\treturn 0;\n> -\tp = m->packs[pack_int_id];\n> \tif (!pack_kept_objects && p->pack_keep)\n> \t\treturn 0;\n> \tif (p->is_cruft)\n\nThe ASSERT() in the context is from earlier in the series. But do we\nneed it once we have this patch? We no longer look at pack_int_id except\nto pass it to prepare_midx_pack(), which handles non-base midx slices\njust fine.\n\nSo we could loosen the assertion now. Or we could wait for later when\nsomebody wants/needs to do so, but I'm not sure how easy they would find\nit to dig in the history. They would find the commit that added the\nASSERT(), but may not realize that this later commit made it OK to\nloosen.\n\nI didn't check the other ASSERT() spots from that earlier patch (IIRC,\nsome of them may actually look use the pack_int_id for other things, and\nwouldn't be ready for non-base slices).\n\n> -int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n> -\t\t uint32_t pack_int_id)\n> +struct packed_git *prepare_midx_pack(struct repository *r,\n> +\t\t\t\t struct multi_pack_index *m,\n> +\t\t\t\t uint32_t pack_int_id)\n\nWe used to return \"1\" for failure and \"0\" for success. Now we're\nreversed: we return NULL for failure and non-zero for success.\n\nSo code like:\n\n if (prepare_midx_pack(...))\n\treturn error(\"yikes\");\n\nneeds to be updated, but the compiler won't help us because it is happy\nto convert both an int and a pointer into a boolean check.\n\nShould we rename the function to make sure we catch any callers for\ntopics in flight?\n\nI'd have thought we could call it nth_midxed_pack(), but that seems to\nexist already, with the caveat that it never prepares the pack, but only\nserves what's in the cache. I wonder if we could simply replace that\nwith what prepare_midx_pack() does, but it may be more conservative to\nleave the two separate. So I guess nth_midxed_pack_load() or something.\n\n-Peff\n"},{"id":"519604","messageId":"aD91ivFa2x1VBGRo@nand.local","threadId":"63469","inReplyTo":"xmqqr007jg9b.fsf@gitster.g","subject":"Re: [PATCH v2 1/4] midx: access pack names through `nth_midxed_pack_name()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-06-03T22:22:02Z","receivedAt":"2025-06-03T22:22:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 29, 2025 at 01:47:44PM -0700, Junio C Hamano wrote:\n> > diff --git a/pack-bitmap.c b/pack-bitmap.c\n> > index b9f1d86604..8ddc150778 100644\n> > --- a/pack-bitmap.c\n> > +++ b/pack-bitmap.c\n> > @@ -490,7 +490,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n> > \tfor (i = 0; i < bitmap_git->midx->num_packs + bitmap_git->midx->num_packs_in_base; i++) {\n> > \t\tif (prepare_midx_pack(bitmap_repo(bitmap_git), bitmap_git->midx, i)) {\n> > \t\t\twarning(_(\"could not open pack %s\"),\n> > -\t\t\t\tbitmap_git->midx->pack_names[i]);\n> > +\t\t\t\tnth_midxed_pack_name(bitmap_git->midx, i));\n>\n> This loop runs from 0 to (num_packs + num_packs_in_base). I\n> understand if it runs from num_packs_in_base to (num_packs +\n> num_packs_in_base), iterating only on this layer, but probably this\n> just tries to open everything (i.e. in addition to num_packs we\n> have, we know num_packs_in_base packs are there in our base layer(s),\n> so we iterate from 0 to that number).\n>\n> The updated code converts the global 'i', which runs from 0 to\n> \"everything under us\" num_packs + num_packs_in_base, to\n> corresponding layer midx plus offset in it, so it looks good, but\n> then, is the original reference to bitmap_git->midx->pack_names[i]\n> even correct? If we have a base, i can run larger than\n> bitmap_git->midx->num_packs, which is the size of the array\n> bitmap_git->midx->pack_names[].\n>\n> Or, unlike how the proposed log message portrayed this change as\n> (i.e. code clean up), does this patch fix real bugs that manifest\n> only when midx files are chained?\n\nRight; the original code was buggy if we had a failure opening a MIDX'd\npack outside of the base layer in an incremental MIDX bitmap. Reading\nthe proposed log message again, I see what you're saying. I am happy to\nclarify that this is indeed a bugfix, not just a cleanup.\n\n> > @@ -2469,7 +2469,7 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n> > \t\t\tstruct bitmapped_pack pack;\n> > \t\t\tif (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {\n> > \t\t\t\twarning(_(\"unable to load pack: '%s', disabling pack-reuse\"),\n> > -\t\t\t\t\tbitmap_git->midx->pack_names[i]);\n> > +\t\t\t\t\tnth_midxed_pack_name(bitmap_git->midx, i));\n> > \t\t\t\tfree(packs);\n> > \t\t\t\treturn;\n> > \t\t\t}\n>\n> Similar to the above, this is also in a loop that runs from 0 to\n> num_packs+num_packs_in_base. Is the array access to find the name\n> for the error message in the original even correct when midx are\n> chained?\n\nRight; this spot suffers from the same bug as the previous hunk.\n\n> > diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c\n> > index ac81390899..fbed0f6919 100644\n> > --- a/t/helper/test-read-midx.c\n> > +++ b/t/helper/test-read-midx.c\n> > @@ -53,8 +53,9 @@ static int read_midx_file(const char *object_dir, const char *checksum,\n> > \tprintf(\"\\nnum_objects: %d\\n\", m->num_objects);\n> >\n> > \tprintf(\"packs:\\n\");\n> > -\tfor (i = 0; i < m->num_packs; i++)\n> > -\t\tprintf(\"%s\\n\", m->pack_names[i]);\n> > +\tfor (i = m->num_packs_in_base; i < m->num_packs + m->num_packs_in_base;\n> > +\t i++)\n> > +\t\tprintf(\"%s\\n\", nth_midxed_pack_name(m, i));\n>\n> OK. This used to iterate from 0 to num_packs using the local\n> offset. Now it iterates from num_packs_in_base to num_packs_in_base+num_packs,\n> meaning we iterate over packs in the given midx. No change in\n> behaviour, as accesses to m->pack_names[i] using the local offset in\n> the original was correct, and the updated code iterates using the\n> global offset. This is not a bugfix but is a code cleanup.\n\nRight.\n\n> > @@ -108,7 +109,7 @@ static int read_midx_preferred_pack(const char *object_dir)\n> > \t\treturn 1;\n> > \t}\n> >\n> > -\tprintf(\"%s\\n\", midx->pack_names[preferred_pack]);\n> > +\tprintf(\"%s\\n\", nth_midxed_pack_name(midx, preferred_pack));\n>\n> Again, is the original buggy when midx are chained?\n\nThis is also a bugfix.\n\nThanks,\nTaylor\n"},{"id":"519605","messageId":"aD91yE8rKkjwe3mf@nand.local","threadId":"63469","inReplyTo":"xmqqmsavjg3s.fsf@gitster.g","subject":"Re: [PATCH v2 1/4] midx: access pack names through `nth_midxed_pack_name()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-06-03T22:23:04Z","receivedAt":"2025-06-03T22:23:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 29, 2025 at 01:51:03PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > Accessing a MIDX's 'pack_names' array is somewhat error-prone when\n> > dealing with incremental MIDX chains, where the (global) pack_int_id for\n> > some pack may differ from the containing layer's index for that pack.\n> >\n> > Introduce `nth_midxed_pack_name()` in an effort to reduce a common\n> > source of errors by discouraging external callers from accessing a\n> > layer's `pack_names` array directly.\n> >\n> > Signed-off-by: Taylor Blau \n> > ---\n> > midx.c | 7 +++++++\n> > midx.h | 2 ++\n> > pack-bitmap.c | 4 ++--\n> > t/helper/test-read-midx.c | 7 ++++---\n> > 4 files changed, 15 insertions(+), 5 deletions(-)\n>\n> One thing I forgot to ask. Should we expect that\n>\n> \t$ git grep -E -e '(\\.|->)pack_names\\['\n>\n> to give hits only from the implementation of nth_midxed_pack_name()?\n\nI waffled on this a bit... and I think the answer is \"almost\". Certainly\ncallers outside of midx.c or midx-write.c should not be touching\n\"pack_names\" directly.\n\nBut there are a couple of spots in midx-write.c that are really more\nconvenient to directly look at the pack_names array rather than going\nthrough the accessor.\n\nThanks,\nTaylor\n"},{"id":"519606","messageId":"aD92wu6Mk/S9Qm4t@nand.local","threadId":"63469","inReplyTo":"20250530065034.GC1321283@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/4] midx: return a `packed_git` pointer from `prepare_midx_pack()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-06-03T22:27:14Z","receivedAt":"2025-06-03T22:27:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 30, 2025 at 02:50:34AM -0400, Jeff King wrote:\n> On Wed, May 28, 2025 at 06:59:09PM -0400, Taylor Blau wrote:\n>\n> > Let's instead have prepare_midx_pack() return a pointer to the\n> > packed_git structure itself, hiding the above as an implementation\n> > detail of prepare_midx_pack(). This patch turns the above snippet into:\n> >\n> > struct packed_git *p = prepare_midx_pack(the_repository, some_midx,\n> > some_pack_int_id);\n> > if (!p)\n> > die(\"could not load pack xyz\");\n> >\n> > making it far easier and less error-prone to access packs by their\n> > pack_int_id in a MIDX chain.\n>\n> So obviously I like this direction, but a few small comments:\n>\n> > (In the future, we may want to consider similar treatment for, e.g., the\n> > pack_names array. Likewise, it might make sense to rename the \"packs\"\n> > member of the MIDX structure to suggest that it shouldn't be accessed\n> > directly outside of midx.c.)\n>\n> Is this note still valid for v2? It looks like patch 1 adds\n> nth_midxed_pack_name() and tries to use it everywhere.\n\nYeah, we should get rid of this. I had written it before I wrote what is\nnow the first patch in this series, and neglected to remove it before\nsending out the latest round.\n\n> > @@ -1649,9 +1646,9 @@ static int want_included_pack(struct repository *r,\n> >\n> > \tASSERT(m && !m->base_midx);\n> >\n> > -\tif (prepare_midx_pack(r, m, pack_int_id))\n> > +\tp = prepare_midx_pack(r, m, pack_int_id);\n> > +\tif (!p)\n> > \t\treturn 0;\n> > -\tp = m->packs[pack_int_id];\n> > \tif (!pack_kept_objects && p->pack_keep)\n> > \t\treturn 0;\n> > \tif (p->is_cruft)\n>\n> The ASSERT() in the context is from earlier in the series. But do we\n> need it once we have this patch? We no longer look at pack_int_id except\n> to pass it to prepare_midx_pack(), which handles non-base midx slices\n> just fine.\n>\n> So we could loosen the assertion now. Or we could wait for later when\n> somebody wants/needs to do so, but I'm not sure how easy they would find\n> it to dig in the history. They would find the commit that added the\n> ASSERT(), but may not realize that this later commit made it OK to\n> loosen.\n>\n> I didn't check the other ASSERT() spots from that earlier patch (IIRC,\n> some of them may actually look use the pack_int_id for other things, and\n> wouldn't be ready for non-base slices).\n\nWe could loosen the assertion here, but part of me likes keeping it as a\nself-documenting note that this function is only intended to be used for\nnon-incremental MIDXs.\n\n> > -int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,\n> > -\t\t uint32_t pack_int_id)\n> > +struct packed_git *prepare_midx_pack(struct repository *r,\n> > +\t\t\t\t struct multi_pack_index *m,\n> > +\t\t\t\t uint32_t pack_int_id)\n>\n> We used to return \"1\" for failure and \"0\" for success. Now we're\n> reversed: we return NULL for failure and non-zero for success.\n>\n> So code like:\n>\n> if (prepare_midx_pack(...))\n> \treturn error(\"yikes\");\n>\n> needs to be updated, but the compiler won't help us because it is happy\n> to convert both an int and a pointer into a boolean check.\n>\n> Should we rename the function to make sure we catch any callers for\n> topics in flight?\n>\n> I'd have thought we could call it nth_midxed_pack(), but that seems to\n> exist already, with the caveat that it never prepares the pack, but only\n> serves what's in the cache. I wonder if we could simply replace that\n> with what prepare_midx_pack() does, but it may be more conservative to\n> leave the two separate. So I guess nth_midxed_pack_load() or something.\n\nIn general there aren't a ton of in-flight changes in the MIDX code at\nany given time, so I think we could get away without renaming it. But I\ndon't mind erring on the side of caution here, either.\n\nThanks,\nTaylor\n"},{"id":"525168","messageId":"xmqqv7m7qbaa.fsf@gitster.g","threadId":"63469","inReplyTo":"aD92wu6Mk/S9Qm4t@nand.local","subject":"Re: [PATCH v2 4/4] midx: return a `packed_git` pointer from `prepare_midx_pack()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-28T23:25:17Z","receivedAt":"2025-08-28T23:25:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Fri, May 30, 2025 at 02:50:34AM -0400, Jeff King wrote:\n>> ...\n>> Is this note still valid for v2? It looks like patch 1 adds\n>> nth_midxed_pack_name() and tries to use it everywhere.\n>\n> Yeah, we should get rid of this. I had written it before I wrote what is\n> now the first patch in this series, and neglected to remove it before\n> sending out the latest round.\n> ...\n>> I'd have thought we could call it nth_midxed_pack(), but that seems to\n>> exist already, with the caveat that it never prepares the pack, but only\n>> serves what's in the cache. I wonder if we could simply replace that\n>> with what prepare_midx_pack() does, but it may be more conservative to\n>> leave the two separate. So I guess nth_midxed_pack_load() or something.\n>\n> In general there aren't a ton of in-flight changes in the MIDX code at\n> any given time, so I think we could get away without renaming it. But I\n> don't mind erring on the side of caution here, either.\n\nThe topic went dark after this message. Are there still unresolved\nissues, or do we want to get it unstuck?\n\nThanks.\n"}]}