{"thread":{"id":"62010","subject":"[PATCH 0/5] pack-objects: brown-paper-bag fixes for multi-pack reuse","startedAt":"2024-08-27T21:13:26Z","lastAt":"2024-09-17T09:58:14Z","messageCount":17,"participants":["Taylor Blau","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"501730","messageId":"cover.1724793201.git.me@ttaylorr.com","threadId":"62010","inReplyTo":null,"subject":"[PATCH 0/5] pack-objects: brown-paper-bag fixes for multi-pack reuse","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-08-27T21:13:24Z","receivedAt":"2024-08-27T21:13:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series fixes a couple of issues (some cosmetic, others less so) in\nmulti-pack reuse noticed when rolling this out over a few real-world,\ninternal repositories on GitHub's servers.\n\nThe patches are laid out as follows:\n\n  - The first three patches demonstrate, prepare for, and fix a\n    significant bug with multi-pack reuse which results in all sorts of\n    strange behavior (explained in detail in the third commit of this\n    series).\n\n  - The fourth patch is a minor (mostly cosmetic) performance\n    optimization that avoids duplicate calls to pack_pos_to_offset()\n    when performing pack-reuse with a MIDX bitmap.\n\n  - The final patch is a cosmetic fix to avoid using the value of a\n    constant instead of the name constant itself.\n\nThanks in advance for your review!\n\nTaylor Blau (5):\n  t/t5332-multi-pack-reuse.sh: verify pack generation with --strict\n  pack-bitmap: tag bitmapped packs with their corresponding MIDX\n  builtin/pack-objects.c: translate bit positions during pack-reuse\n  pack-bitmap.c: avoid repeated `pack_pos_to_offset()` during reuse\n  builtin/pack-objects.c: do not open-code `MAX_PACK_OBJECT_HEADER`\n\n builtin/pack-objects.c      | 46 +++++++++++++++++++++++++++++--------\n midx.c                      |  1 +\n pack-bitmap.c               | 12 ++++++----\n pack-bitmap.h               |  1 +\n t/t5332-multi-pack-reuse.sh | 35 ++++++++++++++++++++++++----\n 5 files changed, 78 insertions(+), 17 deletions(-)\n\n\nbase-commit: 159f2d50e75c17382c9f4eb7cbda671a6fa612d1\n-- \n2.46.0.426.g82754d92509\n"},{"id":"501731","messageId":"160673231c55f51c22171a8c2fbb105f7a501083.1724793201.git.me@ttaylorr.com","threadId":"62010","inReplyTo":"cover.1724793201.git.me@ttaylorr.com","subject":"[PATCH 1/5] t/t5332-multi-pack-reuse.sh: verify pack generation with --strict","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-08-27T21:13:27Z","receivedAt":"2024-08-27T21:13:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In our tests for multi-pack reuse, we have two helper functions:\n\n  - test_pack_objects_reused_all(), and\n  - test_pack_objects_reused()\n\nwhich invoke pack-objects (either with `--all`, or the supplied tips via\nstdin, respectively) and ensure that (a) the number of reused objects,\nand (b) the number of packs which those objects were reused from both\nmatch the expected values.\n\nBoth functions discard the output of pack-objects and assert only on the\ncontents of the trace2 stream.\n\nHowever, if we store the pack and attempt to index it with `--strict`,\nwe find that a number of our tests are broken, indicating a bug within\nmulti-pack reuse.\n\nThat bug will be addressed in a subsequent commit. But let's first\nharden these tests by trying to index the resulting pack, marking the\ntests which fail appropriately.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5332-multi-pack-reuse.sh | 20 ++++++++++++--------\n 1 file changed, 12 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh\nindex 941e73d354..ba888a83d5 100755\n--- a/t/t5332-multi-pack-reuse.sh\n+++ b/t/t5332-multi-pack-reuse.sh\n@@ -31,20 +31,24 @@ test_pack_objects_reused_all () {\n \t: >trace2.txt &&\n \tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n \t\tgit pack-objects --stdout --revs --all --delta-base-offset \\\n-\t\t>/dev/null &&\n+\t\t>got.pack &&\n \n \ttest_pack_reused \"$1\" <trace2.txt &&\n-\ttest_packs_reused \"$2\" <trace2.txt\n+\ttest_packs_reused \"$2\" <trace2.txt &&\n+\n+\tgit index-pack --strict -o got.idx got.pack\n }\n \n # test_pack_objects_reused <pack-reused> <packs-reused>\n test_pack_objects_reused () {\n \t: >trace2.txt &&\n \tGIT_TRACE2_EVENT=\"$PWD/trace2.txt\" \\\n-\t\tgit pack-objects --stdout --revs >/dev/null &&\n+\t\tgit pack-objects --stdout --revs >got.pack &&\n \n \ttest_pack_reused \"$1\" <trace2.txt &&\n-\ttest_packs_reused \"$2\" <trace2.txt\n+\ttest_packs_reused \"$2\" <trace2.txt &&\n+\n+\tgit index-pack --strict -o got.idx got.pack\n }\n \n test_expect_success 'preferred pack is reused for single-pack reuse' '\n@@ -65,7 +69,7 @@ test_expect_success 'multi-pack reuse is disabled by default' '\n \ttest_pack_objects_reused_all 3 1\n '\n \n-test_expect_success 'feature.experimental implies multi-pack reuse' '\n+test_expect_failure 'feature.experimental implies multi-pack reuse' '\n \ttest_config feature.experimental true &&\n \n \ttest_pack_objects_reused_all 6 2\n@@ -82,7 +86,7 @@ test_expect_success 'enable multi-pack reuse' '\n \tgit config pack.allowPackReuse multi\n '\n \n-test_expect_success 'reuse all objects from subset of bitmapped packs' '\n+test_expect_failure 'reuse all objects from subset of bitmapped packs' '\n \ttest_commit C &&\n \tgit repack -d &&\n \n@@ -96,7 +100,7 @@ test_expect_success 'reuse all objects from subset of bitmapped packs' '\n \ttest_pack_objects_reused 6 2 <in\n '\n \n-test_expect_success 'reuse all objects from all packs' '\n+test_expect_failure 'reuse all objects from all packs' '\n \ttest_pack_objects_reused_all 9 3\n '\n \n@@ -190,7 +194,7 @@ test_expect_success 'omit delta with uninteresting base (same pack)' '\n \ttest_pack_objects_reused 3 1 <in\n '\n \n-test_expect_success 'omit delta from uninteresting base (cross pack)' '\n+test_expect_failure 'omit delta from uninteresting base (cross pack)' '\n \tcat >in <<-EOF &&\n \t$(git rev-parse $base)\n \t^$(git rev-parse $delta)\n-- \n2.46.0.426.g82754d92509\n\n"},{"id":"501732","messageId":"1838bbcf7fe6daa58a7db78b81a2d08138fe176e.1724793201.git.me@ttaylorr.com","threadId":"62010","inReplyTo":"cover.1724793201.git.me@ttaylorr.com","subject":"[PATCH 2/5] pack-bitmap: tag bitmapped packs with their corresponding MIDX","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-08-27T21:13:30Z","receivedAt":"2024-08-27T21:13:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The next commit will need to use the bitmap's MIDX (if one exists) to\ntranslate bit positions into pack-relative positions in the source pack.\n\nOrdinarily, we'd use the \"midx\" field of the bitmap_index struct. But\nsince that struct is defined within pack-bitmap.c, and our caller is in\na separate compilation unit, we do not have access to the MIDX field.\n\nInstead, add a \"from_midx\" field to the bitmapped_pack structure so that\nwe can use that piece of data from outside of pack-bitmap.c. The caller\nthat uses this new piece of information will be added in the following\ncommit.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c        | 1 +\n pack-bitmap.c | 1 +\n pack-bitmap.h | 1 +\n 3 files changed, 3 insertions(+)\n\ndiff --git a/midx.c b/midx.c\nindex ca98bfd7c6..67e0d64004 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -496,6 +496,7 @@ int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,\n \t\t\t\t MIDX_CHUNK_BITMAPPED_PACKS_WIDTH * local_pack_int_id +\n \t\t\t\t sizeof(uint32_t));\n \tbp->pack_int_id = pack_int_id;\n+\tbp->from_midx = m;\n \n \treturn 0;\n }\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 2e657a2aa4..218d7ac2eb 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -2322,6 +2322,7 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \t\tpacks[packs_nr].pack_int_id = pack_int_id;\n \t\tpacks[packs_nr].bitmap_nr = pack->num_objects;\n \t\tpacks[packs_nr].bitmap_pos = 0;\n+\t\tpacks[packs_nr].from_midx = bitmap_git->midx;\n \n \t\tobjects_nr = packs[packs_nr++].bitmap_nr;\n \t}\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex ff0fd815b8..d7f4b8b8e9 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -60,6 +60,7 @@ struct bitmapped_pack {\n \tuint32_t bitmap_pos;\n \tuint32_t bitmap_nr;\n \n+\tstruct multi_pack_index *from_midx; /* MIDX only */\n \tuint32_t pack_int_id; /* MIDX only */\n };\n \n-- \n2.46.0.426.g82754d92509\n\n"},{"id":"501733","messageId":"8998e51135e76061e603304a6b7bbe19893fc067.1724793201.git.me@ttaylorr.com","threadId":"62010","inReplyTo":"cover.1724793201.git.me@ttaylorr.com","subject":"[PATCH 3/5] builtin/pack-objects.c: translate bit positions during pack-reuse","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-08-27T21:13:33Z","receivedAt":"2024-08-27T21:13:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When reusing chunks verbatim from an existing source pack, the function\nwrite_reused_pack() first attempts to reuse whole words (via the\nfunction `write_reused_pack_verbatim()`), and then individual bits (via\n`write_reused_pack_one()`).\n\nIn the non-MIDX case, all of this code works fine. Likewise, in the MIDX\ncase, processing bits individually from the first (preferred) pack works\nfine. However, processing subsequent packs in the MIDX case is broken\nwhen there are duplicate objects among the set of MIDX'd packs.\n\nThis is because we treat the individual bit positions as valid pack\npositions within the source pack(s), which does not account for gaps in\nthe source pack, like we see when the MIDX must break ties between\nduplicate objects which appear in multiple packs.\n\nThe broken code looks like:\n\n    for (; i < reuse_packfile_bitmap->word_alloc; i++) {\n            for (offset = 0; offset < BITS_IN_EWORD, offset++) {\n                    /* ... */\n\n                    write_reused_pack_one(reuse_packfile->p,\n                                          pos + offset - reuse_packfile->bitmap_pos,\n                                          f, pack_start, &w_curs);\n            }\n    }\n\n, where the second argument is incorrect and does not account for gaps.\n\nInstead, make sure that we translate bit positions in the MIDX's\npseudo-pack order to pack positions in the respective source packs by:\n\n  - Translating the bit position (pseudo-pack order) to a MIDX position\n    (lexical order).\n\n  - Use the MIDX position to obtain the offset at which the given object\n    occurs in the source pack.\n\n  - Then translate that offset back into a pack relative position within\n    the source pack by calling offset_to_pack_pos().\n\nAfter doing this, then we can safely use the result as a pack position.\nNote that when doing single-pack reuse, as well as reusing objects from\nthe MIDX's preferred pack, such translation is not necessary, since\neither ties are broken in favor of the preferred pack, or there are no\nties to break at all (in the case of non-MIDX bitmaps).\n\nFailing to do this can result in strange failure modes. One example that\ncan occur when misinterpreting bits in the above fashion is that Git\nthinks it's supposed to send a delta that the caller does not want.\nUnder this (incorrect) assumption, we try to look up the delta's base\n(so that we can patch any OFS_DELTAs if necessary). We do this using\nfind_reused_offset().\n\nBut if we try and call that function for an offset belonging to an\nobject we did not send, we'll get back garbage. This can result in us\ncomputing a negative fixup value, which results in memory corruption\nwhen trying to write the (patched) OFS_DELTA header.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c      | 44 ++++++++++++++++++++++++++++++-------\n t/t5332-multi-pack-reuse.sh | 31 ++++++++++++++++++++++----\n 2 files changed, 63 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 778be80f56..700adfb5a8 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1191,6 +1191,7 @@ static void write_reused_pack(struct bitmapped_pack *reuse_packfile,\n \t\tsize_t pos = (i * BITS_IN_EWORD);\n \n \t\tfor (offset = 0; offset < BITS_IN_EWORD; ++offset) {\n+\t\t\tuint32_t pack_pos;\n \t\t\tif ((word >> offset) == 0)\n \t\t\t\tbreak;\n \n@@ -1199,14 +1200,41 @@ static void write_reused_pack(struct bitmapped_pack *reuse_packfile,\n \t\t\t\tcontinue;\n \t\t\tif (pos + offset >= reuse_packfile->bitmap_pos + reuse_packfile->bitmap_nr)\n \t\t\t\tgoto done;\n-\t\t\t/*\n-\t\t\t * Can use bit positions directly, even for MIDX\n-\t\t\t * bitmaps. See comment in try_partial_reuse()\n-\t\t\t * for why.\n-\t\t\t */\n-\t\t\twrite_reused_pack_one(reuse_packfile->p,\n-\t\t\t\t\t      pos + offset - reuse_packfile->bitmap_pos,\n-\t\t\t\t\t      f, pack_start, &w_curs);\n+\n+\t\t\tif (reuse_packfile->bitmap_pos) {\n+\t\t\t\t/*\n+\t\t\t\t * When doing multi-pack reuse on a\n+\t\t\t\t * non-preferred pack, translate bit positions\n+\t\t\t\t * from the MIDX pseudo-pack order back to their\n+\t\t\t\t * pack-relative positions before attempting\n+\t\t\t\t * reuse.\n+\t\t\t\t */\n+\t\t\t\tstruct multi_pack_index *m = reuse_packfile->from_midx;\n+\t\t\t\tuint32_t midx_pos;\n+\t\t\t\toff_t pack_ofs;\n+\n+\t\t\t\tif (!m)\n+\t\t\t\t\tBUG(\"non-zero bitmap position without MIDX\");\n+\n+\t\t\t\tmidx_pos = pack_pos_to_midx(m, pos + offset);\n+\t\t\t\tpack_ofs = nth_midxed_offset(m, midx_pos);\n+\n+\t\t\t\tif (offset_to_pack_pos(reuse_packfile->p,\n+\t\t\t\t\t\t       pack_ofs, &pack_pos) < 0)\n+\t\t\t\t\tBUG(\"could not find expected object at offset %\"PRIuMAX\" in pack %s\",\n+\t\t\t\t\t    (uintmax_t)pack_ofs,\n+\t\t\t\t\t    pack_basename(reuse_packfile->p));\n+\t\t\t} else {\n+\t\t\t\t/*\n+\t\t\t\t * Can use bit positions directly, even for MIDX\n+\t\t\t\t * bitmaps. See comment in try_partial_reuse()\n+\t\t\t\t * for why.\n+\t\t\t\t */\n+\t\t\t\tpack_pos = pos + offset;\n+\t\t\t}\n+\n+\t\t\twrite_reused_pack_one(reuse_packfile->p, pack_pos, f,\n+\t\t\t\t\t      pack_start, &w_curs);\n \t\t\tdisplay_progress(progress_state, ++written);\n \t\t}\n \t}\ndiff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh\nindex ba888a83d5..955ea42769 100755\n--- a/t/t5332-multi-pack-reuse.sh\n+++ b/t/t5332-multi-pack-reuse.sh\n@@ -69,7 +69,7 @@ test_expect_success 'multi-pack reuse is disabled by default' '\n \ttest_pack_objects_reused_all 3 1\n '\n \n-test_expect_failure 'feature.experimental implies multi-pack reuse' '\n+test_expect_success 'feature.experimental implies multi-pack reuse' '\n \ttest_config feature.experimental true &&\n \n \ttest_pack_objects_reused_all 6 2\n@@ -86,7 +86,7 @@ test_expect_success 'enable multi-pack reuse' '\n \tgit config pack.allowPackReuse multi\n '\n \n-test_expect_failure 'reuse all objects from subset of bitmapped packs' '\n+test_expect_success 'reuse all objects from subset of bitmapped packs' '\n \ttest_commit C &&\n \tgit repack -d &&\n \n@@ -100,7 +100,7 @@ test_expect_failure 'reuse all objects from subset of bitmapped packs' '\n \ttest_pack_objects_reused 6 2 <in\n '\n \n-test_expect_failure 'reuse all objects from all packs' '\n+test_expect_success 'reuse all objects from all packs' '\n \ttest_pack_objects_reused_all 9 3\n '\n \n@@ -194,7 +194,7 @@ test_expect_success 'omit delta with uninteresting base (same pack)' '\n \ttest_pack_objects_reused 3 1 <in\n '\n \n-test_expect_failure 'omit delta from uninteresting base (cross pack)' '\n+test_expect_success 'omit delta from uninteresting base (cross pack)' '\n \tcat >in <<-EOF &&\n \t$(git rev-parse $base)\n \t^$(git rev-parse $delta)\n@@ -236,4 +236,27 @@ test_expect_success 'non-omitted delta in MIDX preferred pack' '\n \ttest_pack_objects_reused_all $(wc -l <expect) 1\n '\n \n+test_expect_success 'duplicate objects' '\n+\tgit init duplicate-objects &&\n+\t(\n+\t\tcd duplicate-objects &&\n+\n+\t\tgit config pack.allowPackReuse multi &&\n+\n+\t\ttest_commit base &&\n+\n+\t\tgit repack -a &&\n+\n+\t\tgit rev-parse HEAD^{tree} >in &&\n+\t\tp=\"$(git pack-objects $packdir/pack <in)\" &&\n+\n+\t\tgit multi-pack-index write --bitmap --preferred-pack=pack-$p.idx &&\n+\n+\t\tobjects_nr=\"$(git rev-list --count --all --objects)\" &&\n+\t\tpacks_nr=\"$(find $packdir -type f -name \"pack-*.pack\" | wc -l)\" &&\n+\n+\t\ttest_pack_objects_reused_all $objects_nr $packs_nr\n+\t)\n+'\n+\n test_done\n-- \n2.46.0.426.g82754d92509\n\n"},{"id":"501734","messageId":"afa25c8806b0e80f1d3ed46f29eb164cab4583ac.1724793201.git.me@ttaylorr.com","threadId":"62010","inReplyTo":"cover.1724793201.git.me@ttaylorr.com","subject":"[PATCH 4/5] pack-bitmap.c: avoid repeated `pack_pos_to_offset()` during reuse","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-08-27T21:13:36Z","receivedAt":"2024-08-27T21:13:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When calling `try_partial_reuse()`, the (sole) caller from the function\n`reuse_partial_packfile_from_bitmap_1()` has to translate its bit\nposition to a pack position.\n\nIn the MIDX bitmap case, the caller translates from the bit position, to\na position in the MIDX's pseudo-pack order (with `pack_pos_to_midx()`),\nthen get a pack offset (with `nth_midxed_offset()`) before finally\nworking backwards to get the pack position in the source pack by calling\n`offset_to_pack_pos()`.\n\nIn the non-MIDX bitmap case, we can use the bit position as the pack\nposition directly (see the comment at the beginning of the\n`reuse_partial_packfile_from_bitmap_1()` function for why).\n\nIn either case, the first thing that `try_partial_reuse()` does after\nbeing called is determine the offset of the object at the given pack\nposition by calling `pack_pos_to_offset()`. But we already have that\ninformation in the MIDX case!\n\nAvoid re-computing that information by instead passing it in. In the\nMIDX case, we already have that information stored. In the non-MIDX\ncase, the call to `pack_pos_to_offset()` moves from the function\n`try_partial_reuse()` to its caller. In total, we'll save one call to\n`pack_pos_to_offset()` when processing MIDX bitmaps.\n\n(On my machine, there is a slight speed-up on the order of ~2ms, but it\nis within the margin of error over 10 runs, so I think you'd have to\nhave a truly gigantic repository to confidently measure any significant\nimprovement here).\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 218d7ac2eb..9d9b8c4bfb 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -2055,17 +2055,18 @@ static int try_partial_reuse(struct bitmap_index *bitmap_git,\n \t\t\t     struct bitmapped_pack *pack,\n \t\t\t     size_t bitmap_pos,\n \t\t\t     uint32_t pack_pos,\n+\t\t\t     off_t offset,\n \t\t\t     struct bitmap *reuse,\n \t\t\t     struct pack_window **w_curs)\n {\n-\toff_t offset, delta_obj_offset;\n+\toff_t delta_obj_offset;\n \tenum object_type type;\n \tunsigned long size;\n \n \tif (pack_pos >= pack->p->num_objects)\n \t\treturn -1; /* not actually in the pack */\n \n-\toffset = delta_obj_offset = pack_pos_to_offset(pack->p, pack_pos);\n+\tdelta_obj_offset = offset;\n \ttype = unpack_object_header(pack->p, w_curs, &offset, &size);\n \tif (type < 0)\n \t\treturn -1; /* broken packfile, punt */\n@@ -2184,6 +2185,7 @@ static void reuse_partial_packfile_from_bitmap_1(struct bitmap_index *bitmap_git\n \t\tfor (offset = 0; offset < BITS_IN_EWORD; offset++) {\n \t\t\tsize_t bit_pos;\n \t\t\tuint32_t pack_pos;\n+\t\t\toff_t ofs;\n \n \t\t\tif (word >> offset == 0)\n \t\t\t\tbreak;\n@@ -2198,7 +2200,6 @@ static void reuse_partial_packfile_from_bitmap_1(struct bitmap_index *bitmap_git\n \n \t\t\tif (bitmap_is_midx(bitmap_git)) {\n \t\t\t\tuint32_t midx_pos;\n-\t\t\t\toff_t ofs;\n \n \t\t\t\tmidx_pos = pack_pos_to_midx(bitmap_git->midx, bit_pos);\n \t\t\t\tofs = nth_midxed_offset(bitmap_git->midx, midx_pos);\n@@ -2213,10 +2214,12 @@ static void reuse_partial_packfile_from_bitmap_1(struct bitmap_index *bitmap_git\n \t\t\t\t\tBUG(\"advanced beyond the end of pack %s (%\"PRIuMAX\" > %\"PRIu32\")\",\n \t\t\t\t\t    pack_basename(pack->p), (uintmax_t)pack_pos,\n \t\t\t\t\t    pack->p->num_objects);\n+\n+\t\t\t\tofs = pack_pos_to_offset(pack->p, pack_pos);\n \t\t\t}\n \n \t\t\tif (try_partial_reuse(bitmap_git, pack, bit_pos,\n-\t\t\t\t\t      pack_pos, reuse, &w_curs) < 0) {\n+\t\t\t\t\t      pack_pos, ofs, reuse, &w_curs) < 0) {\n \t\t\t\t/*\n \t\t\t\t * try_partial_reuse indicated we couldn't reuse\n \t\t\t\t * any bits, so there is no point in trying more\n-- \n2.46.0.426.g82754d92509\n\n"},{"id":"501735","messageId":"82754d92509e364e96fe35ca20eb709eb6f38641.1724793201.git.me@ttaylorr.com","threadId":"62010","inReplyTo":"cover.1724793201.git.me@ttaylorr.com","subject":"[PATCH 5/5] builtin/pack-objects.c: do not open-code `MAX_PACK_OBJECT_HEADER`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-08-27T21:13:39Z","receivedAt":"2024-08-27T21:13:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The function `write_reused_pack_one()` defines an header to store the\nOFS_DELTA header, but uses the constant \"10\" instead of\n\"MAX_PACK_OBJECT_HEADER\" (as is done elsewhere in the same patch, circa\nbb514de356c (pack-objects: improve partial packfile reuse, 2019-12-18)).\n\nDeclare the `ofs_header` field to be sized according to\n`MAX_PACK_OBJECT_HEADER` (which is 10, as defined in \"pack.h\") instead\nof the constant 10.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 700adfb5a8..c6e2852d3c 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1072,7 +1072,7 @@ static void write_reused_pack_one(struct packed_git *reuse_packfile,\n \t\tfixup = find_reused_offset(offset) -\n \t\t\tfind_reused_offset(base_offset);\n \t\tif (fixup) {\n-\t\t\tunsigned char ofs_header[10];\n+\t\t\tunsigned char ofs_header[MAX_PACK_OBJECT_HEADER];\n \t\t\tunsigned i, ofs_len;\n \t\t\toff_t ofs = offset - base_offset - fixup;\n \n-- \n2.46.0.426.g82754d92509\n"},{"id":"501742","messageId":"xmqqplpt5wrt.fsf@gitster.g","threadId":"62010","inReplyTo":"1838bbcf7fe6daa58a7db78b81a2d08138fe176e.1724793201.git.me@ttaylorr.com","subject":"Re: [PATCH 2/5] pack-bitmap: tag bitmapped packs with their corresponding MIDX","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-28T00:14:14Z","receivedAt":"2024-08-28T00:14:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> The next commit will need to use the bitmap's MIDX (if one exists) to\n> translate bit positions into pack-relative positions in the source pack.\n>\n> Ordinarily, we'd use the \"midx\" field of the bitmap_index struct. But\n> since that struct is defined within pack-bitmap.c, and our caller is in\n> a separate compilation unit, we do not have access to the MIDX field.\n>\n> Instead, add a \"from_midx\" field to the bitmapped_pack structure so that\n> we can use that piece of data from outside of pack-bitmap.c. The caller\n> that uses this new piece of information will be added in the following\n> commit.\n>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  midx.c        | 1 +\n>  pack-bitmap.c | 1 +\n>  pack-bitmap.h | 1 +\n>  3 files changed, 3 insertions(+)\n>\n> diff --git a/midx.c b/midx.c\n> index ca98bfd7c6..67e0d64004 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -496,6 +496,7 @@ int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,\n>  \t\t\t\t MIDX_CHUNK_BITMAPPED_PACKS_WIDTH * local_pack_int_id +\n>  \t\t\t\t sizeof(uint32_t));\n>  \tbp->pack_int_id = pack_int_id;\n> +\tbp->from_midx = m;\n\nDo multi_pack_index objects live as long as bitmapped_pack objects\nthat point at them live?  If m later goes away without letting the\nbitmapped_pack know about it, the borrowed pointer in from_midx\nwould become dangling, which is not what we want to see.\n\n\"None of these objects are released or relocated while we are\nrunning pack-objects, so once the .from_midx member is assigned\nhere, it will always be pointing at a valid multi_pack_index object\"\nis a satisfactory answer, I guess.\n"},{"id":"501854","messageId":"ZtDEy/teamtXMAbn@nand.local","threadId":"62010","inReplyTo":"xmqqplpt5wrt.fsf@gitster.g","subject":"Re: [PATCH 2/5] pack-bitmap: tag bitmapped packs with their corresponding MIDX","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-08-29T18:58:19Z","receivedAt":"2024-08-29T18:58:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 27, 2024 at 05:14:14PM -0700, Junio C Hamano wrote:\n> > diff --git a/midx.c b/midx.c\n> > index ca98bfd7c6..67e0d64004 100644\n> > --- a/midx.c\n> > +++ b/midx.c\n> > @@ -496,6 +496,7 @@ int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,\n> >  \t\t\t\t MIDX_CHUNK_BITMAPPED_PACKS_WIDTH * local_pack_int_id +\n> >  \t\t\t\t sizeof(uint32_t));\n> >  \tbp->pack_int_id = pack_int_id;\n> > +\tbp->from_midx = m;\n>\n> Do multi_pack_index objects live as long as bitmapped_pack objects\n> that point at them live?  If m later goes away without letting the\n> bitmapped_pack know about it, the borrowed pointer in from_midx\n> would become dangling, which is not what we want to see.\n>\n> \"None of these objects are released or relocated while we are\n> running pack-objects, so once the .from_midx member is assigned\n> here, it will always be pointing at a valid multi_pack_index object\"\n> is a satisfactory answer, I guess.\n\nGood question, and good answer ;-).\n\nThis is only relevant in a read-only path where we're generating a new\npack from existing packs and not altering those pack or rewriting /\ndeleting the MIDX attached to them. So I think we're OK here and don't\nhave any lifetime/scope issues.\n\nThanks,\nTaylor\n"},{"id":"502161","messageId":"xmqqh6av9tag.fsf@gitster.g","threadId":"62010","inReplyTo":"8998e51135e76061e603304a6b7bbe19893fc067.1724793201.git.me@ttaylorr.com","subject":"Re: [PATCH 3/5] builtin/pack-objects.c: translate bit positions during pack-reuse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-04T18:18:31Z","receivedAt":"2024-09-04T18:18:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> Instead, make sure that we translate bit positions in the MIDX's\n> pseudo-pack order to pack positions in the respective source packs by:\n>\n>   - Translating the bit position (pseudo-pack order) to a MIDX position\n>     (lexical order).\n>\n>   - Use the MIDX position to obtain the offset at which the given object\n>     occurs in the source pack.\n>\n>   - Then translate that offset back into a pack relative position within\n>     the source pack by calling offset_to_pack_pos().\n\nQuite straight-forward, and the implementation below matches the\ndesign well.\n"},{"id":"502164","messageId":"xmqq1q1z9rmt.fsf@gitster.g","threadId":"62010","inReplyTo":"afa25c8806b0e80f1d3ed46f29eb164cab4583ac.1724793201.git.me@ttaylorr.com","subject":"Re: [PATCH 4/5] pack-bitmap.c: avoid repeated `pack_pos_to_offset()` during reuse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-04T18:54:18Z","receivedAt":"2024-09-04T18:54:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> @@ -2055,17 +2055,18 @@ static int try_partial_reuse(struct bitmap_index *bitmap_git,\n>  \t\t\t     struct bitmapped_pack *pack,\n>  \t\t\t     size_t bitmap_pos,\n>  \t\t\t     uint32_t pack_pos,\n> +\t\t\t     off_t offset,\n>  \t\t\t     struct bitmap *reuse,\n>  \t\t\t     struct pack_window **w_curs)\n>  {\n> -\toff_t offset, delta_obj_offset;\n> +\toff_t delta_obj_offset;\n>  \tenum object_type type;\n>  \tunsigned long size;\n>  \n>  \tif (pack_pos >= pack->p->num_objects)\n>  \t\treturn -1; /* not actually in the pack */\n>  \n> -\toffset = delta_obj_offset = pack_pos_to_offset(pack->p, pack_pos);\n\nWe are now passing redundant piece of information \"offset\" that\ncould be derived from two other parameters, which feels a bit icky,\n\nI suspect that try_partial_reuse() can be taught not to require\npack_pos and instead work entirely on offset\n\n - The caller can pass a very large offset to represent \"not\n   actually in the pack\" early return condition.\n\n - The logic to punt on a delta pointing backwards can be done by\n   comparing the base_offset and offset, instead of comparing the\n   base_pos and pack_pos.\n\nbut it may not be worth the effort, unless we are going to do\nsimilar clean-up globally in all the codepaths that access\npackfiles.\n\nThanks.\n"},{"id":"502165","messageId":"xmqqv7zb8cys.fsf@gitster.g","threadId":"62010","inReplyTo":"cover.1724793201.git.me@ttaylorr.com","subject":"Re: [PATCH 0/5] pack-objects: brown-paper-bag fixes for multi-pack reuse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-04T18:56:27Z","receivedAt":"2024-09-04T18:56:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> This series fixes a couple of issues (some cosmetic, others less so) in\n> multi-pack reuse noticed when rolling this out over a few real-world,\n> internal repositories on GitHub's servers.\n\nI cannot claim I got all the detail that went into steps 2 & 3\nright, but I was irritated enough that the topic was left in the\n\"Needs review\" state, so I gave a look at the tail end of the\nseries, and they were pleasant read.\n\nThanks.\n\n"},{"id":"502167","messageId":"Zti0xrzCtpWScPjz@nand.local","threadId":"62010","inReplyTo":"xmqq1q1z9rmt.fsf@gitster.g","subject":"Re: [PATCH 4/5] pack-bitmap.c: avoid repeated `pack_pos_to_offset()` during reuse","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-04T19:28:06Z","receivedAt":"2024-09-04T19:28:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 04, 2024 at 11:54:18AM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > @@ -2055,17 +2055,18 @@ static int try_partial_reuse(struct bitmap_index *bitmap_git,\n> >  \t\t\t     struct bitmapped_pack *pack,\n> >  \t\t\t     size_t bitmap_pos,\n> >  \t\t\t     uint32_t pack_pos,\n> > +\t\t\t     off_t offset,\n> >  \t\t\t     struct bitmap *reuse,\n> >  \t\t\t     struct pack_window **w_curs)\n> >  {\n> > -\toff_t offset, delta_obj_offset;\n> > +\toff_t delta_obj_offset;\n> >  \tenum object_type type;\n> >  \tunsigned long size;\n> >\n> >  \tif (pack_pos >= pack->p->num_objects)\n> >  \t\treturn -1; /* not actually in the pack */\n> >\n> > -\toffset = delta_obj_offset = pack_pos_to_offset(pack->p, pack_pos);\n>\n> We are now passing redundant piece of information \"offset\" that\n> could be derived from two other parameters, which feels a bit icky,\n\nYeah, I agree. The only spot we use pack_pos in this function is in the\nhunk above, and in the check where we reject any deltas whose base has a\ngreater than or equal to bit position than the delta object's bit\nposition.\n\nBut I think both of those are redundant. The only caller in\nreuse_partial_packfile_from_bitmap_1() only passes bit positions that\nare set, so we'll never trip the bounds check above. For the latter\ncheck, it would suffice to compare object offsets in the single-pack\ncase, since the bit positions corresponding to objects in a single pack\nare derived from the ordering of their actual offset.\n\n>  - The logic to punt on a delta pointing backwards can be done by\n>    comparing the base_offset and offset, instead of comparing the\n>    base_pos and pack_pos.\n\nAh, yes -- exactly ;-). I should have read the email in its entirety\nbefore starting to respond.\n\n> but it may not be worth the effort, unless we are going to do\n> similar clean-up globally in all the codepaths that access\n> packfiles.\n\nI'm not so sure we can do it in all cases, but at least in the case of\ntry_partial_reuse(), doing something like this (which compiles and\npasses \"make test\") is fine:\n\n--- 8< ---\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 9d9b8c4bfb..fc51f298d5 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -2054,7 +2054,6 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs,\n static int try_partial_reuse(struct bitmap_index *bitmap_git,\n \t\t\t     struct bitmapped_pack *pack,\n \t\t\t     size_t bitmap_pos,\n-\t\t\t     uint32_t pack_pos,\n \t\t\t     off_t offset,\n \t\t\t     struct bitmap *reuse,\n \t\t\t     struct pack_window **w_curs)\n@@ -2063,9 +2062,6 @@ static int try_partial_reuse(struct bitmap_index *bitmap_git,\n \tenum object_type type;\n \tunsigned long size;\n\n-\tif (pack_pos >= pack->p->num_objects)\n-\t\treturn -1; /* not actually in the pack */\n-\n \tdelta_obj_offset = offset;\n \ttype = unpack_object_header(pack->p, w_curs, &offset, &size);\n \tif (type < 0)\n@@ -2121,8 +2117,13 @@ static int try_partial_reuse(struct bitmap_index *bitmap_git,\n \t\t\t * OFS_DELTA is the default). But let's double check to\n \t\t\t * make sure the pack wasn't written with odd\n \t\t\t * parameters.\n+\t\t\t *\n+\t\t\t * Since we're working on a single-pack bitmap, we can\n+\t\t\t * use the object offset as a proxy for the bit\n+\t\t\t * position, since the bits are ordered by their\n+\t\t\t * positions within the pack.\n \t\t\t */\n-\t\t\tif (base_pos >= pack_pos)\n+\t\t\tif (base_offset >= offset)\n \t\t\t\treturn 0;\n \t\t\tbase_bitmap_pos = pack->bitmap_pos + base_pos;\n \t\t}\n@@ -2218,8 +2219,8 @@ static void reuse_partial_packfile_from_bitmap_1(struct bitmap_index *bitmap_git\n \t\t\t\tofs = pack_pos_to_offset(pack->p, pack_pos);\n \t\t\t}\n\n-\t\t\tif (try_partial_reuse(bitmap_git, pack, bit_pos,\n-\t\t\t\t\t      pack_pos, ofs, reuse, &w_curs) < 0) {\n+\t\t\tif (try_partial_reuse(bitmap_git, pack, bit_pos, ofs,\n+\t\t\t\t\t      reuse, &w_curs) < 0) {\n \t\t\t\t/*\n \t\t\t\t * try_partial_reuse indicated we couldn't reuse\n \t\t\t\t * any bits, so there is no point in trying more\n--- >8 ---\n\nThanks,\nTaylor\n"},{"id":"502168","messageId":"Zti04yZXGiCGJBKo@nand.local","threadId":"62010","inReplyTo":"xmqqv7zb8cys.fsf@gitster.g","subject":"Re: [PATCH 0/5] pack-objects: brown-paper-bag fixes for multi-pack reuse","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-04T19:28:35Z","receivedAt":"2024-09-04T19:28:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 04, 2024 at 11:56:27AM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > This series fixes a couple of issues (some cosmetic, others less so) in\n> > multi-pack reuse noticed when rolling this out over a few real-world,\n> > internal repositories on GitHub's servers.\n>\n> I cannot claim I got all the detail that went into steps 2 & 3\n> right, but I was irritated enough that the topic was left in the\n> \"Needs review\" state, so I gave a look at the tail end of the\n> series, and they were pleasant read.\n\nThanks. They were anything *but* a pleasant debugging session, but I'm\nglad that the end result was palatable ;-).\n\nThanks,\nTaylor\n"},{"id":"502202","messageId":"20240905090024.GA2556395@coredump.intra.peff.net","threadId":"62010","inReplyTo":"ZtDEy/teamtXMAbn@nand.local","subject":"Re: [PATCH 2/5] pack-bitmap: tag bitmapped packs with their corresponding MIDX","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-05T09:00:24Z","receivedAt":"2024-09-05T09:00:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 29, 2024 at 02:58:19PM -0400, Taylor Blau wrote:\n\n> On Tue, Aug 27, 2024 at 05:14:14PM -0700, Junio C Hamano wrote:\n> > > diff --git a/midx.c b/midx.c\n> > > index ca98bfd7c6..67e0d64004 100644\n> > > --- a/midx.c\n> > > +++ b/midx.c\n> > > @@ -496,6 +496,7 @@ int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,\n> > >  \t\t\t\t MIDX_CHUNK_BITMAPPED_PACKS_WIDTH * local_pack_int_id +\n> > >  \t\t\t\t sizeof(uint32_t));\n> > >  \tbp->pack_int_id = pack_int_id;\n> > > +\tbp->from_midx = m;\n> >\n> > Do multi_pack_index objects live as long as bitmapped_pack objects\n> > that point at them live?  If m later goes away without letting the\n> > bitmapped_pack know about it, the borrowed pointer in from_midx\n> > would become dangling, which is not what we want to see.\n> >\n> > \"None of these objects are released or relocated while we are\n> > running pack-objects, so once the .from_midx member is assigned\n> > here, it will always be pointing at a valid multi_pack_index object\"\n> > is a satisfactory answer, I guess.\n> \n> Good question, and good answer ;-).\n> \n> This is only relevant in a read-only path where we're generating a new\n> pack from existing packs and not altering those pack or rewriting /\n> deleting the MIDX attached to them. So I think we're OK here and don't\n> have any lifetime/scope issues.\n\nDo we ever close/reopen the midx? For example, if a simultaneous process\nwrote a new one and we triggered reprepare_packed_git()?\n\nI think the answer is \"no\"; we might read a new midx, but we never ditch\nthe old one (just like we do for packed_git structs). Which I suspect is\nneeded even before this patch, since various other parts of the bitmap\ncode (and probably others) rely on the struct staying in place across as\nwe read many objects.\n\n-Peff\n"},{"id":"502204","messageId":"20240905091043.GB2556395@coredump.intra.peff.net","threadId":"62010","inReplyTo":"xmqqv7zb8cys.fsf@gitster.g","subject":"Re: [PATCH 0/5] pack-objects: brown-paper-bag fixes for multi-pack reuse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-05T09:10:43Z","receivedAt":"2024-09-05T09:10:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 04, 2024 at 11:56:27AM -0700, Junio C Hamano wrote:\n\n> Taylor Blau <me@ttaylorr.com> writes:\n> \n> > This series fixes a couple of issues (some cosmetic, others less so) in\n> > multi-pack reuse noticed when rolling this out over a few real-world,\n> > internal repositories on GitHub's servers.\n> \n> I cannot claim I got all the detail that went into steps 2 & 3\n> right, but I was irritated enough that the topic was left in the\n> \"Needs review\" state, so I gave a look at the tail end of the\n> series, and they were pleasant read.\n\nSorry, I'm probably the most qualified reviewer here. I read through\nthe early patches, and I think the fix is correct, along with the\npreparation in patch 2.\n\nWith the partial disclaimer that I helped with the early debugging, and\nmy blind flailing at suggestions accidentally led Taylor to the right\nanswer. I don't think that biases my perspective, but maybe. ;)\n\n-Peff\n"},{"id":"502251","messageId":"xmqqbk125doa.fsf@gitster.g","threadId":"62010","inReplyTo":"20240905091043.GB2556395@coredump.intra.peff.net","subject":"Re: [PATCH 0/5] pack-objects: brown-paper-bag fixes for multi-pack reuse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-05T15:21:41Z","receivedAt":"2024-09-05T15:21:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Sep 04, 2024 at 11:56:27AM -0700, Junio C Hamano wrote:\n>\n>> Taylor Blau <me@ttaylorr.com> writes:\n>> \n>> > This series fixes a couple of issues (some cosmetic, others less so) in\n>> > multi-pack reuse noticed when rolling this out over a few real-world,\n>> > internal repositories on GitHub's servers.\n>> \n>> I cannot claim I got all the detail that went into steps 2 & 3\n>> right, but I was irritated enough that the topic was left in the\n>> \"Needs review\" state, so I gave a look at the tail end of the\n>> series, and they were pleasant read.\n>\n> Sorry, I'm probably the most qualified reviewer here. I read through\n> the early patches, and I think the fix is correct, along with the\n> preparation in patch 2.\n>\n> With the partial disclaimer that I helped with the early debugging, and\n> my blind flailing at suggestions accidentally led Taylor to the right\n> answer. I don't think that biases my perspective, but maybe. ;)\n\nThanks.\n"},{"id":"502935","messageId":"ZulSsfi/XouIlWtq@nand.local","threadId":"62010","inReplyTo":"20240905090024.GA2556395@coredump.intra.peff.net","subject":"Re: [PATCH 2/5] pack-bitmap: tag bitmapped packs with their corresponding MIDX","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-09-17T09:58:09Z","receivedAt":"2024-09-17T09:58:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 05, 2024 at 05:00:24AM -0400, Jeff King wrote:\n> On Thu, Aug 29, 2024 at 02:58:19PM -0400, Taylor Blau wrote:\n>\n> > On Tue, Aug 27, 2024 at 05:14:14PM -0700, Junio C Hamano wrote:\n> > > > diff --git a/midx.c b/midx.c\n> > > > index ca98bfd7c6..67e0d64004 100644\n> > > > --- a/midx.c\n> > > > +++ b/midx.c\n> > > > @@ -496,6 +496,7 @@ int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,\n> > > >  \t\t\t\t MIDX_CHUNK_BITMAPPED_PACKS_WIDTH * local_pack_int_id +\n> > > >  \t\t\t\t sizeof(uint32_t));\n> > > >  \tbp->pack_int_id = pack_int_id;\n> > > > +\tbp->from_midx = m;\n> > >\n> > > Do multi_pack_index objects live as long as bitmapped_pack objects\n> > > that point at them live?  If m later goes away without letting the\n> > > bitmapped_pack know about it, the borrowed pointer in from_midx\n> > > would become dangling, which is not what we want to see.\n> > >\n> > > \"None of these objects are released or relocated while we are\n> > > running pack-objects, so once the .from_midx member is assigned\n> > > here, it will always be pointing at a valid multi_pack_index object\"\n> > > is a satisfactory answer, I guess.\n> >\n> > Good question, and good answer ;-).\n> >\n> > This is only relevant in a read-only path where we're generating a new\n> > pack from existing packs and not altering those pack or rewriting /\n> > deleting the MIDX attached to them. So I think we're OK here and don't\n> > have any lifetime/scope issues.\n>\n> Do we ever close/reopen the midx? For example, if a simultaneous process\n> wrote a new one and we triggered reprepare_packed_git()?\n>\n> I think the answer is \"no\"; we might read a new midx, but we never ditch\n> the old one (just like we do for packed_git structs). Which I suspect is\n> needed even before this patch, since various other parts of the bitmap\n> code (and probably others) rely on the struct staying in place across as\n> we read many objects.\n\nI think that we *could* close and reopen the MIDX via a call to\nclose_object_store() (which dispatches a call to close_midx(), before\nNULL-ing out o->multi_pack_index) and then calling prepare_packed_git()\nor similar.\n\nBut I'm not aware of any such codepaths that deal with pack reuse and\ncalling nth_bitmapped_pack that would do such a thing, so I think that\nwe're OK here.\n\nThanks,\nTaylor\n"}]}