{"thread":{"id":"59610","subject":"[PATCH 0/4] git fsck: check pack rev-index files","startedAt":"2023-04-17T16:21:48Z","lastAt":"2023-04-18T16:59:57Z","messageCount":19,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Taylor Blau","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"475527","messageId":"pull.1512.git.1681748502.gitgitgadget@gmail.com","threadId":"59610","inReplyTo":null,"subject":"[PATCH 0/4] git fsck: check pack rev-index files","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-17T16:21:37Z","receivedAt":"2023-04-17T16:21:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This series is based on tb/pack-revindex-on-disk.\n\nThe git fsck builtin does not look at the .rev files that pair with .pack\nand .idx files, but should. If these files suffer a bit flip on disk, then\nthe invalid data may cause fetches and clones from that repository to start\nfailing. The fix is simple: delete the .rev file (and regenerate, if\nnecessary), but detection is the first step.\n\nThis series adds those checks. The initial check to verify the checksum is\nprobably sufficient for most real-world scenarios, but going the extra mile\nto verify the rev-index contents against an in-memory rev-index helps us be\nsure that there isn't a bug in the rev-index writing code of Git (which\nwould result in a valid checksum). Much like other file formats, an invalid\nheader needs to be handled separately as a malformed header may prevent the\ndata structures from being initialized in the first place.\n\nThis series does not validate a multi-pack-index-<hash>.rev file or the\nrev-index chunk of a multi-pack-index file. These could be fast-follows,\nexcept that there is no existing equivalent for an in-memory rev-index for\neasy comparison. The rev-index chunk (which is the most-common way for a\nmulti-pack-index to have this information) is already covered by existing\nchecksum validation, at least.\n\nThanks, -Stolee\n\nDerrick Stolee (4):\n  fsck: create scaffolding for rev-index checks\n  fsck: check rev-index checksums\n  fsck: check rev-index position values\n  fsck: validate .rev file header\n\n builtin/fsck.c           | 36 +++++++++++++++++++\n pack-bitmap.c            |  4 +--\n pack-revindex.c          | 43 +++++++++++++++++++++--\n pack-revindex.h          | 16 +++++++++\n t/t5325-reverse-index.sh | 74 ++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 169 insertions(+), 4 deletions(-)\n\n\nbase-commit: 9f7f10a282d8adeb9da0990aa0eb2adf93a47ca7\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1512%2Fderrickstolee%2Ffsck-rev-indexes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1512/derrickstolee/fsck-rev-indexes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1512\n-- \ngitgitgadget\n"},{"id":"475528","messageId":"6bc3c56453ee2d0263210c233dbc946b5dbdcb4d.1681748502.git.gitgitgadget@gmail.com","threadId":"59610","inReplyTo":"pull.1512.git.1681748502.gitgitgadget@gmail.com","subject":"[PATCH 1/4] fsck: create scaffolding for rev-index checks","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-17T16:21:38Z","receivedAt":"2023-04-17T16:21:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nThe 'fsck' builtin checks many of Git's on-disk data structures, but\ndoes not currently validate the pack rev-index files (a .rev file to\npair with a .pack and .idx file).\n\nBefore doing a more-involved check process, create the scaffolding\nwithin builtin/fsck.c to have a new error type and add that error type\nwhen the API method verify_pack_revindex() returns an error. That method\ndoes nothing currently, but we will add checks to it in later changes.\n\nFor now, check that 'git fsck' succeeds without any errors in the normal\ncase. Future checks will be paired with tests that corrupt the .rev file\nappropriately.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n builtin/fsck.c           | 30 ++++++++++++++++++++++++++++++\n pack-revindex.c          | 11 +++++++++++\n pack-revindex.h          |  8 ++++++++\n t/t5325-reverse-index.sh | 14 ++++++++++++++\n 4 files changed, 63 insertions(+)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 095b39d3980..2ab78129bde 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -24,6 +24,7 @@\n #include \"resolve-undo.h\"\n #include \"run-command.h\"\n #include \"worktree.h\"\n+#include \"pack-revindex.h\"\n \n #define REACHABLE 0x0001\n #define SEEN      0x0002\n@@ -53,6 +54,7 @@ static int name_objects;\n #define ERROR_REFS 010\n #define ERROR_COMMIT_GRAPH 020\n #define ERROR_MULTI_PACK_INDEX 040\n+#define ERROR_PACK_REV_INDEX 0100\n \n static const char *describe_object(const struct object_id *oid)\n {\n@@ -856,6 +858,32 @@ static int mark_packed_for_connectivity(const struct object_id *oid,\n \treturn 0;\n }\n \n+static int check_pack_rev_indexes(struct repository *r, int show_progress)\n+{\n+\tstruct progress *progress = NULL;\n+\tuint32_t pack_count = 0;\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\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+\t\tif (!load_pack_revindex(the_repository, p) &&\n+\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+\t\t}\n+\t\tdisplay_progress(progress, ++pack_count);\n+\t}\n+\tstop_progress(&progress);\n+\n+\treturn res;\n+}\n+\n static char const * const fsck_usage[] = {\n \tN_(\"git fsck [--tags] [--root] [--unreachable] [--cache] [--no-reflogs]\\n\"\n \t   \"         [--[no-]full] [--strict] [--verbose] [--lost-found]\\n\"\n@@ -1019,6 +1047,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\tfree_worktrees(worktrees);\n \t}\n \n+\terrors_found |= check_pack_rev_indexes(the_repository, show_progress);\n+\n \tcheck_connectivity();\n \n \tif (the_repository->settings.core_commit_graph) {\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 29f5358b256..c3f2aaa3fea 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -301,6 +301,17 @@ int load_pack_revindex(struct repository *r, struct packed_git *p)\n \treturn -1;\n }\n \n+/*\n+ * verify_pack_revindex verifies that the on-disk rev-index for the given\n+ * pack-file is the same that would be created if written from scratch.\n+ *\n+ * A negative number is returned on error.\n+ */\n+int verify_pack_revindex(struct packed_git *p)\n+{\n+\treturn 0;\n+}\n+\n int load_midx_revindex(struct multi_pack_index *m)\n {\n \tstruct strbuf revindex_name = STRBUF_INIT;\ndiff --git a/pack-revindex.h b/pack-revindex.h\nindex 46e834064e1..c8861873b02 100644\n--- a/pack-revindex.h\n+++ b/pack-revindex.h\n@@ -51,6 +51,14 @@ struct repository;\n  */\n int load_pack_revindex(struct repository *r, struct packed_git *p);\n \n+/*\n+ * verify_pack_revindex verifies that the on-disk rev-index for the given\n+ * pack-file is the same that would be created if written from scratch.\n+ *\n+ * A negative number is returned on error.\n+ */\n+int verify_pack_revindex(struct packed_git *p);\n+\n /*\n  * load_midx_revindex loads the '.rev' file corresponding to the given\n  * multi-pack index by mmap-ing it and assigning pointers in the\ndiff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\nindex 0548fce1aa6..206c412f50b 100755\n--- a/t/t5325-reverse-index.sh\n+++ b/t/t5325-reverse-index.sh\n@@ -131,4 +131,18 @@ test_expect_success 'revindex in-memory vs on-disk' '\n \t\ttest_cmp on-disk in-core\n \t)\n '\n+\n+test_expect_success 'fsck succeeds on good rev-index' '\n+\ttest_when_finished rm -fr repo &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit commit &&\n+\t\tgit -c pack.writeReverseIndex=true repack -ad &&\n+\t\tgit fsck 2>err &&\n+\t\ttest_must_be_empty err\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"475529","messageId":"7db4ec3e327ed3695f4f5409cb2dc80c72688758.1681748502.git.gitgitgadget@gmail.com","threadId":"59610","inReplyTo":"pull.1512.git.1681748502.gitgitgadget@gmail.com","subject":"[PATCH 2/4] fsck: check rev-index checksums","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-17T16:21:39Z","receivedAt":"2023-04-17T16:21:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nThe previous change added calls to verify_pack_revindex() in\nbuiltin/fsck.c, but the implementation of the method was left empty. Add\nthe first and most-obvious check to this method: checksum verification.\n\nWhile here, create a helper method in the test script that makes it easy\nto adjust the .rev file and check that 'git fsck' reports the correct\nerror message.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n pack-revindex.c          | 10 ++++++++++\n t/t5325-reverse-index.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 50 insertions(+)\n\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex c3f2aaa3fea..007a806994f 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -5,6 +5,7 @@\n #include \"packfile.h\"\n #include \"config.h\"\n #include \"midx.h\"\n+#include \"csum-file.h\"\n \n struct revindex_entry {\n \toff_t offset;\n@@ -309,6 +310,15 @@ int load_pack_revindex(struct repository *r, struct packed_git *p)\n  */\n int verify_pack_revindex(struct packed_git *p)\n {\n+\t/* Do not bother checking if not initialized. */\n+\tif (!p->revindex_map)\n+\t\treturn 0;\n+\n+\tif (!hashfile_checksum_valid((const unsigned char *)p->revindex_map, p->revindex_size)) {\n+\t\terror(_(\"invalid checksum\"));\n+\t\treturn -1;\n+\t}\n+\n \treturn 0;\n }\n \ndiff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\nindex 206c412f50b..6b7c709a1f6 100755\n--- a/t/t5325-reverse-index.sh\n+++ b/t/t5325-reverse-index.sh\n@@ -145,4 +145,44 @@ test_expect_success 'fsck succeeds on good rev-index' '\n \t)\n '\n \n+test_expect_success 'set up rev-index corruption tests' '\n+\tgit init corrupt &&\n+\t(\n+\t\tcd corrupt &&\n+\n+\t\ttest_commit commit &&\n+\t\tgit -c pack.writeReverseIndex=true repack -ad &&\n+\n+\t\trevfile=$(ls .git/objects/pack/pack-*.rev) &&\n+\t\tchmod a+w $revfile &&\n+\t\tcp $revfile $revfile.bak\n+\t)\n+'\n+\n+corrupt_rev_and_verify () {\n+\t(\n+\t\tpos=\"$1\" &&\n+\t\tvalue=\"$2\" &&\n+\t\terror=\"$3\" &&\n+\n+\t\tcd corrupt &&\n+\t\trevfile=$(ls .git/objects/pack/pack-*.rev) &&\n+\n+\t\t# Reset to original rev-file.\n+\t\tcp $revfile.bak $revfile &&\n+\n+\t\tprintf \"$value\" | dd of=$revfile bs=1 seek=\"$pos\" conv=notrunc &&\n+\t\ttest_must_fail git fsck 2>err &&\n+\t\tgrep \"$error\" err\n+\t)\n+}\n+\n+test_expect_success 'fsck catches invalid checksum' '\n+\trevfile=$(ls corrupt/.git/objects/pack/pack-*.rev) &&\n+\torig_size=$(wc -c <$revfile) &&\n+\thashpos=$((orig_size - 10)) &&\n+\tcorrupt_rev_and_verify $hashpos bogus \\\n+\t\t\"invalid checksum\"\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"475531","messageId":"adbe9c8ee90e087e864bd9e0c67338974b5dbc8d.1681748502.git.gitgitgadget@gmail.com","threadId":"59610","inReplyTo":"pull.1512.git.1681748502.gitgitgadget@gmail.com","subject":"[PATCH 3/4] fsck: check rev-index position values","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-17T16:21:40Z","receivedAt":"2023-04-17T16:22:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nWhen checking a rev-index file, it may be helpful to identify exactly\nwhich positions are incorrect. Compare the rev-index to a\nfreshly-computed in-memory rev-index and report the comparison failures.\n\nThis additional check (on top of the checksum validation) can help find\nfiles that were corrupt by a single bit flip on-disk or perhaps were\nwritten incorrectly due to a bug in Git.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n pack-revindex.c          | 25 +++++++++++++++++++++----\n t/t5325-reverse-index.sh |  5 +++++\n 2 files changed, 26 insertions(+), 4 deletions(-)\n\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 007a806994f..62a9846470c 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -310,16 +310,33 @@ int load_pack_revindex(struct repository *r, struct packed_git *p)\n  */\n int verify_pack_revindex(struct packed_git *p)\n {\n+\tint res = 0;\n+\n \t/* Do not bother checking if not initialized. */\n-\tif (!p->revindex_map)\n-\t\treturn 0;\n+\tif (!p->revindex_map || !p->revindex_data)\n+\t\treturn res;\n \n \tif (!hashfile_checksum_valid((const unsigned char *)p->revindex_map, p->revindex_size)) {\n \t\terror(_(\"invalid checksum\"));\n-\t\treturn -1;\n+\t\tres = -1;\n \t}\n \n-\treturn 0;\n+\t/* This may fail due to a broken .idx. */\n+\tif (create_pack_revindex_in_memory(p))\n+\t\treturn res;\n+\n+\tfor (size_t i = 0; i < p->num_objects; i++) {\n+\t\tuint32_t nr = p->revindex[i].nr;\n+\t\tuint32_t rev_val = get_be32(p->revindex_data + i);\n+\n+\t\tif (nr != rev_val) {\n+\t\t\terror(_(\"invalid rev-index position at %\"PRIu64\": %\"PRIu32\" != %\"PRIu32\"\"),\n+\t\t\t      (uint64_t)i, nr, rev_val);\n+\t\t\tres = -1;\n+\t\t}\n+\t}\n+\n+\treturn res;\n }\n \n int load_midx_revindex(struct multi_pack_index *m)\ndiff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\nindex 6b7c709a1f6..5c3c80f88f0 100755\n--- a/t/t5325-reverse-index.sh\n+++ b/t/t5325-reverse-index.sh\n@@ -185,4 +185,9 @@ test_expect_success 'fsck catches invalid checksum' '\n \t\t\"invalid checksum\"\n '\n \n+test_expect_success 'fsck catches invalid row position' '\n+\tcorrupt_rev_and_verify 14 \"\\07\" \\\n+\t\t\"invalid rev-index position\"\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"475532","messageId":"7d894d859ef109091b86bc4e2e4f6cea0e808370.1681748502.git.gitgitgadget@gmail.com","threadId":"59610","inReplyTo":"pull.1512.git.1681748502.gitgitgadget@gmail.com","subject":"[PATCH 4/4] fsck: validate .rev file header","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-17T16:21:41Z","receivedAt":"2023-04-17T16:22:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nWhile parsing a .rev file, we check the header information to be sure it\nmakes sense. This happens before doing any additional validation such as\na checksum or value check. In order to differentiate between a bad\nheader and a non-existent file, we need to update the API for loading a\nreverse index.\n\nMake load_pack_revindex_from_disk() non-static and specify that a\npositive value means \"the file does not exist\" while other errors during\nparsing are negative values. Since an invalid header prevents setting up\nthe structures we would use for further validations, we can stop at that\npoint.\n\nThe place where we can distinguish between a missing file and a corrupt\nfile is inside load_revindex_from_disk(), which is used both by pack\nrev-indexes and multi-pack-index rev-indexes. Some tests in t5326\ndemonstrate that it is critical to take some conditions to allow\npositive error signals.\n\nAdd tests that check the three header values.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n builtin/fsck.c           | 10 ++++++++--\n pack-bitmap.c            |  4 ++--\n pack-revindex.c          |  5 +++--\n pack-revindex.h          |  8 ++++++++\n t/t5325-reverse-index.sh | 15 +++++++++++++++\n 5 files changed, 36 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 2ab78129bde..2414190c049 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -872,8 +872,14 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)\n \t}\n \n \tfor (struct packed_git *p = get_all_packs(the_repository); p; p = p->next) {\n-\t\tif (!load_pack_revindex(the_repository, p) &&\n-\t\t    verify_pack_revindex(p)) {\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   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 \t\t}\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 38b35c48237..3828aab612a 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -379,7 +379,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n \t\tgoto cleanup;\n \t}\n \n-\tif (load_midx_revindex(bitmap_git->midx) < 0) {\n+\tif (load_midx_revindex(bitmap_git->midx)) {\n \t\twarning(_(\"multi-pack bitmap is missing required reverse index\"));\n \t\tgoto cleanup;\n \t}\n@@ -2140,7 +2140,7 @@ uint32_t *create_bitmap_mapping(struct bitmap_index *bitmap_git,\n \n \tif (!bitmap_is_midx(bitmap_git))\n \t\tload_reverse_index(r, bitmap_git);\n-\telse if (load_midx_revindex(bitmap_git->midx) < 0)\n+\telse if (load_midx_revindex(bitmap_git->midx))\n \t\tBUG(\"rebuild_existing_bitmaps: missing required rev-cache \"\n \t\t    \"extension\");\n \ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 62a9846470c..146334e2c96 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -212,7 +212,8 @@ static int load_revindex_from_disk(char *revindex_name,\n \tfd = git_open(revindex_name);\n \n \tif (fd < 0) {\n-\t\tret = -1;\n+\t\t/* \"No file\" means return 1. */\n+\t\tret = 1;\n \t\tgoto cleanup;\n \t}\n \tif (fstat(fd, &st)) {\n@@ -264,7 +265,7 @@ cleanup:\n \treturn ret;\n }\n \n-static int load_pack_revindex_from_disk(struct packed_git *p)\n+int load_pack_revindex_from_disk(struct packed_git *p)\n {\n \tchar *revindex_name;\n \tint ret;\ndiff --git a/pack-revindex.h b/pack-revindex.h\nindex c8861873b02..6dd47efea10 100644\n--- a/pack-revindex.h\n+++ b/pack-revindex.h\n@@ -51,6 +51,14 @@ struct repository;\n  */\n int load_pack_revindex(struct repository *r, struct packed_git *p);\n \n+/*\n+ * Specifically load a pack revindex from disk.\n+ *\n+ * Returns 0 on success, 1 on \"no .rev file\", and -1 when there is an\n+ * error parsing the .rev file.\n+ */\n+int load_pack_revindex_from_disk(struct packed_git *p);\n+\n /*\n  * verify_pack_revindex verifies that the on-disk rev-index for the given\n  * pack-file is the same that would be created if written from scratch.\ndiff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\nindex 5c3c80f88f0..431a603ca0e 100755\n--- a/t/t5325-reverse-index.sh\n+++ b/t/t5325-reverse-index.sh\n@@ -190,4 +190,19 @@ test_expect_success 'fsck catches invalid row position' '\n \t\t\"invalid rev-index position\"\n '\n \n+test_expect_success 'fsck catches invalid header: magic number' '\n+\tcorrupt_rev_and_verify 1 \"\\07\" \\\n+\t\t\"reverse-index file .* has unknown signature\"\n+'\n+\n+test_expect_success 'fsck catches invalid header: version' '\n+\tcorrupt_rev_and_verify 7 \"\\02\" \\\n+\t\t\"reverse-index file .* has unsupported version\"\n+'\n+\n+test_expect_success 'fsck catches invalid header: hash function' '\n+\tcorrupt_rev_and_verify 11 \"\\03\" \\\n+\t\t\"reverse-index file .* has unsupported hash id\"\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"475563","messageId":"xmqqa5z6jilb.fsf@gitster.g","threadId":"59610","inReplyTo":"pull.1512.git.1681748502.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] git fsck: check pack rev-index files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-17T21:37:36Z","receivedAt":"2023-04-17T21:37:40Z","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> This series is based on tb/pack-revindex-on-disk.\n>\n> The git fsck builtin does not look at the .rev files that pair with .pack\n> and .idx files, but should.\n\nYes, it should.\n\n> ... The fix is simple: delete the .rev file (and regenerate, if\n> necessary), but detection is the first step.\n\n> This series adds those checks. The initial check to verify the checksum is\n> probably sufficient for most real-world scenarios, but going the extra mile\n> to verify the rev-index contents against an in-memory rev-index helps us be\n> sure that there isn't a bug in the rev-index writing code of Git (which\n> would result in a valid checksum).\n\nGood description.\n\n> Much like other file formats, an invalid\n> header needs to be handled separately as a malformed header may prevent the\n> data structures from being initialized in the first place.\n\n> This series does not validate a multi-pack-index-<hash>.rev file or the\n> rev-index chunk of a multi-pack-index file. These could be fast-follows,\n> except that there is no existing equivalent for an in-memory rev-index for\n> easy comparison. The rev-index chunk (which is the most-common way for a\n> multi-pack-index to have this information) is already covered by existing\n> checksum validation, at least.\n\nTIL what \"fast-follow\" means ;-)\n"},{"id":"475564","messageId":"xmqq1qkijhho.fsf@gitster.g","threadId":"59610","inReplyTo":"adbe9c8ee90e087e864bd9e0c67338974b5dbc8d.1681748502.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] fsck: check rev-index position values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-17T22:01:23Z","receivedAt":"2023-04-17T22:01:29Z","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>  \tif (!hashfile_checksum_valid((const unsigned char *)p->revindex_map, p->revindex_size)) {\n>  \t\terror(_(\"invalid checksum\"));\n> -\t\treturn -1;\n> +\t\tres = -1;\n>  \t}\n>  \n> -\treturn 0;\n> +\t/* This may fail due to a broken .idx. */\n> +\tif (create_pack_revindex_in_memory(p))\n> +\t\treturn res;\n\nHere, if the revindex file itself is self consistent, res would\nstill be 0, so we will end up silently returning.  Is the idea that\nwhile whatever causes in-memory revindex fail to be computed is a\nconcern from the point of view of the repository's health, it would\nbe caught elsewhere as a problem for the <pack,idx> pair we are\nseeing here?\n\n> +\tfor (size_t i = 0; i < p->num_objects; i++) {\n> +\t\tuint32_t nr = p->revindex[i].nr;\n> +\t\tuint32_t rev_val = get_be32(p->revindex_data + i);\n> +\n> +\t\tif (nr != rev_val) {\n> +\t\t\terror(_(\"invalid rev-index position at %\"PRIu64\": %\"PRIu32\" != %\"PRIu32\"\"),\n> +\t\t\t      (uint64_t)i, nr, rev_val);\n> +\t\t\tres = -1;\n> +\t\t}\n> +\t}\n> +\n> +\treturn res;\n>  }\n>  \n>  int load_midx_revindex(struct multi_pack_index *m)\n> diff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\n> index 6b7c709a1f6..5c3c80f88f0 100755\n> --- a/t/t5325-reverse-index.sh\n> +++ b/t/t5325-reverse-index.sh\n> @@ -185,4 +185,9 @@ test_expect_success 'fsck catches invalid checksum' '\n>  \t\t\"invalid checksum\"\n>  '\n>  \n> +test_expect_success 'fsck catches invalid row position' '\n> +\tcorrupt_rev_and_verify 14 \"\\07\" \\\n\n;-)\n\nI wondered how the patch made this \"\\07\" portably available; it is\nfed as the format string to printf in the helper, which is clever\nbut obviously correct way to do this.  Nice.\n\n> +\t\t\"invalid rev-index position\"\n> +'\n> +\n>  test_done\n"},{"id":"475565","messageId":"xmqqwn2ai291.fsf@gitster.g","threadId":"59610","inReplyTo":"7db4ec3e327ed3695f4f5409cb2dc80c72688758.1681748502.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] fsck: check rev-index checksums","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-17T22:15:54Z","receivedAt":"2023-04-17T22:16:38Z","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> +corrupt_rev_and_verify () {\n> +\t(\n> +\t\tpos=\"$1\" &&\n> +...\n> +\t\tgrep \"$error\" err\n> +\t)\n> +}\n\nCurious.  Would it work equally well to write\n\n\tcorrupt_rev_and_verify () (\n\t\tpos=\"$1\" &&\n\t\t...\n\t\tgrep \"$error\" err\n\t)\n\nwithout an extra level of indentation?\n"},{"id":"475566","messageId":"ZD3GEIsPptxRetUV@nand.local","threadId":"59610","inReplyTo":"6bc3c56453ee2d0263210c233dbc946b5dbdcb4d.1681748502.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] fsck: create scaffolding for rev-index checks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-17T22:20:00Z","receivedAt":"2023-04-17T22:20:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 17, 2023 at 04:21:38PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee <derrickstolee@github.com>\n>\n> The 'fsck' builtin checks many of Git's on-disk data structures, but\n> does not currently validate the pack rev-index files (a .rev file to\n> pair with a .pack and .idx file).\n>\n> Before doing a more-involved check process, create the scaffolding\n> within builtin/fsck.c to have a new error type and add that error type\n> when the API method verify_pack_revindex() returns an error. That method\n> does nothing currently, but we will add checks to it in later changes.\n>\n> For now, check that 'git fsck' succeeds without any errors in the normal\n> case. Future checks will be paired with tests that corrupt the .rev file\n> appropriately.\n>\n> Signed-off-by: Derrick Stolee <derrickstolee@github.com>\n> ---\n>  builtin/fsck.c           | 30 ++++++++++++++++++++++++++++++\n>  pack-revindex.c          | 11 +++++++++++\n>  pack-revindex.h          |  8 ++++++++\n>  t/t5325-reverse-index.sh | 14 ++++++++++++++\n>  4 files changed, 63 insertions(+)\n>\n> diff --git a/builtin/fsck.c b/builtin/fsck.c\n> index 095b39d3980..2ab78129bde 100644\n> --- a/builtin/fsck.c\n> +++ b/builtin/fsck.c\n> @@ -24,6 +24,7 @@\n>  #include \"resolve-undo.h\"\n>  #include \"run-command.h\"\n>  #include \"worktree.h\"\n> +#include \"pack-revindex.h\"\n>\n>  #define REACHABLE 0x0001\n>  #define SEEN      0x0002\n> @@ -53,6 +54,7 @@ static int name_objects;\n>  #define ERROR_REFS 010\n>  #define ERROR_COMMIT_GRAPH 020\n>  #define ERROR_MULTI_PACK_INDEX 040\n> +#define ERROR_PACK_REV_INDEX 0100\n>\n>  static const char *describe_object(const struct object_id *oid)\n>  {\n> @@ -856,6 +858,32 @@ static int mark_packed_for_connectivity(const struct object_id *oid,\n>  \treturn 0;\n>  }\n>\n> +static int check_pack_rev_indexes(struct repository *r, int show_progress)\n> +{\n> +\tstruct progress *progress = NULL;\n> +\tuint32_t pack_count = 0;\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\nIt's going to take me a while to get used to these declarations inside\nof for-loops!\n\n> +\t\t\tpack_count++;\n> +\t\tprogress = start_delayed_progress(\"Verifying reverse pack-indexes\", pack_count);\n\nI wonder if we want to count over the sum of objects in packs rather\nthan the number of packs themselves. My worry would be that a rather\nlarge pack would make it appear as if nothing is happening when in\nreality we're just churning through a lot of objects.\n\n> +\t\tpack_count = 0;\n> +\t}\n> +\n> +\tfor (struct packed_git *p = get_all_packs(the_repository); p; p = p->next) {\n> +\t\tif (!load_pack_revindex(the_repository, p) &&\n\nI was going to comment that I wasn't sure if `load_pack_revindex()` was\nthe right thing here, since we don't care about validating the\non-the-fly reverse indexes that we generate.\n\nBut I see in your 3/4 that you are comparing the values on disk to those\nin memory, which is very nice.\n\n> +\t\t    verify_pack_revindex(p)) {\n\nInside of verify_pack_revindex(), it says that a negative number is\nreturned on error. Do we care about disambiguating >= 0 here? IOW,\nshould this be:\n\n    if (!load_pack_revindex(the_repository, p) || verify_pack_revindex(p) < 0)\n\n?\n\nAll looking good otherwise.\n\nThanks,\nTaylor\n"},{"id":"475567","messageId":"ZD3HI/LIXVv6Pacn@nand.local","threadId":"59610","inReplyTo":"7db4ec3e327ed3695f4f5409cb2dc80c72688758.1681748502.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] fsck: check rev-index checksums","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-17T22:24:35Z","receivedAt":"2023-04-17T22:24:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 17, 2023 at 04:21:39PM +0000, Derrick Stolee via GitGitGadget wrote:\n> @@ -309,6 +310,15 @@ int load_pack_revindex(struct repository *r, struct packed_git *p)\n>   */\n>  int verify_pack_revindex(struct packed_git *p)\n>  {\n> +\t/* Do not bother checking if not initialized. */\n\nYep, makes sense; if we don't have an on-disk reverse index (which is\nmmap'd into `revindex_map` we don't have anything to verify), so we can\nbail here.\n\n> +\tif (!p->revindex_map)\n> +\t\treturn 0;\n> +\n> +\tif (!hashfile_checksum_valid((const unsigned char *)p->revindex_map, p->revindex_size)) {\n> +\t\terror(_(\"invalid checksum\"));\n> +\t\treturn -1;\n> +\t}\n> +\n>  \treturn 0;\n>  }\n>\n> diff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\n> index 206c412f50b..6b7c709a1f6 100755\n> --- a/t/t5325-reverse-index.sh\n> +++ b/t/t5325-reverse-index.sh\n> @@ -145,4 +145,44 @@ test_expect_success 'fsck succeeds on good rev-index' '\n>  \t)\n>  '\n>\n> +test_expect_success 'set up rev-index corruption tests' '\n\ns/set up/setup for easy `--run`-ing (e.g., ./t5325-*.sh --run=setup,fsck).\n\n> +\tgit init corrupt &&\n> +\t(\n> +\t\tcd corrupt &&\n> +\n> +\t\ttest_commit commit &&\n> +\t\tgit -c pack.writeReverseIndex=true repack -ad &&\n> +\n> +\t\trevfile=$(ls .git/objects/pack/pack-*.rev) &&\n> +\t\tchmod a+w $revfile &&\n> +\t\tcp $revfile $revfile.bak\n> +\t)\n> +'\n> +\n> +corrupt_rev_and_verify () {\n> +\t(\n> +\t\tpos=\"$1\" &&\n> +\t\tvalue=\"$2\" &&\n> +\t\terror=\"$3\" &&\n> +\n> +\t\tcd corrupt &&\n> +\t\trevfile=$(ls .git/objects/pack/pack-*.rev) &&\n> +\n> +\t\t# Reset to original rev-file.\n> +\t\tcp $revfile.bak $revfile &&\n> +\n> +\t\tprintf \"$value\" | dd of=$revfile bs=1 seek=\"$pos\" conv=notrunc &&\n> +\t\ttest_must_fail git fsck 2>err &&\n> +\t\tgrep \"$error\" err\n> +\t)\n> +}\n> +\n> +test_expect_success 'fsck catches invalid checksum' '\n> +\trevfile=$(ls corrupt/.git/objects/pack/pack-*.rev) &&\n\nWould this test be tighter if we introduced a sub-shell and cd'd into\n\"corrupt\" here?\n\n> +\torig_size=$(wc -c <$revfile) &&\n\nI'm nitpicking, but we may want to use `test_file_size` here instead of\n`wc -c`. The latter outnumbers the former in terms of number of uses,\nbut I think we consider `test_file_size` to be canonical these days.\n\n> +\thashpos=$((orig_size - 10)) &&\n> +\tcorrupt_rev_and_verify $hashpos bogus \\\n> +\t\t\"invalid checksum\"\n> +'\n\nThis looks good.\n\nThanks,\nTaylor\n"},{"id":"475568","messageId":"ZD3NscowbkXizxCv@nand.local","threadId":"59610","inReplyTo":"adbe9c8ee90e087e864bd9e0c67338974b5dbc8d.1681748502.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] fsck: check rev-index position values","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-17T22:52:33Z","receivedAt":"2023-04-17T22:52:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 17, 2023 at 04:21:40PM +0000, Derrick Stolee via GitGitGadget wrote:\n> +\tfor (size_t i = 0; i < p->num_objects; i++) {\n> +\t\tuint32_t nr = p->revindex[i].nr;\n> +\t\tuint32_t rev_val = get_be32(p->revindex_data + i);\n> +\n> +\t\tif (nr != rev_val) {\n> +\t\t\terror(_(\"invalid rev-index position at %\"PRIu64\": %\"PRIu32\" != %\"PRIu32\"\"),\n> +\t\t\t      (uint64_t)i, nr, rev_val);\n> +\t\t\tres = -1;\n> +\t\t}\n> +\t}\n> +\n> +\treturn res;\n\nVery clever.\n\nI thought about it for a while, and I am pretty confident that this is\nthe cleanest you can get this loop to be. You could do something like\nbuilding the forward index out of inverting the values in the .rev file,\nbut everything I came up with based on that ended up being circular and\nnot demonstrating anything interesting at all.\n\nThanks,\nTaylor\n"},{"id":"475616","messageId":"c787f354-1cd8-1f27-0559-b0603c259d22@github.com","threadId":"59610","inReplyTo":"xmqqwn2ai291.fsf@gitster.g","subject":"Re: [PATCH 2/4] fsck: check rev-index checksums","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-18T14:24:28Z","receivedAt":"2023-04-18T14:24:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/17/2023 6:15 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> +corrupt_rev_and_verify () {\n>> +\t(\n>> +\t\tpos=\"$1\" &&\n>> +...\n>> +\t\tgrep \"$error\" err\n>> +\t)\n>> +}\n> \n> Curious.  Would it work equally well to write\n> \n> \tcorrupt_rev_and_verify () (\n> \t\tpos=\"$1\" &&\n> \t\t...\n> \t\tgrep \"$error\" err\n> \t)\n> \n> without an extra level of indentation?\n\nI've never seen that replacement (and it only exists in our tree\nin t4018/bash-arithmetic-function and t4018/bash-subshell-function)\nbut it works and looks nicer.\n\nThanks,\n-Stolee\n"},{"id":"475617","messageId":"2628249e-fe9a-d15c-5414-33d815b35cd1@github.com","threadId":"59610","inReplyTo":"ZD3HI/LIXVv6Pacn@nand.local","subject":"Re: [PATCH 2/4] fsck: check rev-index checksums","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-18T14:27:57Z","receivedAt":"2023-04-18T14:28:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/17/2023 6:24 PM, Taylor Blau wrote:\n> On Mon, Apr 17, 2023 at 04:21:39PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n>> +test_expect_success 'set up rev-index corruption tests' '\n> \n> s/set up/setup for easy `--run`-ing (e.g., ./t5325-*.sh --run=setup,fsck).\n\nMakes sense.\n\n>> +test_expect_success 'fsck catches invalid checksum' '\n>> +\trevfile=$(ls corrupt/.git/objects/pack/pack-*.rev) &&\n> \n> Would this test be tighter if we introduced a sub-shell and cd'd into\n> \"corrupt\" here?\n\ncorrupt_rev_and_verify does the subshell thing. Why should we do that\nhere in the test?\n \n>> +\torig_size=$(wc -c <$revfile) &&\n> \n> I'm nitpicking, but we may want to use `test_file_size` here instead of\n> `wc -c`. The latter outnumbers the former in terms of number of uses,\n> but I think we consider `test_file_size` to be canonical these days.\n\nMakes sense.\n\nThanks,\n-Stolee\n"},{"id":"475618","messageId":"e29e6584-709f-d172-e73a-88a2027a9283@github.com","threadId":"59610","inReplyTo":"xmqq1qkijhho.fsf@gitster.g","subject":"Re: [PATCH 3/4] fsck: check rev-index position values","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-18T14:32:13Z","receivedAt":"2023-04-18T14:32:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/17/2023 6:01 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>>  \tif (!hashfile_checksum_valid((const unsigned char *)p->revindex_map, p->revindex_size)) {\n>>  \t\terror(_(\"invalid checksum\"));\n>> -\t\treturn -1;\n>> +\t\tres = -1;\n>>  \t}\n>>  \n>> -\treturn 0;\n>> +\t/* This may fail due to a broken .idx. */\n>> +\tif (create_pack_revindex_in_memory(p))\n>> +\t\treturn res;\n> \n> Here, if the revindex file itself is self consistent, res would\n> still be 0, so we will end up silently returning.  Is the idea that\n> while whatever causes in-memory revindex fail to be computed is a\n> concern from the point of view of the repository's health, it would\n> be caught elsewhere as a problem for the <pack,idx> pair we are\n> seeing here?\n\nThis is something I noticed during a test suite run, where the .idx\nwas corrupt, so the .rev file could not be checked. In this situation,\nwe are not using the .rev file for anything since Git processes would\nfail to lookup objects in the associated packfile.\n\nIn this case, we have no way to validate the rest of the .rev file's\ncontents, but at least we checked its header and checksum, which is\nthe best we can hope to do in this case.\n\nThanks,\n-Stolee\n"},{"id":"475619","messageId":"ZD6ua4dSynRWmW2a@nand.local","threadId":"59610","inReplyTo":"2628249e-fe9a-d15c-5414-33d815b35cd1@github.com","subject":"Re: [PATCH 2/4] fsck: check rev-index checksums","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T14:51:23Z","receivedAt":"2023-04-18T14:51:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 18, 2023 at 10:27:57AM -0400, Derrick Stolee wrote:\n> >> +test_expect_success 'fsck catches invalid checksum' '\n> >> +\trevfile=$(ls corrupt/.git/objects/pack/pack-*.rev) &&\n> >\n> > Would this test be tighter if we introduced a sub-shell and cd'd into\n> > \"corrupt\" here?\n>\n> corrupt_rev_and_verify does the subshell thing. Why should we do that\n> here in the test?\n\nI was thinking that it might be more concise if you moved the subshell\nto the test and out of corrupt_rev_and_verify. In addition to making\ncorrupt_rev_and_verify work in other instances where the repository\nisn't required to be in a directory named \"corrupt\", I think it\nsimplifies the result.\n\nHere's what I was thinking, as a diff on top of this patch:\n\n--- >8 ---\ndiff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh\nindex 6b7c709a1f..7dfbaf6b37 100755\n--- a/t/t5325-reverse-index.sh\n+++ b/t/t5325-reverse-index.sh\n@@ -160,29 +160,30 @@ test_expect_success 'set up rev-index corruption tests' '\n '\n\n corrupt_rev_and_verify () {\n-\t(\n-\t\tpos=\"$1\" &&\n-\t\tvalue=\"$2\" &&\n-\t\terror=\"$3\" &&\n+\tpos=\"$1\" &&\n+\tvalue=\"$2\" &&\n+\terror=\"$3\" &&\n\n-\t\tcd corrupt &&\n-\t\trevfile=$(ls .git/objects/pack/pack-*.rev) &&\n+\trevfile=$(ls .git/objects/pack/pack-*.rev) &&\n\n-\t\t# Reset to original rev-file.\n-\t\tcp $revfile.bak $revfile &&\n+\t# Reset to original rev-file.\n+\tcp $revfile.bak $revfile &&\n\n-\t\tprintf \"$value\" | dd of=$revfile bs=1 seek=\"$pos\" conv=notrunc &&\n-\t\ttest_must_fail git fsck 2>err &&\n-\t\tgrep \"$error\" err\n-\t)\n+\tprintf \"$value\" | dd of=$revfile bs=1 seek=\"$pos\" conv=notrunc &&\n+\ttest_must_fail git fsck 2>err &&\n+\tgrep \"$error\" err\n }\n\n test_expect_success 'fsck catches invalid checksum' '\n-\trevfile=$(ls corrupt/.git/objects/pack/pack-*.rev) &&\n-\torig_size=$(wc -c <$revfile) &&\n-\thashpos=$((orig_size - 10)) &&\n-\tcorrupt_rev_and_verify $hashpos bogus \\\n-\t\t\"invalid checksum\"\n+\t(\n+\t\tcd corrupt &&\n+\n+\t\trevfile=$(ls .git/objects/pack/pack-*.rev) &&\n+\t\torig_size=$(wc -c <$revfile) &&\n+\t\thashpos=$((orig_size - 10)) &&\n+\t\tcorrupt_rev_and_verify $hashpos bogus \\\n+\t\t\t\"invalid checksum\"\n+\t)\n '\n\n test_done\n--- 8< ---\n\nIf you do take my suggestion, make sure to remember to come back in\npatches 3/4 and 4/4 and adjust those instances of\n'corrupt_rev_and_verify' to first change into \"corrupt\".\n\nThanks,\nTaylor\n"},{"id":"475624","messageId":"fe72e184-a252-dab4-e9aa-cf53e1499976@github.com","threadId":"59610","inReplyTo":"ZD6ua4dSynRWmW2a@nand.local","subject":"Re: [PATCH 2/4] fsck: check rev-index checksums","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-04-18T14:57:15Z","receivedAt":"2023-04-18T14:57:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/18/2023 10:51 AM, Taylor Blau wrote:\n> On Tue, Apr 18, 2023 at 10:27:57AM -0400, Derrick Stolee wrote:\n>>>> +test_expect_success 'fsck catches invalid checksum' '\n>>>> +\trevfile=$(ls corrupt/.git/objects/pack/pack-*.rev) &&\n>>>\n>>> Would this test be tighter if we introduced a sub-shell and cd'd into\n>>> \"corrupt\" here?\n>>\n>> corrupt_rev_and_verify does the subshell thing. Why should we do that\n>> here in the test?\n> \n> I was thinking that it might be more concise if you moved the subshell\n> to the test and out of corrupt_rev_and_verify. In addition to making\n> corrupt_rev_and_verify work in other instances where the repository\n> isn't required to be in a directory named \"corrupt\", I think it\n> simplifies the result.\n\nI don't think there is a good reason to allow using a different repo\nname. This is the only test that requires doing anything but calling\ncorrupt_rev_and_verify with different parameters, so I think this\nmakes the test script at the end of the series noisier.\n\nThanks,\n-Stolee\n"},{"id":"475625","messageId":"ZD6xJR7vrCzvW+Xw@nand.local","threadId":"59610","inReplyTo":"fe72e184-a252-dab4-e9aa-cf53e1499976@github.com","subject":"Re: [PATCH 2/4] fsck: check rev-index checksums","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T15:03:01Z","receivedAt":"2023-04-18T15:03:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 18, 2023 at 10:57:15AM -0400, Derrick Stolee wrote:\n> On 4/18/2023 10:51 AM, Taylor Blau wrote:\n> > On Tue, Apr 18, 2023 at 10:27:57AM -0400, Derrick Stolee wrote:\n> >>>> +test_expect_success 'fsck catches invalid checksum' '\n> >>>> +\trevfile=$(ls corrupt/.git/objects/pack/pack-*.rev) &&\n> >>>\n> >>> Would this test be tighter if we introduced a sub-shell and cd'd into\n> >>> \"corrupt\" here?\n> >>\n> >> corrupt_rev_and_verify does the subshell thing. Why should we do that\n> >> here in the test?\n> >\n> > I was thinking that it might be more concise if you moved the subshell\n> > to the test and out of corrupt_rev_and_verify. In addition to making\n> > corrupt_rev_and_verify work in other instances where the repository\n> > isn't required to be in a directory named \"corrupt\", I think it\n> > simplifies the result.\n>\n> I don't think there is a good reason to allow using a different repo\n> name. This is the only test that requires doing anything but calling\n> corrupt_rev_and_verify with different parameters, so I think this\n> makes the test script at the end of the series noisier.\n\nNo worries. I was thinking that it might be convenient in the future if\nwe wanted to corrupt a .rev file in a different repository, but that's\nabsolutely a bridge we can cross if/when we get to it.\n\nThanks,\nTaylor\n"},{"id":"475627","messageId":"ZD615MLzKVwk1jVT@nand.local","threadId":"59610","inReplyTo":"pull.1512.git.1681748502.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] git fsck: check pack rev-index files","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-18T15:23:16Z","receivedAt":"2023-04-18T15:23:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 17, 2023 at 04:21:37PM +0000, Derrick Stolee via GitGitGadget wrote:\n>  builtin/fsck.c           | 36 +++++++++++++++++++\n>  pack-bitmap.c            |  4 +--\n>  pack-revindex.c          | 43 +++++++++++++++++++++--\n>  pack-revindex.h          | 16 +++++++++\n>  t/t5325-reverse-index.sh | 74 ++++++++++++++++++++++++++++++++++++++++\n>  5 files changed, 169 insertions(+), 4 deletions(-)\n\nI gave this a thorough look and it all looks good to me.\n\nI left a couple of minor comments throughout, e.g., s/set up/setup, and\nreplacing 'wc -c' with 'test_file_size', but I doubt either of those are\na big enough deal to merit a reroll.\n\nSo I'd be happy to see this start moving forward as-is. I think that the\ntopic it depends on, tb/pack-revindex-on-disk is similarly ready for\nmerging.\n\nThanks,\nTaylor\n"},{"id":"475633","messageId":"xmqqjzy9dt3a.fsf@gitster.g","threadId":"59610","inReplyTo":"ZD615MLzKVwk1jVT@nand.local","subject":"Re: [PATCH 0/4] git fsck: check pack rev-index files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-18T16:59:37Z","receivedAt":"2023-04-18T16:59:57Z","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 17, 2023 at 04:21:37PM +0000, Derrick Stolee via GitGitGadget wrote:\n>>  builtin/fsck.c           | 36 +++++++++++++++++++\n>>  pack-bitmap.c            |  4 +--\n>>  pack-revindex.c          | 43 +++++++++++++++++++++--\n>>  pack-revindex.h          | 16 +++++++++\n>>  t/t5325-reverse-index.sh | 74 ++++++++++++++++++++++++++++++++++++++++\n>>  5 files changed, 169 insertions(+), 4 deletions(-)\n>\n> I gave this a thorough look and it all looks good to me.\n\nThis looked good to me, too.  I was planning to wait for a few days\nto see what others find.\n\nThanks.\n"}]}