{"thread":{"id":"59678","subject":"[PATCH 0/2] fsck: verify .bitmap checksums, cleanup","startedAt":"2023-05-01T18:21:44Z","lastAt":"2023-05-02T13:27:35Z","messageCount":8,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"476367","messageId":"pull.1526.git.1682965295.gitgitgadget@gmail.com","threadId":"59678","inReplyTo":null,"subject":"[PATCH 0/2] fsck: verify .bitmap checksums, cleanup","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-01T18:21:33Z","receivedAt":"2023-05-01T18:21:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Similar to the recent work on rev-indexes [1], add checks for .bitmap files\nto 'git fsck'.\n\nPatch 1 includes the full details for .bitmap files.\n\nPatch 2 includes a cleanup of ds/fsck-pack-revindex that I noticed while\npreparing this series.\n\nThanks, Stolee\n\n[1]\nhttps://lore.kernel.org/git/pull.1512.git.1681748502.gitgitgadget@gmail.com/\n\nDerrick Stolee (2):\n  fsck: verify checksums of all .bitmap files\n  fsck: use local repository\n\n builtin/fsck.c                | 10 +++++---\n pack-bitmap.c                 | 44 +++++++++++++++++++++++++++++++++++\n pack-bitmap.h                 |  2 ++\n t/t5326-multi-pack-bitmaps.sh | 38 ++++++++++++++++++++++++++++++\n 4 files changed, 91 insertions(+), 3 deletions(-)\n\n\nbase-commit: 48d89b51b3bb8a60580c36731b96a7206ce1e5b9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1526%2Fderrickstolee%2Ffsck-bitmap-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1526/derrickstolee/fsck-bitmap-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1526\n-- \ngitgitgadget\n"},{"id":"476368","messageId":"2162a352a46dc0c40afccc4defc88a1e8940ce6c.1682965296.git.gitgitgadget@gmail.com","threadId":"59678","inReplyTo":"pull.1526.git.1682965295.gitgitgadget@gmail.com","subject":"[PATCH 2/2] fsck: use local repository","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-01T18:21:35Z","receivedAt":"2023-05-01T18:21:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nIn 0d30feef3c5 (fsck: create scaffolding for rev-index checks,\n2023-04-17) and later 5a6072f631d (fsck: validate .rev file header,\n2023-04-17), the check_pack_rev_indexes() method was created with a\n'struct repository *r' parameter. However, this parameter was unused and\ninstead 'the_repository' was used in its place.\n\nFix this situation with the obvious replacement.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n builtin/fsck.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 75b30d1d00c..dcc165bf0c5 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -869,20 +869,20 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)\n \tint res = 0;\n \n \tif (show_progress) {\n-\t\tfor (struct packed_git *p = get_all_packs(the_repository); p; p = p->next)\n+\t\tfor (struct packed_git *p = get_all_packs(r); p; p = p->next)\n \t\t\tpack_count++;\n \t\tprogress = start_delayed_progress(\"Verifying reverse pack-indexes\", pack_count);\n \t\tpack_count = 0;\n \t}\n \n-\tfor (struct packed_git *p = get_all_packs(the_repository); p; p = p->next) {\n+\tfor (struct packed_git *p = get_all_packs(r); p; p = p->next) {\n \t\tint load_error = load_pack_revindex_from_disk(p);\n \n \t\tif (load_error < 0) {\n \t\t\terror(_(\"unable to load rev-index for pack '%s'\"), p->pack_name);\n \t\t\tres = ERROR_PACK_REV_INDEX;\n \t\t} else if (!load_error &&\n-\t\t\t   !load_pack_revindex(the_repository, p) &&\n+\t\t\t   !load_pack_revindex(r, p) &&\n \t\t\t   verify_pack_revindex(p)) {\n \t\t\terror(_(\"invalid rev-index for pack '%s'\"), p->pack_name);\n \t\t\tres = ERROR_PACK_REV_INDEX;\n-- \ngitgitgadget\n"},{"id":"476369","messageId":"d608d2faa8602df6a52117c8c57c0ca8e43beb4f.1682965295.git.gitgitgadget@gmail.com","threadId":"59678","inReplyTo":"pull.1526.git.1682965295.gitgitgadget@gmail.com","subject":"[PATCH 1/2] fsck: verify checksums of all .bitmap files","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-01T18:21:34Z","receivedAt":"2023-05-01T18:21:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nIf a filesystem-level corruption occurs in a .bitmap file, Git can react\npoorly. This could take the form of a run-time error due to failing to\nparse an EWAH bitmap or be more subtle such as returning the wrong set\nof objects to a fetch or clone.\n\nA natural first response to either of these kinds of errors is to run\n'git fsck' to see if any files are corrupt. This currently ignores all\n.bitmap files.\n\nAdd checks to 'git fsck' for all .bitmap files that are currently\nassociated with a multi-pack-index or pack file. Verify their checksums\nusing the hashfile API.\n\nWe iterate through all multi-pack-indexes and pack-files to be sure to\ncheck all .bitmap files, not just the one that would be read by the\nprocess. For example, a multi-pack-index bitmap overrules a pack-bitmap.\nHowever, if the multi-pack-index is removed, the pack-bitmap may be\nselected instead. Be thorough to include every file that could become\nactive in such a way. This includes checking files in alternates.\n\nThere is potential that we could extend this effort to check the\nstructure of the reachability bitmaps themselves, but it is very\nexpensive to do so. At minimum, it's as expensive as generating the\nbitmaps in the first place, and that's assuming that we don't use the\ntrivial algorithm of verifying each bitmap individually. The trivial\nalgorithm will result in quadratic behavior (number of objects times\nnumber of bitmapped commits) while the bitmap building operation\nconstructs a lattice of commits to build bitmaps incrementally and then\ngenerate the final bitmaps from a subset of those commits.\n\nIf we were to extend 'git fsck' to check .bitmap file contents more\nclosely like this, then we would likely want to hide it behind an option\nthat signals the user is more willing to do expensive operations such as\nthis.\n\nFor testing, set up a repository with a pack-bitmap _and_ a\nmulti-pack-index bitmap. This requires some file movement to avoid\ndeleting the pack-bitmap during the repack that creates the\nmulti-pack-index bitmap. We can then verify that 'git fsck' is checking\nall files, not just the \"active\" bitmap.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n builtin/fsck.c                |  4 ++++\n pack-bitmap.c                 | 44 +++++++++++++++++++++++++++++++++++\n pack-bitmap.h                 |  2 ++\n t/t5326-multi-pack-bitmaps.sh | 38 ++++++++++++++++++++++++++++++\n 4 files changed, 88 insertions(+)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 2cd461b84c1..75b30d1d00c 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -27,6 +27,7 @@\n #include \"run-command.h\"\n #include \"worktree.h\"\n #include \"pack-revindex.h\"\n+#include \"pack-bitmap.h\"\n \n #define REACHABLE 0x0001\n #define SEEN      0x0002\n@@ -57,6 +58,7 @@ static int name_objects;\n #define ERROR_COMMIT_GRAPH 020\n #define ERROR_MULTI_PACK_INDEX 040\n #define ERROR_PACK_REV_INDEX 0100\n+#define ERROR_BITMAP 0200\n \n static const char *describe_object(const struct object_id *oid)\n {\n@@ -1056,6 +1058,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t}\n \n \terrors_found |= check_pack_rev_indexes(the_repository, show_progress);\n+\tif (verify_bitmap_files(the_repository))\n+\t\terrors_found |= ERROR_BITMAP;\n \n \tcheck_connectivity();\n \ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex e0fad723bf3..ded58929a81 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -2346,3 +2346,47 @@ int bitmap_is_preferred_refname(struct repository *r, const char *refname)\n \n \treturn 0;\n }\n+\n+static int verify_bitmap_file(const char *name)\n+{\n+\tstruct stat st;\n+\tunsigned char *data;\n+\tint fd = git_open(name);\n+\tint res = 0;\n+\n+\t/* A non-existent file is valid. */\n+\tif (fstat(fd, &st)) {\n+\t\tclose(fd);\n+\t\treturn 0;\n+\t}\n+\n+\tdata = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tclose(fd);\n+\tif (!hashfile_checksum_valid(data, st.st_size))\n+\t\tres = error(_(\"bitmap file '%s' has invalid checksum\"),\n+\t\t\t    name);\n+\n+\tmunmap(data, st.st_size);\n+\treturn res;\n+}\n+\n+int verify_bitmap_files(struct repository *r)\n+{\n+\tint res = 0;\n+\n+\tfor (struct multi_pack_index *m = get_multi_pack_index(r);\n+\t     m; m = m->next) {\n+\t\tchar *midx_bitmap_name = midx_bitmap_filename(m);\n+\t\tres |= verify_bitmap_file(midx_bitmap_name);\n+\t\tfree(midx_bitmap_name);\n+\t}\n+\n+\tfor (struct packed_git *p = get_all_packs(r);\n+\t     p; p = p->next) {\n+\t\tchar *pack_bitmap_name = pack_bitmap_filename(p);\n+\t\tres |= verify_bitmap_file(pack_bitmap_name);\n+\t\tfree(pack_bitmap_name);\n+\t}\n+\n+\treturn res;\n+}\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex f0180b5276b..84591f041bf 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -111,4 +111,6 @@ int bitmap_is_midx(struct bitmap_index *bitmap_git);\n const struct string_list *bitmap_preferred_tips(struct repository *r);\n int bitmap_is_preferred_refname(struct repository *r, const char *refname);\n \n+int verify_bitmap_files(struct repository *r);\n+\n #endif\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 0882cbb6e4a..d7353d8d443 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -434,4 +434,42 @@ test_expect_success 'tagged commits are selected for bitmapping' '\n \t)\n '\n \n+corrupt_file () {\n+\tchmod a+w \"$1\" &&\n+\tprintf \"bogus\" | dd of=\"$1\" bs=1 seek=\"12\" conv=notrunc\n+}\n+\n+test_expect_success 'git fsck correctly identifies good and bad bitmaps' '\n+\tgit init valid &&\n+\ttest_when_finished rm -rf valid &&\n+\n+\ttest_commit_bulk 20 &&\n+\tgit repack -adbf &&\n+\n+\t# Move pack-bitmap aside so it is not deleted\n+\t# in next repack.\n+\tpackbitmap=$(ls .git/objects/pack/pack-*.bitmap) &&\n+\tmv \"$packbitmap\" \"$packbitmap.bak\" &&\n+\n+\ttest_commit_bulk 10 &&\n+\tgit repack -b --write-midx &&\n+\tmidxbitmap=$(ls .git/objects/pack/multi-pack-index-*.bitmap) &&\n+\n+\t# Copy MIDX bitmap to backup. Copy pack bitmap from backup.\n+\tcp \"$midxbitmap\" \"$midxbitmap.bak\" &&\n+\tcp \"$packbitmap.bak\" \"$packbitmap\" &&\n+\n+\t# fsck works at first\n+\tgit fsck &&\n+\n+\tcorrupt_file \"$packbitmap\" &&\n+\ttest_must_fail git fsck 2>err &&\n+\tgrep \"bitmap file '\\''$packbitmap'\\'' has invalid checksum\" err &&\n+\n+\tcp \"$packbitmap.bak\" \"$packbitmap\" &&\n+\tcorrupt_file \"$midxbitmap\" &&\n+\ttest_must_fail git fsck 2>err &&\n+\tgrep \"bitmap file '\\''$midxbitmap'\\'' has invalid checksum\" err\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"476370","messageId":"xmqqv8hb50qs.fsf@gitster.g","threadId":"59678","inReplyTo":"d608d2faa8602df6a52117c8c57c0ca8e43beb4f.1682965295.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] fsck: verify checksums of all .bitmap files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-01T19:07:39Z","receivedAt":"2023-05-01T19:07:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> +static int verify_bitmap_file(const char *name)\n> +{\n> +\tstruct stat st;\n> +\tunsigned char *data;\n> +\tint fd = git_open(name);\n> +\tint res = 0;\n> +\n> +\t/* A non-existent file is valid. */\n> +\tif (fstat(fd, &st)) {\n> +\t\tclose(fd);\n> +\t\treturn 0;\n> +\t}\n\ngit_open() may return a negative value to signal an error, and\nfstat() would return negative to signal its own error, and feeding\nthe negative fd to close() would also be an error without molesting\nother open file descriptors we would want to keep, and we do not\ncheck the return value from close(), so all of the above may be\nsafe, but makes us feel a bit dirty.\n\n\t/* It is OK not to have the file */\n\tif (fd < 0 || fstat(fd, &st)) {\n\t\tif (0 <= fd)\n\t\t\tclose(fd);\n\t\treturn 0;\n\t}\n\nis probably not overly too verbose and makes the result a bit more\npalatable, perhaps.\n\n> +\tdata = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n> +\tclose(fd);\n> +\tif (!hashfile_checksum_valid(data, st.st_size))\n> +\t\tres = error(_(\"bitmap file '%s' has invalid checksum\"),\n> +\t\t\t    name);\n> +\n> +\tmunmap(data, st.st_size);\n> +\treturn res;\n> +}\n\nOK.\n\n> +int verify_bitmap_files(struct repository *r)\n> +{\n> +\tint res = 0;\n> +\n> +\tfor (struct multi_pack_index *m = get_multi_pack_index(r);\n> +\t     m; m = m->next) {\n> +\t\tchar *midx_bitmap_name = midx_bitmap_filename(m);\n> +\t\tres |= verify_bitmap_file(midx_bitmap_name);\n> +\t\tfree(midx_bitmap_name);\n> +\t}\n> +\n> +\tfor (struct packed_git *p = get_all_packs(r);\n> +\t     p; p = p->next) {\n> +\t\tchar *pack_bitmap_name = pack_bitmap_filename(p);\n> +\t\tres |= verify_bitmap_file(pack_bitmap_name);\n> +\t\tfree(pack_bitmap_name);\n> +\t}\n> +\n> +\treturn res;\n> +}\n\nOK.\n\n> diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\n> index 0882cbb6e4a..d7353d8d443 100755\n> --- a/t/t5326-multi-pack-bitmaps.sh\n> +++ b/t/t5326-multi-pack-bitmaps.sh\n> @@ -434,4 +434,42 @@ test_expect_success 'tagged commits are selected for bitmapping' '\n>  \t)\n>  '\n>  \n> +corrupt_file () {\n> +\tchmod a+w \"$1\" &&\n> +\tprintf \"bogus\" | dd of=\"$1\" bs=1 seek=\"12\" conv=notrunc\n> +}\n> +\n> +test_expect_success 'git fsck correctly identifies good and bad bitmaps' '\n> +\tgit init valid &&\n> +\ttest_when_finished rm -rf valid &&\n> +\n> +\ttest_commit_bulk 20 &&\n> +\tgit repack -adbf &&\n> +\n> +\t# Move pack-bitmap aside so it is not deleted\n> +\t# in next repack.\n> +\tpackbitmap=$(ls .git/objects/pack/pack-*.bitmap) &&\n> +\tmv \"$packbitmap\" \"$packbitmap.bak\" &&\n> +\n> +\ttest_commit_bulk 10 &&\n> +\tgit repack -b --write-midx &&\n> +\tmidxbitmap=$(ls .git/objects/pack/multi-pack-index-*.bitmap) &&\n> +\n> +\t# Copy MIDX bitmap to backup. Copy pack bitmap from backup.\n> +\tcp \"$midxbitmap\" \"$midxbitmap.bak\" &&\n> +\tcp \"$packbitmap.bak\" \"$packbitmap\" &&\n\nSo, at this point, we have only one pack, and we could enable the\nmidx bitmap and/or pack bitmap from their .bak.  We first enable\nonly the pack bitmap while leaving midx bitmap disabled.\n\n> +\t# fsck works at first\n> +\tgit fsck &&\n\nOK.\n\n> +\tcorrupt_file \"$packbitmap\" &&\n> +\ttest_must_fail git fsck 2>err &&\n> +\tgrep \"bitmap file '\\''$packbitmap'\\'' has invalid checksum\" err &&\n\nAnd when the only thing enabled, i.e. pack bitmap, is broken, we\nnotice the breakage.  OK.\n\n> +\tcp \"$packbitmap.bak\" \"$packbitmap\" &&\n> +\tcorrupt_file \"$midxbitmap\" &&\n\nNow we repair the pack bitmap and break midx bitmap.  Both are\nenabled in this case.\n\n> +\ttest_must_fail git fsck 2>err &&\n> +\tgrep \"bitmap file '\\''$midxbitmap'\\'' has invalid checksum\" err\n\nAnd we notice midx bitmap is corrupt.\n\nIs it worth checking at this point if we detect breakages in both\nbitmaps, if we have pack bitmap and midx bitmap, both broken?\n\nOther than these tiny nits, looking very good.\n\nThanks.\n"},{"id":"476371","messageId":"xmqqr0rz50m3.fsf@gitster.g","threadId":"59678","inReplyTo":"2162a352a46dc0c40afccc4defc88a1e8940ce6c.1682965296.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] fsck: use local repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-01T19:10:28Z","receivedAt":"2023-05-01T19:10:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <derrickstolee@github.com>\n>\n> In 0d30feef3c5 (fsck: create scaffolding for rev-index checks,\n> 2023-04-17) and later 5a6072f631d (fsck: validate .rev file header,\n> 2023-04-17), the check_pack_rev_indexes() method was created with a\n> 'struct repository *r' parameter. However, this parameter was unused and\n> instead 'the_repository' was used in its place.\n>\n> Fix this situation with the obvious replacement.\n\nIndeed obvious it is.  Thanks, will queue.\n\n>\n> Signed-off-by: Derrick Stolee <derrickstolee@github.com>\n> ---\n>  builtin/fsck.c | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/fsck.c b/builtin/fsck.c\n> index 75b30d1d00c..dcc165bf0c5 100644\n> --- a/builtin/fsck.c\n> +++ b/builtin/fsck.c\n> @@ -869,20 +869,20 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)\n>  \tint res = 0;\n>  \n>  \tif (show_progress) {\n> -\t\tfor (struct packed_git *p = get_all_packs(the_repository); p; p = p->next)\n> +\t\tfor (struct packed_git *p = get_all_packs(r); p; p = p->next)\n>  \t\t\tpack_count++;\n>  \t\tprogress = start_delayed_progress(\"Verifying reverse pack-indexes\", pack_count);\n>  \t\tpack_count = 0;\n>  \t}\n>  \n> -\tfor (struct packed_git *p = get_all_packs(the_repository); p; p = p->next) {\n> +\tfor (struct packed_git *p = get_all_packs(r); p; p = p->next) {\n>  \t\tint load_error = load_pack_revindex_from_disk(p);\n>  \n>  \t\tif (load_error < 0) {\n>  \t\t\terror(_(\"unable to load rev-index for pack '%s'\"), p->pack_name);\n>  \t\t\tres = ERROR_PACK_REV_INDEX;\n>  \t\t} else if (!load_error &&\n> -\t\t\t   !load_pack_revindex(the_repository, p) &&\n> +\t\t\t   !load_pack_revindex(r, p) &&\n>  \t\t\t   verify_pack_revindex(p)) {\n>  \t\t\terror(_(\"invalid rev-index for pack '%s'\"), p->pack_name);\n>  \t\t\tres = ERROR_PACK_REV_INDEX;\n"},{"id":"476418","messageId":"pull.1526.v2.git.1683034042.gitgitgadget@gmail.com","threadId":"59678","inReplyTo":"pull.1526.git.1682965295.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] fsck: verify .bitmap checksums, cleanup","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-02T13:27:20Z","receivedAt":"2023-05-02T13:27:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"Similar to the recent work on rev-indexes [1], add checks for .bitmap files\nto 'git fsck'.\n\nPatch 1 includes the full details for .bitmap files.\n\nPatch 2 includes a cleanup of ds/fsck-pack-revindex that I noticed while\npreparing this series.\n\n\nUpdates since v2\n================\n\n * The code for verify_bitmap_file() added checks for a negative file\n   descriptor, as well as some other cleanup.\n * The test in t5326 is expanded to include the \"both are corrupt\" case, as\n   well as a reorganization because the test was not confined to an inner\n   repo.\n\nThanks, Stolee\n\n[1]\nhttps://lore.kernel.org/git/pull.1512.git.1681748502.gitgitgadget@gmail.com/\n\nDerrick Stolee (2):\n  fsck: verify checksums of all .bitmap files\n  fsck: use local repository\n\n builtin/fsck.c                | 10 +++++---\n pack-bitmap.c                 | 45 +++++++++++++++++++++++++++++++++++\n pack-bitmap.h                 |  2 ++\n t/t5326-multi-pack-bitmaps.sh | 44 ++++++++++++++++++++++++++++++++++\n 4 files changed, 98 insertions(+), 3 deletions(-)\n\n\nbase-commit: 48d89b51b3bb8a60580c36731b96a7206ce1e5b9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1526%2Fderrickstolee%2Ffsck-bitmap-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1526/derrickstolee/fsck-bitmap-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1526\n\nRange-diff vs v1:\n\n 1:  d608d2faa86 ! 1:  5d6d6658633 fsck: verify checksums of all .bitmap files\n     @@ pack-bitmap.c: int bitmap_is_preferred_refname(struct repository *r, const char\n      +\tint fd = git_open(name);\n      +\tint res = 0;\n      +\n     -+\t/* A non-existent file is valid. */\n     -+\tif (fstat(fd, &st)) {\n     -+\t\tclose(fd);\n     ++\t/* It is OK to not have the file. */\n     ++\tif (fd < 0 || fstat(fd, &st)) {\n     ++\t\tif (fd >= 0)\n     ++\t\t\tclose(fd);\n      +\t\treturn 0;\n      +\t}\n      +\n     @@ t/t5326-multi-pack-bitmaps.sh: test_expect_success 'tagged commits are selected\n      +\tcp \"$packbitmap.bak\" \"$packbitmap\" &&\n      +\n      +\t# fsck works at first\n     -+\tgit fsck &&\n     ++\tgit fsck 2>err &&\n     ++\ttest_must_be_empty err &&\n      +\n      +\tcorrupt_file \"$packbitmap\" &&\n      +\ttest_must_fail git fsck 2>err &&\n     @@ t/t5326-multi-pack-bitmaps.sh: test_expect_success 'tagged commits are selected\n      +\tcp \"$packbitmap.bak\" \"$packbitmap\" &&\n      +\tcorrupt_file \"$midxbitmap\" &&\n      +\ttest_must_fail git fsck 2>err &&\n     -+\tgrep \"bitmap file '\\''$midxbitmap'\\'' has invalid checksum\" err\n     ++\tgrep \"bitmap file '\\''$midxbitmap'\\'' has invalid checksum\" err &&\n     ++\n     ++\tcorrupt_file \"$packbitmap\" &&\n     ++\ttest_must_fail git fsck 2>err &&\n     ++\tgrep \"bitmap file '\\''$midxbitmap'\\'' has invalid checksum\" err &&\n     ++\tgrep \"bitmap file '\\''$packbitmap'\\'' has invalid checksum\" err\n      +'\n      +\n       test_done\n 2:  2162a352a46 = 2:  00789bbc558 fsck: use local repository\n\n-- \ngitgitgadget\n"},{"id":"476419","messageId":"5d6d66586332a411b05b1f345e7456fda858ddb3.1683034042.git.gitgitgadget@gmail.com","threadId":"59678","inReplyTo":"pull.1526.v2.git.1683034042.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] fsck: verify checksums of all .bitmap files","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-02T13:27:21Z","receivedAt":"2023-05-02T13:27:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nIf a filesystem-level corruption occurs in a .bitmap file, Git can react\npoorly. This could take the form of a run-time error due to failing to\nparse an EWAH bitmap or be more subtle such as returning the wrong set\nof objects to a fetch or clone.\n\nA natural first response to either of these kinds of errors is to run\n'git fsck' to see if any files are corrupt. This currently ignores all\n.bitmap files.\n\nAdd checks to 'git fsck' for all .bitmap files that are currently\nassociated with a multi-pack-index or pack file. Verify their checksums\nusing the hashfile API.\n\nWe iterate through all multi-pack-indexes and pack-files to be sure to\ncheck all .bitmap files, not just the one that would be read by the\nprocess. For example, a multi-pack-index bitmap overrules a pack-bitmap.\nHowever, if the multi-pack-index is removed, the pack-bitmap may be\nselected instead. Be thorough to include every file that could become\nactive in such a way. This includes checking files in alternates.\n\nThere is potential that we could extend this effort to check the\nstructure of the reachability bitmaps themselves, but it is very\nexpensive to do so. At minimum, it's as expensive as generating the\nbitmaps in the first place, and that's assuming that we don't use the\ntrivial algorithm of verifying each bitmap individually. The trivial\nalgorithm will result in quadratic behavior (number of objects times\nnumber of bitmapped commits) while the bitmap building operation\nconstructs a lattice of commits to build bitmaps incrementally and then\ngenerate the final bitmaps from a subset of those commits.\n\nIf we were to extend 'git fsck' to check .bitmap file contents more\nclosely like this, then we would likely want to hide it behind an option\nthat signals the user is more willing to do expensive operations such as\nthis.\n\nFor testing, set up a repository with a pack-bitmap _and_ a\nmulti-pack-index bitmap. This requires some file movement to avoid\ndeleting the pack-bitmap during the repack that creates the\nmulti-pack-index bitmap. We can then verify that 'git fsck' is checking\nall files, not just the \"active\" bitmap.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n builtin/fsck.c                |  4 ++++\n pack-bitmap.c                 | 45 +++++++++++++++++++++++++++++++++++\n pack-bitmap.h                 |  2 ++\n t/t5326-multi-pack-bitmaps.sh | 44 ++++++++++++++++++++++++++++++++++\n 4 files changed, 95 insertions(+)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 2cd461b84c1..75b30d1d00c 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -27,6 +27,7 @@\n #include \"run-command.h\"\n #include \"worktree.h\"\n #include \"pack-revindex.h\"\n+#include \"pack-bitmap.h\"\n \n #define REACHABLE 0x0001\n #define SEEN      0x0002\n@@ -57,6 +58,7 @@ static int name_objects;\n #define ERROR_COMMIT_GRAPH 020\n #define ERROR_MULTI_PACK_INDEX 040\n #define ERROR_PACK_REV_INDEX 0100\n+#define ERROR_BITMAP 0200\n \n static const char *describe_object(const struct object_id *oid)\n {\n@@ -1056,6 +1058,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t}\n \n \terrors_found |= check_pack_rev_indexes(the_repository, show_progress);\n+\tif (verify_bitmap_files(the_repository))\n+\t\terrors_found |= ERROR_BITMAP;\n \n \tcheck_connectivity();\n \ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex e0fad723bf3..999f962602d 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -2346,3 +2346,48 @@ int bitmap_is_preferred_refname(struct repository *r, const char *refname)\n \n \treturn 0;\n }\n+\n+static int verify_bitmap_file(const char *name)\n+{\n+\tstruct stat st;\n+\tunsigned char *data;\n+\tint fd = git_open(name);\n+\tint res = 0;\n+\n+\t/* It is OK to not have the file. */\n+\tif (fd < 0 || fstat(fd, &st)) {\n+\t\tif (fd >= 0)\n+\t\t\tclose(fd);\n+\t\treturn 0;\n+\t}\n+\n+\tdata = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tclose(fd);\n+\tif (!hashfile_checksum_valid(data, st.st_size))\n+\t\tres = error(_(\"bitmap file '%s' has invalid checksum\"),\n+\t\t\t    name);\n+\n+\tmunmap(data, st.st_size);\n+\treturn res;\n+}\n+\n+int verify_bitmap_files(struct repository *r)\n+{\n+\tint res = 0;\n+\n+\tfor (struct multi_pack_index *m = get_multi_pack_index(r);\n+\t     m; m = m->next) {\n+\t\tchar *midx_bitmap_name = midx_bitmap_filename(m);\n+\t\tres |= verify_bitmap_file(midx_bitmap_name);\n+\t\tfree(midx_bitmap_name);\n+\t}\n+\n+\tfor (struct packed_git *p = get_all_packs(r);\n+\t     p; p = p->next) {\n+\t\tchar *pack_bitmap_name = pack_bitmap_filename(p);\n+\t\tres |= verify_bitmap_file(pack_bitmap_name);\n+\t\tfree(pack_bitmap_name);\n+\t}\n+\n+\treturn res;\n+}\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex f0180b5276b..84591f041bf 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -111,4 +111,6 @@ int bitmap_is_midx(struct bitmap_index *bitmap_git);\n const struct string_list *bitmap_preferred_tips(struct repository *r);\n int bitmap_is_preferred_refname(struct repository *r, const char *refname);\n \n+int verify_bitmap_files(struct repository *r);\n+\n #endif\ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 0882cbb6e4a..f771c442d41 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -434,4 +434,48 @@ test_expect_success 'tagged commits are selected for bitmapping' '\n \t)\n '\n \n+corrupt_file () {\n+\tchmod a+w \"$1\" &&\n+\tprintf \"bogus\" | dd of=\"$1\" bs=1 seek=\"12\" conv=notrunc\n+}\n+\n+test_expect_success 'git fsck correctly identifies good and bad bitmaps' '\n+\tgit init valid &&\n+\ttest_when_finished rm -rf valid &&\n+\n+\ttest_commit_bulk 20 &&\n+\tgit repack -adbf &&\n+\n+\t# Move pack-bitmap aside so it is not deleted\n+\t# in next repack.\n+\tpackbitmap=$(ls .git/objects/pack/pack-*.bitmap) &&\n+\tmv \"$packbitmap\" \"$packbitmap.bak\" &&\n+\n+\ttest_commit_bulk 10 &&\n+\tgit repack -b --write-midx &&\n+\tmidxbitmap=$(ls .git/objects/pack/multi-pack-index-*.bitmap) &&\n+\n+\t# Copy MIDX bitmap to backup. Copy pack bitmap from backup.\n+\tcp \"$midxbitmap\" \"$midxbitmap.bak\" &&\n+\tcp \"$packbitmap.bak\" \"$packbitmap\" &&\n+\n+\t# fsck works at first\n+\tgit fsck 2>err &&\n+\ttest_must_be_empty err &&\n+\n+\tcorrupt_file \"$packbitmap\" &&\n+\ttest_must_fail git fsck 2>err &&\n+\tgrep \"bitmap file '\\''$packbitmap'\\'' has invalid checksum\" err &&\n+\n+\tcp \"$packbitmap.bak\" \"$packbitmap\" &&\n+\tcorrupt_file \"$midxbitmap\" &&\n+\ttest_must_fail git fsck 2>err &&\n+\tgrep \"bitmap file '\\''$midxbitmap'\\'' has invalid checksum\" err &&\n+\n+\tcorrupt_file \"$packbitmap\" &&\n+\ttest_must_fail git fsck 2>err &&\n+\tgrep \"bitmap file '\\''$midxbitmap'\\'' has invalid checksum\" err &&\n+\tgrep \"bitmap file '\\''$packbitmap'\\'' has invalid checksum\" err\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"476420","messageId":"00789bbc55894a1677b4f1e7722e856d1ec1153f.1683034042.git.gitgitgadget@gmail.com","threadId":"59678","inReplyTo":"pull.1526.v2.git.1683034042.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] fsck: use local repository","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-05-02T13:27:22Z","receivedAt":"2023-05-02T13:27:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nIn 0d30feef3c5 (fsck: create scaffolding for rev-index checks,\n2023-04-17) and later 5a6072f631d (fsck: validate .rev file header,\n2023-04-17), the check_pack_rev_indexes() method was created with a\n'struct repository *r' parameter. However, this parameter was unused and\ninstead 'the_repository' was used in its place.\n\nFix this situation with the obvious replacement.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n builtin/fsck.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 75b30d1d00c..dcc165bf0c5 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -869,20 +869,20 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)\n \tint res = 0;\n \n \tif (show_progress) {\n-\t\tfor (struct packed_git *p = get_all_packs(the_repository); p; p = p->next)\n+\t\tfor (struct packed_git *p = get_all_packs(r); p; p = p->next)\n \t\t\tpack_count++;\n \t\tprogress = start_delayed_progress(\"Verifying reverse pack-indexes\", pack_count);\n \t\tpack_count = 0;\n \t}\n \n-\tfor (struct packed_git *p = get_all_packs(the_repository); p; p = p->next) {\n+\tfor (struct packed_git *p = get_all_packs(r); p; p = p->next) {\n \t\tint load_error = load_pack_revindex_from_disk(p);\n \n \t\tif (load_error < 0) {\n \t\t\terror(_(\"unable to load rev-index for pack '%s'\"), p->pack_name);\n \t\t\tres = ERROR_PACK_REV_INDEX;\n \t\t} else if (!load_error &&\n-\t\t\t   !load_pack_revindex(the_repository, p) &&\n+\t\t\t   !load_pack_revindex(r, p) &&\n \t\t\t   verify_pack_revindex(p)) {\n \t\t\terror(_(\"invalid rev-index for pack '%s'\"), p->pack_name);\n \t\t\tres = ERROR_PACK_REV_INDEX;\n-- \ngitgitgadget\n"}]}