{"thread":{"id":"61298","subject":"[PATCH] pack-bitmap: gracefully handle missing BTMP chunks","startedAt":"2024-04-09T05:59:31Z","lastAt":"2024-04-16T05:14:06Z","messageCount":13,"participants":["Patrick Steinhardt","Taylor Blau","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"492621","messageId":"5933a302b581670183a6f3c881f62e96f61ff192.1712642313.git.ps@pks.im","threadId":"61298","inReplyTo":null,"subject":"[PATCH] pack-bitmap: gracefully handle missing BTMP chunks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-09T05:59:25Z","receivedAt":"2024-04-09T05:59:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 0fea6b73f1 (Merge branch 'tb/multi-pack-verbatim-reuse', 2024-01-12)\nwe have introduced multi-pack verbatim reuse of objects. This series has\nintroduced a new BTMP chunk, which encodes information about bitmapped\nobjects in the multi-pack index. Starting with dab60934e3 (pack-bitmap:\npass `bitmapped_pack` struct to pack-reuse functions, 2023-12-14) we use\nthis information to figure out objects which we can reuse from each of\nthe packfiles.\n\nOne thing that we glossed over though is backwards compatibility with\nrepositories that do not yet have BTMP chunks in their multi-pack index.\nIn that case, `nth_bitmapped_pack()` would return an error, which causes\nus to emit a warning followed by another error message. These warnings\nare visible to users that fetch from a repository:\n\n```\n$ git fetch\n...\nremote: error: MIDX does not contain the BTMP chunk\nremote: warning: unable to load pack: 'pack-f6bb7bd71d345ea9fe604b60cab9ba9ece54ffbe.idx', disabling pack-reuse\nremote: Enumerating objects: 40, done.\nremote: Counting objects: 100% (40/40), done.\nremote: Compressing objects: 100% (39/39), done.\nremote: Total 40 (delta 5), reused 0 (delta 0), pack-reused 0 (from 0)\n...\n```\n\nWhile the fetch succeeds the user is left wondering what they did wrong.\nFurthermore, as visible both from the warning and from the reuse stats,\npack-reuse is completely disabled in such repositories.\n\nWhat is quite interesting is that this issue can even be triggered in\ncase `pack.allowPackReuse=single` is set, which is the default value.\nOne could have expected that in this case we fall back to the old logic,\nwhich is to use the preferred packfile without consulting BTMP chunks at\nall. But either we fail with the above error in case they are missing,\nor we use the first pack in the multi-pack-index. The former case\ndisables pack-reuse altogether, whereas the latter case may result in\nreusing objects from a suboptimal packfile.\n\nFix this issue by partially reverting the logic back to what we had\nbefore this patch series landed. Namely, in the case where we have no\nBTMP chunks or when `pack.allowPackReuse=single` are set, we use the\npreferred pack instead of consulting the BTMP chunks.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n midx.c                        |  7 ++++---\n pack-bitmap.c                 | 36 ++++++++++++++++++-----------------\n t/t5326-multi-pack-bitmaps.sh | 22 +++++++++++++++++++++\n 3 files changed, 45 insertions(+), 20 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 41521e019c..6903e9dfd2 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1661,9 +1661,10 @@ static int write_midx_internal(const char *object_dir,\n \t\tadd_chunk(cf, MIDX_CHUNKID_REVINDEX,\n \t\t\t  st_mult(ctx.entries_nr, sizeof(uint32_t)),\n \t\t\t  write_midx_revindex);\n-\t\tadd_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n-\t\t\t  bitmapped_packs_concat_len,\n-\t\t\t  write_midx_bitmapped_packs);\n+\t\tif (git_env_bool(\"GIT_TEST_MIDX_WRITE_BTMP\", 1))\n+\t\t\tadd_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n+\t\t\t\t  bitmapped_packs_concat_len,\n+\t\t\t\t  write_midx_bitmapped_packs);\n \t}\n \n \twrite_midx_header(f, get_num_chunks(cf), ctx.nr - dropped_packs);\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 2baeabacee..f286805724 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -2049,7 +2049,25 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \n \tload_reverse_index(r, bitmap_git);\n \n-\tif (bitmap_is_midx(bitmap_git)) {\n+\tif (bitmap_is_midx(bitmap_git) &&\n+\t    (!multi_pack_reuse || !bitmap_git->midx->chunk_bitmapped_packs)) {\n+\t\tuint32_t preferred_pack_pos;\n+\t\tstruct packed_git *pack;\n+\n+\t\tif (midx_preferred_pack(bitmap_git->midx, &preferred_pack_pos) < 0) {\n+\t\t\twarning(_(\"unable to compute preferred pack, disabling pack-reuse\"));\n+\t\t\treturn;\n+\t\t}\n+\n+\t\tpack = bitmap_git->midx->packs[preferred_pack_pos];\n+\n+\t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n+\t\tpacks[packs_nr].p = pack;\n+\t\tpacks[packs_nr].bitmap_nr = pack->num_objects;\n+\t\tpacks[packs_nr].bitmap_pos = 0;\n+\n+\t\tobjects_nr = packs[packs_nr++].bitmap_nr;\n+\t} else if (bitmap_is_midx(bitmap_git)) {\n \t\tfor (i = 0; i < bitmap_git->midx->num_packs; i++) {\n \t\t\tstruct bitmapped_pack pack;\n \t\t\tif (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {\n@@ -2062,26 +2080,10 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \t\t\tif (!pack.bitmap_nr)\n \t\t\t\tcontinue;\n \n-\t\t\tif (!multi_pack_reuse && pack.bitmap_pos) {\n-\t\t\t\t/*\n-\t\t\t\t * If we're only reusing a single pack, skip\n-\t\t\t\t * over any packs which are not positioned at\n-\t\t\t\t * the beginning of the MIDX bitmap.\n-\t\t\t\t *\n-\t\t\t\t * This is consistent with the existing\n-\t\t\t\t * single-pack reuse behavior, which only reuses\n-\t\t\t\t * parts of the MIDX's preferred pack.\n-\t\t\t\t */\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\n \t\t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n \t\t\tmemcpy(&packs[packs_nr++], &pack, sizeof(pack));\n \n \t\t\tobjects_nr += pack.p->num_objects;\n-\n-\t\t\tif (!multi_pack_reuse)\n-\t\t\t\tbreak;\n \t\t}\n \n \t\tQSORT(packs, packs_nr, bitmapped_pack_cmp);\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 70d1b58709..ee3843b239 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -513,4 +513,26 @@ test_expect_success 'corrupt MIDX with bitmap causes fallback' '\n \t)\n '\n \n+for allow_pack_reuse in single multi\n+do\n+\ttest_expect_success \"MIDX without BTMP chunk does not complain with $allow_pack_reuse pack reuse\" '\n+\t\ttest_when_finished \"rm -rf midx-without-btmp\" &&\n+\t\tgit init midx-without-btmp &&\n+\t\t(\n+\t\t\tcd midx-without-btmp &&\n+\t\t\ttest_commit initial &&\n+\n+\t\t\t# Write a multi-pack index that does have a bitmap, but\n+\t\t\t# no BTMP chunk. Such MIDX files would not be generated\n+\t\t\t# by modern Git anymore, but they were generated by\n+\t\t\t# older Git versions.\n+\t\t\tGIT_TEST_MIDX_WRITE_BTMP=false \\\n+\t\t\t\tgit repack -Adbl --write-bitmap-index --write-midx &&\n+\t\t\tgit -c pack.allowPackReuse=$allow_pack_reuse \\\n+\t\t\t\tpack-objects --all --use-bitmap-index --stdout </dev/null >/dev/null 2>err &&\n+\t\t\ttest_must_be_empty err\n+\t\t)\n+\t'\n+done\n+\n test_done\n-- \n2.44.GIT\n\n"},{"id":"492715","messageId":"Zhap8iyMYytCM+on@nand.local","threadId":"61298","inReplyTo":"5933a302b581670183a6f3c881f62e96f61ff192.1712642313.git.ps@pks.im","subject":"Re: [PATCH] pack-bitmap: gracefully handle missing BTMP chunks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-04-10T15:02:10Z","receivedAt":"2024-04-10T15:02:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 09, 2024 at 07:59:25AM +0200, Patrick Steinhardt wrote:\n> One thing that we glossed over though is backwards compatibility with\n> repositories that do not yet have BTMP chunks in their multi-pack index.\n> In that case, `nth_bitmapped_pack()` would return an error, which causes\n> us to emit a warning followed by another error message. These warnings\n> are visible to users that fetch from a repository:\n>\n> ```\n> $ git fetch\n> ...\n> remote: error: MIDX does not contain the BTMP chunk\n> remote: warning: unable to load pack: 'pack-f6bb7bd71d345ea9fe604b60cab9ba9ece54ffbe.idx', disabling pack-reuse\n> remote: Enumerating objects: 40, done.\n> remote: Counting objects: 100% (40/40), done.\n> remote: Compressing objects: 100% (39/39), done.\n> remote: Total 40 (delta 5), reused 0 (delta 0), pack-reused 0 (from 0)\n> ...\n> ```\n\nNice catch. This is definitely an oversight from my original series,\nwhich should not disrupt single-pack reuse from the MIDX's preferred\npack when using a MIDX/bitmap written prior to the introduction of the\nBTMP chunk.\n\n> Fix this issue by partially reverting the logic back to what we had\n> before this patch series landed. Namely, in the case where we have no\n> BTMP chunks or when `pack.allowPackReuse=single` are set, we use the\n> preferred pack instead of consulting the BTMP chunks.\n\nI think that the approach here makes sense to me. The gist is:\n\n- If we don't have a BTMP chunk, then only reuse objects from the\n  preferred pack.\n\n- Otherwise, we do have a BTMP chunk (or we're not doing multi-pack\n  reuse). If we are reusing objects from multiple packs, then do the\n  usual MIDX reuse routines.\n\n- Otherwise, we're doing single-pack reuse. In that case, only reuse\n  objects from the preferred pack.\n\n> diff --git a/midx.c b/midx.c\n> index 41521e019c..6903e9dfd2 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -1661,9 +1661,10 @@ static int write_midx_internal(const char *object_dir,\n>  \t\tadd_chunk(cf, MIDX_CHUNKID_REVINDEX,\n>  \t\t\t  st_mult(ctx.entries_nr, sizeof(uint32_t)),\n>  \t\t\t  write_midx_revindex);\n> -\t\tadd_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n> -\t\t\t  bitmapped_packs_concat_len,\n> -\t\t\t  write_midx_bitmapped_packs);\n> +\t\tif (git_env_bool(\"GIT_TEST_MIDX_WRITE_BTMP\", 1))\n> +\t\t\tadd_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n> +\t\t\t\t  bitmapped_packs_concat_len,\n> +\t\t\t\t  write_midx_bitmapped_packs);\n\nI wish that this were possible to exercise without a new\nGIT_TEST_-variable. I think there are a couple of alternatives:\n\nYou could introduce a new GIT_TEST_MIDX_READ_BTMP variable, and then set\nthat to control whether or not we read the BTMP chunk. This is what we\ndid in:\n\n  - 28cd730680d (pack-bitmap: prepare to read lookup table extension,\n    2022-08-14), as well as in\n\n  - 7f514b7a5e7 (midx: read `RIDX` chunk when present, 2022-01-25)\n\n. I have a vague preference towards controlling whether or not we read\nthe BTMP chunk (as opposed to whether or not we write it) as this\nremoves a potential footgun for users who might accidentally disable\nwriting a BTMP chunk (in which case you have to rewrite the whole MIDX)\nas opposed to reading it (in which case you just change your environment\nvariable).\n\nOf course, that is still using a GIT_TEST_-variable, which is less than\nideal IMHO. The other alternative would be to store a MIDX file as a\ntest fixture in the tree (which we do in a couple of places). But with\nthe recent xz shenanigans, I'm not sure that's a great idea either ;-).\n\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index 2baeabacee..f286805724 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -2049,7 +2049,25 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n>\n>  \tload_reverse_index(r, bitmap_git);\n>\n> -\tif (bitmap_is_midx(bitmap_git)) {\n> +\tif (bitmap_is_midx(bitmap_git) &&\n> +\t    (!multi_pack_reuse || !bitmap_git->midx->chunk_bitmapped_packs)) {\n> +\t\tuint32_t preferred_pack_pos;\n> +\t\tstruct packed_git *pack;\n> +\n> +\t\tif (midx_preferred_pack(bitmap_git->midx, &preferred_pack_pos) < 0) {\n> +\t\t\twarning(_(\"unable to compute preferred pack, disabling pack-reuse\"));\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\tpack = bitmap_git->midx->packs[preferred_pack_pos];\n> +\n> +\t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n> +\t\tpacks[packs_nr].p = pack;\n> +\t\tpacks[packs_nr].bitmap_nr = pack->num_objects;\n> +\t\tpacks[packs_nr].bitmap_pos = 0;\n> +\n> +\t\tobjects_nr = packs[packs_nr++].bitmap_nr;\n> +\t} else if (bitmap_is_midx(bitmap_git)) {\n>  \t\tfor (i = 0; i < bitmap_git->midx->num_packs; i++) {\n>  \t\t\tstruct bitmapped_pack pack;\n>  \t\t\tif (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {\n\nThis all makes sense to me. I think we could make the result slightly\nmore readable by handling the case where we're doing multi-pack reuse\nseparately from the case where we're not.\n\nI tried to make that change locally to see if it was a good idea, and\nI'm reasonably happy with the result. I can't think of a great way to\ntalk about it without just showing the resulting patch (as the\ninter-diff is fairly difficult to read IMHO). So here is the resulting\npatch (forging your s-o-b):\n\n--- 8< ---\nSubject: [PATCH] pack-bitmap: gracefully handle missing BTMP chunks\n\nIn 0fea6b73f1 (Merge branch 'tb/multi-pack-verbatim-reuse', 2024-01-12)\nwe have introduced multi-pack verbatim reuse of objects. This series has\nintroduced a new BTMP chunk, which encodes information about bitmapped\nobjects in the multi-pack index. Starting with dab60934e3 (pack-bitmap:\npass `bitmapped_pack` struct to pack-reuse functions, 2023-12-14) we use\nthis information to figure out objects which we can reuse from each of\nthe packfiles.\n\nOne thing that we glossed over though is backwards compatibility with\nrepositories that do not yet have BTMP chunks in their multi-pack index.\nIn that case, `nth_bitmapped_pack()` would return an error, which causes\nus to emit a warning followed by another error message. These warnings\nare visible to users that fetch from a repository:\n\n```\n$ git fetch\n...\nremote: error: MIDX does not contain the BTMP chunk\nremote: warning: unable to load pack: 'pack-f6bb7bd71d345ea9fe604b60cab9ba9ece54ffbe.idx', disabling pack-reuse\nremote: Enumerating objects: 40, done.\nremote: Counting objects: 100% (40/40), done.\nremote: Compressing objects: 100% (39/39), done.\nremote: Total 40 (delta 5), reused 0 (delta 0), pack-reused 0 (from 0)\n...\n```\n\nWhile the fetch succeeds the user is left wondering what they did wrong.\nFurthermore, as visible both from the warning and from the reuse stats,\npack-reuse is completely disabled in such repositories.\n\nWhat is quite interesting is that this issue can even be triggered in\ncase `pack.allowPackReuse=single` is set, which is the default value.\nOne could have expected that in this case we fall back to the old logic,\nwhich is to use the preferred packfile without consulting BTMP chunks at\nall. But either we fail with the above error in case they are missing,\nor we use the first pack in the multi-pack-index. The former case\ndisables pack-reuse altogether, whereas the latter case may result in\nreusing objects from a suboptimal packfile.\n\nFix this issue by partially reverting the logic back to what we had\nbefore this patch series landed. Namely, in the case where we have no\nBTMP chunks or when `pack.allowPackReuse=single` are set, we use the\npreferred pack instead of consulting the BTMP chunks.\n\nHelped-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c                        |  7 +++---\n pack-bitmap.c                 | 44 +++++++++++++++++++----------------\n t/t5326-multi-pack-bitmaps.sh | 22 ++++++++++++++++++\n 3 files changed, 50 insertions(+), 23 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 41521e019c6..6903e9dfd25 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1661,9 +1661,10 @@ static int write_midx_internal(const char *object_dir,\n \t\tadd_chunk(cf, MIDX_CHUNKID_REVINDEX,\n \t\t\t  st_mult(ctx.entries_nr, sizeof(uint32_t)),\n \t\t\t  write_midx_revindex);\n-\t\tadd_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n-\t\t\t  bitmapped_packs_concat_len,\n-\t\t\t  write_midx_bitmapped_packs);\n+\t\tif (git_env_bool(\"GIT_TEST_MIDX_WRITE_BTMP\", 1))\n+\t\t\tadd_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n+\t\t\t\t  bitmapped_packs_concat_len,\n+\t\t\t\t  write_midx_bitmapped_packs);\n \t}\n\n \twrite_midx_header(f, get_num_chunks(cf), ctx.nr - dropped_packs);\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 2baeabacee1..44b32ee3561 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -2049,7 +2049,14 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n\n \tload_reverse_index(r, bitmap_git);\n\n-\tif (bitmap_is_midx(bitmap_git)) {\n+\tif (!bitmap_is_midx(bitmap_git) ||\n+\t    !bitmap_git->midx->chunk_bitmapped_packs)\n+\t\tmulti_pack_reuse = 0;\n+\n+\tif (multi_pack_reuse) {\n+\t\tif (!bitmap_is_midx(bitmap_git))\n+\t\t\tBUG(\"attempting to perform multi-pack reuse on non-MIDX bitmap\");\n+\n \t\tfor (i = 0; i < bitmap_git->midx->num_packs; i++) {\n \t\t\tstruct bitmapped_pack pack;\n \t\t\tif (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {\n@@ -2062,36 +2069,33 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \t\t\tif (!pack.bitmap_nr)\n \t\t\t\tcontinue;\n\n-\t\t\tif (!multi_pack_reuse && pack.bitmap_pos) {\n-\t\t\t\t/*\n-\t\t\t\t * If we're only reusing a single pack, skip\n-\t\t\t\t * over any packs which are not positioned at\n-\t\t\t\t * the beginning of the MIDX bitmap.\n-\t\t\t\t *\n-\t\t\t\t * This is consistent with the existing\n-\t\t\t\t * single-pack reuse behavior, which only reuses\n-\t\t\t\t * parts of the MIDX's preferred pack.\n-\t\t\t\t */\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\n \t\t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n \t\t\tmemcpy(&packs[packs_nr++], &pack, sizeof(pack));\n\n \t\t\tobjects_nr += pack.p->num_objects;\n-\n-\t\t\tif (!multi_pack_reuse)\n-\t\t\t\tbreak;\n \t\t}\n\n \t\tQSORT(packs, packs_nr, bitmapped_pack_cmp);\n \t} else {\n+\t\tstruct packed_git *pack;\n+\t\tif (bitmap_is_midx(bitmap_git)) {\n+\t\t\tuint32_t pack_int_id;\n+\t\t\tif (midx_preferred_pack(bitmap_git->midx, &pack_int_id) < 0) {\n+\t\t\t\twarning(_(\"unable to compute preferred pack, \"\n+\t\t\t\t\t  \"disabling pack-reuse\"));\n+\t\t\t\treturn;\n+\t\t\t}\n+\n+\t\t\tpack = bitmap_git->midx->packs[pack_int_id];\n+\t\t} else {\n+\t\t\tpack = bitmap_git->pack;\n+\t\t}\n+\n \t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n\n-\t\tpacks[packs_nr].p = bitmap_git->pack;\n-\t\tpacks[packs_nr].bitmap_nr = bitmap_git->pack->num_objects;\n+\t\tpacks[packs_nr].p = pack;\n+\t\tpacks[packs_nr].bitmap_nr = pack->num_objects;\n \t\tpacks[packs_nr].bitmap_pos = 0;\n-\n \t\tobjects_nr = packs[packs_nr++].bitmap_nr;\n \t}\n\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 70d1b58709a..ee3843b2390 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -513,4 +513,26 @@ test_expect_success 'corrupt MIDX with bitmap causes fallback' '\n \t)\n '\n\n+for allow_pack_reuse in single multi\n+do\n+\ttest_expect_success \"MIDX without BTMP chunk does not complain with $allow_pack_reuse pack reuse\" '\n+\t\ttest_when_finished \"rm -rf midx-without-btmp\" &&\n+\t\tgit init midx-without-btmp &&\n+\t\t(\n+\t\t\tcd midx-without-btmp &&\n+\t\t\ttest_commit initial &&\n+\n+\t\t\t# Write a multi-pack index that does have a bitmap, but\n+\t\t\t# no BTMP chunk. Such MIDX files would not be generated\n+\t\t\t# by modern Git anymore, but they were generated by\n+\t\t\t# older Git versions.\n+\t\t\tGIT_TEST_MIDX_WRITE_BTMP=false \\\n+\t\t\t\tgit repack -Adbl --write-bitmap-index --write-midx &&\n+\t\t\tgit -c pack.allowPackReuse=$allow_pack_reuse \\\n+\t\t\t\tpack-objects --all --use-bitmap-index --stdout </dev/null >/dev/null 2>err &&\n+\t\t\ttest_must_be_empty err\n+\t\t)\n+\t'\n+done\n+\n test_done\n\nbase-commit: 91ec36f2cca02d33ab0ed6e87195c6fe801debae\n--\n2.44.0.549.g74a2f60dcb0\n--- >8 ---\n\nThe way I would structure this series is to first apply the portion of\nthe above patch *without* these lines:\n\n-\tif (bitmap_is_midx(bitmap_git)) {\n+\tif (!bitmap_is_midx(bitmap_git) ||\n+\t    !bitmap_git->midx->chunk_bitmapped_packs)\n+\t\tmulti_pack_reuse = 0;\n+\n\n, so we're still able to reproduce the issue. Then, apply the remaining\nportions (the above diff, the test, and the GIT_TEST_MIDX_WRITE_BTMP\nstuff) to demonstrate that the issue is fixed via a separate commit.\n\nI'm happy to write that up, and equally happy to not do it ;-). Sorry\nfor the lengthy review, but thank you very much for spotting and fixing\nthis issue.\n\nThanks,\nTaylor\n"},{"id":"492927","messageId":"ZhzKeR9C4UVpEm29@tanuki","threadId":"61298","inReplyTo":"Zhap8iyMYytCM+on@nand.local","subject":"Re: [PATCH] pack-bitmap: gracefully handle missing BTMP chunks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-15T06:34:33Z","receivedAt":"2024-04-15T06:34:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 10, 2024 at 11:02:10AM -0400, Taylor Blau wrote:\n> On Tue, Apr 09, 2024 at 07:59:25AM +0200, Patrick Steinhardt wrote:\n[snip]\n> > diff --git a/midx.c b/midx.c\n> > index 41521e019c..6903e9dfd2 100644\n> > --- a/midx.c\n> > +++ b/midx.c\n> > @@ -1661,9 +1661,10 @@ static int write_midx_internal(const char *object_dir,\n> >  \t\tadd_chunk(cf, MIDX_CHUNKID_REVINDEX,\n> >  \t\t\t  st_mult(ctx.entries_nr, sizeof(uint32_t)),\n> >  \t\t\t  write_midx_revindex);\n> > -\t\tadd_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n> > -\t\t\t  bitmapped_packs_concat_len,\n> > -\t\t\t  write_midx_bitmapped_packs);\n> > +\t\tif (git_env_bool(\"GIT_TEST_MIDX_WRITE_BTMP\", 1))\n> > +\t\t\tadd_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n> > +\t\t\t\t  bitmapped_packs_concat_len,\n> > +\t\t\t\t  write_midx_bitmapped_packs);\n> \n> I wish that this were possible to exercise without a new\n> GIT_TEST_-variable. I think there are a couple of alternatives:\n> \n> You could introduce a new GIT_TEST_MIDX_READ_BTMP variable, and then set\n> that to control whether or not we read the BTMP chunk. This is what we\n> did in:\n> \n>   - 28cd730680d (pack-bitmap: prepare to read lookup table extension,\n>     2022-08-14), as well as in\n> \n>   - 7f514b7a5e7 (midx: read `RIDX` chunk when present, 2022-01-25)\n> \n> . I have a vague preference towards controlling whether or not we read\n> the BTMP chunk (as opposed to whether or not we write it) as this\n> removes a potential footgun for users who might accidentally disable\n> writing a BTMP chunk (in which case you have to rewrite the whole MIDX)\n> as opposed to reading it (in which case you just change your environment\n> variable).\n> \n> Of course, that is still using a GIT_TEST_-variable, which is less than\n> ideal IMHO. The other alternative would be to store a MIDX file as a\n> test fixture in the tree (which we do in a couple of places). But with\n> the recent xz shenanigans, I'm not sure that's a great idea either ;-).\n\nI'm happy to convert this to use `GIT_TEST_MIDX_READ_BTMP` instead.\n\n> > diff --git a/pack-bitmap.c b/pack-bitmap.c\n> > index 2baeabacee..f286805724 100644\n> > --- a/pack-bitmap.c\n> > +++ b/pack-bitmap.c\n> > @@ -2049,7 +2049,25 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n> >\n> >  \tload_reverse_index(r, bitmap_git);\n> >\n> > -\tif (bitmap_is_midx(bitmap_git)) {\n> > +\tif (bitmap_is_midx(bitmap_git) &&\n> > +\t    (!multi_pack_reuse || !bitmap_git->midx->chunk_bitmapped_packs)) {\n> > +\t\tuint32_t preferred_pack_pos;\n> > +\t\tstruct packed_git *pack;\n> > +\n> > +\t\tif (midx_preferred_pack(bitmap_git->midx, &preferred_pack_pos) < 0) {\n> > +\t\t\twarning(_(\"unable to compute preferred pack, disabling pack-reuse\"));\n> > +\t\t\treturn;\n> > +\t\t}\n> > +\n> > +\t\tpack = bitmap_git->midx->packs[preferred_pack_pos];\n> > +\n> > +\t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n> > +\t\tpacks[packs_nr].p = pack;\n> > +\t\tpacks[packs_nr].bitmap_nr = pack->num_objects;\n> > +\t\tpacks[packs_nr].bitmap_pos = 0;\n> > +\n> > +\t\tobjects_nr = packs[packs_nr++].bitmap_nr;\n> > +\t} else if (bitmap_is_midx(bitmap_git)) {\n> >  \t\tfor (i = 0; i < bitmap_git->midx->num_packs; i++) {\n> >  \t\t\tstruct bitmapped_pack pack;\n> >  \t\t\tif (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {\n> \n> This all makes sense to me. I think we could make the result slightly\n> more readable by handling the case where we're doing multi-pack reuse\n> separately from the case where we're not.\n> \n> I tried to make that change locally to see if it was a good idea, and\n> I'm reasonably happy with the result. I can't think of a great way to\n> talk about it without just showing the resulting patch (as the\n> inter-diff is fairly difficult to read IMHO). So here is the resulting\n> patch (forging your s-o-b):\n\nYup, the result indeed looks nicer, thanks!\n\n[snip]\n> The way I would structure this series is to first apply the portion of\n> the above patch *without* these lines:\n> \n> -\tif (bitmap_is_midx(bitmap_git)) {\n> +\tif (!bitmap_is_midx(bitmap_git) ||\n> +\t    !bitmap_git->midx->chunk_bitmapped_packs)\n> +\t\tmulti_pack_reuse = 0;\n> +\n> \n> , so we're still able to reproduce the issue. Then, apply the remaining\n> portions (the above diff, the test, and the GIT_TEST_MIDX_WRITE_BTMP\n> stuff) to demonstrate that the issue is fixed via a separate commit.\n> \n> I'm happy to write that up, and equally happy to not do it ;-). Sorry\n> for the lengthy review, but thank you very much for spotting and fixing\n> this issue.\n\nI'd prefer to leave it as a single patch. Junio has expressed at times\nthat he doesn't see much value in these splits only to demonstrate a\nbroken test. An interested reader can easily revert the fix and see that\nthe test would fail.\n\nPatrick\n"},{"id":"492928","messageId":"a8251f8278ba9a3b41a8e299cb4918a62df6d1c7.1713163238.git.ps@pks.im","threadId":"61298","inReplyTo":"5933a302b581670183a6f3c881f62e96f61ff192.1712642313.git.ps@pks.im","subject":"[PATCH v2] pack-bitmap: gracefully handle missing BTMP chunks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-15T06:41:25Z","receivedAt":"2024-04-15T06:41:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 0fea6b73f1 (Merge branch 'tb/multi-pack-verbatim-reuse', 2024-01-12)\nwe have introduced multi-pack verbatim reuse of objects. This series has\nintroduced a new BTMP chunk, which encodes information about bitmapped\nobjects in the multi-pack index. Starting with dab60934e3 (pack-bitmap:\npass `bitmapped_pack` struct to pack-reuse functions, 2023-12-14) we use\nthis information to figure out objects which we can reuse from each of\nthe packfiles.\n\nOne thing that we glossed over though is backwards compatibility with\nrepositories that do not yet have BTMP chunks in their multi-pack index.\nIn that case, `nth_bitmapped_pack()` would return an error, which causes\nus to emit a warning followed by another error message. These warnings\nare visible to users that fetch from a repository:\n\n```\n$ git fetch\n...\nremote: error: MIDX does not contain the BTMP chunk\nremote: warning: unable to load pack: 'pack-f6bb7bd71d345ea9fe604b60cab9ba9ece54ffbe.idx', disabling pack-reuse\nremote: Enumerating objects: 40, done.\nremote: Counting objects: 100% (40/40), done.\nremote: Compressing objects: 100% (39/39), done.\nremote: Total 40 (delta 5), reused 0 (delta 0), pack-reused 0 (from 0)\n...\n```\n\nWhile the fetch succeeds the user is left wondering what they did wrong.\nFurthermore, as visible both from the warning and from the reuse stats,\npack-reuse is completely disabled in such repositories.\n\nWhat is quite interesting is that this issue can even be triggered in\ncase `pack.allowPackReuse=single` is set, which is the default value.\nOne could have expected that in this case we fall back to the old logic,\nwhich is to use the preferred packfile without consulting BTMP chunks at\nall. But either we fail with the above error in case they are missing,\nor we use the first pack in the multi-pack-index. The former case\ndisables pack-reuse altogether, whereas the latter case may result in\nreusing objects from a suboptimal packfile.\n\nFix this issue by partially reverting the logic back to what we had\nbefore this patch series landed. Namely, in the case where we have no\nBTMP chunks or when `pack.allowPackReuse=single` are set, we use the\npreferred pack instead of consulting the BTMP chunks.\n\nHelped-by: Taylor Blau <me@ttaylorr.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nRange-diff against v1:\n1:  5933a302b5 ! 1:  a8251f8278 pack-bitmap: gracefully handle missing BTMP chunks\n    @@ Commit message\n         BTMP chunks or when `pack.allowPackReuse=single` are set, we use the\n         preferred pack instead of consulting the BTMP chunks.\n     \n    +    Helped-by: Taylor Blau <me@ttaylorr.com>\n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n     \n      ## midx.c ##\n    -@@ midx.c: static int write_midx_internal(const char *object_dir,\n    - \t\tadd_chunk(cf, MIDX_CHUNKID_REVINDEX,\n    - \t\t\t  st_mult(ctx.entries_nr, sizeof(uint32_t)),\n    - \t\t\t  write_midx_revindex);\n    --\t\tadd_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n    --\t\t\t  bitmapped_packs_concat_len,\n    --\t\t\t  write_midx_bitmapped_packs);\n    -+\t\tif (git_env_bool(\"GIT_TEST_MIDX_WRITE_BTMP\", 1))\n    -+\t\t\tadd_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n    -+\t\t\t\t  bitmapped_packs_concat_len,\n    -+\t\t\t\t  write_midx_bitmapped_packs);\n    - \t}\n    +@@ midx.c: struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n      \n    - \twrite_midx_header(f, get_num_chunks(cf), ctx.nr - dropped_packs);\n    + \tpair_chunk(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets,\n    + \t\t   &m->chunk_large_offsets_len);\n    +-\tpair_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n    +-\t\t   (const unsigned char **)&m->chunk_bitmapped_packs,\n    +-\t\t   &m->chunk_bitmapped_packs_len);\n    ++\tif (git_env_bool(\"GIT_TEST_MIDX_READ_BTMP\", 1))\n    ++\t\tpair_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n    ++\t\t\t   (const unsigned char **)&m->chunk_bitmapped_packs,\n    ++\t\t\t   &m->chunk_bitmapped_packs_len);\n    + \n    + \tif (git_env_bool(\"GIT_TEST_MIDX_READ_RIDX\", 1))\n    + \t\tpair_chunk(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex,\n     \n      ## pack-bitmap.c ##\n     @@ pack-bitmap.c: void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n    @@ pack-bitmap.c: void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitm\n      \tload_reverse_index(r, bitmap_git);\n      \n     -\tif (bitmap_is_midx(bitmap_git)) {\n    -+\tif (bitmap_is_midx(bitmap_git) &&\n    -+\t    (!multi_pack_reuse || !bitmap_git->midx->chunk_bitmapped_packs)) {\n    -+\t\tuint32_t preferred_pack_pos;\n    -+\t\tstruct packed_git *pack;\n    -+\n    -+\t\tif (midx_preferred_pack(bitmap_git->midx, &preferred_pack_pos) < 0) {\n    -+\t\t\twarning(_(\"unable to compute preferred pack, disabling pack-reuse\"));\n    -+\t\t\treturn;\n    -+\t\t}\n    -+\n    -+\t\tpack = bitmap_git->midx->packs[preferred_pack_pos];\n    ++\tif (!bitmap_is_midx(bitmap_git) || !bitmap_git->midx->chunk_bitmapped_packs)\n    ++\t\tmulti_pack_reuse = 0;\n     +\n    -+\t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n    -+\t\tpacks[packs_nr].p = pack;\n    -+\t\tpacks[packs_nr].bitmap_nr = pack->num_objects;\n    -+\t\tpacks[packs_nr].bitmap_pos = 0;\n    -+\n    -+\t\tobjects_nr = packs[packs_nr++].bitmap_nr;\n    -+\t} else if (bitmap_is_midx(bitmap_git)) {\n    ++\tif (multi_pack_reuse) {\n      \t\tfor (i = 0; i < bitmap_git->midx->num_packs; i++) {\n      \t\t\tstruct bitmapped_pack pack;\n      \t\t\tif (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {\n    @@ pack-bitmap.c: void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitm\n      \t\t}\n      \n      \t\tQSORT(packs, packs_nr, bitmapped_pack_cmp);\n    + \t} else {\n    +-\t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n    ++\t\tstruct packed_git *pack;\n    ++\n    ++\t\tif (bitmap_is_midx(bitmap_git)) {\n    ++\t\t\tuint32_t preferred_pack_pos;\n    ++\n    ++\t\t\tif (midx_preferred_pack(bitmap_git->midx, &preferred_pack_pos) < 0) {\n    ++\t\t\t\twarning(_(\"unable to compute preferred pack, disabling pack-reuse\"));\n    ++\t\t\t\treturn;\n    ++\t\t\t}\n    + \n    +-\t\tpacks[packs_nr].p = bitmap_git->pack;\n    +-\t\tpacks[packs_nr].bitmap_nr = bitmap_git->pack->num_objects;\n    ++\t\t\tpack = bitmap_git->midx->packs[preferred_pack_pos];\n    ++\t\t} else {\n    ++\t\t\tpack = bitmap_git->pack;\n    ++\t\t}\n    ++\n    ++\t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n    ++\t\tpacks[packs_nr].p = pack;\n    ++\t\tpacks[packs_nr].bitmap_nr = pack->num_objects;\n    + \t\tpacks[packs_nr].bitmap_pos = 0;\n    + \n    + \t\tobjects_nr = packs[packs_nr++].bitmap_nr;\n     \n      ## t/t5326-multi-pack-bitmaps.sh ##\n     @@ t/t5326-multi-pack-bitmaps.sh: test_expect_success 'corrupt MIDX with bitmap causes fallback' '\n    @@ t/t5326-multi-pack-bitmaps.sh: test_expect_success 'corrupt MIDX with bitmap cau\n      \n     +for allow_pack_reuse in single multi\n     +do\n    -+\ttest_expect_success \"MIDX without BTMP chunk does not complain with $allow_pack_reuse pack reuse\" '\n    ++\ttest_expect_success \"reading MIDX without BTMP chunk does not complain with $allow_pack_reuse pack reuse\" '\n     +\t\ttest_when_finished \"rm -rf midx-without-btmp\" &&\n     +\t\tgit init midx-without-btmp &&\n     +\t\t(\n     +\t\t\tcd midx-without-btmp &&\n     +\t\t\ttest_commit initial &&\n     +\n    -+\t\t\t# Write a multi-pack index that does have a bitmap, but\n    -+\t\t\t# no BTMP chunk. Such MIDX files would not be generated\n    -+\t\t\t# by modern Git anymore, but they were generated by\n    -+\t\t\t# older Git versions.\n    -+\t\t\tGIT_TEST_MIDX_WRITE_BTMP=false \\\n    -+\t\t\t\tgit repack -Adbl --write-bitmap-index --write-midx &&\n    -+\t\t\tgit -c pack.allowPackReuse=$allow_pack_reuse \\\n    ++\t\t\tgit repack -Adbl --write-bitmap-index --write-midx &&\n    ++\t\t\tGIT_TEST_MIDX_READ_BTMP=false git -c pack.allowPackReuse=$allow_pack_reuse \\\n     +\t\t\t\tpack-objects --all --use-bitmap-index --stdout </dev/null >/dev/null 2>err &&\n     +\t\t\ttest_must_be_empty err\n     +\t\t)\n\n midx.c                        |  7 +++---\n pack-bitmap.c                 | 41 ++++++++++++++++++-----------------\n t/t5326-multi-pack-bitmaps.sh | 17 +++++++++++++++\n 3 files changed, 42 insertions(+), 23 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex ae3b49166c..6f07de3688 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -170,9 +170,10 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \n \tpair_chunk(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets,\n \t\t   &m->chunk_large_offsets_len);\n-\tpair_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n-\t\t   (const unsigned char **)&m->chunk_bitmapped_packs,\n-\t\t   &m->chunk_bitmapped_packs_len);\n+\tif (git_env_bool(\"GIT_TEST_MIDX_READ_BTMP\", 1))\n+\t\tpair_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n+\t\t\t   (const unsigned char **)&m->chunk_bitmapped_packs,\n+\t\t\t   &m->chunk_bitmapped_packs_len);\n \n \tif (git_env_bool(\"GIT_TEST_MIDX_READ_RIDX\", 1))\n \t\tpair_chunk(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex,\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 2baeabacee..35c5ef9d3c 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -2049,7 +2049,10 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \n \tload_reverse_index(r, bitmap_git);\n \n-\tif (bitmap_is_midx(bitmap_git)) {\n+\tif (!bitmap_is_midx(bitmap_git) || !bitmap_git->midx->chunk_bitmapped_packs)\n+\t\tmulti_pack_reuse = 0;\n+\n+\tif (multi_pack_reuse) {\n \t\tfor (i = 0; i < bitmap_git->midx->num_packs; i++) {\n \t\t\tstruct bitmapped_pack pack;\n \t\t\tif (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {\n@@ -2062,34 +2065,32 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n \t\t\tif (!pack.bitmap_nr)\n \t\t\t\tcontinue;\n \n-\t\t\tif (!multi_pack_reuse && pack.bitmap_pos) {\n-\t\t\t\t/*\n-\t\t\t\t * If we're only reusing a single pack, skip\n-\t\t\t\t * over any packs which are not positioned at\n-\t\t\t\t * the beginning of the MIDX bitmap.\n-\t\t\t\t *\n-\t\t\t\t * This is consistent with the existing\n-\t\t\t\t * single-pack reuse behavior, which only reuses\n-\t\t\t\t * parts of the MIDX's preferred pack.\n-\t\t\t\t */\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\n \t\t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n \t\t\tmemcpy(&packs[packs_nr++], &pack, sizeof(pack));\n \n \t\t\tobjects_nr += pack.p->num_objects;\n-\n-\t\t\tif (!multi_pack_reuse)\n-\t\t\t\tbreak;\n \t\t}\n \n \t\tQSORT(packs, packs_nr, bitmapped_pack_cmp);\n \t} else {\n+\t\tstruct packed_git *pack;\n+\n+\t\tif (bitmap_is_midx(bitmap_git)) {\n+\t\t\tuint32_t preferred_pack_pos;\n+\n+\t\t\tif (midx_preferred_pack(bitmap_git->midx, &preferred_pack_pos) < 0) {\n+\t\t\t\twarning(_(\"unable to compute preferred pack, disabling pack-reuse\"));\n+\t\t\t\treturn;\n+\t\t\t}\n+\n+\t\t\tpack = bitmap_git->midx->packs[preferred_pack_pos];\n+\t\t} else {\n+\t\t\tpack = bitmap_git->pack;\n+\t\t}\n+\n \t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n-\n-\t\tpacks[packs_nr].p = bitmap_git->pack;\n-\t\tpacks[packs_nr].bitmap_nr = bitmap_git->pack->num_objects;\n+\t\tpacks[packs_nr].p = pack;\n+\t\tpacks[packs_nr].bitmap_nr = pack->num_objects;\n \t\tpacks[packs_nr].bitmap_pos = 0;\n \n \t\tobjects_nr = packs[packs_nr++].bitmap_nr;\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 70d1b58709..5d7d321840 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -513,4 +513,21 @@ test_expect_success 'corrupt MIDX with bitmap causes fallback' '\n \t)\n '\n \n+for allow_pack_reuse in single multi\n+do\n+\ttest_expect_success \"reading MIDX without BTMP chunk does not complain with $allow_pack_reuse pack reuse\" '\n+\t\ttest_when_finished \"rm -rf midx-without-btmp\" &&\n+\t\tgit init midx-without-btmp &&\n+\t\t(\n+\t\t\tcd midx-without-btmp &&\n+\t\t\ttest_commit initial &&\n+\n+\t\t\tgit repack -Adbl --write-bitmap-index --write-midx &&\n+\t\t\tGIT_TEST_MIDX_READ_BTMP=false git -c pack.allowPackReuse=$allow_pack_reuse \\\n+\t\t\t\tpack-objects --all --use-bitmap-index --stdout </dev/null >/dev/null 2>err &&\n+\t\t\ttest_must_be_empty err\n+\t\t)\n+\t'\n+done\n+\n test_done\n\nbase-commit: 19981daefd7c147444462739375462b49412ce33\n-- \n2.44.GIT\n\n"},{"id":"492934","messageId":"ZhzqeRIcyR3lmBme@tanuki","threadId":"61298","inReplyTo":"a8251f8278ba9a3b41a8e299cb4918a62df6d1c7.1713163238.git.ps@pks.im","subject":"Re: [PATCH v2] pack-bitmap: gracefully handle missing BTMP chunks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-15T08:51:05Z","receivedAt":"2024-04-15T08:51:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 15, 2024 at 08:41:25AM +0200, Patrick Steinhardt wrote:\n> In 0fea6b73f1 (Merge branch 'tb/multi-pack-verbatim-reuse', 2024-01-12)\n> we have introduced multi-pack verbatim reuse of objects. This series has\n> introduced a new BTMP chunk, which encodes information about bitmapped\n> objects in the multi-pack index. Starting with dab60934e3 (pack-bitmap:\n> pass `bitmapped_pack` struct to pack-reuse functions, 2023-12-14) we use\n> this information to figure out objects which we can reuse from each of\n> the packfiles.\n> \n> One thing that we glossed over though is backwards compatibility with\n> repositories that do not yet have BTMP chunks in their multi-pack index.\n> In that case, `nth_bitmapped_pack()` would return an error, which causes\n> us to emit a warning followed by another error message. These warnings\n> are visible to users that fetch from a repository:\n> \n> ```\n> $ git fetch\n> ...\n> remote: error: MIDX does not contain the BTMP chunk\n> remote: warning: unable to load pack: 'pack-f6bb7bd71d345ea9fe604b60cab9ba9ece54ffbe.idx', disabling pack-reuse\n> remote: Enumerating objects: 40, done.\n> remote: Counting objects: 100% (40/40), done.\n> remote: Compressing objects: 100% (39/39), done.\n> remote: Total 40 (delta 5), reused 0 (delta 0), pack-reused 0 (from 0)\n> ...\n> ```\n> \n> While the fetch succeeds the user is left wondering what they did wrong.\n> Furthermore, as visible both from the warning and from the reuse stats,\n> pack-reuse is completely disabled in such repositories.\n> \n> What is quite interesting is that this issue can even be triggered in\n> case `pack.allowPackReuse=single` is set, which is the default value.\n> One could have expected that in this case we fall back to the old logic,\n> which is to use the preferred packfile without consulting BTMP chunks at\n> all. But either we fail with the above error in case they are missing,\n> or we use the first pack in the multi-pack-index. The former case\n> disables pack-reuse altogether, whereas the latter case may result in\n> reusing objects from a suboptimal packfile.\n> \n> Fix this issue by partially reverting the logic back to what we had\n> before this patch series landed. Namely, in the case where we have no\n> BTMP chunks or when `pack.allowPackReuse=single` are set, we use the\n> preferred pack instead of consulting the BTMP chunks.\n> \n> Helped-by: Taylor Blau <me@ttaylorr.com>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n\nJunio, it would be great if we could still land this fix in Git v2.45\ngiven that it is addressing a regression in Git v2.44. This of course\nassumes that the current version of this patch looks good to Taylor.\n\nPatrick\n"},{"id":"492955","messageId":"xmqqil0io762.fsf@gitster.g","threadId":"61298","inReplyTo":"ZhzqeRIcyR3lmBme@tanuki","subject":"Re: [PATCH v2] pack-bitmap: gracefully handle missing BTMP chunks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-15T17:41:09Z","receivedAt":"2024-04-15T17:41:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> Helped-by: Taylor Blau <me@ttaylorr.com>\n>> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n>\n> Junio, it would be great if we could still land this fix in Git v2.45\n> given that it is addressing a regression in Git v2.44. This of course\n> assumes that the current version of this patch looks good to Taylor.\n\nIndeed.  It would be nice to see an acked by or something.\n\nWill queue, in the meantime.  Thanks for a ping.\n"},{"id":"492968","messageId":"Zh2tPdvhQgD4342C@nand.local","threadId":"61298","inReplyTo":"ZhzKeR9C4UVpEm29@tanuki","subject":"Re: [PATCH] pack-bitmap: gracefully handle missing BTMP chunks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-04-15T22:42:05Z","receivedAt":"2024-04-15T22:42:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 15, 2024 at 08:34:33AM +0200, Patrick Steinhardt wrote:\n> > Of course, that is still using a GIT_TEST_-variable, which is less than\n> > ideal IMHO. The other alternative would be to store a MIDX file as a\n> > test fixture in the tree (which we do in a couple of places). But with\n> > the recent xz shenanigans, I'm not sure that's a great idea either ;-).\n>\n> I'm happy to convert this to use `GIT_TEST_MIDX_READ_BTMP` instead.\n\nThanks, I think that is definitely an improvement.\n\n> > The way I would structure this series is to first apply the portion of\n> > the above patch *without* these lines:\n> >\n> > -\tif (bitmap_is_midx(bitmap_git)) {\n> > +\tif (!bitmap_is_midx(bitmap_git) ||\n> > +\t    !bitmap_git->midx->chunk_bitmapped_packs)\n> > +\t\tmulti_pack_reuse = 0;\n> > +\n> >\n> > , so we're still able to reproduce the issue. Then, apply the remaining\n> > portions (the above diff, the test, and the GIT_TEST_MIDX_WRITE_BTMP\n> > stuff) to demonstrate that the issue is fixed via a separate commit.\n> >\n> > I'm happy to write that up, and equally happy to not do it ;-). Sorry\n> > for the lengthy review, but thank you very much for spotting and fixing\n> > this issue.\n>\n> I'd prefer to leave it as a single patch. Junio has expressed at times\n> that he doesn't see much value in these splits only to demonstrate a\n> broken test. An interested reader can easily revert the fix and see that\n> the test would fail.\n\nYeah, to be clear, I wasn't suggesting adding a test_expect_failure\nhere. I was suggesting instead that you do a refactoring that doesn't\nchange the behavior in the first step, and then add the test plus the\nfix (after it is made easier to land by the first step) in the second\nstep.\n\nBut I don't feel strongly about it whatsoever, so I think that the new\nversion you have here is fine. I'll take a closer look at it right\nnow...\n\nThanks,\nTaylor\n"},{"id":"492969","messageId":"Zh2vZB/60QlLYjUZ@nand.local","threadId":"61298","inReplyTo":"a8251f8278ba9a3b41a8e299cb4918a62df6d1c7.1713163238.git.ps@pks.im","subject":"Re: [PATCH v2] pack-bitmap: gracefully handle missing BTMP chunks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-04-15T22:51:16Z","receivedAt":"2024-04-15T22:51:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 15, 2024 at 08:41:25AM +0200, Patrick Steinhardt wrote:\n> diff --git a/midx.c b/midx.c\n> index ae3b49166c..6f07de3688 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -170,9 +170,10 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n>\n>  \tpair_chunk(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets,\n>  \t\t   &m->chunk_large_offsets_len);\n> -\tpair_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n> -\t\t   (const unsigned char **)&m->chunk_bitmapped_packs,\n> -\t\t   &m->chunk_bitmapped_packs_len);\n> +\tif (git_env_bool(\"GIT_TEST_MIDX_READ_BTMP\", 1))\n> +\t\tpair_chunk(cf, MIDX_CHUNKID_BITMAPPEDPACKS,\n> +\t\t\t   (const unsigned char **)&m->chunk_bitmapped_packs,\n> +\t\t\t   &m->chunk_bitmapped_packs_len);\n\nOK, so we're switching to a new GIT_TEST_-variable here, which controls\nwhether or not we read the BTMP chunk. That makes sense, and is much\nappreciated :-).\n\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index 2baeabacee..35c5ef9d3c 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -2049,7 +2049,10 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n>\n>  \tload_reverse_index(r, bitmap_git);\n>\n> -\tif (bitmap_is_midx(bitmap_git)) {\n> +\tif (!bitmap_is_midx(bitmap_git) || !bitmap_git->midx->chunk_bitmapped_packs)\n> +\t\tmulti_pack_reuse = 0;\n> +\n\nEither we don't have a MIDX, or we do, but it doesn't have a BTMP chunk.\nIn either case, we should disable multi-pack reuse (either using the\nsingle pack corresponding with a classic pack-bitmap, or the preferred\npack if using a MIDX bitamp written prior to the BTMP chunk).\n\nLooking good.\n\n> +\tif (multi_pack_reuse) {\n>  \t\tfor (i = 0; i < bitmap_git->midx->num_packs; i++) {\n>  \t\t\tstruct bitmapped_pack pack;\n>  \t\t\tif (nth_bitmapped_pack(r, bitmap_git->midx, &pack, i) < 0) {\n> @@ -2062,34 +2065,32 @@ void reuse_partial_packfile_from_bitmap(struct bitmap_index *bitmap_git,\n>  \t\t\tif (!pack.bitmap_nr)\n>  \t\t\t\tcontinue;\n>\n> -\t\t\tif (!multi_pack_reuse && pack.bitmap_pos) {\n> -\t\t\t\t/*\n> -\t\t\t\t * If we're only reusing a single pack, skip\n> -\t\t\t\t * over any packs which are not positioned at\n> -\t\t\t\t * the beginning of the MIDX bitmap.\n> -\t\t\t\t *\n> -\t\t\t\t * This is consistent with the existing\n> -\t\t\t\t * single-pack reuse behavior, which only reuses\n> -\t\t\t\t * parts of the MIDX's preferred pack.\n> -\t\t\t\t */\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n\nYep, this hunk can go since it used to belong to the outer if-statement\nin the pre-image that was conditioned on 'bitmap_is_midx()'. This is\ndealt with separately, since we know ahead of time we're doing\nmulti-pack reuse (and can do so).\n> -\n>  \t\t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n>  \t\t\tmemcpy(&packs[packs_nr++], &pack, sizeof(pack));\n>\n>  \t\t\tobjects_nr += pack.p->num_objects;\n> -\n> -\t\t\tif (!multi_pack_reuse)\n> -\t\t\t\tbreak;\n>  \t\t}\n>\n>  \t\tQSORT(packs, packs_nr, bitmapped_pack_cmp);\n>  \t} else {\n> +\t\tstruct packed_git *pack;\n> +\n> +\t\tif (bitmap_is_midx(bitmap_git)) {\n> +\t\t\tuint32_t preferred_pack_pos;\n> +\n> +\t\t\tif (midx_preferred_pack(bitmap_git->midx, &preferred_pack_pos) < 0) {\n> +\t\t\t\twarning(_(\"unable to compute preferred pack, disabling pack-reuse\"));\n> +\t\t\t\treturn;\n> +\t\t\t}\n> +\n> +\t\t\tpack = bitmap_git->midx->packs[preferred_pack_pos];\n> +\t\t} else {\n> +\t\t\tpack = bitmap_git->pack;\n> +\t\t}\n> +\n\nLooking good. Here we're doing single-pack reuse (either from the pack\ncorresponding with the bitmap or the MIDX's preferred pack). Either way\nwe set the 'pack' variable to point at the appropriate pack, and then\nadd that pack to the list of reusable packs below. Good.\n\n>  \t\tALLOC_GROW(packs, packs_nr + 1, packs_alloc);\n> -\n> -\t\tpacks[packs_nr].p = bitmap_git->pack;\n> -\t\tpacks[packs_nr].bitmap_nr = bitmap_git->pack->num_objects;\n> +\t\tpacks[packs_nr].p = pack;\n> +\t\tpacks[packs_nr].bitmap_nr = pack->num_objects;\n>  \t\tpacks[packs_nr].bitmap_pos = 0;\n>\n>  \t\tobjects_nr = packs[packs_nr++].bitmap_nr;\n\nMakes sense.\n\n> diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\n> index 70d1b58709..5d7d321840 100755\n> --- a/t/t5326-multi-pack-bitmaps.sh\n> +++ b/t/t5326-multi-pack-bitmaps.sh\n> @@ -513,4 +513,21 @@ test_expect_success 'corrupt MIDX with bitmap causes fallback' '\n>  \t)\n>  '\n>\n> +for allow_pack_reuse in single multi\n> +do\n> +\ttest_expect_success \"reading MIDX without BTMP chunk does not complain with $allow_pack_reuse pack reuse\" '\n> +\t\ttest_when_finished \"rm -rf midx-without-btmp\" &&\n> +\t\tgit init midx-without-btmp &&\n> +\t\t(\n> +\t\t\tcd midx-without-btmp &&\n> +\t\t\ttest_commit initial &&\n> +\n> +\t\t\tgit repack -Adbl --write-bitmap-index --write-midx &&\n\n`-b` is redundant with `--write-bitmap-index`.\n\n> +\t\t\tGIT_TEST_MIDX_READ_BTMP=false git -c pack.allowPackReuse=$allow_pack_reuse \\\n> +\t\t\t\tpack-objects --all --use-bitmap-index --stdout </dev/null >/dev/null 2>err &&\n\nA small note here, but setting stdin to read from /dev/null is\nunnecessary with `--all.`\n\n> +\t\t\ttest_must_be_empty err\n> +\t\t)\n> +\t'\n> +done\n> +\n\nThis test looks like it's exercising the right thing, but I'm not sure\nwhy it was split into two separate tests. Perhaps to allow the two to\nfail separately?\n\nEither way, the repository initialization, test_commit, and repacking\ncould probably be combined into a single step to avoid re-running them\nfor different values of $allow_pack_reuse.\n\nI would probably have written:\n\n    git init midx-without-btmp &&\n    (\n        cd midx-without-btmp &&\n\n        test_commit base &&\n        git repack -adb --write-midx &&\n\n        for c in single multi\n        do\n            GIT_TEST_MIDX_READ_BTMP=false git -c pack.allowPackReuse=$c pack-objects \\\n              --all --use-bitmap-index --stdout >/dev/null 2>err &&\n            test_must_be_empty err || return 1\n        done\n    )\n\nTBH, I would like to see this test cleaned up before merging this one\ndown. But otherwise this patch is looking good.\n\nThanks,\nTaylor\n"},{"id":"492970","messageId":"Zh2vhDraCnqw4YFL@nand.local","threadId":"61298","inReplyTo":"xmqqil0io762.fsf@gitster.g","subject":"Re: [PATCH v2] pack-bitmap: gracefully handle missing BTMP chunks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-04-15T22:51:48Z","receivedAt":"2024-04-15T22:51:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 15, 2024 at 10:41:09AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n> >> Helped-by: Taylor Blau <me@ttaylorr.com>\n> >> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> >\n> > Junio, it would be great if we could still land this fix in Git v2.45\n> > given that it is addressing a regression in Git v2.44. This of course\n> > assumes that the current version of this patch looks good to Taylor.\n>\n> Indeed.  It would be nice to see an acked by or something.\n>\n> Will queue, in the meantime.  Thanks for a ping.\n\nI took a look, and I think the patch is good. I have a couple of notes\non the test that I would prefer to see addressed before merging it down,\nthough.\n\nThanks,\nTaylor\n"},{"id":"492972","messageId":"xmqq8r1ei3yz.fsf@gitster.g","threadId":"61298","inReplyTo":"Zh2vhDraCnqw4YFL@nand.local","subject":"Re: [PATCH v2] pack-bitmap: gracefully handle missing BTMP chunks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-15T23:46:44Z","receivedAt":"2024-04-15T23:46:49Z","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> On Mon, Apr 15, 2024 at 10:41:09AM -0700, Junio C Hamano wrote:\n>> Patrick Steinhardt <ps@pks.im> writes:\n>>\n>> >> Helped-by: Taylor Blau <me@ttaylorr.com>\n>> >> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n>> >\n>> > Junio, it would be great if we could still land this fix in Git v2.45\n>> > given that it is addressing a regression in Git v2.44. This of course\n>> > assumes that the current version of this patch looks good to Taylor.\n>>\n>> Indeed.  It would be nice to see an acked by or something.\n>>\n>> Will queue, in the meantime.  Thanks for a ping.\n>\n> I took a look, and I think the patch is good. I have a couple of notes\n> on the test that I would prefer to see addressed before merging it down,\n> though.\n\nThanks.\n"},{"id":"492978","messageId":"Zh4C9xhOC4BVslyX@tanuki","threadId":"61298","inReplyTo":"Zh2vZB/60QlLYjUZ@nand.local","subject":"Re: [PATCH v2] pack-bitmap: gracefully handle missing BTMP chunks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-16T04:47:51Z","receivedAt":"2024-04-16T04:47:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 15, 2024 at 06:51:16PM -0400, Taylor Blau wrote:\n> On Mon, Apr 15, 2024 at 08:41:25AM +0200, Patrick Steinhardt wrote:\n[snip]\n> > diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\n> > index 70d1b58709..5d7d321840 100755\n> > --- a/t/t5326-multi-pack-bitmaps.sh\n> > +++ b/t/t5326-multi-pack-bitmaps.sh\n> > @@ -513,4 +513,21 @@ test_expect_success 'corrupt MIDX with bitmap causes fallback' '\n> >  \t)\n> >  '\n> >\n> > +for allow_pack_reuse in single multi\n> > +do\n> > +\ttest_expect_success \"reading MIDX without BTMP chunk does not complain with $allow_pack_reuse pack reuse\" '\n> > +\t\ttest_when_finished \"rm -rf midx-without-btmp\" &&\n> > +\t\tgit init midx-without-btmp &&\n> > +\t\t(\n> > +\t\t\tcd midx-without-btmp &&\n> > +\t\t\ttest_commit initial &&\n> > +\n> > +\t\t\tgit repack -Adbl --write-bitmap-index --write-midx &&\n> \n> `-b` is redundant with `--write-bitmap-index`.\n\nOops, right.\n\n> > +\t\t\tGIT_TEST_MIDX_READ_BTMP=false git -c pack.allowPackReuse=$allow_pack_reuse \\\n> > +\t\t\t\tpack-objects --all --use-bitmap-index --stdout </dev/null >/dev/null 2>err &&\n> \n> A small note here, but setting stdin to read from /dev/null is\n> unnecessary with `--all.`\n\nIs it really? Executing `git pack-objects --all --stdout` on my system\nblocks until stdin is closed. It _seems_ to work in the tests alright,\nbut doesn't work outside of them. Which is puzzling on its own.\n\n> > +\t\t\ttest_must_be_empty err\n> > +\t\t)\n> > +\t'\n> > +done\n> > +\n> \n> This test looks like it's exercising the right thing, but I'm not sure\n> why it was split into two separate tests. Perhaps to allow the two to\n> fail separately?\n\nExactly. It makes it easier to see which of both tests fails in case\nonly one does.\n\n> Either way, the repository initialization, test_commit, and repacking\n> could probably be combined into a single step to avoid re-running them\n> for different values of $allow_pack_reuse.\n> \n> I would probably have written:\n> \n>     git init midx-without-btmp &&\n>     (\n>         cd midx-without-btmp &&\n> \n>         test_commit base &&\n>         git repack -adb --write-midx &&\n> \n>         for c in single multi\n>         do\n>             GIT_TEST_MIDX_READ_BTMP=false git -c pack.allowPackReuse=$c pack-objects \\\n>               --all --use-bitmap-index --stdout >/dev/null 2>err &&\n>             test_must_be_empty err || return 1\n>         done\n>     )\n> \n> TBH, I would like to see this test cleaned up before merging this one\n> down. But otherwise this patch is looking good.\n\nSo I'm a bit torn here. I think your proposed way to test things is\ninferior regarding usability, even though it is superior regarding\nperformance. We could move the common setup into a separate test, but\nthat has the issue that tests cannot easily be run as self-contained\nunits.\n\nPatrick\n"},{"id":"492979","messageId":"20240416051232.GA1745631@coredump.intra.peff.net","threadId":"61298","inReplyTo":"Zh4C9xhOC4BVslyX@tanuki","subject":"Re: [PATCH v2] pack-bitmap: gracefully handle missing BTMP chunks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-16T05:12:32Z","receivedAt":"2024-04-16T05:12:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 16, 2024 at 06:47:51AM +0200, Patrick Steinhardt wrote:\n\n> > > +\t\t\tGIT_TEST_MIDX_READ_BTMP=false git -c pack.allowPackReuse=$allow_pack_reuse \\\n> > > +\t\t\t\tpack-objects --all --use-bitmap-index --stdout </dev/null >/dev/null 2>err &&\n> > \n> > A small note here, but setting stdin to read from /dev/null is\n> > unnecessary with `--all.`\n> \n> Is it really? Executing `git pack-objects --all --stdout` on my system\n> blocks until stdin is closed. It _seems_ to work in the tests alright,\n> but doesn't work outside of them. Which is puzzling on its own.\n\nInside a test_expect block, stdin is already redirected from /dev/null.\nSee 781f76b158 (test-lib: redirect stdin of tests, 2011-12-15).\n\nI do think it's still good practice to redirect from /dev/null\nexplicitly to indicate the intent.\n\n-Peff\n"},{"id":"492981","messageId":"Zh4JGJm6cYFjk4x_@tanuki","threadId":"61298","inReplyTo":"20240416051232.GA1745631@coredump.intra.peff.net","subject":"Re: [PATCH v2] pack-bitmap: gracefully handle missing BTMP chunks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-16T05:14:00Z","receivedAt":"2024-04-16T05:14:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 16, 2024 at 01:12:32AM -0400, Jeff King wrote:\n> On Tue, Apr 16, 2024 at 06:47:51AM +0200, Patrick Steinhardt wrote:\n> \n> > > > +\t\t\tGIT_TEST_MIDX_READ_BTMP=false git -c pack.allowPackReuse=$allow_pack_reuse \\\n> > > > +\t\t\t\tpack-objects --all --use-bitmap-index --stdout </dev/null >/dev/null 2>err &&\n> > > \n> > > A small note here, but setting stdin to read from /dev/null is\n> > > unnecessary with `--all.`\n> > \n> > Is it really? Executing `git pack-objects --all --stdout` on my system\n> > blocks until stdin is closed. It _seems_ to work in the tests alright,\n> > but doesn't work outside of them. Which is puzzling on its own.\n> \n> Inside a test_expect block, stdin is already redirected from /dev/null.\n> See 781f76b158 (test-lib: redirect stdin of tests, 2011-12-15).\n> \n> I do think it's still good practice to redirect from /dev/null\n> explicitly to indicate the intent.\n\nAh, that explains. Thanks!\n\nPatrick\n"}]}