# [PATCH v2 1/5] fsck: order 'fsck_msg_type' alphabetically 18 messages from 2025-09-02 to 2025-09-18. Participants: Karthik Nayak, Junio C Hamano, Patrick Steinhardt, shejialuo. Thread: https://gitlist.dev/t/64068 ## Karthik Nayak, 2025-09-02 07:05 Subject: [PATCH v2 0/5] refs/reftable: add fsck checks Message-ID: <20250902-228-reftable-introduce-consistency-checks-v2-0-4f96b3834779@gmail.com> URL: https://gitlist.dev/e/20250902-228-reftable-introduce-consistency-checks-v2-0-4f96b3834779%40gmail.com In-Reply-To: <20250819-228-reftable-introduce-consistency-checks-v1-0-8b8f6879fa9e@gmail.com> ``` This series adds the required infrastructure and also some fsck checks for the reftable backend. Since the reftable backend is treated as a library within the Git codebase, we don't want to spillover our internal fsck implementation into the library. At the same time, the fsck checks need to access internal structures of the reftable library which aren't exposed outside the library. So we solve this by adding a 'reftable/fsck.[ch]' which implements and exposes a checker for the reftable library and returns specific errors as defined by the library. We then add glue code within 'refs/reftable-backend.c' to map these errors to errors which Git's fsck implementation would understand. This allows us to separate concerns. This series then adds some checks on the stack ('reftable/tables.list') level of reftable, namely: 1. The table name is as per the spec 2. The number of tables are consistent 3. The tables.list has a newline at the end of file 4. The table names follow correct index sequences I also plan to send in follow up series's which will implement further checks and go into deeper layers (tables, block, references). Signed-off-by: Karthik Nayak --- Changes in v2: - Ensured that 'struct reftable_fsck_info' is passed around as a pointer, this provides a smaller footprint (pointer size vs struct size). - Run FSCK checks for other worktrees too, even if one of them fails. - Separate messaging for table name vs table check and add additional test. - Use the relative path in messages used. - Small style and typo fixes. - Link to v1: https://lore.kernel.org/r/20250819-228-reftable-introduce-consistency-checks-v1-0-8b8f6879fa9e@gmail.com --- Documentation/fsck-msgids.adoc | 15 +++- Makefile | 1 + fsck.h | 154 ++++++++++++++++++++------------------- meson.build | 1 + refs/reftable-backend.c | 66 +++++++++++++++-- reftable/fsck.c | 134 ++++++++++++++++++++++++++++++++++ reftable/reftable-fsck.h | 44 +++++++++++ t/meson.build | 3 +- t/t0614-reftable-fsck.sh | 161 +++++++++++++++++++++++++++++++++++++++++ 9 files changed, 494 insertions(+), 85 deletions(-) Karthik Nayak (5): fsck: order 'fsck_msg_type' alphabetically refs/reftable: add fsck check for checking the table name refs/reftable: add fsck check for number of tables refs/reftable: add fsck check for trailing newline refs/reftable: add fsck check for incorrect update index Range-diff versus v1: 1: d1875fbbc7 = 1: c049cd428a fsck: order 'fsck_msg_type' alphabetically 2: b63799aad1 ! 2: 1e46786745 refs/reftable: add fsck check for checking the table name @@ Commit message Also add 'badReftableTableName' as a corresponding error within Git. Add a test to check for this behavior. + While here, remove a unused header `#include "../lockfile.h"` from + 'refs/reftable-backend.c'. + Signed-off-by: Karthik Nayak ## Documentation/fsck-msgids.adoc ## @@ refs/reftable-backend.c: static int reftable_be_reflog_expire(struct ref_store * + fprintf_ln(stderr, "%s", _(msg)); +} + -+static int reftable_fsck_error_handler(struct reftable_fsck_info info, ++static int reftable_fsck_error_handler(struct reftable_fsck_info *info, + void *cb_data) +{ ++ struct fsck_ref_report report = { .path = info->path }; + struct fsck_options *o = cb_data; -+ struct fsck_ref_report report = { .path = info.path }; + enum fsck_msg_id msg_id; + -+ switch (info.error) { ++ switch (info->error) { + case REFTABLE_FSCK_ERROR_TABLE_NAME: + msg_id = FSCK_MSG_BAD_REFTABLE_TABLE_NAME; + break; + default: -+ BUG("unknown fsck error: %d", info.error); ++ BUG("unknown fsck error: %d", info->error); + } + -+ return fsck_report_ref(o, &report, msg_id, "%s", info.msg); ++ return fsck_report_ref(o, &report, msg_id, "%s", info->msg); +} + +static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o, @@ refs/reftable-backend.c: static int reftable_be_reflog_expire(struct ref_store * + if (o->verbose) + fprintf_ln(stderr, _("Checking references consistency")); + -+ ret = reftable_fsck_check(refs->main_backend.stack, reftable_fsck_error_handler, ++ ret |= reftable_fsck_check(refs->main_backend.stack, reftable_fsck_error_handler, + reftable_fsck_verbose_handler, o); -+ if (!ret) -+ return ret; + + strmap_for_each_entry(&refs->worktree_backends, &iter, entry) { + struct reftable_backend *b = (struct reftable_backend *)entry->value; -+ ret = reftable_fsck_check(b->stack, reftable_fsck_error_handler, ++ ret |= reftable_fsck_check(b->stack, reftable_fsck_error_handler, + reftable_fsck_verbose_handler, o); -+ if (!ret) -+ return ret; + } + + return ret; @@ reftable/fsck.c (new) + reftable_fsck_verbose_fn verbose_fn, + void *cb_data) +{ ++ + char **names = NULL; + uint64_t min, max; + int err = 0; @@ reftable/fsck.c (new) + struct reftable_fsck_info info = { + .error = REFTABLE_FSCK_ERROR_TABLE_NAME, + .path = names[i], -+ .msg = "invalid reftable name" + }; + uint32_t rnd; + /* @@ reftable/fsck.c (new) + + if (sscanf(names[i], "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x%5s", + &min, &max, &rnd, tail) != 4) { -+ err = report_fn(info, cb_data); ++ info.msg = "invalid reftable table name"; ++ err = report_fn(&info, cb_data); ++ continue; + } + + if (strcmp(tail, ".ref")) { -+ err = report_fn(info, cb_data); ++ info.msg = "invalid reftable table extension"; ++ err = report_fn(&info, cb_data); + } + } + @@ reftable/reftable-fsck.h (new) + REFTABLE_FSCK_ERROR_TABLE_NAME = -1, +}; + -+/* Represents an individual error encounctered during the FSCK checks. */ ++/* Represents an individual error encountered during the FSCK checks. */ +struct reftable_fsck_info { + enum reftable_fsck_error error; + const char *msg; + const char *path; +}; + -+typedef int reftable_fsck_report_fn(struct reftable_fsck_info info, ++typedef int reftable_fsck_report_fn(struct reftable_fsck_info *info, + void *cb_data); +typedef void reftable_fsck_verbose_fn(const char *msg, void *cb_data); + @@ reftable/reftable-fsck.h (new) + * + * If an issue is encountered, the issue is reported to the callee via the + * provided 'report_fn'. If the issue is non-recoverable the flow will not -+ * conitnue. If it is recoverable, the flow will continue and further issues ++ * continue. If it is recoverable, the flow will continue and further issues + * will be reported as identified. + * + * The 'verbose_fn' will be invoked to provide verbose information about @@ t/t0614-reftable-fsck.sh (new) + test_must_be_empty err && + + TABLE_NAME=$(cat .git/reftable/tables.list | head -n1) && ++ sed "1s/^/extra/" .git/reftable/tables.list >.git/reftable/tables.list.tmp && ++ mv .git/reftable/tables.list.tmp .git/reftable/tables.list && ++ mv .git/reftable/${TABLE_NAME} .git/reftable/extra${TABLE_NAME} && ++ ++ test_must_fail git refs verify 2>err && ++ cat >expect <<-EOF && ++ error: extra${TABLE_NAME}: badReftableTableName: invalid reftable table name ++ EOF ++ test_cmp expect err ++ ) ++' ++ ++test_expect_success 'table name should be checked' ' ++ test_when_finished "rm -rf repo" && ++ git init repo && ++ ( ++ cd repo && ++ git commit --allow-empty -m initial && ++ ++ git refs verify 2>err && ++ test_must_be_empty err && ++ ++ TABLE_NAME=$(cat .git/reftable/tables.list | head -n1) && + sed "1s/$/extra/" .git/reftable/tables.list >.git/reftable/tables.list.tmp && + mv .git/reftable/tables.list.tmp .git/reftable/tables.list && + mv .git/reftable/${TABLE_NAME} .git/reftable/${TABLE_NAME}extra && + + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && -+ error: ${TABLE_NAME}extra: badReftableTableName: invalid reftable name ++ error: ${TABLE_NAME}extra: badReftableTableName: invalid reftable table extension + EOF + test_cmp expect err + ) 3: 4c6c99ded3 ! 3: 52fc14fdeb refs/reftable: add fsck check for number of tables @@ fsck.h: enum fsck_msg_type { FUNC(BAD_REF_FILETYPE, ERROR) \ ## refs/reftable-backend.c ## -@@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_fsck_info info, +@@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_fsck_info *info, case REFTABLE_FSCK_ERROR_TABLE_NAME: msg_id = FSCK_MSG_BAD_REFTABLE_TABLE_NAME; break; @@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_ + msg_id = FSCK_MSG_BAD_REFTABLE_STACK_COUNT; + break; default: - BUG("unknown fsck error: %d", info.error); + BUG("unknown fsck error: %d", info->error); } ## reftable/fsck.c ## @@ reftable/fsck.c: int reftable_fsck_check(struct reftable_stack *stack, + if (!reftable_fsck_valid_stack_count(stack)) { + struct reftable_fsck_info info = { + .error = REFTABLE_FSCK_ERROR_STACK_COUNT, -+ .path = stack->list_file, ++ .path = "reftable/tables.list", + .msg = "mismatch in number of tables" + }; + -+ err = report_fn(info, cb_data); ++ err = report_fn(&info, cb_data); + } + out: @@ reftable/reftable-fsck.h + REFTABLE_FSCK_ERROR_STACK_COUNT = -2, }; - /* Represents an individual error encounctered during the FSCK checks. */ + /* Represents an individual error encountered during the FSCK checks. */ ## t/t0614-reftable-fsck.sh ## @@ t/t0614-reftable-fsck.sh: test_expect_success 'table name should be checked' ' @@ t/t0614-reftable-fsck.sh: test_expect_success 'table name should be checked' ' + + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && -+ error: $(pwd)/.git/reftable/tables.list: badReftableStackCount: mismatch in number of tables ++ error: reftable/tables.list: badReftableStackCount: mismatch in number of tables + EOF + test_cmp expect err + ) 4: 7e8a14c77e ! 4: 4099878ceb refs/reftable: add fsck check for trailing newline @@ fsck.h: enum fsck_msg_type { FUNC(BAD_REF_FILETYPE, ERROR) \ ## refs/reftable-backend.c ## -@@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_fsck_info info, +@@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_fsck_info *info, case REFTABLE_FSCK_ERROR_STACK_COUNT: msg_id = FSCK_MSG_BAD_REFTABLE_STACK_COUNT; break; @@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_ + msg_id = FSCK_MSG_BAD_REFTABLE_STACK_LIST_NEWLINE; + break; default: - BUG("unknown fsck error: %d", info.error); + BUG("unknown fsck error: %d", info->error); } ## reftable/fsck.c ## @@ reftable/fsck.c: int reftable_fsck_check(struct reftable_stack *stack, + if (!reftable_fsck_stack_contains_newline(stack->list_file)) { + struct reftable_fsck_info info = { + .error = REFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE, -+ .path = stack->list_file, ++ .path = "reftable/tables.list", + .msg = "trailing newline missing in stack list" + }; + -+ err = report_fn(info, cb_data); ++ err = report_fn(&info, cb_data); + } + verbose_fn("Checking reftable tables count", cb_data); @@ reftable/reftable-fsck.h: enum reftable_fsck_error { + REFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE = -3, }; - /* Represents an individual error encounctered during the FSCK checks. */ + /* Represents an individual error encountered during the FSCK checks. */ ## t/t0614-reftable-fsck.sh ## @@ t/t0614-reftable-fsck.sh: test_expect_success 'table count should be checked' ' @@ t/t0614-reftable-fsck.sh: test_expect_success 'table count should be checked' ' + + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && -+ error: $(pwd)/.git/reftable/tables.list: badReftableStackListNewline: trailing newline missing in stack list ++ error: reftable/tables.list: badReftableStackListNewline: trailing newline missing in stack list + EOF + test_cmp expect err + ) 5: 56ee4348d5 ! 5: e33345088b refs/reftable: add fsck check for incorrect update index @@ fsck.h: enum fsck_msg_type { FUNC(BAD_REF_NAME, ERROR) \ ## refs/reftable-backend.c ## -@@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_fsck_info info, +@@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_fsck_info *info, case REFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE: msg_id = FSCK_MSG_BAD_REFTABLE_STACK_LIST_NEWLINE; break; @@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_ + msg_id = FSCK_MSG_BAD_REFTABLE_UPDATE_INDEX; + break; default: - BUG("unknown fsck error: %d", info.error); + BUG("unknown fsck error: %d", info->error); } ## reftable/fsck.c ## @@ reftable/fsck.c: int reftable_fsck_check(struct reftable_stack *stack, reftable_fsck_verbose_fn verbose_fn, void *cb_data) { +- + uint64_t min, max, prev_max = 0; char **names = NULL; - uint64_t min, max; @@ reftable/fsck.c: int reftable_fsck_check(struct reftable_stack *stack, if (stack == NULL) @@ reftable/fsck.c: int reftable_fsck_check(struct reftable_stack *stack, - err = report_fn(info, cb_data); + continue; } + if (min != (prev_max + 1) || max < min) { @@ reftable/fsck.c: int reftable_fsck_check(struct reftable_stack *stack, + .msg = "incorrect update index in table name" + }; + -+ err = report_fn(info, cb_data); ++ err = report_fn(&info, cb_data); + } + if (strcmp(tail, ".ref")) { - err = report_fn(info, cb_data); + info.msg = "invalid reftable table extension"; + err = report_fn(&info, cb_data); } + + prev_max = max; @@ reftable/reftable-fsck.h: enum reftable_fsck_error { + REFTABLE_FSCK_ERROR_UPDATE_INDEX = -4, }; - /* Represents an individual error encounctered during the FSCK checks. */ + /* Represents an individual error encountered during the FSCK checks. */ ## t/t0614-reftable-fsck.sh ## @@ t/t0614-reftable-fsck.sh: test_expect_success 'stack list must contain trailing newline' ' base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0 change-id: 20250714-228-reftable-introduce-consistency-checks-379ded93c544 Thanks - Karthik ``` ## Karthik Nayak, 2025-09-02 07:05 Subject: [PATCH v2 1/5] fsck: order 'fsck_msg_type' alphabetically Message-ID: <20250902-228-reftable-introduce-consistency-checks-v2-1-4f96b3834779@gmail.com> URL: https://gitlist.dev/e/20250902-228-reftable-introduce-consistency-checks-v2-1-4f96b3834779%40gmail.com In-Reply-To: <20250902-228-reftable-introduce-consistency-checks-v2-0-4f96b3834779@gmail.com> ``` The list of 'fsck_msg_type' seem to be alphabetically ordered, but there are a few small misses. Fix this by sorting the sub-sections of the list to maintain alphabetical ordering. Also fix a clang-format issue where the escaped newlines are not aligned. While here, remove a duplicate instance of 'gitmodulesLarge' in the 'fsck-msgids' documentation. Signed-off-by: Karthik Nayak --- Documentation/fsck-msgids.adoc | 3 - fsck.h | 150 ++++++++++++++++++++--------------------- 2 files changed, 75 insertions(+), 78 deletions(-) diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc index 0ba4f9a27e..1c912615f9 100644 --- a/Documentation/fsck-msgids.adoc +++ b/Documentation/fsck-msgids.adoc @@ -104,9 +104,6 @@ `gitmodulesParse`:: (INFO) Could not parse `.gitmodules` blob. -`gitmodulesLarge`; - (ERROR) `.gitmodules` blob is too large to parse. - `gitmodulesPath`:: (ERROR) `.gitmodules` path is invalid. diff --git a/fsck.h b/fsck.h index dd7df3d5b3..559ad57807 100644 --- a/fsck.h +++ b/fsck.h @@ -20,82 +20,82 @@ enum fsck_msg_type { * two in sync. */ -#define FOREACH_FSCK_MSG_ID(FUNC) \ - /* fatal errors */ \ - FUNC(NUL_IN_HEADER, FATAL) \ - FUNC(UNTERMINATED_HEADER, FATAL) \ - /* errors */ \ - FUNC(BAD_DATE, ERROR) \ - FUNC(BAD_DATE_OVERFLOW, ERROR) \ - FUNC(BAD_EMAIL, ERROR) \ - FUNC(BAD_NAME, ERROR) \ - FUNC(BAD_OBJECT_SHA1, ERROR) \ - FUNC(BAD_PACKED_REF_ENTRY, ERROR) \ - FUNC(BAD_PACKED_REF_HEADER, ERROR) \ - FUNC(BAD_PARENT_SHA1, ERROR) \ - FUNC(BAD_REF_CONTENT, ERROR) \ - FUNC(BAD_REF_FILETYPE, ERROR) \ - FUNC(BAD_REF_NAME, ERROR) \ - FUNC(BAD_REFERENT_NAME, ERROR) \ - FUNC(BAD_TIMEZONE, ERROR) \ - FUNC(BAD_TREE, ERROR) \ - FUNC(BAD_TREE_SHA1, ERROR) \ - FUNC(BAD_TYPE, ERROR) \ - FUNC(DUPLICATE_ENTRIES, ERROR) \ - FUNC(MISSING_AUTHOR, ERROR) \ - FUNC(MISSING_COMMITTER, ERROR) \ - FUNC(MISSING_EMAIL, ERROR) \ - FUNC(MISSING_NAME_BEFORE_EMAIL, ERROR) \ - FUNC(MISSING_OBJECT, ERROR) \ - FUNC(MISSING_SPACE_BEFORE_DATE, ERROR) \ - FUNC(MISSING_SPACE_BEFORE_EMAIL, ERROR) \ - FUNC(MISSING_TAG, ERROR) \ - FUNC(MISSING_TAG_ENTRY, ERROR) \ - FUNC(MISSING_TREE, ERROR) \ - FUNC(MISSING_TYPE, ERROR) \ - FUNC(MISSING_TYPE_ENTRY, ERROR) \ - FUNC(MULTIPLE_AUTHORS, ERROR) \ - FUNC(PACKED_REF_ENTRY_NOT_TERMINATED, ERROR) \ - FUNC(PACKED_REF_UNSORTED, ERROR) \ - FUNC(TREE_NOT_SORTED, ERROR) \ - FUNC(UNKNOWN_TYPE, ERROR) \ - FUNC(ZERO_PADDED_DATE, ERROR) \ - FUNC(GITMODULES_MISSING, ERROR) \ - FUNC(GITMODULES_BLOB, ERROR) \ - FUNC(GITMODULES_LARGE, ERROR) \ - FUNC(GITMODULES_NAME, ERROR) \ - FUNC(GITMODULES_SYMLINK, ERROR) \ - FUNC(GITMODULES_URL, ERROR) \ - FUNC(GITMODULES_PATH, ERROR) \ - FUNC(GITMODULES_UPDATE, ERROR) \ - FUNC(GITATTRIBUTES_MISSING, ERROR) \ - FUNC(GITATTRIBUTES_LARGE, ERROR) \ - FUNC(GITATTRIBUTES_LINE_LENGTH, ERROR) \ - FUNC(GITATTRIBUTES_BLOB, ERROR) \ - /* warnings */ \ - FUNC(EMPTY_NAME, WARN) \ - FUNC(FULL_PATHNAME, WARN) \ - FUNC(HAS_DOT, WARN) \ - FUNC(HAS_DOTDOT, WARN) \ - FUNC(HAS_DOTGIT, WARN) \ - FUNC(NULL_SHA1, WARN) \ - FUNC(ZERO_PADDED_FILEMODE, WARN) \ - FUNC(NUL_IN_COMMIT, WARN) \ - FUNC(LARGE_PATHNAME, WARN) \ +#define FOREACH_FSCK_MSG_ID(FUNC) \ + /* fatal errors */ \ + FUNC(NUL_IN_HEADER, FATAL) \ + FUNC(UNTERMINATED_HEADER, FATAL) \ + /* errors */ \ + FUNC(BAD_DATE, ERROR) \ + FUNC(BAD_DATE_OVERFLOW, ERROR) \ + FUNC(BAD_EMAIL, ERROR) \ + FUNC(BAD_NAME, ERROR) \ + FUNC(BAD_OBJECT_SHA1, ERROR) \ + FUNC(BAD_PACKED_REF_ENTRY, ERROR) \ + FUNC(BAD_PACKED_REF_HEADER, ERROR) \ + FUNC(BAD_PARENT_SHA1, ERROR) \ + FUNC(BAD_REFERENT_NAME, ERROR) \ + FUNC(BAD_REF_CONTENT, ERROR) \ + FUNC(BAD_REF_FILETYPE, ERROR) \ + FUNC(BAD_REF_NAME, ERROR) \ + FUNC(BAD_TIMEZONE, ERROR) \ + FUNC(BAD_TREE, ERROR) \ + FUNC(BAD_TREE_SHA1, ERROR) \ + FUNC(BAD_TYPE, ERROR) \ + FUNC(DUPLICATE_ENTRIES, ERROR) \ + FUNC(GITATTRIBUTES_BLOB, ERROR) \ + FUNC(GITATTRIBUTES_LARGE, ERROR) \ + FUNC(GITATTRIBUTES_LINE_LENGTH, ERROR) \ + FUNC(GITATTRIBUTES_MISSING, ERROR) \ + FUNC(GITMODULES_BLOB, ERROR) \ + FUNC(GITMODULES_LARGE, ERROR) \ + FUNC(GITMODULES_MISSING, ERROR) \ + FUNC(GITMODULES_NAME, ERROR) \ + FUNC(GITMODULES_PATH, ERROR) \ + FUNC(GITMODULES_SYMLINK, ERROR) \ + FUNC(GITMODULES_UPDATE, ERROR) \ + FUNC(GITMODULES_URL, ERROR) \ + FUNC(MISSING_AUTHOR, ERROR) \ + FUNC(MISSING_COMMITTER, ERROR) \ + FUNC(MISSING_EMAIL, ERROR) \ + FUNC(MISSING_NAME_BEFORE_EMAIL, ERROR) \ + FUNC(MISSING_OBJECT, ERROR) \ + FUNC(MISSING_SPACE_BEFORE_DATE, ERROR) \ + FUNC(MISSING_SPACE_BEFORE_EMAIL, ERROR) \ + FUNC(MISSING_TAG, ERROR) \ + FUNC(MISSING_TAG_ENTRY, ERROR) \ + FUNC(MISSING_TREE, ERROR) \ + FUNC(MISSING_TYPE, ERROR) \ + FUNC(MISSING_TYPE_ENTRY, ERROR) \ + FUNC(MULTIPLE_AUTHORS, ERROR) \ + FUNC(PACKED_REF_ENTRY_NOT_TERMINATED, ERROR) \ + FUNC(PACKED_REF_UNSORTED, ERROR) \ + FUNC(TREE_NOT_SORTED, ERROR) \ + FUNC(UNKNOWN_TYPE, ERROR) \ + FUNC(ZERO_PADDED_DATE, ERROR) \ + /* warnings */ \ + FUNC(EMPTY_NAME, WARN) \ + FUNC(FULL_PATHNAME, WARN) \ + FUNC(HAS_DOT, WARN) \ + FUNC(HAS_DOTDOT, WARN) \ + FUNC(HAS_DOTGIT, WARN) \ + FUNC(LARGE_PATHNAME, WARN) \ + FUNC(NULL_SHA1, WARN) \ + FUNC(NUL_IN_COMMIT, WARN) \ + FUNC(ZERO_PADDED_FILEMODE, WARN) \ /* infos (reported as warnings, but ignored by default) */ \ - FUNC(BAD_FILEMODE, INFO) \ - FUNC(EMPTY_PACKED_REFS_FILE, INFO) \ - FUNC(GITMODULES_PARSE, INFO) \ - FUNC(GITIGNORE_SYMLINK, INFO) \ - FUNC(GITATTRIBUTES_SYMLINK, INFO) \ - FUNC(MAILMAP_SYMLINK, INFO) \ - FUNC(BAD_TAG_NAME, INFO) \ - FUNC(MISSING_TAGGER_ENTRY, INFO) \ - FUNC(SYMLINK_REF, INFO) \ - FUNC(REF_MISSING_NEWLINE, INFO) \ - FUNC(SYMREF_TARGET_IS_NOT_A_REF, INFO) \ - FUNC(TRAILING_REF_CONTENT, INFO) \ - /* ignored (elevated when requested) */ \ + FUNC(BAD_FILEMODE, INFO) \ + FUNC(BAD_TAG_NAME, INFO) \ + FUNC(EMPTY_PACKED_REFS_FILE, INFO) \ + FUNC(GITATTRIBUTES_SYMLINK, INFO) \ + FUNC(GITIGNORE_SYMLINK, INFO) \ + FUNC(GITMODULES_PARSE, INFO) \ + FUNC(MAILMAP_SYMLINK, INFO) \ + FUNC(MISSING_TAGGER_ENTRY, INFO) \ + FUNC(REF_MISSING_NEWLINE, INFO) \ + FUNC(SYMLINK_REF, INFO) \ + FUNC(SYMREF_TARGET_IS_NOT_A_REF, INFO) \ + FUNC(TRAILING_REF_CONTENT, INFO) \ + /* ignored (elevated when requested) */ \ FUNC(EXTRA_HEADER_ENTRY, IGNORE) #define MSG_ID(id, msg_type) FSCK_MSG_##id, -- 2.50.1 ``` ## Karthik Nayak, 2025-09-02 07:05 Subject: [PATCH v2 2/5] refs/reftable: add fsck check for checking the table name Message-ID: <20250902-228-reftable-introduce-consistency-checks-v2-2-4f96b3834779@gmail.com> URL: https://gitlist.dev/e/20250902-228-reftable-introduce-consistency-checks-v2-2-4f96b3834779%40gmail.com In-Reply-To: <20250902-228-reftable-introduce-consistency-checks-v2-0-4f96b3834779@gmail.com> ``` The `git refs verify` command is used to run fsck checks on the reference backends. This command is also invoked when users run 'git fsck'. While the files-backend has some fsck checks added, the reftable backend lacks such checks. Let's add the required infrastructure and a check to test for the table names in the 'tables.list' of reftables. For the infrastructure, since the reftable library is treated as an independent library we should ensure that the library code works independently without knowledge about Git's internals. To do this, add both 'reftable/fsck.c' and 'reftable/reftable-fsck.h'. Which provide an entry point 'reftable_fsck_check' for running fsck checks over a provided reftable stack. The callee provides the function with callbacks to handle issue and information reporting. Add glue code in 'refs/reftable-backend.c' which calls the reftable library to perform the fsck checks. Here we also map the reftable errors to Git' fsck errors. Introduce a check to validate table names for a given reftable stack. Also add 'badReftableTableName' as a corresponding error within Git. Add a test to check for this behavior. While here, remove a unused header `#include "../lockfile.h"` from 'refs/reftable-backend.c'. Signed-off-by: Karthik Nayak --- Documentation/fsck-msgids.adoc | 3 +++ Makefile | 1 + fsck.h | 1 + meson.build | 1 + refs/reftable-backend.c | 57 ++++++++++++++++++++++++++++++++++++----- reftable/fsck.c | 53 ++++++++++++++++++++++++++++++++++++++ reftable/reftable-fsck.h | 38 +++++++++++++++++++++++++++ t/meson.build | 3 ++- t/t0614-reftable-fsck.sh | 58 ++++++++++++++++++++++++++++++++++++++++++ 9 files changed, 208 insertions(+), 7 deletions(-) diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc index 1c912615f9..784ddc0df5 100644 --- a/Documentation/fsck-msgids.adoc +++ b/Documentation/fsck-msgids.adoc @@ -38,6 +38,9 @@ `badReferentName`:: (ERROR) The referent name of a symref is invalid. +`badReftableTableName`:: + (ERROR) A reftable table has an invalid name. + `badTagName`:: (INFO) A tag has an invalid format. diff --git a/Makefile b/Makefile index e11340c1ae..f2ddcc8d7c 100644 --- a/Makefile +++ b/Makefile @@ -2733,6 +2733,7 @@ REFTABLE_OBJS += reftable/error.o REFTABLE_OBJS += reftable/block.o REFTABLE_OBJS += reftable/blocksource.o REFTABLE_OBJS += reftable/iter.o +REFTABLE_OBJS += reftable/fsck.o REFTABLE_OBJS += reftable/merged.o REFTABLE_OBJS += reftable/pq.o REFTABLE_OBJS += reftable/record.o diff --git a/fsck.h b/fsck.h index 559ad57807..5901f944a1 100644 --- a/fsck.h +++ b/fsck.h @@ -34,6 +34,7 @@ enum fsck_msg_type { FUNC(BAD_PACKED_REF_HEADER, ERROR) \ FUNC(BAD_PARENT_SHA1, ERROR) \ FUNC(BAD_REFERENT_NAME, ERROR) \ + FUNC(BAD_REFTABLE_TABLE_NAME, ERROR) \ FUNC(BAD_REF_CONTENT, ERROR) \ FUNC(BAD_REF_FILETYPE, ERROR) \ FUNC(BAD_REF_NAME, ERROR) \ diff --git a/meson.build b/meson.build index 5dd299b496..82879fbfaa 100644 --- a/meson.build +++ b/meson.build @@ -452,6 +452,7 @@ libgit_sources = [ 'reftable/error.c', 'reftable/block.c', 'reftable/blocksource.c', + 'reftable/fsck.c', 'reftable/iter.c', 'reftable/merged.c', 'reftable/pq.c', diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 8dae1e1112..c38c6422f8 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -6,20 +6,21 @@ #include "../config.h" #include "../dir.h" #include "../environment.h" +#include "../fsck.h" #include "../gettext.h" #include "../hash.h" #include "../hex.h" #include "../iterator.h" #include "../ident.h" -#include "../lockfile.h" #include "../object.h" #include "../path.h" #include "../refs.h" #include "../reftable/reftable-basics.h" -#include "../reftable/reftable-stack.h" -#include "../reftable/reftable-record.h" #include "../reftable/reftable-error.h" +#include "../reftable/reftable-fsck.h" #include "../reftable/reftable-iterator.h" +#include "../reftable/reftable-record.h" +#include "../reftable/reftable-stack.h" #include "../repo-settings.h" #include "../setup.h" #include "../strmap.h" @@ -2675,11 +2676,55 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, return ret; } -static int reftable_be_fsck(struct ref_store *ref_store UNUSED, - struct fsck_options *o UNUSED, +static void reftable_fsck_verbose_handler(const char *msg, void *cb_data) +{ + struct fsck_options *o = cb_data; + + if (o->verbose) + fprintf_ln(stderr, "%s", _(msg)); +} + +static int reftable_fsck_error_handler(struct reftable_fsck_info *info, + void *cb_data) +{ + struct fsck_ref_report report = { .path = info->path }; + struct fsck_options *o = cb_data; + enum fsck_msg_id msg_id; + + switch (info->error) { + case REFTABLE_FSCK_ERROR_TABLE_NAME: + msg_id = FSCK_MSG_BAD_REFTABLE_TABLE_NAME; + break; + default: + BUG("unknown fsck error: %d", info->error); + } + + return fsck_report_ref(o, &report, msg_id, "%s", info->msg); +} + +static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o, struct worktree *wt UNUSED) { - return 0; + struct reftable_ref_store *refs; + struct strmap_entry *entry; + struct hashmap_iter iter; + int ret = 0; + + refs = reftable_be_downcast(ref_store, REF_STORE_READ, "fsck"); + + if (o->verbose) + fprintf_ln(stderr, _("Checking references consistency")); + + ret |= reftable_fsck_check(refs->main_backend.stack, reftable_fsck_error_handler, + reftable_fsck_verbose_handler, o); + + strmap_for_each_entry(&refs->worktree_backends, &iter, entry) { + struct reftable_backend *b = (struct reftable_backend *)entry->value; + ret |= reftable_fsck_check(b->stack, reftable_fsck_error_handler, + reftable_fsck_verbose_handler, o); + } + + return ret; } struct ref_storage_be refs_be_reftable = { diff --git a/reftable/fsck.c b/reftable/fsck.c new file mode 100644 index 0000000000..4282b1413e --- /dev/null +++ b/reftable/fsck.c @@ -0,0 +1,53 @@ +#include "basics.h" +#include "reftable-fsck.h" +#include "stack.h" + +int reftable_fsck_check(struct reftable_stack *stack, + reftable_fsck_report_fn report_fn, + reftable_fsck_verbose_fn verbose_fn, + void *cb_data) +{ + + char **names = NULL; + uint64_t min, max; + int err = 0; + + if (stack == NULL) + goto out; + + err = read_lines(stack->list_file, &names); + if (err < 0) + goto out; + + verbose_fn("Checking reftable table names", cb_data); + + for (size_t i = 0; names[i]; i++) { + struct reftable_fsck_info info = { + .error = REFTABLE_FSCK_ERROR_TABLE_NAME, + .path = names[i], + }; + uint32_t rnd; + /* + * We want to match the tail '.ref'. One extra byte to ensure + * that there is no unexpected extra character and one byte for + * the null terminator added by sscanf. + */ + char tail[6]; + + if (sscanf(names[i], "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x%5s", + &min, &max, &rnd, tail) != 4) { + info.msg = "invalid reftable table name"; + err = report_fn(&info, cb_data); + continue; + } + + if (strcmp(tail, ".ref")) { + info.msg = "invalid reftable table extension"; + err = report_fn(&info, cb_data); + } + } + +out: + free_names(names); + return err; +} diff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h new file mode 100644 index 0000000000..4cf0053234 --- /dev/null +++ b/reftable/reftable-fsck.h @@ -0,0 +1,38 @@ +#ifndef REFTABLE_FSCK_H +#define REFTABLE_FSCK_H + +#include "reftable-stack.h" + +enum reftable_fsck_error { + /* Invalid table name */ + REFTABLE_FSCK_ERROR_TABLE_NAME = -1, +}; + +/* Represents an individual error encountered during the FSCK checks. */ +struct reftable_fsck_info { + enum reftable_fsck_error error; + const char *msg; + const char *path; +}; + +typedef int reftable_fsck_report_fn(struct reftable_fsck_info *info, + void *cb_data); +typedef void reftable_fsck_verbose_fn(const char *msg, void *cb_data); + +/* + * Given a reftable stack, perform FSCK check on the stack. + * + * If an issue is encountered, the issue is reported to the callee via the + * provided 'report_fn'. If the issue is non-recoverable the flow will not + * continue. If it is recoverable, the flow will continue and further issues + * will be reported as identified. + * + * The 'verbose_fn' will be invoked to provide verbose information about + * the progress and state of the FSCK checks. + */ +int reftable_fsck_check(struct reftable_stack *stack, + reftable_fsck_report_fn report_fn, + reftable_fsck_verbose_fn verbose_fn, + void *cb_data); + +#endif /* REFTABLE_FSCK_H */ diff --git a/t/meson.build b/t/meson.build index bbeba1a8d5..a8eb44eb30 100644 --- a/t/meson.build +++ b/t/meson.build @@ -145,6 +145,7 @@ integration_tests = [ 't0611-reftable-httpd.sh', 't0612-reftable-jgit-compatibility.sh', 't0613-reftable-write-options.sh', + 't0614-reftable-fsck.sh', 't1000-read-tree-m-3way.sh', 't1001-read-tree-m-2way.sh', 't1002-read-tree-m-u-2way.sh', @@ -1214,4 +1215,4 @@ if perl.found() and time.found() timeout: 0, ) endforeach -endif \ No newline at end of file +endif diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh new file mode 100755 index 0000000000..81d30df2d7 --- /dev/null +++ b/t/t0614-reftable-fsck.sh @@ -0,0 +1,58 @@ +#!/bin/sh + +test_description='Test reftable backend consistency check' + +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +GIT_TEST_DEFAULT_REF_FORMAT=reftable +export GIT_TEST_DEFAULT_REF_FORMAT + +. ./test-lib.sh + +test_expect_success 'table name should be checked' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + git commit --allow-empty -m initial && + + git refs verify 2>err && + test_must_be_empty err && + + TABLE_NAME=$(cat .git/reftable/tables.list | head -n1) && + sed "1s/^/extra/" .git/reftable/tables.list >.git/reftable/tables.list.tmp && + mv .git/reftable/tables.list.tmp .git/reftable/tables.list && + mv .git/reftable/${TABLE_NAME} .git/reftable/extra${TABLE_NAME} && + + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && + error: extra${TABLE_NAME}: badReftableTableName: invalid reftable table name + EOF + test_cmp expect err + ) +' + +test_expect_success 'table name should be checked' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + git commit --allow-empty -m initial && + + git refs verify 2>err && + test_must_be_empty err && + + TABLE_NAME=$(cat .git/reftable/tables.list | head -n1) && + sed "1s/$/extra/" .git/reftable/tables.list >.git/reftable/tables.list.tmp && + mv .git/reftable/tables.list.tmp .git/reftable/tables.list && + mv .git/reftable/${TABLE_NAME} .git/reftable/${TABLE_NAME}extra && + + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && + error: ${TABLE_NAME}extra: badReftableTableName: invalid reftable table extension + EOF + test_cmp expect err + ) +' + +test_done -- 2.50.1 ``` ## Karthik Nayak, 2025-09-02 07:05 Subject: [PATCH v2 3/5] refs/reftable: add fsck check for number of tables Message-ID: <20250902-228-reftable-introduce-consistency-checks-v2-3-4f96b3834779@gmail.com> URL: https://gitlist.dev/e/20250902-228-reftable-introduce-consistency-checks-v2-3-4f96b3834779%40gmail.com In-Reply-To: <20250902-228-reftable-introduce-consistency-checks-v2-0-4f96b3834779@gmail.com> ``` Introduce a reftable fsck check to check that the number of files in the reftable directory matches the number of files listed in 'tables.list'. We do this by iterating over the files in the reftable directory and counting all the files present excluding the 'tables.list'. This is also exposed over Git's fsck checks as a 'badReftableStackCount' error. Signed-off-by: Karthik Nayak --- Documentation/fsck-msgids.adoc | 3 +++ fsck.h | 1 + refs/reftable-backend.c | 3 +++ reftable/fsck.c | 34 ++++++++++++++++++++++++++++++++++ reftable/reftable-fsck.h | 2 ++ t/t0614-reftable-fsck.sh | 20 ++++++++++++++++++++ 6 files changed, 63 insertions(+) diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc index 784ddc0df5..707e2fc50a 100644 --- a/Documentation/fsck-msgids.adoc +++ b/Documentation/fsck-msgids.adoc @@ -38,6 +38,9 @@ `badReferentName`:: (ERROR) The referent name of a symref is invalid. +`badReftableStackCount`:: + (ERROR) Mismatch in number of tables. + `badReftableTableName`:: (ERROR) A reftable table has an invalid name. diff --git a/fsck.h b/fsck.h index 5901f944a1..256effc4f8 100644 --- a/fsck.h +++ b/fsck.h @@ -34,6 +34,7 @@ enum fsck_msg_type { FUNC(BAD_PACKED_REF_HEADER, ERROR) \ FUNC(BAD_PARENT_SHA1, ERROR) \ FUNC(BAD_REFERENT_NAME, ERROR) \ + FUNC(BAD_REFTABLE_STACK_COUNT, ERROR) \ FUNC(BAD_REFTABLE_TABLE_NAME, ERROR) \ FUNC(BAD_REF_CONTENT, ERROR) \ FUNC(BAD_REF_FILETYPE, ERROR) \ diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index c38c6422f8..59c39f9b52 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -2695,6 +2695,9 @@ static int reftable_fsck_error_handler(struct reftable_fsck_info *info, case REFTABLE_FSCK_ERROR_TABLE_NAME: msg_id = FSCK_MSG_BAD_REFTABLE_TABLE_NAME; break; + case REFTABLE_FSCK_ERROR_STACK_COUNT: + msg_id = FSCK_MSG_BAD_REFTABLE_STACK_COUNT; + break; default: BUG("unknown fsck error: %d", info->error); } diff --git a/reftable/fsck.c b/reftable/fsck.c index 4282b1413e..20e6bfb0f1 100644 --- a/reftable/fsck.c +++ b/reftable/fsck.c @@ -2,6 +2,28 @@ #include "reftable-fsck.h" #include "stack.h" +static int reftable_fsck_valid_stack_count(struct reftable_stack *st) +{ + DIR *dir = opendir(st->reftable_dir); + struct dirent *d = NULL; + unsigned int count = 0; + + if (!dir) + return 0; + + while ((d = readdir(dir))) { + if (!strcmp(d->d_name, "tables.list")) + continue; + + if (d->d_type == DT_REG) + count++; + } + + closedir(dir); + + return count == st->tables_len; +} + int reftable_fsck_check(struct reftable_stack *stack, reftable_fsck_report_fn report_fn, reftable_fsck_verbose_fn verbose_fn, @@ -47,6 +69,18 @@ int reftable_fsck_check(struct reftable_stack *stack, } } + verbose_fn("Checking reftable tables count", cb_data); + + if (!reftable_fsck_valid_stack_count(stack)) { + struct reftable_fsck_info info = { + .error = REFTABLE_FSCK_ERROR_STACK_COUNT, + .path = "reftable/tables.list", + .msg = "mismatch in number of tables" + }; + + err = report_fn(&info, cb_data); + } + out: free_names(names); return err; diff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h index 4cf0053234..beba1bdd1a 100644 --- a/reftable/reftable-fsck.h +++ b/reftable/reftable-fsck.h @@ -6,6 +6,8 @@ enum reftable_fsck_error { /* Invalid table name */ REFTABLE_FSCK_ERROR_TABLE_NAME = -1, + /* Incorrect number of tables present */ + REFTABLE_FSCK_ERROR_STACK_COUNT = -2, }; /* Represents an individual error encountered during the FSCK checks. */ diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh index 81d30df2d7..3a34a31890 100755 --- a/t/t0614-reftable-fsck.sh +++ b/t/t0614-reftable-fsck.sh @@ -55,4 +55,24 @@ test_expect_success 'table name should be checked' ' ) ' +test_expect_success 'table count should be checked' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + git commit --allow-empty -m initial && + + git refs verify 2>err && + test_must_be_empty err && + + touch .git/reftable/0x000000002812-0x000000002813-c830a596.ref && + + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && + error: reftable/tables.list: badReftableStackCount: mismatch in number of tables + EOF + test_cmp expect err + ) +' + test_done -- 2.50.1 ``` ## Karthik Nayak, 2025-09-02 07:05 Subject: [PATCH v2 4/5] refs/reftable: add fsck check for trailing newline Message-ID: <20250902-228-reftable-introduce-consistency-checks-v2-4-4f96b3834779@gmail.com> URL: https://gitlist.dev/e/20250902-228-reftable-introduce-consistency-checks-v2-4-4f96b3834779%40gmail.com In-Reply-To: <20250902-228-reftable-introduce-consistency-checks-v2-0-4f96b3834779@gmail.com> ``` Introduce a fsck check for the reftable backend, which checks if the 'tables.list' contains a newline. The reftable backend writes a trailing newline when writing the 'tables.list', but it doesn't check for it when reading the file. A missing newline however indicates that the file was manually tampered with, so let's raise this as an error to the user. Signed-off-by: Karthik Nayak --- Documentation/fsck-msgids.adoc | 3 +++ fsck.h | 1 + refs/reftable-backend.c | 3 +++ reftable/fsck.c | 36 ++++++++++++++++++++++++++++++++++++ reftable/reftable-fsck.h | 2 ++ t/t0614-reftable-fsck.sh | 21 +++++++++++++++++++++ 6 files changed, 66 insertions(+) diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc index 707e2fc50a..1432b1de06 100644 --- a/Documentation/fsck-msgids.adoc +++ b/Documentation/fsck-msgids.adoc @@ -41,6 +41,9 @@ `badReftableStackCount`:: (ERROR) Mismatch in number of tables. +`badReftableStackListNewline`:: + (ERROR) Reftable stack list missing trailing newline. + `badReftableTableName`:: (ERROR) A reftable table has an invalid name. diff --git a/fsck.h b/fsck.h index 256effc4f8..33432bae79 100644 --- a/fsck.h +++ b/fsck.h @@ -35,6 +35,7 @@ enum fsck_msg_type { FUNC(BAD_PARENT_SHA1, ERROR) \ FUNC(BAD_REFERENT_NAME, ERROR) \ FUNC(BAD_REFTABLE_STACK_COUNT, ERROR) \ + FUNC(BAD_REFTABLE_STACK_LIST_NEWLINE, ERROR) \ FUNC(BAD_REFTABLE_TABLE_NAME, ERROR) \ FUNC(BAD_REF_CONTENT, ERROR) \ FUNC(BAD_REF_FILETYPE, ERROR) \ diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 59c39f9b52..7331513b19 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -2698,6 +2698,9 @@ static int reftable_fsck_error_handler(struct reftable_fsck_info *info, case REFTABLE_FSCK_ERROR_STACK_COUNT: msg_id = FSCK_MSG_BAD_REFTABLE_STACK_COUNT; break; + case REFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE: + msg_id = FSCK_MSG_BAD_REFTABLE_STACK_LIST_NEWLINE; + break; default: BUG("unknown fsck error: %d", info->error); } diff --git a/reftable/fsck.c b/reftable/fsck.c index 20e6bfb0f1..9a7f22c56b 100644 --- a/reftable/fsck.c +++ b/reftable/fsck.c @@ -1,7 +1,31 @@ #include "basics.h" +#include "reftable-error.h" #include "reftable-fsck.h" #include "stack.h" +static int reftable_fsck_stack_contains_newline(const char *list_file) +{ + FILE *f = fopen(list_file, "r"); + int c = 0; + + if (f == NULL) { + if (errno == ENOENT) + return 0; + return REFTABLE_IO_ERROR; + } + + if (fseek(f, 0, SEEK_END) == 0) { + long size = ftell(f); + if (size <= 0) + return REFTABLE_IO_ERROR; + fseek(f, -1, SEEK_END); + c = fgetc(f); + } + fclose(f); + + return c == '\n'; +} + static int reftable_fsck_valid_stack_count(struct reftable_stack *st) { DIR *dir = opendir(st->reftable_dir); @@ -69,6 +93,18 @@ int reftable_fsck_check(struct reftable_stack *stack, } } + verbose_fn("Checking trailing newline in stack list", cb_data); + + if (!reftable_fsck_stack_contains_newline(stack->list_file)) { + struct reftable_fsck_info info = { + .error = REFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE, + .path = "reftable/tables.list", + .msg = "trailing newline missing in stack list" + }; + + err = report_fn(&info, cb_data); + } + verbose_fn("Checking reftable tables count", cb_data); if (!reftable_fsck_valid_stack_count(stack)) { diff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h index beba1bdd1a..17df661da8 100644 --- a/reftable/reftable-fsck.h +++ b/reftable/reftable-fsck.h @@ -8,6 +8,8 @@ enum reftable_fsck_error { REFTABLE_FSCK_ERROR_TABLE_NAME = -1, /* Incorrect number of tables present */ REFTABLE_FSCK_ERROR_STACK_COUNT = -2, + /* Newline missing at the end of the stack list */ + REFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE = -3, }; /* Represents an individual error encountered during the FSCK checks. */ diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh index 3a34a31890..3b119eae62 100755 --- a/t/t0614-reftable-fsck.sh +++ b/t/t0614-reftable-fsck.sh @@ -75,4 +75,25 @@ test_expect_success 'table count should be checked' ' ) ' +test_expect_success 'stack list must contain trailing newline' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + git commit --allow-empty -m initial && + + git refs verify 2>err && + test_must_be_empty err && + + printf "%s" "$(cat .git/reftable/tables.list)" >.git/reftable/tables.list.tmp && + mv .git/reftable/tables.list.tmp .git/reftable/tables.list && + + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && + error: reftable/tables.list: badReftableStackListNewline: trailing newline missing in stack list + EOF + test_cmp expect err + ) +' + test_done -- 2.50.1 ``` ## Karthik Nayak, 2025-09-02 07:05 Subject: [PATCH v2 5/5] refs/reftable: add fsck check for incorrect update index Message-ID: <20250902-228-reftable-introduce-consistency-checks-v2-5-4f96b3834779@gmail.com> URL: https://gitlist.dev/e/20250902-228-reftable-introduce-consistency-checks-v2-5-4f96b3834779%40gmail.com In-Reply-To: <20250902-228-reftable-introduce-consistency-checks-v2-0-4f96b3834779@gmail.com> ``` Introduce a fsck check for the reftable backend, which checks if the tables in 'tables.list' contain sequential update index. The tables in the reftable backend should contain sequential update index. This fsck check ensures that. We must note that the reftable backend itself doesn't check to ensure this and it also doesn't check to ensure that the index in the table name matches the index in the header or the table. The latter is not implemented in this fsck check either and will be added in a future patch where we add fsck checks for internals of a table. Signed-off-by: Karthik Nayak --- Documentation/fsck-msgids.adoc | 3 ++ fsck.h | 1 + refs/reftable-backend.c | 3 ++ reftable/fsck.c | 15 ++++++++-- reftable/reftable-fsck.h | 2 ++ t/t0614-reftable-fsck.sh | 62 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 84 insertions(+), 2 deletions(-) diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc index 1432b1de06..982d51876c 100644 --- a/Documentation/fsck-msgids.adoc +++ b/Documentation/fsck-msgids.adoc @@ -47,6 +47,9 @@ `badReftableTableName`:: (ERROR) A reftable table has an invalid name. +`badReftableUpdateIndex`:: + (ERROR) Incorrect update index found for table. + `badTagName`:: (INFO) A tag has an invalid format. diff --git a/fsck.h b/fsck.h index 33432bae79..60e9b84183 100644 --- a/fsck.h +++ b/fsck.h @@ -37,6 +37,7 @@ enum fsck_msg_type { FUNC(BAD_REFTABLE_STACK_COUNT, ERROR) \ FUNC(BAD_REFTABLE_STACK_LIST_NEWLINE, ERROR) \ FUNC(BAD_REFTABLE_TABLE_NAME, ERROR) \ + FUNC(BAD_REFTABLE_UPDATE_INDEX, ERROR) \ FUNC(BAD_REF_CONTENT, ERROR) \ FUNC(BAD_REF_FILETYPE, ERROR) \ FUNC(BAD_REF_NAME, ERROR) \ diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 7331513b19..519ade24b8 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -2701,6 +2701,9 @@ static int reftable_fsck_error_handler(struct reftable_fsck_info *info, case REFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE: msg_id = FSCK_MSG_BAD_REFTABLE_STACK_LIST_NEWLINE; break; + case REFTABLE_FSCK_ERROR_UPDATE_INDEX: + msg_id = FSCK_MSG_BAD_REFTABLE_UPDATE_INDEX; + break; default: BUG("unknown fsck error: %d", info->error); } diff --git a/reftable/fsck.c b/reftable/fsck.c index 9a7f22c56b..5c6d842ac1 100644 --- a/reftable/fsck.c +++ b/reftable/fsck.c @@ -53,9 +53,8 @@ int reftable_fsck_check(struct reftable_stack *stack, reftable_fsck_verbose_fn verbose_fn, void *cb_data) { - + uint64_t min, max, prev_max = 0; char **names = NULL; - uint64_t min, max; int err = 0; if (stack == NULL) @@ -87,10 +86,22 @@ int reftable_fsck_check(struct reftable_stack *stack, continue; } + if (min != (prev_max + 1) || max < min) { + struct reftable_fsck_info info = { + .error = REFTABLE_FSCK_ERROR_UPDATE_INDEX, + .path = names[i], + .msg = "incorrect update index in table name" + }; + + err = report_fn(&info, cb_data); + } + if (strcmp(tail, ".ref")) { info.msg = "invalid reftable table extension"; err = report_fn(&info, cb_data); } + + prev_max = max; } verbose_fn("Checking trailing newline in stack list", cb_data); diff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h index 17df661da8..0ab20a99b6 100644 --- a/reftable/reftable-fsck.h +++ b/reftable/reftable-fsck.h @@ -10,6 +10,8 @@ enum reftable_fsck_error { REFTABLE_FSCK_ERROR_STACK_COUNT = -2, /* Newline missing at the end of the stack list */ REFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE = -3, + /* Incorrect update index for table */ + REFTABLE_FSCK_ERROR_UPDATE_INDEX = -4, }; /* Represents an individual error encountered during the FSCK checks. */ diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh index 3b119eae62..1f37691b2e 100755 --- a/t/t0614-reftable-fsck.sh +++ b/t/t0614-reftable-fsck.sh @@ -96,4 +96,66 @@ test_expect_success 'stack list must contain trailing newline' ' ) ' +test_expect_success 'table update index should be sequential between tables' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + git commit --allow-empty -m initial && + + # Lock the existing table to disable auto-compaction + CUR_TABLE=$(cat .git/reftable/tables.list | tail -n1) && + touch .git/reftable/${CUR_TABLE}.lock && + git update-ref refs/heads/sample @ && + rm .git/reftable/${CUR_TABLE}.lock && + + git refs verify 2>err && + test_must_be_empty err && + + TABLE_NAME=$(cat .git/reftable/tables.list | tail -n1) && + NEW_TABLE_NAME=$(echo ${TABLE_NAME} | sed "s/0003/0009/g") && + + sed "2s/.*/${NEW_TABLE_NAME}/" .git/reftable/tables.list >.git/reftable/tables.list.tmp && + mv .git/reftable/tables.list.tmp .git/reftable/tables.list && + mv .git/reftable/${TABLE_NAME} .git/reftable/${NEW_TABLE_NAME} && + + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && + error: ${NEW_TABLE_NAME}: badReftableUpdateIndex: incorrect update index in table name + EOF + test_cmp expect err + ) +' + +test_expect_success 'table update index should be sequential within a table' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + git commit --allow-empty -m initial && + + # Lock the existing table to disable auto-compaction + CUR_TABLE=$(cat .git/reftable/tables.list | tail -n1) && + touch .git/reftable/${CUR_TABLE}.lock && + git update-ref refs/heads/sample @ && + rm .git/reftable/${CUR_TABLE}.lock && + + git refs verify 2>err && + test_must_be_empty err && + + TABLE_NAME=$(cat .git/reftable/tables.list | tail -n1) && + NEW_TABLE_NAME=$(echo ${TABLE_NAME} | sed "s/\(.*\)0003/\10002/") && + + sed "2s/.*/${NEW_TABLE_NAME}/" .git/reftable/tables.list >.git/reftable/tables.list.tmp && + mv .git/reftable/tables.list.tmp .git/reftable/tables.list && + mv .git/reftable/${TABLE_NAME} .git/reftable/${NEW_TABLE_NAME} && + + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && + error: ${NEW_TABLE_NAME}: badReftableUpdateIndex: incorrect update index in table name + EOF + test_cmp expect err + ) +' + test_done -- 2.50.1 ``` ## Junio C Hamano, 2025-09-02 22:25 Subject: Re: [PATCH v2 1/5] fsck: order 'fsck_msg_type' alphabetically Message-ID: URL: https://gitlist.dev/e/xmqqwm6gh4q3.fsf%40gitster.g In-Reply-To: <20250902-228-reftable-introduce-consistency-checks-v2-1-4f96b3834779@gmail.com> ``` Karthik Nayak writes: > The list of 'fsck_msg_type' seem to be alphabetically ordered, but there > are a few small misses. Fix this by sorting the sub-sections of the > list to maintain alphabetical ordering. Also fix a clang-format issue > where the escaped newlines are not aligned. > > While here, remove a duplicate instance of 'gitmodulesLarge' in the > 'fsck-msgids' documentation. "A few small misses". > diff --git a/fsck.h b/fsck.h > index dd7df3d5b3..559ad57807 100644 > --- a/fsck.h > +++ b/fsck.h > @@ -20,82 +20,82 @@ enum fsck_msg_type { > ... > -#define FOREACH_FSCK_MSG_ID(FUNC) \ > - /* fatal errors */ \ > - FUNC(NUL_IN_HEADER, FATAL) \ > - FUNC(UNTERMINATED_HEADER, FATAL) \ > ... > +#define FOREACH_FSCK_MSG_ID(FUNC) \ > + /* fatal errors */ \ > + FUNC(NUL_IN_HEADER, FATAL) \ > + FUNC(UNTERMINATED_HEADER, FATAL) \ > ... Please undo these "pad by spaces before backslash"; otherwise we cannot tell which ones are "a few small misses". Thanks. ``` ## Junio C Hamano, 2025-09-02 22:38 Subject: Re: [PATCH v2 4/5] refs/reftable: add fsck check for trailing newline Message-ID: URL: https://gitlist.dev/e/xmqqseh4h446.fsf%40gitster.g In-Reply-To: <20250902-228-reftable-introduce-consistency-checks-v2-4-4f96b3834779@gmail.com> ``` Karthik Nayak writes: > Introduce a fsck check for the reftable backend, which checks if the > 'tables.list' contains a newline. The reftable backend writes a trailing > newline when writing the 'tables.list', but it doesn't check for it when > reading the file. A missing newline however indicates that the file was > manually tampered with, so let's raise this as an error to the user. Hmph, how does the code react to other kinds of "manual tampering"? For example, if an empty line is inserted between two existing lines (or at the beginning of the file, for that matter), would the parser detect it as a corrupt file and die? If so, it makes me strongly suspect that we are better off enforcing that the file does not end in an incomplete line at runtime and barf just the same way, instead of "most of the anomalies that the write codepath would never produce would cause error on the read codepath, but only this one that the read codepath is happy with is caught by the fsck", which does not sound quite right. ``` ## Junio C Hamano, 2025-09-02 22:42 Subject: Re: [PATCH v2 5/5] refs/reftable: add fsck check for incorrect update index Message-ID: URL: https://gitlist.dev/e/xmqqo6rsh3xw.fsf%40gitster.g In-Reply-To: <20250902-228-reftable-introduce-consistency-checks-v2-5-4f96b3834779@gmail.com> ``` Karthik Nayak writes: > Introduce a fsck check for the reftable backend, which checks if the > tables in 'tables.list' contain sequential update index. The tables in > the reftable backend should contain sequential update index. This fsck > check ensures that. > > We must note that the reftable backend itself doesn't check to ensure > this and it also doesn't check to ensure that the index in the table > name matches the index in the header or the table. The latter is not > implemented in this fsck check either and will be added in a future > patch where we add fsck checks for internals of a table. Similar to the previous step, I am not sure why this should not be checked at runtime and is flagged as an error. In general, we do try to avoid retroactively tightening rules, but the reftable is so new and not even the default. If we noticed that the runtime has been overly loose, the time to tighten it is now, not after even more installations use it. ``` ## Patrick Steinhardt, 2025-09-03 08:07 Subject: Re: [PATCH v2 2/5] refs/reftable: add fsck check for checking the table name Message-ID: URL: https://gitlist.dev/e/aLf3MaKHZSQfnBlT%40pks.im In-Reply-To: <20250902-228-reftable-introduce-consistency-checks-v2-2-4f96b3834779@gmail.com> ``` On Tue, Sep 02, 2025 at 09:05:22AM +0200, Karthik Nayak wrote: > The `git refs verify` command is used to run fsck checks on the > reference backends. This command is also invoked when users run 'git > fsck'. While the files-backend has some fsck checks added, the reftable > backend lacks such checks. Let's add the required infrastructure and a > check to test for the table names in the 'tables.list' of reftables. > > For the infrastructure, since the reftable library is treated as an > independent library we should ensure that the library code works > independently without knowledge about Git's internals. To do this, > add both 'reftable/fsck.c' and 'reftable/reftable-fsck.h'. Which > provide an entry point 'reftable_fsck_check' for running fsck checks > over a provided reftable stack. The callee provides the function with > callbacks to handle issue and information reporting. > > Add glue code in 'refs/reftable-backend.c' which calls the reftable > library to perform the fsck checks. Here we also map the reftable errors > to Git' fsck errors. > > Introduce a check to validate table names for a given reftable stack. > Also add 'badReftableTableName' as a corresponding error within Git. Add > a test to check for this behavior. > > While here, remove a unused header `#include "../lockfile.h"` from > 'refs/reftable-backend.c'. It's quite a bunch of changes overall that could've been reasonably split up into multiple commits. E.g. one to introduce the reftable-side logic, one to start calling it in Git, and one to drop the superfluous header. > diff --git a/Makefile b/Makefile > index e11340c1ae..f2ddcc8d7c 100644 > --- a/Makefile > +++ b/Makefile > @@ -2733,6 +2733,7 @@ REFTABLE_OBJS += reftable/error.o > REFTABLE_OBJS += reftable/block.o > REFTABLE_OBJS += reftable/blocksource.o > REFTABLE_OBJS += reftable/iter.o > +REFTABLE_OBJS += reftable/fsck.o "f" is before "i" in the alphabet I'm accustomed to :) So let's retain lexicographic ordering here. > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c > index 8dae1e1112..c38c6422f8 100644 > --- a/refs/reftable-backend.c > +++ b/refs/reftable-backend.c > @@ -2675,11 +2676,55 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, > return ret; > } > > -static int reftable_be_fsck(struct ref_store *ref_store UNUSED, > - struct fsck_options *o UNUSED, > +static void reftable_fsck_verbose_handler(const char *msg, void *cb_data) > +{ > + struct fsck_options *o = cb_data; > + > + if (o->verbose) > + fprintf_ln(stderr, "%s", _(msg)); > +} Is this `_()` marker correct here? There isn't really any reasonable way for somebody to translate a variable with unknown contents. So shouldn't it only be the caller of `reftable_fsck_verbose_handler()` that should mark the string as translatable? > +static int reftable_fsck_error_handler(struct reftable_fsck_info *info, > + void *cb_data) > +{ > + struct fsck_ref_report report = { .path = info->path }; > + struct fsck_options *o = cb_data; > + enum fsck_msg_id msg_id; > + > + switch (info->error) { > + case REFTABLE_FSCK_ERROR_TABLE_NAME: > + msg_id = FSCK_MSG_BAD_REFTABLE_TABLE_NAME; > + break; > + default: > + BUG("unknown fsck error: %d", info->error); > + } > + > + return fsck_report_ref(o, &report, msg_id, "%s", info->msg); > +} I think this function will become a bit unwieldy over time. We might instead want to have an array that maps from reftable-specific to fsck-specific error code: static const fsck_msg_id[] = { [REFTABLE_FSCK_ERROR_TABLE_NAME] = FSCK_MSG_BAD_REFTABLE_TABLE_NAME, }; So in that case, all we'd have to do is to perform bounds checking in the above function. And maybe verify that the developer didn't forget to fill in a new msg ID by checking that the derived message ID is non-zero. > +static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o, > struct worktree *wt UNUSED) > { > - return 0; > + struct reftable_ref_store *refs; > + struct strmap_entry *entry; > + struct hashmap_iter iter; > + int ret = 0; > + > + refs = reftable_be_downcast(ref_store, REF_STORE_READ, "fsck"); > + > + if (o->verbose) > + fprintf_ln(stderr, _("Checking references consistency")); This line is duplicate across both backends, right? Maybe it's something that we can do in the generic logic? > + ret |= reftable_fsck_check(refs->main_backend.stack, reftable_fsck_error_handler, > + reftable_fsck_verbose_handler, o); > + > + strmap_for_each_entry(&refs->worktree_backends, &iter, entry) { > + struct reftable_backend *b = (struct reftable_backend *)entry->value; > + ret |= reftable_fsck_check(b->stack, reftable_fsck_error_handler, > + reftable_fsck_verbose_handler, o); > + } > + > + return ret; > } > > struct ref_storage_be refs_be_reftable = { Looks good. > diff --git a/reftable/fsck.c b/reftable/fsck.c > new file mode 100644 > index 0000000000..4282b1413e > --- /dev/null > +++ b/reftable/fsck.c > @@ -0,0 +1,53 @@ > +#include "basics.h" > +#include "reftable-fsck.h" > +#include "stack.h" > + > +int reftable_fsck_check(struct reftable_stack *stack, > + reftable_fsck_report_fn report_fn, > + reftable_fsck_verbose_fn verbose_fn, > + void *cb_data) > +{ > + > + char **names = NULL; > + uint64_t min, max; > + int err = 0; > + > + if (stack == NULL) > + goto out; > + > + err = read_lines(stack->list_file, &names); > + if (err < 0) > + goto out; > + > + verbose_fn("Checking reftable table names", cb_data); > + > + for (size_t i = 0; names[i]; i++) { > + struct reftable_fsck_info info = { > + .error = REFTABLE_FSCK_ERROR_TABLE_NAME, > + .path = names[i], > + }; > + uint32_t rnd; > + /* > + * We want to match the tail '.ref'. One extra byte to ensure > + * that there is no unexpected extra character and one byte for > + * the null terminator added by sscanf. > + */ > + char tail[6]; > + > + if (sscanf(names[i], "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x%5s", > + &min, &max, &rnd, tail) != 4) { > + info.msg = "invalid reftable table name"; This here is where the string should be translated. > + err = report_fn(&info, cb_data); > + continue; > + } I think sscanf is quite frowned-upon in the Git codebase. Maybe we should manually parse through the string instead? Furthermore, I think we should move every single check into a separate function, similar to how the files backend does it. This ensures that checks are self-contained and that it's way easier to add new checks over time. Another angle: did you verify that reftables written by JGit follow this format? > + if (strcmp(tail, ".ref")) { > + info.msg = "invalid reftable table extension"; Same here, this should be translated. > diff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h > new file mode 100644 > index 0000000000..4cf0053234 > --- /dev/null > +++ b/reftable/reftable-fsck.h > @@ -0,0 +1,38 @@ > +#ifndef REFTABLE_FSCK_H > +#define REFTABLE_FSCK_H > + > +#include "reftable-stack.h" > + > +enum reftable_fsck_error { > + /* Invalid table name */ > + REFTABLE_FSCK_ERROR_TABLE_NAME = -1, > +}; Wouldn't it be more natural to give these positive numbers? > +/* Represents an individual error encountered during the FSCK checks. */ > +struct reftable_fsck_info { > + enum reftable_fsck_error error; > + const char *msg; > + const char *path; > +}; I wonder whether it should be the reftable library that decides on the severity of each generated finding. > +typedef int reftable_fsck_report_fn(struct reftable_fsck_info *info, > + void *cb_data); > +typedef void reftable_fsck_verbose_fn(const char *msg, void *cb_data); > + > +/* > + * Given a reftable stack, perform FSCK check on the stack. s/FSCK check/consistency checks/ > + * > + * If an issue is encountered, the issue is reported to the callee via the > + * provided 'report_fn'. If the issue is non-recoverable the flow will not > + * continue. If it is recoverable, the flow will continue and further issues > + * will be reported as identified. > + * > + * The 'verbose_fn' will be invoked to provide verbose information about > + * the progress and state of the FSCK checks. Same here. > diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh > new file mode 100755 > index 0000000000..81d30df2d7 > --- /dev/null > +++ b/t/t0614-reftable-fsck.sh > @@ -0,0 +1,58 @@ > +#!/bin/sh > + > +test_description='Test reftable backend consistency check' > + > +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME Tests shouldn't define these variables, but should dynamically figure out what the default branch name is as required, e.g. by using git-symbolic-ref(1). > +GIT_TEST_DEFAULT_REF_FORMAT=reftable > +export GIT_TEST_DEFAULT_REF_FORMAT > + > +. ./test-lib.sh > + > +test_expect_success 'table name should be checked' ' > + test_when_finished "rm -rf repo" && > + git init repo && > + ( > + cd repo && > + git commit --allow-empty -m initial && > + > + git refs verify 2>err && > + test_must_be_empty err && > + > + TABLE_NAME=$(cat .git/reftable/tables.list | head -n1) && You can drop the cat(1) invocation and directly say `head -n1 file`. > + sed "1s/^/extra/" .git/reftable/tables.list >.git/reftable/tables.list.tmp && > + mv .git/reftable/tables.list.tmp .git/reftable/tables.list && > + mv .git/reftable/${TABLE_NAME} .git/reftable/extra${TABLE_NAME} && No need for the curly braces around TABLE_NAME here and further down. It would be nice to quote these strings though. > + > + test_must_fail git refs verify 2>err && > + cat >expect <<-EOF && > + error: extra${TABLE_NAME}: badReftableTableName: invalid reftable table name > + EOF > + test_cmp expect err > + ) > +' > + > +test_expect_success 'table name should be checked' ' > + test_when_finished "rm -rf repo" && > + git init repo && > + ( > + cd repo && > + git commit --allow-empty -m initial && > + > + git refs verify 2>err && > + test_must_be_empty err && > + > + TABLE_NAME=$(cat .git/reftable/tables.list | head -n1) && Same here wrt the extra invocation of cat(1). Patrick ``` ## Patrick Steinhardt, 2025-09-03 08:07 Subject: Re: [PATCH v2 3/5] refs/reftable: add fsck check for number of tables Message-ID: URL: https://gitlist.dev/e/aLf3NyodTZ38N8vY%40pks.im In-Reply-To: <20250902-228-reftable-introduce-consistency-checks-v2-3-4f96b3834779@gmail.com> ``` On Tue, Sep 02, 2025 at 09:05:23AM +0200, Karthik Nayak wrote: > Introduce a reftable fsck check to check that the number of files in the > reftable directory matches the number of files listed in 'tables.list'. > We do this by iterating over the files in the reftable directory and > counting all the files present excluding the 'tables.list'. This is also > exposed over Git's fsck checks as a 'badReftableStackCount' error. This feels overly strict, as it can always be the case that a concurrent process is currently updating the stack. Furthermore, it's expected that on Windows systems deletion of an old table may not work because the file is still kept open by another process. The reftable library is prepared to handle this alright and will re-try deleting the table at a later point in time. So maybe a better check would be to verify that there are no files with unexpected names in the directory? Patrick ``` ## Patrick Steinhardt, 2025-09-03 08:07 Subject: Re: [PATCH v2 4/5] refs/reftable: add fsck check for trailing newline Message-ID: URL: https://gitlist.dev/e/aLf3PFUkt49bvi1S%40pks.im In-Reply-To: ``` On Tue, Sep 02, 2025 at 03:38:33PM -0700, Junio C Hamano wrote: > Karthik Nayak writes: > > > Introduce a fsck check for the reftable backend, which checks if the > > 'tables.list' contains a newline. The reftable backend writes a trailing > > newline when writing the 'tables.list', but it doesn't check for it when > > reading the file. A missing newline however indicates that the file was > > manually tampered with, so let's raise this as an error to the user. > > Hmph, how does the code react to other kinds of "manual tampering"? > For example, if an empty line is inserted between two existing lines > (or at the beginning of the file, for that matter), would the parser > detect it as a corrupt file and die? > > If so, it makes me strongly suspect that we are better off enforcing > that the file does not end in an incomplete line at runtime and barf > just the same way, instead of "most of the anomalies that the write > codepath would never produce would cause error on the read codepath, > but only this one that the read codepath is happy with is caught by > the fsck", which does not sound quite right. Fair, I'm also of the opinion that we should tighten the parser logic to detect and reject any invalid files. For previous checks where we verify that the table names are sane I think it's fair to live with it and warn about those, as the actual names don't really matter. But as soon as we hit actually-broken formats I also think that we're better of rejecting those altogether. Patrick ``` ## shejialuo, 2025-09-03 16:51 Subject: Re: [PATCH v2 2/5] refs/reftable: add fsck check for checking the table name Message-ID: URL: https://gitlist.dev/e/aLhx-HdIMCB3c2WK%40ArchLinux In-Reply-To: ``` On Wed, Sep 03, 2025 at 10:07:13AM +0200, Patrick Steinhardt wrote: [snip] > > +static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o, > > struct worktree *wt UNUSED) > > { > > - return 0; > > + struct reftable_ref_store *refs; > > + struct strmap_entry *entry; > > + struct hashmap_iter iter; > > + int ret = 0; > > + > > + refs = reftable_be_downcast(ref_store, REF_STORE_READ, "fsck"); > > + > > + if (o->verbose) > > + fprintf_ln(stderr, _("Checking references consistency")); > > This line is duplicate across both backends, right? Maybe it's something > that we can do in the generic logic? > That's right, it is duplicate. If we want to remove this, we need to do this in the "builtin/refs.c". But I wonder whether we should do this in the first place. Should we rather add more detailed information just like the following code for packed backend? if (o->verbose) fprintf_ln(stderr, "Checking packed-refs file %s", refs->path); Instead of just using Checking references consistency Could we use Checking reftable references consistency However, I also feel strange about above, :) [snip] > > +/* Represents an individual error encountered during the FSCK checks. */ > > +struct reftable_fsck_info { > > + enum reftable_fsck_error error; > > + const char *msg; > > + const char *path; > > +}; > > I wonder whether it should be the reftable library that decides on the > severity of each generated finding. > That's an interesting question. Let's inspect how Git handles the severity. When defining the fsck message id, we need to specify its severity like the following shows, this happens at compile time: FUNC(BAD_REFERENT_NAME, ERROR) And we could set the configuration "fsck.[message id]=" to change the fsck message severity. Then let's think if reftable library decides the severity. It means that we need to use the API from reftable library to update "fsck_option->msg_type" at the runtime. And it is bad because the fsck infrastructure would be highly coupled with the reftable library. So, I don't think it's a good idea for reftable library to choose the severity. Instead, reftable library should just provide users with error types and let the users decide the severity. Thanks, Jialuo ``` ## Karthik Nayak, 2025-09-08 13:00 Subject: Re: [PATCH v2 1/5] fsck: order 'fsck_msg_type' alphabetically Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZTuZ3fOr_wkQ7TsEVRKjKynei4uF18YHygpcafXT9Qjmw%40mail.gmail.com In-Reply-To: ``` Junio C Hamano writes: > Karthik Nayak writes: > >> The list of 'fsck_msg_type' seem to be alphabetically ordered, but there >> are a few small misses. Fix this by sorting the sub-sections of the >> list to maintain alphabetical ordering. Also fix a clang-format issue >> where the escaped newlines are not aligned. >> >> While here, remove a duplicate instance of 'gitmodulesLarge' in the >> 'fsck-msgids' documentation. > > "A few small misses". > >> diff --git a/fsck.h b/fsck.h >> index dd7df3d5b3..559ad57807 100644 >> --- a/fsck.h >> +++ b/fsck.h >> @@ -20,82 +20,82 @@ enum fsck_msg_type { >> ... >> -#define FOREACH_FSCK_MSG_ID(FUNC) \ >> - /* fatal errors */ \ >> - FUNC(NUL_IN_HEADER, FATAL) \ >> - FUNC(UNTERMINATED_HEADER, FATAL) \ >> ... >> +#define FOREACH_FSCK_MSG_ID(FUNC) \ >> + /* fatal errors */ \ >> + FUNC(NUL_IN_HEADER, FATAL) \ >> + FUNC(UNTERMINATED_HEADER, FATAL) \ >> ... > > Please undo these "pad by spaces before backslash"; otherwise we > cannot tell which ones are "a few small misses". > > Thanks. Yeah, you're right, its much harder to review this way. Let me add in a commit at the end to do the clang-formatting for this section, that way we can drop it if it is too much noise. - Karthik ``` ## Karthik Nayak, 2025-09-09 08:42 Subject: Re: [PATCH v2 2/5] refs/reftable: add fsck check for checking the table name Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZQoOX9y2_KwnAcyS-dVeQm1QnFdZocomt05BkxL9xDFuA%40mail.gmail.com In-Reply-To: ``` Patrick Steinhardt writes: > On Tue, Sep 02, 2025 at 09:05:22AM +0200, Karthik Nayak wrote: >> The `git refs verify` command is used to run fsck checks on the >> reference backends. This command is also invoked when users run 'git >> fsck'. While the files-backend has some fsck checks added, the reftable >> backend lacks such checks. Let's add the required infrastructure and a >> check to test for the table names in the 'tables.list' of reftables. >> >> For the infrastructure, since the reftable library is treated as an >> independent library we should ensure that the library code works >> independently without knowledge about Git's internals. To do this, >> add both 'reftable/fsck.c' and 'reftable/reftable-fsck.h'. Which >> provide an entry point 'reftable_fsck_check' for running fsck checks >> over a provided reftable stack. The callee provides the function with >> callbacks to handle issue and information reporting. >> >> Add glue code in 'refs/reftable-backend.c' which calls the reftable >> library to perform the fsck checks. Here we also map the reftable errors >> to Git' fsck errors. >> >> Introduce a check to validate table names for a given reftable stack. >> Also add 'badReftableTableName' as a corresponding error within Git. Add >> a test to check for this behavior. >> >> While here, remove a unused header `#include "../lockfile.h"` from >> 'refs/reftable-backend.c'. > > It's quite a bunch of changes overall that could've been reasonably > split up into multiple commits. E.g. one to introduce the reftable-side > logic, one to start calling it in Git, and one to drop the superfluous > header. > I'm always hesitant to have small commits for some reason. Thanks for calling out, I'll split it up. Sidenote: I used `git history split` for this, and it was just perfect. >> diff --git a/Makefile b/Makefile >> index e11340c1ae..f2ddcc8d7c 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -2733,6 +2733,7 @@ REFTABLE_OBJS += reftable/error.o >> REFTABLE_OBJS += reftable/block.o >> REFTABLE_OBJS += reftable/blocksource.o >> REFTABLE_OBJS += reftable/iter.o >> +REFTABLE_OBJS += reftable/fsck.o > > "f" is before "i" in the alphabet I'm accustomed to :) So let's retain > lexicographic ordering here. > This ordering was already broken, but that's no reason to break it more. Let' me fix it. >> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c >> index 8dae1e1112..c38c6422f8 100644 >> --- a/refs/reftable-backend.c >> +++ b/refs/reftable-backend.c >> @@ -2675,11 +2676,55 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, >> return ret; >> } >> >> -static int reftable_be_fsck(struct ref_store *ref_store UNUSED, >> - struct fsck_options *o UNUSED, >> +static void reftable_fsck_verbose_handler(const char *msg, void *cb_data) >> +{ >> + struct fsck_options *o = cb_data; >> + >> + if (o->verbose) >> + fprintf_ln(stderr, "%s", _(msg)); >> +} > > Is this `_()` marker correct here? There isn't really any reasonable way > for somebody to translate a variable with unknown contents. So shouldn't > it only be the caller of `reftable_fsck_verbose_handler()` that should > mark the string as translatable? > True, but this is a callback function called from within the reftable library. I guess for now I can leave out the translation and we can think about the best way to fix that later. We need a more generic way to translate output strings originating from the reftable library. >> +static int reftable_fsck_error_handler(struct reftable_fsck_info *info, >> + void *cb_data) >> +{ >> + struct fsck_ref_report report = { .path = info->path }; >> + struct fsck_options *o = cb_data; >> + enum fsck_msg_id msg_id; >> + >> + switch (info->error) { >> + case REFTABLE_FSCK_ERROR_TABLE_NAME: >> + msg_id = FSCK_MSG_BAD_REFTABLE_TABLE_NAME; >> + break; >> + default: >> + BUG("unknown fsck error: %d", info->error); >> + } >> + >> + return fsck_report_ref(o, &report, msg_id, "%s", info->msg); >> +} > > I think this function will become a bit unwieldy over time. We might > instead want to have an array that maps from reftable-specific to > fsck-specific error code: > > static const fsck_msg_id[] = { > [REFTABLE_FSCK_ERROR_TABLE_NAME] = FSCK_MSG_BAD_REFTABLE_TABLE_NAME, > }; > > So in that case, all we'd have to do is to perform bounds checking in > the above function. And maybe verify that the developer didn't forget to > fill in a new msg ID by checking that the derived message ID is non-zero. > Yeah that sounds like a really good improvement, let me add that. >> +static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o, >> struct worktree *wt UNUSED) >> { >> - return 0; >> + struct reftable_ref_store *refs; >> + struct strmap_entry *entry; >> + struct hashmap_iter iter; >> + int ret = 0; >> + >> + refs = reftable_be_downcast(ref_store, REF_STORE_READ, "fsck"); >> + >> + if (o->verbose) >> + fprintf_ln(stderr, _("Checking references consistency")); > > This line is duplicate across both backends, right? Maybe it's something > that we can do in the generic logic? > Yeah, we can. Will do. >> + ret |= reftable_fsck_check(refs->main_backend.stack, reftable_fsck_error_handler, >> + reftable_fsck_verbose_handler, o); >> + >> + strmap_for_each_entry(&refs->worktree_backends, &iter, entry) { >> + struct reftable_backend *b = (struct reftable_backend *)entry->value; >> + ret |= reftable_fsck_check(b->stack, reftable_fsck_error_handler, >> + reftable_fsck_verbose_handler, o); >> + } >> + >> + return ret; >> } >> >> struct ref_storage_be refs_be_reftable = { > > Looks good. > >> diff --git a/reftable/fsck.c b/reftable/fsck.c >> new file mode 100644 >> index 0000000000..4282b1413e >> --- /dev/null >> +++ b/reftable/fsck.c >> @@ -0,0 +1,53 @@ >> +#include "basics.h" >> +#include "reftable-fsck.h" >> +#include "stack.h" >> + >> +int reftable_fsck_check(struct reftable_stack *stack, >> + reftable_fsck_report_fn report_fn, >> + reftable_fsck_verbose_fn verbose_fn, >> + void *cb_data) >> +{ >> + >> + char **names = NULL; >> + uint64_t min, max; >> + int err = 0; >> + >> + if (stack == NULL) >> + goto out; >> + >> + err = read_lines(stack->list_file, &names); >> + if (err < 0) >> + goto out; >> + >> + verbose_fn("Checking reftable table names", cb_data); >> + >> + for (size_t i = 0; names[i]; i++) { >> + struct reftable_fsck_info info = { >> + .error = REFTABLE_FSCK_ERROR_TABLE_NAME, >> + .path = names[i], >> + }; >> + uint32_t rnd; >> + /* >> + * We want to match the tail '.ref'. One extra byte to ensure >> + * that there is no unexpected extra character and one byte for >> + * the null terminator added by sscanf. >> + */ >> + char tail[6]; >> + >> + if (sscanf(names[i], "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x%5s", >> + &min, &max, &rnd, tail) != 4) { >> + info.msg = "invalid reftable table name"; > > This here is where the string should be translated. > But we don't have translation capabilities within the reftable lib no? Or am I mistaken? >> + err = report_fn(&info, cb_data); >> + continue; >> + } > > I think sscanf is quite frowned-upon in the Git codebase. Maybe we > should manually parse through the string instead? > That would be cumbersome. This isn't user input data, so I thought this would be okay. But let me do the change. > Furthermore, I think we should move every single check into a separate > function, similar to how the files backend does it. This ensures that > checks are self-contained and that it's way easier to add new checks > over time. > I think Shejialuo also mentioned this, let me do that. > Another angle: did you verify that reftables written by JGit follow this > format? > No I haven't. >> + if (strcmp(tail, ".ref")) { >> + info.msg = "invalid reftable table extension"; > > Same here, this should be translated. > >> diff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h >> new file mode 100644 >> index 0000000000..4cf0053234 >> --- /dev/null >> +++ b/reftable/reftable-fsck.h >> @@ -0,0 +1,38 @@ >> +#ifndef REFTABLE_FSCK_H >> +#define REFTABLE_FSCK_H >> + >> +#include "reftable-stack.h" >> + >> +enum reftable_fsck_error { >> + /* Invalid table name */ >> + REFTABLE_FSCK_ERROR_TABLE_NAME = -1, >> +}; > > Wouldn't it be more natural to give these positive numbers? > Yes, that would be better and also fits in with the array suggstion you made earlier. >> +/* Represents an individual error encountered during the FSCK checks. */ >> +struct reftable_fsck_info { >> + enum reftable_fsck_error error; >> + const char *msg; >> + const char *path; >> +}; > > I wonder whether it should be the reftable library that decides on the > severity of each generated finding. > I think that'd make sense. Let me add that in. >> +typedef int reftable_fsck_report_fn(struct reftable_fsck_info *info, >> + void *cb_data); >> +typedef void reftable_fsck_verbose_fn(const char *msg, void *cb_data); >> + >> +/* >> + * Given a reftable stack, perform FSCK check on the stack. > > s/FSCK check/consistency checks/ > >> + * >> + * If an issue is encountered, the issue is reported to the callee via the >> + * provided 'report_fn'. If the issue is non-recoverable the flow will not >> + * continue. If it is recoverable, the flow will continue and further issues >> + * will be reported as identified. >> + * >> + * The 'verbose_fn' will be invoked to provide verbose information about >> + * the progress and state of the FSCK checks. > > Same here. > Thanks, changed both. >> diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh >> new file mode 100755 >> index 0000000000..81d30df2d7 >> --- /dev/null >> +++ b/t/t0614-reftable-fsck.sh >> @@ -0,0 +1,58 @@ >> +#!/bin/sh >> + >> +test_description='Test reftable backend consistency check' >> + >> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main >> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > Tests shouldn't define these variables, but should dynamically figure > out what the default branch name is as required, e.g. by using > git-symbolic-ref(1). > Yeah, makes sense. Will change it. >> +GIT_TEST_DEFAULT_REF_FORMAT=reftable >> +export GIT_TEST_DEFAULT_REF_FORMAT >> + >> +. ./test-lib.sh >> + >> +test_expect_success 'table name should be checked' ' >> + test_when_finished "rm -rf repo" && >> + git init repo && >> + ( >> + cd repo && >> + git commit --allow-empty -m initial && >> + >> + git refs verify 2>err && >> + test_must_be_empty err && >> + >> + TABLE_NAME=$(cat .git/reftable/tables.list | head -n1) && > > You can drop the cat(1) invocation and directly say `head -n1 file`. > Indeed, thanks! >> + sed "1s/^/extra/" .git/reftable/tables.list >.git/reftable/tables.list.tmp && >> + mv .git/reftable/tables.list.tmp .git/reftable/tables.list && >> + mv .git/reftable/${TABLE_NAME} .git/reftable/extra${TABLE_NAME} && > > No need for the curly braces around TABLE_NAME here and further down. It > would be nice to quote these strings though. > Understandable. I always prefer using them, since they make it much easier to read, even without the ambiguity issue. >> + >> + test_must_fail git refs verify 2>err && >> + cat >expect <<-EOF && >> + error: extra${TABLE_NAME}: badReftableTableName: invalid reftable table name >> + EOF >> + test_cmp expect err >> + ) >> +' >> + >> +test_expect_success 'table name should be checked' ' >> + test_when_finished "rm -rf repo" && >> + git init repo && >> + ( >> + cd repo && >> + git commit --allow-empty -m initial && >> + >> + git refs verify 2>err && >> + test_must_be_empty err && >> + >> + TABLE_NAME=$(cat .git/reftable/tables.list | head -n1) && > > Same here wrt the extra invocation of cat(1). > Will change! Thanks. > Patrick ``` ## Karthik Nayak, 2025-09-09 13:49 Subject: Re: [PATCH v2 2/5] refs/reftable: add fsck check for checking the table name Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZRzEY7VTcRTLz1u6%3DA5Mz0XFOhqNWK4s_wr1WOP%3D%3DB1rg%40mail.gmail.com In-Reply-To: ``` shejialuo writes: > On Wed, Sep 03, 2025 at 10:07:13AM +0200, Patrick Steinhardt wrote: > > [snip] > >> > +static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o, >> > struct worktree *wt UNUSED) >> > { >> > - return 0; >> > + struct reftable_ref_store *refs; >> > + struct strmap_entry *entry; >> > + struct hashmap_iter iter; >> > + int ret = 0; >> > + >> > + refs = reftable_be_downcast(ref_store, REF_STORE_READ, "fsck"); >> > + >> > + if (o->verbose) >> > + fprintf_ln(stderr, _("Checking references consistency")); >> >> This line is duplicate across both backends, right? Maybe it's something >> that we can do in the generic logic? >> > > That's right, it is duplicate. If we want to remove this, we need to do > this in the "builtin/refs.c". But I wonder whether we should do this in > the first place. Should we rather add more detailed information just > like the following code for packed backend? > > if (o->verbose) > fprintf_ln(stderr, "Checking packed-refs file %s", refs->path); > > Instead of just using > > Checking references consistency > > Could we use > > Checking reftable references consistency > > However, I also feel strange about above, :) > > [snip] > >> > +/* Represents an individual error encountered during the FSCK checks. */ >> > +struct reftable_fsck_info { >> > + enum reftable_fsck_error error; >> > + const char *msg; >> > + const char *path; >> > +}; >> >> I wonder whether it should be the reftable library that decides on the >> severity of each generated finding. >> > I think I did rush while agreeing to do this change and didn't realize the complexity of it. > That's an interesting question. Let's inspect how Git handles the > severity. When defining the fsck message id, we need to specify its > severity like the following shows, this happens at compile time: > > FUNC(BAD_REFERENT_NAME, ERROR) > This is used to create the enum of all values, but there is a complimentary structure `msg_id_info` which holds the mapping for each message id to its error category. Both of these could be extended at compile time by including the errors from the reftable header. But to do this in a backend agnostic way, we'd have to receive and re-expose it via `refs.h`. > And we could set the configuration "fsck.[message id]=" to change the > fsck message severity. > > Then let's think if reftable library decides the severity. It means that > we need to use the API from reftable library to update > "fsck_option->msg_type" at the runtime. And it is bad because the fsck > infrastructure would be highly coupled with the reftable library. > > So, I don't think it's a good idea for reftable library to choose the > severity. Instead, reftable library should just provide users with error > types and let the users decide the severity. > So while there are ways to do it, it won't be simple/elegant and I'm not sure it'd be worth it. > Thanks, > Jialuo ``` ## Karthik Nayak, 2025-09-15 09:27 Subject: Re: [PATCH v2 3/5] refs/reftable: add fsck check for number of tables Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZR9nXkdXuMik2uq8%2BkpxQwmB72r7XZB0rnJSvKU7mKmkw%40mail.gmail.com In-Reply-To: ``` Patrick Steinhardt writes: > On Tue, Sep 02, 2025 at 09:05:23AM +0200, Karthik Nayak wrote: >> Introduce a reftable fsck check to check that the number of files in the >> reftable directory matches the number of files listed in 'tables.list'. >> We do this by iterating over the files in the reftable directory and >> counting all the files present excluding the 'tables.list'. This is also >> exposed over Git's fsck checks as a 'badReftableStackCount' error. > > This feels overly strict, as it can always be the case that a concurrent > process is currently updating the stack. Furthermore, it's expected that > on Windows systems deletion of an old table may not work because the > file is still kept open by another process. The reftable library is > prepared to handle this alright and will re-try deleting the table at a > later point in time. > Yeah that makes sense. > So maybe a better check would be to verify that there are no files with > unexpected names in the directory? > I was hoping to add structured consistency check in a layered format - Stack - For each `Table` - For each `Block` - For each `Ref` But this wouldn't belong to that, since it isn't part of the stack. So, I'll keep the above structure and also add this to the stack level. So we'll have - Stack - For each `Table` - For each `Block` - For each `Ref` - Other Stack level checks - Check other files in the repo > Patrick ``` ## Karthik Nayak, 2025-09-18 08:11 Subject: Re: [PATCH v2 5/5] refs/reftable: add fsck check for incorrect update index Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZTR0X%2BZa-8O04%2B6GzSUK83GUwaHTaWij3MA0vty2WUJUg%40mail.gmail.com In-Reply-To: ``` Junio C Hamano writes: > Karthik Nayak writes: > >> Introduce a fsck check for the reftable backend, which checks if the >> tables in 'tables.list' contain sequential update index. The tables in >> the reftable backend should contain sequential update index. This fsck >> check ensures that. >> >> We must note that the reftable backend itself doesn't check to ensure >> this and it also doesn't check to ensure that the index in the table >> name matches the index in the header or the table. The latter is not >> implemented in this fsck check either and will be added in a future >> patch where we add fsck checks for internals of a table. > > Similar to the previous step, I am not sure why this should not be > checked at runtime and is flagged as an error. > > In general, we do try to avoid retroactively tightening rules, but > the reftable is so new and not even the default. If we noticed that > the runtime has been overly loose, the time to tighten it is now, > not after even more installations use it. I think your point is fair and I agree. I did consider it, but didn't want to 'retroactively tightening rules'. But I think it is justified, more so since we're not introducing any changes on the format, but just more validation around it. I should've replied here sooner, but I didn't ever get to it. I found some time to finally fix the comments from this series and will send in a new version. I will be more diligent about it here on. - Karthik ```