{"thread":{"id":"62488","subject":"[PATCH 0/2] pack-objects: more brown-paper-bag multi-pack reuse fixes","startedAt":"2024-11-13T17:32:54Z","lastAt":"2024-11-22T09:17:09Z","messageCount":14,"participants":["Taylor Blau","Jeff King","Junio C Hamano","Kristoffer Haugsbakk"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"507226","messageId":"cover.1731518931.git.me@ttaylorr.com","threadId":"62488","inReplyTo":null,"subject":"[PATCH 0/2] pack-objects: more brown-paper-bag multi-pack reuse fixes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-13T17:32:51Z","receivedAt":"2024-11-13T17:32:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"While starting to use multi-pack reuse in more places throughout\nGitHub, I noticed a rare but persistent segfault, which is the result\nof a broken assumption in write_reused_pack_verbatim().\n\nThe first patch demonstrates the problem, and the second patch fixes\nit.  The second patch explains the broken assumption in detail, but\nthe gist is that we can't infer that an all-1s word in the reuse\nbitmap from a non-preferred pack means that we want all objects\nbetween the ones corresponding to the first and last bit.\n\nI'm fairly disappointed that I didn't catch this obviously-broken\nimplementation during the original development of this feature, or in\nthe first round of bugfixes. But these patches should conclusively\nresolve at least this issue.\n\nThanks in advance for your review!\n\nTaylor Blau (2):\n  t5332-multi-pack-reuse.sh: demonstrate duplicate packing failure\n  pack-objects: only perform verbatim reuse on the preferred pack\n\n builtin/pack-objects.c      | 101 +++++++++++++++---------------------\n t/t5332-multi-pack-reuse.sh |  23 ++++++++\n 2 files changed, 66 insertions(+), 58 deletions(-)\n\n\nbase-commit: 25b0f41288718625b18495de23cc066394c09a92\n-- \n2.46.0.421.g159f2d50e75\n"},{"id":"507227","messageId":"7a69cf84ae5b92d99e5777d4600270712424c4d7.1731518931.git.me@ttaylorr.com","threadId":"62488","inReplyTo":"cover.1731518931.git.me@ttaylorr.com","subject":"[PATCH 1/2] t5332-multi-pack-reuse.sh: demonstrate duplicate packing failure","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-13T17:32:55Z","receivedAt":"2024-11-13T17:32:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the multi-pack reuse code, there are two paths for reusing the\non-disk representation of an object, handled by:\n\n  - builtin/pack-objects.c::write_reused_pack_one()\n  - builtin/pack-objects.c::write_reused_pack_verbatim()\n\nThe former is responsible for copying the bytes for a single object out\nof an existing source pack. The latter does the same but for a region of\nobjects aligned at eword_t boundaries.\n\nDemonstrate a bug whereby write_reused_pack_verbatim() can be tricked\ninto writing out objects from some source pack, even when those objects\nwere selected from a different source pack in the MIDX bitmap.\n\nWhen the caller wants at least one of the objects in that region,\npack-objects will write the same object twice as a result of this bug.\nIn the other case where the caller doesn't want any of the objects in\nthe region of interest, we will write out objects that weren't\nrequested.\n\nDemonstrate this bug by creating two packs, where the preferred one of\nthose packs contains a single object which also appears in the main\n(non-preferred) pack. A separate bug[^1] prevents us from triggering the\nmain bug when the duplicated object is the last one in the main pack,\nbut any earlier object will suffice.\n\nWe could fix that separate bug, but the following commit will simplify\nwrite_reused_pack_verbatim() and only call it on the preferred pack, so\ndoing so would have little point.\n\n[^1]: Because write_reused_pack_verbatim() only reuses bits in the range\n\n    off_t pack_start_off = pack_pos_to_offset(reuse_packfile->p, 0);\n    off_t pack_end_off = pack_pos_to_offset(reuse_packfile->p,\n                                            pos - reuse_packfile->bitmap_pos);\n\n    written += pos - reuse_packfile->bitmap_pos;\n\n    /* We're recording one chunk, not one object. */\n    record_reused_object(pack_start_off,\n                         pack_start_off - (hashfile_total(out) - pack_start));\n\n  , or in other words excluding the object beginning at position 'pos -\n  reuse_packfile->bitmap_pos' in the source pack. But since\n  reuse_packfile->bitmap_pos is '1' in the non-preferred pack\n  (accounting for the single-object pack which is preferred), we don't\n  actually copy the bytes from the last object.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5332-multi-pack-reuse.sh | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh\nindex 955ea42769b..8f403d9fdaa 100755\n--- a/t/t5332-multi-pack-reuse.sh\n+++ b/t/t5332-multi-pack-reuse.sh\n@@ -259,4 +259,27 @@ test_expect_success 'duplicate objects' '\n \t)\n '\n \n+test_expect_failure 'duplicate objects with verbatim reuse' '\n+\tgit init duplicate-objects-verbatim &&\n+\t(\n+\t\tcd duplicate-objects-verbatim &&\n+\n+\t\tgit config pack.allowPackReuse multi &&\n+\n+\t\ttest_commit_bulk 64 &&\n+\n+\t\t# take the first object from the main pack...\n+\t\tgit show-index <$(ls $packdir/pack-*.idx) >obj.raw &&\n+\t\tsort -nk1 <obj.raw | head -n1 | cut -d\" \" -f2 >in &&\n+\n+\t\t# ...and create a separate pack containing just that object\n+\t\tp=\"$(git pack-objects $packdir/pack <in)\" &&\n+\t\tgit show-index <$packdir/pack-$p.idx &&\n+\n+\t\tgit multi-pack-index write --bitmap --preferred-pack=pack-$p.idx &&\n+\n+\t\ttest_pack_objects_reused_all 192 2\n+\t)\n+'\n+\n test_done\n-- \n2.46.0.421.g159f2d50e75\n\n"},{"id":"507228","messageId":"2520abf24a8a194b3f7040e218f878dc88a740a0.1731518931.git.me@ttaylorr.com","threadId":"62488","inReplyTo":"cover.1731518931.git.me@ttaylorr.com","subject":"[PATCH 2/2] pack-objects: only perform verbatim reuse on the preferred pack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-13T17:32:58Z","receivedAt":"2024-11-13T17:33:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When reusing objects from source pack(s), write_reused_pack_verbatim()\nis responsible for reusing objects whole eword_t's at a time. It works\nby taking the longest continuous run of objects from the beginning of\neach source pack that the caller wants, and reuses the entirety of that\nsection from each pack.\n\nThis is based on the assumption that we don't have any gaps within the\nregion. This assumption relieves us from having to patch any\nOFS_DELTAs, since we know that there aren't any gaps between any delta\nand its base in that region.\n\nTo illustrate why this assumption is necessary, suppose we have some\npack P, which has objects X, Y, and Z. If the MIDX's copy of Y was\nselected from a pack other than P, then the bit corresponding to object\nY will appear earlier in the bitmap than the bits corresponding to X and\nZ.\n\nIf pack-objects already has or will use the copy of Y from the pack it\nwas selected from in the MIDX, then it is an error to reuse all objects\nbetween X and Z in the source pack. Doing so will cause us to reuse Y\nfrom a different pack than the one which represents Y in the MIDX,\ncausing us to either:\n\n - include the object twice, assuming that the caller wants Y in the\n   pack, or\n\n - include the object once, resulting in us packing more objects than\n   necessary.\n\nThis regression comes from ca0fd69e37 (pack-objects: prepare\n`write_reused_pack_verbatim()` for multi-pack reuse, 2023-12-14), which\nincorrectly assumed that there would be no gaps in reusable regions of\nnon-preferred packs.\n\nInstead, we can only safely perform the whole-word reuse optimization on\nthe preferred pack, where we know with certainty that no gaps exist in\nthat region of the bitmap. We can still reuse objects from non-preferred\npacks, but we have to inspect them individually in write_reused_pack()\nto ensure that any gaps that may exist are accounted for.\n\nThis allows us to simplify the implementation of\nwrite_reused_pack_verbatim() back to almost its pre-multi-pack reuse\nform, since we can now assume that the beginning of the pack appears at\nthe beginning of the bitmap, meaning that we don't have to account for\nany bits up to the first word boundary (like we had to special case in\nca0fd69e37).\n\nThe only significant changes from the pre-ca0fd69e37 implementation are:\n\n - that we can no longer inspect words up to the end of\n   reuse_packfile_bitmap->word_alloc, since we only want to look at\n   words whose bits all correspond to objects in the given packfile, and\n\n - that we return early when given a reuse_packfile which is not\n   preferred, making the call a noop.\n\nIn the future, it might be possible to restore this optimization if we\ncould guarantee that some reuse packs don't contain any gaps by\nconstruction (similar to the \"disjoint packs\" idea in very early\nversions of multi-pack reuse).\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c      | 101 +++++++++++++++---------------------\n t/t5332-multi-pack-reuse.sh |   2 +-\n 2 files changed, 44 insertions(+), 59 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 08007142671..f413344e90c 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1100,78 +1100,64 @@ static void write_reused_pack_one(struct packed_git *reuse_packfile,\n \n static size_t write_reused_pack_verbatim(struct bitmapped_pack *reuse_packfile,\n \t\t\t\t\t struct hashfile *out,\n-\t\t\t\t\t off_t pack_start,\n \t\t\t\t\t struct pack_window **w_curs)\n {\n-\tsize_t pos = reuse_packfile->bitmap_pos;\n+\tsize_t pos = 0;\n \tsize_t end;\n \n-\tif (pos % BITS_IN_EWORD) {\n-\t\tsize_t word_pos = (pos / BITS_IN_EWORD);\n-\t\tsize_t offset = pos % BITS_IN_EWORD;\n-\t\tsize_t last;\n-\t\teword_t word = reuse_packfile_bitmap->words[word_pos];\n-\n-\t\tif (offset + reuse_packfile->bitmap_nr < BITS_IN_EWORD)\n-\t\t\tlast = offset + reuse_packfile->bitmap_nr;\n-\t\telse\n-\t\t\tlast = BITS_IN_EWORD;\n-\n-\t\tfor (; offset < last; offset++) {\n-\t\t\tif (word >> offset == 0)\n-\t\t\t\treturn word_pos;\n-\t\t\tif (!bitmap_get(reuse_packfile_bitmap,\n-\t\t\t\t\tword_pos * BITS_IN_EWORD + offset))\n-\t\t\t\treturn word_pos;\n-\t\t}\n-\n-\t\tpos += BITS_IN_EWORD - (pos % BITS_IN_EWORD);\n-\t}\n-\n-\t/*\n-\t * Now we're going to copy as many whole eword_t's as possible.\n-\t * \"end\" is the index of the last whole eword_t we copy, but\n-\t * there may be additional bits to process. Those are handled\n-\t * individually by write_reused_pack().\n-\t *\n-\t * Begin by advancing to the first word boundary in range of the\n-\t * bit positions occupied by objects in \"reuse_packfile\". Then\n-\t * pick the last word boundary in the same range. If we have at\n-\t * least one word's worth of bits to process, continue on.\n-\t */\n-\tend = reuse_packfile->bitmap_pos + reuse_packfile->bitmap_nr;\n-\tif (end % BITS_IN_EWORD)\n-\t\tend -= end % BITS_IN_EWORD;\n-\tif (pos >= end)\n+\tif (reuse_packfile->bitmap_pos) {\n+\t\t/*\n+\t\t * We can't reuse whole chunks verbatim out of\n+\t\t * non-preferred packs since we can't guarantee that\n+\t\t * all duplicate objects were resolved in favor of\n+\t\t * that pack.\n+\t\t *\n+\t\t * Even if we have a whole eword_t worth of bits that\n+\t\t * could be reused, there may be objects between the\n+\t\t * objects corresponding to the first and last bit of\n+\t\t * that word which were selected from a different\n+\t\t * pack, causing us to send duplicate or unwanted\n+\t\t * objects.\n+\t\t *\n+\t\t * Handle non-preferred packs from within\n+\t\t * write_reused_pack(), which inspects and reuses\n+\t\t * individual bits.\n+\t\t */\n \t\treturn reuse_packfile->bitmap_pos / BITS_IN_EWORD;\n+\t}\n \n-\twhile (pos < end &&\n-\t       reuse_packfile_bitmap->words[pos / BITS_IN_EWORD] == (eword_t)~0)\n-\t\tpos += BITS_IN_EWORD;\n+\t/*\n+\t * Only read through the last word whose bits all correspond\n+\t * to objects in the given packfile, since we must stop at a\n+\t * word boundary.\n+\t *\n+\t * If there is no whole word to read (i.e. the packfile\n+\t * contains fewer than BITS_IN_EWORD objects), then we'll\n+\t * inspect bits one-by-one in write_reused_pack().\n+\t */\n+\tend = reuse_packfile->bitmap_nr / BITS_IN_EWORD;\n+\tif (reuse_packfile_bitmap->word_alloc < end)\n+\t\tBUG(\"fewer words than expected in reuse_packfile_bitmap\");\n \n-\tif (pos > end)\n-\t\tpos = end;\n+\twhile (pos < end && reuse_packfile_bitmap->words[pos] == (eword_t)~0)\n+\t\tpos++;\n \n-\tif (reuse_packfile->bitmap_pos < pos) {\n-\t\toff_t pack_start_off = pack_pos_to_offset(reuse_packfile->p, 0);\n-\t\toff_t pack_end_off = pack_pos_to_offset(reuse_packfile->p,\n-\t\t\t\t\t\t\tpos - reuse_packfile->bitmap_pos);\n+\tif (pos) {\n+\t\toff_t to_write;\n \n-\t\twritten += pos - reuse_packfile->bitmap_pos;\n+\t\twritten = (pos * BITS_IN_EWORD);\n+\t\tto_write = pack_pos_to_offset(reuse_packfile->p, written)\n+\t\t\t- sizeof(struct pack_header);\n \n \t\t/* We're recording one chunk, not one object. */\n-\t\trecord_reused_object(pack_start_off,\n-\t\t\t\t     pack_start_off - (hashfile_total(out) - pack_start));\n+\t\trecord_reused_object(sizeof(struct pack_header), 0);\n \t\thashflush(out);\n \t\tcopy_pack_data(out, reuse_packfile->p, w_curs,\n-\t\t\tpack_start_off, pack_end_off - pack_start_off);\n+\t\t\tsizeof(struct pack_header), to_write);\n \n \t\tdisplay_progress(progress_state, written);\n \t}\n-\tif (pos % BITS_IN_EWORD)\n-\t\tBUG(\"attempted to jump past a word boundary to %\"PRIuMAX,\n-\t\t    (uintmax_t)pos);\n-\treturn pos / BITS_IN_EWORD;\n+\treturn pos;\n }\n \n static void write_reused_pack(struct bitmapped_pack *reuse_packfile,\n@@ -1183,8 +1169,7 @@ static void write_reused_pack(struct bitmapped_pack *reuse_packfile,\n \tstruct pack_window *w_curs = NULL;\n \n \tif (allow_ofs_delta)\n-\t\ti = write_reused_pack_verbatim(reuse_packfile, f, pack_start,\n-\t\t\t\t\t       &w_curs);\n+\t\ti = write_reused_pack_verbatim(reuse_packfile, f, &w_curs);\n \n \tfor (; i < reuse_packfile_bitmap->word_alloc; ++i) {\n \t\teword_t word = reuse_packfile_bitmap->words[i];\ndiff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh\nindex 8f403d9fdaa..06836a4206c 100755\n--- a/t/t5332-multi-pack-reuse.sh\n+++ b/t/t5332-multi-pack-reuse.sh\n@@ -259,7 +259,7 @@ test_expect_success 'duplicate objects' '\n \t)\n '\n \n-test_expect_failure 'duplicate objects with verbatim reuse' '\n+test_expect_success 'duplicate objects with verbatim reuse' '\n \tgit init duplicate-objects-verbatim &&\n \t(\n \t\tcd duplicate-objects-verbatim &&\n-- \n2.46.0.421.g159f2d50e75\n"},{"id":"507240","messageId":"20241114002504.GB1140565@coredump.intra.peff.net","threadId":"62488","inReplyTo":"2520abf24a8a194b3f7040e218f878dc88a740a0.1731518931.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] pack-objects: only perform verbatim reuse on the preferred pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-14T00:25:04Z","receivedAt":"2024-11-14T00:25:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 13, 2024 at 12:32:58PM -0500, Taylor Blau wrote:\n\n> Instead, we can only safely perform the whole-word reuse optimization on\n> the preferred pack, where we know with certainty that no gaps exist in\n> that region of the bitmap. We can still reuse objects from non-preferred\n> packs, but we have to inspect them individually in write_reused_pack()\n> to ensure that any gaps that may exist are accounted for.\n\nYep. With the disclaimer that I'm biased because I helped a little with\ndebugging, this change is obviously correct. Forgetting the bug you saw\nin the real world, we know this function cannot work as-is because of\nthe potential for those gaps.\n\n> This allows us to simplify the implementation of\n> write_reused_pack_verbatim() back to almost its pre-multi-pack reuse\n> form, since we can now assume that the beginning of the pack appears at\n> the beginning of the bitmap, meaning that we don't have to account for\n> any bits up to the first word boundary (like we had to special case in\n> ca0fd69e37).\n> \n> The only significant changes from the pre-ca0fd69e37 implementation are:\n> [...]\n\nThanks for this section. My first instinct was to go back and look at\nthe diff to the pre-midx version of the function, and this nicely\nexplains the hunks I see there.\n\nSo this patch looks good to me. I was able to follow your explanation in\nthe commit message, but that may not count for much since I'm probably\nthe only other person with deep knowledge of the verbatim-reuse code in\nthe first place. ;)\n\nI do think the explanation in the message of the first commit would be a\nlot simpler if it were simply combined into this patch. With them split\nyou effectively have to explain the problem twice. I don't feel that\nstrongly about changing it, though.\n\n-Peff\n"},{"id":"507245","messageId":"xmqq8qtm8vps.fsf@gitster.g","threadId":"62488","inReplyTo":"7a69cf84ae5b92d99e5777d4600270712424c4d7.1731518931.git.me@ttaylorr.com","subject":"Re: [PATCH 1/2] t5332-multi-pack-reuse.sh: demonstrate duplicate packing failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-14T01:12:15Z","receivedAt":"2024-11-14T01:12: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> +test_expect_failure 'duplicate objects with verbatim reuse' '\n> +\tgit init duplicate-objects-verbatim &&\n> +\t(\n> +\t\tcd duplicate-objects-verbatim &&\n> +\n> +\t\tgit config pack.allowPackReuse multi &&\n> +\n> +\t\ttest_commit_bulk 64 &&\n> +\n> +\t\t# take the first object from the main pack...\n> +\t\tgit show-index <$(ls $packdir/pack-*.idx) >obj.raw &&\n> +\t\tsort -nk1 <obj.raw | head -n1 | cut -d\" \" -f2 >in &&\n> +\n> +\t\t# ...and create a separate pack containing just that object\n> +\t\tp=\"$(git pack-objects $packdir/pack <in)\" &&\n> +\t\tgit show-index <$packdir/pack-$p.idx &&\n\nIs this done so that \"git show-index\" fails when the .idx file fed\nis malformed?  Or is it a leftover debugging aid, where a human\ndeveloper was helped by eyeballing the contents of the .idx file in\nhuman readable form?  If the latter, do we perhaps want to \"parse\"\nthe output the same way in this test to validate our expectation?\n\n> +\t\tgit multi-pack-index write --bitmap --preferred-pack=pack-$p.idx &&\n> +\n> +\t\ttest_pack_objects_reused_all 192 2\n> +\t)\n> +'\n> +\n>  test_done\n"},{"id":"507268","messageId":"ZzX9BgmxTgi6Pk8z@nand.local","threadId":"62488","inReplyTo":"xmqq8qtm8vps.fsf@gitster.g","subject":"Re: [PATCH 1/2] t5332-multi-pack-reuse.sh: demonstrate duplicate packing failure","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-14T13:37:10Z","receivedAt":"2024-11-14T13:37:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 14, 2024 at 10:12:15AM +0900, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > +test_expect_failure 'duplicate objects with verbatim reuse' '\n> > +\tgit init duplicate-objects-verbatim &&\n> > +\t(\n> > +\t\tcd duplicate-objects-verbatim &&\n> > +\n> > +\t\tgit config pack.allowPackReuse multi &&\n> > +\n> > +\t\ttest_commit_bulk 64 &&\n> > +\n> > +\t\t# take the first object from the main pack...\n> > +\t\tgit show-index <$(ls $packdir/pack-*.idx) >obj.raw &&\n> > +\t\tsort -nk1 <obj.raw | head -n1 | cut -d\" \" -f2 >in &&\n> > +\n> > +\t\t# ...and create a separate pack containing just that object\n> > +\t\tp=\"$(git pack-objects $packdir/pack <in)\" &&\n> > +\t\tgit show-index <$packdir/pack-$p.idx &&\n>\n> Is this done so that \"git show-index\" fails when the .idx file fed\n> is malformed?  Or is it a leftover debugging aid, where a human\n> developer was helped by eyeballing the contents of the .idx file in\n> human readable form?  If the latter, do we perhaps want to \"parse\"\n> the output the same way in this test to validate our expectation?\n\nOops. This is stray debugging left over that I forgot to take out before\ncommitting. That makes it the latter of the two you mentioned, but I\ndon't think we need to validate the output of 'git show-index' in this\ninstance.\n\nWe're just relying on pack-objects to generate a pack containing a\nsingle object, which feels like basic functionality not worth explicitly\nmaking an assertion on.\n\nThere is a subtle assertion on the line below here:\n\n> > +\t\tgit multi-pack-index write --bitmap --preferred-pack=pack-$p.idx &&\n\nthat the pack (a) exists, and (b) has at least one object, since both\nconditions must be met for a pack to become preferred in a MIDX bitmap.\n\nBut beyond that, I don't think we need to validate the output of\nshow-index here. I'll remove the stray debugging line and send a new\nround. Sorry about that, and thanks for spotting!\n\nThanks,\nTaylor\n"},{"id":"507269","messageId":"ZzX9vT4GVqCyfFUE@nand.local","threadId":"62488","inReplyTo":"20241114002504.GB1140565@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] pack-objects: only perform verbatim reuse on the preferred pack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-14T13:40:13Z","receivedAt":"2024-11-14T13:40:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 13, 2024 at 07:25:04PM -0500, Jeff King wrote:\n> On Wed, Nov 13, 2024 at 12:32:58PM -0500, Taylor Blau wrote:\n>\n> > Instead, we can only safely perform the whole-word reuse optimization on\n> > the preferred pack, where we know with certainty that no gaps exist in\n> > that region of the bitmap. We can still reuse objects from non-preferred\n> > packs, but we have to inspect them individually in write_reused_pack()\n> > to ensure that any gaps that may exist are accounted for.\n>\n> Yep. With the disclaimer that I'm biased because I helped a little with\n> debugging, this change is obviously correct. Forgetting the bug you saw\n> in the real world, we know this function cannot work as-is because of\n> the potential for those gaps.\n\nYep, and thanks again for your help ;-).\n\n> > This allows us to simplify the implementation of\n> > write_reused_pack_verbatim() back to almost its pre-multi-pack reuse\n> > form, since we can now assume that the beginning of the pack appears at\n> > the beginning of the bitmap, meaning that we don't have to account for\n> > any bits up to the first word boundary (like we had to special case in\n> > ca0fd69e37).\n> >\n> > The only significant changes from the pre-ca0fd69e37 implementation are:\n> > [...]\n>\n> Thanks for this section. My first instinct was to go back and look at\n> the diff to the pre-midx version of the function, and this nicely\n> explains the hunks I see there.\n>\n> So this patch looks good to me. I was able to follow your explanation in\n> the commit message, but that may not count for much since I'm probably\n> the only other person with deep knowledge of the verbatim-reuse code in\n> the first place. ;)\n\nHeh.\n\n> I do think the explanation in the message of the first commit would be a\n> lot simpler if it were simply combined into this patch. With them split\n> you effectively have to explain the problem twice. I don't feel that\n> strongly about changing it, though.\n\nI always seem to go back and forth on that. I feel somewhat strongly\nthat for complicated regression fixes that we should demonstrate the\nexisting failure mode in a separate commit with a test_expect_failure.\nThat forces the author to ensure they really understand the bug and can\nproduce a minimal (or close to it) reproduction.\n\nIt also makes it easier to demonstrate that the fix actually does what\nit says, instead of assuming that the test fails without the fix applied\n(and passes with it applied).\n\nThat does force the author to potentially explain the bug twice. In my\nexperience, I tend to keep the explanation in the first patch relatively\nbrief, hinting at details that will appear in the subsequent patch\ninstead of explaining them in full detail.\n\nSo I dunno. It's a tradeoff for sure, but I think having an explicit\npoint in the log that demonstrates the existing bug is valuable.\n\nThanks,\nTaylor\n"},{"id":"507270","messageId":"cover.1731591708.git.me@ttaylorr.com","threadId":"62488","inReplyTo":"cover.1731518931.git.me@ttaylorr.com","subject":"[PATCH v2 0/2] pack-objects: more brown-paper-bag multi-pack reuse fixes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-14T13:42:05Z","receivedAt":"2024-11-14T13:42:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This is a small re-roll of my series to fix a regression in multi-pack\nreuse based on a broken assumption in write_reused_pack_verbatim().\n\nThe only change since last time is removing a stray debugging aid, and\nthe series is otherwise identical as in v1. A range-diff is included\nbelow for convenience.\n\nThanks in advance for your review!\n\nTaylor Blau (2):\n  t5332-multi-pack-reuse.sh: demonstrate duplicate packing failure\n  pack-objects: only perform verbatim reuse on the preferred pack\n\n builtin/pack-objects.c      | 101 +++++++++++++++---------------------\n t/t5332-multi-pack-reuse.sh |  22 ++++++++\n 2 files changed, 65 insertions(+), 58 deletions(-)\n\nRange-diff against v1:\n1:  7a69cf84ae5 ! 1:  d791b7b20c9 t5332-multi-pack-reuse.sh: demonstrate duplicate packing failure\n    @@ t/t5332-multi-pack-reuse.sh: test_expect_success 'duplicate objects' '\n     +\n     +\t\t# ...and create a separate pack containing just that object\n     +\t\tp=\"$(git pack-objects $packdir/pack <in)\" &&\n    -+\t\tgit show-index <$packdir/pack-$p.idx &&\n     +\n     +\t\tgit multi-pack-index write --bitmap --preferred-pack=pack-$p.idx &&\n     +\n2:  2520abf24a8 = 2:  d73b8fe2d63 pack-objects: only perform verbatim reuse on the preferred pack\n\nbase-commit: 25b0f41288718625b18495de23cc066394c09a92\n-- \n2.46.0.421.g159f2d50e75\n"},{"id":"507271","messageId":"d791b7b20c94d637e52bb645ff8f06ea25e4bd77.1731591708.git.me@ttaylorr.com","threadId":"62488","inReplyTo":"cover.1731591708.git.me@ttaylorr.com","subject":"[PATCH v2 1/2] t5332-multi-pack-reuse.sh: demonstrate duplicate packing failure","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-14T13:42:09Z","receivedAt":"2024-11-14T13:42:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the multi-pack reuse code, there are two paths for reusing the\non-disk representation of an object, handled by:\n\n  - builtin/pack-objects.c::write_reused_pack_one()\n  - builtin/pack-objects.c::write_reused_pack_verbatim()\n\nThe former is responsible for copying the bytes for a single object out\nof an existing source pack. The latter does the same but for a region of\nobjects aligned at eword_t boundaries.\n\nDemonstrate a bug whereby write_reused_pack_verbatim() can be tricked\ninto writing out objects from some source pack, even when those objects\nwere selected from a different source pack in the MIDX bitmap.\n\nWhen the caller wants at least one of the objects in that region,\npack-objects will write the same object twice as a result of this bug.\nIn the other case where the caller doesn't want any of the objects in\nthe region of interest, we will write out objects that weren't\nrequested.\n\nDemonstrate this bug by creating two packs, where the preferred one of\nthose packs contains a single object which also appears in the main\n(non-preferred) pack. A separate bug[^1] prevents us from triggering the\nmain bug when the duplicated object is the last one in the main pack,\nbut any earlier object will suffice.\n\nWe could fix that separate bug, but the following commit will simplify\nwrite_reused_pack_verbatim() and only call it on the preferred pack, so\ndoing so would have little point.\n\n[^1]: Because write_reused_pack_verbatim() only reuses bits in the range\n\n    off_t pack_start_off = pack_pos_to_offset(reuse_packfile->p, 0);\n    off_t pack_end_off = pack_pos_to_offset(reuse_packfile->p,\n                                            pos - reuse_packfile->bitmap_pos);\n\n    written += pos - reuse_packfile->bitmap_pos;\n\n    /* We're recording one chunk, not one object. */\n    record_reused_object(pack_start_off,\n                         pack_start_off - (hashfile_total(out) - pack_start));\n\n  , or in other words excluding the object beginning at position 'pos -\n  reuse_packfile->bitmap_pos' in the source pack. But since\n  reuse_packfile->bitmap_pos is '1' in the non-preferred pack\n  (accounting for the single-object pack which is preferred), we don't\n  actually copy the bytes from the last object.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5332-multi-pack-reuse.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh\nindex 955ea42769b..d87ea0ae19b 100755\n--- a/t/t5332-multi-pack-reuse.sh\n+++ b/t/t5332-multi-pack-reuse.sh\n@@ -259,4 +259,26 @@ test_expect_success 'duplicate objects' '\n \t)\n '\n \n+test_expect_failure 'duplicate objects with verbatim reuse' '\n+\tgit init duplicate-objects-verbatim &&\n+\t(\n+\t\tcd duplicate-objects-verbatim &&\n+\n+\t\tgit config pack.allowPackReuse multi &&\n+\n+\t\ttest_commit_bulk 64 &&\n+\n+\t\t# take the first object from the main pack...\n+\t\tgit show-index <$(ls $packdir/pack-*.idx) >obj.raw &&\n+\t\tsort -nk1 <obj.raw | head -n1 | cut -d\" \" -f2 >in &&\n+\n+\t\t# ...and create a separate pack containing just that object\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\ttest_pack_objects_reused_all 192 2\n+\t)\n+'\n+\n test_done\n-- \n2.46.0.421.g159f2d50e75\n\n"},{"id":"507272","messageId":"d73b8fe2d63c8ad08ad0c4ffeffb5ee2509b2d50.1731591708.git.me@ttaylorr.com","threadId":"62488","inReplyTo":"cover.1731591708.git.me@ttaylorr.com","subject":"[PATCH v2 2/2] pack-objects: only perform verbatim reuse on the preferred pack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-14T13:42:12Z","receivedAt":"2024-11-14T13:42:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When reusing objects from source pack(s), write_reused_pack_verbatim()\nis responsible for reusing objects whole eword_t's at a time. It works\nby taking the longest continuous run of objects from the beginning of\neach source pack that the caller wants, and reuses the entirety of that\nsection from each pack.\n\nThis is based on the assumption that we don't have any gaps within the\nregion. This assumption relieves us from having to patch any\nOFS_DELTAs, since we know that there aren't any gaps between any delta\nand its base in that region.\n\nTo illustrate why this assumption is necessary, suppose we have some\npack P, which has objects X, Y, and Z. If the MIDX's copy of Y was\nselected from a pack other than P, then the bit corresponding to object\nY will appear earlier in the bitmap than the bits corresponding to X and\nZ.\n\nIf pack-objects already has or will use the copy of Y from the pack it\nwas selected from in the MIDX, then it is an error to reuse all objects\nbetween X and Z in the source pack. Doing so will cause us to reuse Y\nfrom a different pack than the one which represents Y in the MIDX,\ncausing us to either:\n\n - include the object twice, assuming that the caller wants Y in the\n   pack, or\n\n - include the object once, resulting in us packing more objects than\n   necessary.\n\nThis regression comes from ca0fd69e37 (pack-objects: prepare\n`write_reused_pack_verbatim()` for multi-pack reuse, 2023-12-14), which\nincorrectly assumed that there would be no gaps in reusable regions of\nnon-preferred packs.\n\nInstead, we can only safely perform the whole-word reuse optimization on\nthe preferred pack, where we know with certainty that no gaps exist in\nthat region of the bitmap. We can still reuse objects from non-preferred\npacks, but we have to inspect them individually in write_reused_pack()\nto ensure that any gaps that may exist are accounted for.\n\nThis allows us to simplify the implementation of\nwrite_reused_pack_verbatim() back to almost its pre-multi-pack reuse\nform, since we can now assume that the beginning of the pack appears at\nthe beginning of the bitmap, meaning that we don't have to account for\nany bits up to the first word boundary (like we had to special case in\nca0fd69e37).\n\nThe only significant changes from the pre-ca0fd69e37 implementation are:\n\n - that we can no longer inspect words up to the end of\n   reuse_packfile_bitmap->word_alloc, since we only want to look at\n   words whose bits all correspond to objects in the given packfile, and\n\n - that we return early when given a reuse_packfile which is not\n   preferred, making the call a noop.\n\nIn the future, it might be possible to restore this optimization if we\ncould guarantee that some reuse packs don't contain any gaps by\nconstruction (similar to the \"disjoint packs\" idea in very early\nversions of multi-pack reuse).\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/pack-objects.c      | 101 +++++++++++++++---------------------\n t/t5332-multi-pack-reuse.sh |   2 +-\n 2 files changed, 44 insertions(+), 59 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 08007142671..f413344e90c 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1100,78 +1100,64 @@ static void write_reused_pack_one(struct packed_git *reuse_packfile,\n \n static size_t write_reused_pack_verbatim(struct bitmapped_pack *reuse_packfile,\n \t\t\t\t\t struct hashfile *out,\n-\t\t\t\t\t off_t pack_start,\n \t\t\t\t\t struct pack_window **w_curs)\n {\n-\tsize_t pos = reuse_packfile->bitmap_pos;\n+\tsize_t pos = 0;\n \tsize_t end;\n \n-\tif (pos % BITS_IN_EWORD) {\n-\t\tsize_t word_pos = (pos / BITS_IN_EWORD);\n-\t\tsize_t offset = pos % BITS_IN_EWORD;\n-\t\tsize_t last;\n-\t\teword_t word = reuse_packfile_bitmap->words[word_pos];\n-\n-\t\tif (offset + reuse_packfile->bitmap_nr < BITS_IN_EWORD)\n-\t\t\tlast = offset + reuse_packfile->bitmap_nr;\n-\t\telse\n-\t\t\tlast = BITS_IN_EWORD;\n-\n-\t\tfor (; offset < last; offset++) {\n-\t\t\tif (word >> offset == 0)\n-\t\t\t\treturn word_pos;\n-\t\t\tif (!bitmap_get(reuse_packfile_bitmap,\n-\t\t\t\t\tword_pos * BITS_IN_EWORD + offset))\n-\t\t\t\treturn word_pos;\n-\t\t}\n-\n-\t\tpos += BITS_IN_EWORD - (pos % BITS_IN_EWORD);\n-\t}\n-\n-\t/*\n-\t * Now we're going to copy as many whole eword_t's as possible.\n-\t * \"end\" is the index of the last whole eword_t we copy, but\n-\t * there may be additional bits to process. Those are handled\n-\t * individually by write_reused_pack().\n-\t *\n-\t * Begin by advancing to the first word boundary in range of the\n-\t * bit positions occupied by objects in \"reuse_packfile\". Then\n-\t * pick the last word boundary in the same range. If we have at\n-\t * least one word's worth of bits to process, continue on.\n-\t */\n-\tend = reuse_packfile->bitmap_pos + reuse_packfile->bitmap_nr;\n-\tif (end % BITS_IN_EWORD)\n-\t\tend -= end % BITS_IN_EWORD;\n-\tif (pos >= end)\n+\tif (reuse_packfile->bitmap_pos) {\n+\t\t/*\n+\t\t * We can't reuse whole chunks verbatim out of\n+\t\t * non-preferred packs since we can't guarantee that\n+\t\t * all duplicate objects were resolved in favor of\n+\t\t * that pack.\n+\t\t *\n+\t\t * Even if we have a whole eword_t worth of bits that\n+\t\t * could be reused, there may be objects between the\n+\t\t * objects corresponding to the first and last bit of\n+\t\t * that word which were selected from a different\n+\t\t * pack, causing us to send duplicate or unwanted\n+\t\t * objects.\n+\t\t *\n+\t\t * Handle non-preferred packs from within\n+\t\t * write_reused_pack(), which inspects and reuses\n+\t\t * individual bits.\n+\t\t */\n \t\treturn reuse_packfile->bitmap_pos / BITS_IN_EWORD;\n+\t}\n \n-\twhile (pos < end &&\n-\t       reuse_packfile_bitmap->words[pos / BITS_IN_EWORD] == (eword_t)~0)\n-\t\tpos += BITS_IN_EWORD;\n+\t/*\n+\t * Only read through the last word whose bits all correspond\n+\t * to objects in the given packfile, since we must stop at a\n+\t * word boundary.\n+\t *\n+\t * If there is no whole word to read (i.e. the packfile\n+\t * contains fewer than BITS_IN_EWORD objects), then we'll\n+\t * inspect bits one-by-one in write_reused_pack().\n+\t */\n+\tend = reuse_packfile->bitmap_nr / BITS_IN_EWORD;\n+\tif (reuse_packfile_bitmap->word_alloc < end)\n+\t\tBUG(\"fewer words than expected in reuse_packfile_bitmap\");\n \n-\tif (pos > end)\n-\t\tpos = end;\n+\twhile (pos < end && reuse_packfile_bitmap->words[pos] == (eword_t)~0)\n+\t\tpos++;\n \n-\tif (reuse_packfile->bitmap_pos < pos) {\n-\t\toff_t pack_start_off = pack_pos_to_offset(reuse_packfile->p, 0);\n-\t\toff_t pack_end_off = pack_pos_to_offset(reuse_packfile->p,\n-\t\t\t\t\t\t\tpos - reuse_packfile->bitmap_pos);\n+\tif (pos) {\n+\t\toff_t to_write;\n \n-\t\twritten += pos - reuse_packfile->bitmap_pos;\n+\t\twritten = (pos * BITS_IN_EWORD);\n+\t\tto_write = pack_pos_to_offset(reuse_packfile->p, written)\n+\t\t\t- sizeof(struct pack_header);\n \n \t\t/* We're recording one chunk, not one object. */\n-\t\trecord_reused_object(pack_start_off,\n-\t\t\t\t     pack_start_off - (hashfile_total(out) - pack_start));\n+\t\trecord_reused_object(sizeof(struct pack_header), 0);\n \t\thashflush(out);\n \t\tcopy_pack_data(out, reuse_packfile->p, w_curs,\n-\t\t\tpack_start_off, pack_end_off - pack_start_off);\n+\t\t\tsizeof(struct pack_header), to_write);\n \n \t\tdisplay_progress(progress_state, written);\n \t}\n-\tif (pos % BITS_IN_EWORD)\n-\t\tBUG(\"attempted to jump past a word boundary to %\"PRIuMAX,\n-\t\t    (uintmax_t)pos);\n-\treturn pos / BITS_IN_EWORD;\n+\treturn pos;\n }\n \n static void write_reused_pack(struct bitmapped_pack *reuse_packfile,\n@@ -1183,8 +1169,7 @@ static void write_reused_pack(struct bitmapped_pack *reuse_packfile,\n \tstruct pack_window *w_curs = NULL;\n \n \tif (allow_ofs_delta)\n-\t\ti = write_reused_pack_verbatim(reuse_packfile, f, pack_start,\n-\t\t\t\t\t       &w_curs);\n+\t\ti = write_reused_pack_verbatim(reuse_packfile, f, &w_curs);\n \n \tfor (; i < reuse_packfile_bitmap->word_alloc; ++i) {\n \t\teword_t word = reuse_packfile_bitmap->words[i];\ndiff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh\nindex d87ea0ae19b..e9d80186ec6 100755\n--- a/t/t5332-multi-pack-reuse.sh\n+++ b/t/t5332-multi-pack-reuse.sh\n@@ -259,7 +259,7 @@ test_expect_success 'duplicate objects' '\n \t)\n '\n \n-test_expect_failure 'duplicate objects with verbatim reuse' '\n+test_expect_success 'duplicate objects with verbatim reuse' '\n \tgit init duplicate-objects-verbatim &&\n \t(\n \t\tcd duplicate-objects-verbatim &&\n-- \n2.46.0.421.g159f2d50e75\n"},{"id":"507354","messageId":"20241115095746.GD1749331@coredump.intra.peff.net","threadId":"62488","inReplyTo":"ZzX9vT4GVqCyfFUE@nand.local","subject":"Re: [PATCH 2/2] pack-objects: only perform verbatim reuse on the preferred pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-15T09:57:46Z","receivedAt":"2024-11-15T09:57:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 14, 2024 at 08:40:13AM -0500, Taylor Blau wrote:\n\n> > I do think the explanation in the message of the first commit would be a\n> > lot simpler if it were simply combined into this patch. With them split\n> > you effectively have to explain the problem twice. I don't feel that\n> > strongly about changing it, though.\n> \n> I always seem to go back and forth on that. I feel somewhat strongly\n> that for complicated regression fixes that we should demonstrate the\n> existing failure mode in a separate commit with a test_expect_failure.\n> That forces the author to ensure they really understand the bug and can\n> produce a minimal (or close to it) reproduction.\n> \n> It also makes it easier to demonstrate that the fix actually does what\n> it says, instead of assuming that the test fails without the fix applied\n> (and passes with it applied).\n> \n> That does force the author to potentially explain the bug twice. In my\n> experience, I tend to keep the explanation in the first patch relatively\n> brief, hinting at details that will appear in the subsequent patch\n> instead of explaining them in full detail.\n> \n> So I dunno. It's a tradeoff for sure, but I think having an explicit\n> point in the log that demonstrates the existing bug is valuable.\n\nI do think it's important to make sure your tests fail in the way you\nexpect. I can't count the number of times I have _thought_ I had a\nsolution and then after seeing that the test didn't fail without my\npatch, had to go back and study it more. :)\n\nBut I generally do it with:\n\n  # build the old version\n  git checkout HEAD^\n  make\n\n  # now go back to the new and test _without_ building\n  git checkout -\n  cd t && ./t1234-whatever.sh\n\n(or more likely with a \"break\" in \"git rebase -i\" for a longer series).\n\nThat is, I'm running a mis-matched test/build combo. I guess in one\nsense that is horrible and I'm a bad person. It is easy to make a\nmistake and test the wrong thing. But it does make the resulting patches\neasier to read/review, IMHO.\n\n  As an aside, I think this kind of mismatch gets awkward with the\n  proposed meson out-of-tree build stuff. Running the tests through\n  meson would presumably trigger a build. And running without means\n  pointing to the built git binaries manually.\n\n-Peff\n"},{"id":"507884","messageId":"xmqqjzcvand6.fsf@gitster.g","threadId":"62488","inReplyTo":"d73b8fe2d63c8ad08ad0c4ffeffb5ee2509b2d50.1731591708.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/2] pack-objects: only perform verbatim reuse on the preferred pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-22T04:44:37Z","receivedAt":"2024-11-22T04:44:40Z","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 regression comes from ca0fd69e37 (pack-objects: prepare\n> `write_reused_pack_verbatim()` for multi-pack reuse, 2023-12-14), which\n> incorrectly assumed that there would be no gaps in reusable regions of\n> non-preferred packs.\n\nI was reviewing the past release notes from this year, and saw this\nsnippet in the 2.44 release notes:\n\n * Streaming spans of packfile data used to be done only from a\n   single, primary, pack in a repository with multiple packfiles.  It\n   has been extended to allow reuse from other packfiles, too.\n\nEssentially this two-patch series the change completely so we only\nreuse pack data from the primary pack?\n\nThanks.\n\n\n"},{"id":"507889","messageId":"20241122083350.GD1179306@coredump.intra.peff.net","threadId":"62488","inReplyTo":"xmqqjzcvand6.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] pack-objects: only perform verbatim reuse on the preferred pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-22T08:33:50Z","receivedAt":"2024-11-22T08:33:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 22, 2024 at 01:44:37PM +0900, Junio C Hamano wrote:\n\n> Taylor Blau <me@ttaylorr.com> writes:\n> \n> > This regression comes from ca0fd69e37 (pack-objects: prepare\n> > `write_reused_pack_verbatim()` for multi-pack reuse, 2023-12-14), which\n> > incorrectly assumed that there would be no gaps in reusable regions of\n> > non-preferred packs.\n> \n> I was reviewing the past release notes from this year, and saw this\n> snippet in the 2.44 release notes:\n> \n>  * Streaming spans of packfile data used to be done only from a\n>    single, primary, pack in a repository with multiple packfiles.  It\n>    has been extended to allow reuse from other packfiles, too.\n> \n> Essentially this two-patch series the change completely so we only\n> reuse pack data from the primary pack?\n\nNo. There are really three levels in increasing order of cost:\n\n  1. we directly copy out the bytes from the start of a pack completely,\n     up until the first object that is not needed to be sent\n\n  2. from there we process objects one by one, with a few checks and\n     adjustments to make sure they can be copied verbatim (like ofs\n     delta adjustment to account for earlier missing objects)\n\n  3. we add the objects to the big object_list struct, they become\n     candidates for delta compression, etc\n\nWe call the first two generically \"pack reuse\". And this two-patch\nseries is fixing a case where we were doing (1), but needed to do (2).\n\nSo I think the changelog entry still applies (there are other cases\nwhere we might jump from 1 to 2 as well, like if the output needs to\nconvert to REF_DELTA).\n\n-Peff\n"},{"id":"507891","messageId":"9aad5aad-707e-4dcf-aa8d-8b3c82724133@app.fastmail.com","threadId":"62488","inReplyTo":"ZzX9vT4GVqCyfFUE@nand.local","subject":"Re: [PATCH 2/2] pack-objects: only perform verbatim reuse on the preferred pack","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-11-22T09:16:47Z","receivedAt":"2024-11-22T09:17:09Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Thu, Nov 14, 2024, at 14:40, Taylor Blau wrote:\n> On Wed, Nov 13, 2024 at 07:25:04PM -0500, Jeff King wrote:\n>> I do think the explanation in the message of the first commit would be a\n>> lot simpler if it were simply combined into this patch. With them split\n>> you effectively have to explain the problem twice. I don't feel that\n>> strongly about changing it, though.\n>\n> I always seem to go back and forth on that. I feel somewhat strongly\n> that for complicated regression fixes that we should demonstrate the\n> existing failure mode in a separate commit with a test_expect_failure.\n> That forces the author to ensure they really understand the bug and can\n> produce a minimal (or close to it) reproduction.\n>\n> It also makes it easier to demonstrate that the fix actually does what\n> it says, instead of assuming that the test fails without the fix applied\n> (and passes with it applied).\n\nI recently made a parallel branch for a topic where the parallel branch had\n`test_expect_failure` for each commit (i.e. the commits had only `t/` changes).\nThat ended up catching a bug I introduced when I tried to simplify the test: the\ntest was OK on my topic branch but didn’t fail (`test_expect_failure`) on the\nparallel branch.\n\nI use a worktree for `master` so at least I didn’t have to build specifically\nfor that branch.\n\n-- \nKristoffer Haugsbakk\n"}]}