{"thread":{"id":"64068","subject":"[PATCH v2 1/5] fsck: order 'fsck_msg_type' alphabetically","startedAt":"2025-09-02T07:05:37Z","lastAt":"2025-09-18T08:11:26Z","messageCount":18,"participants":["Karthik Nayak","Junio C Hamano","Patrick Steinhardt","shejialuo"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"525293","messageId":"20250902-228-reftable-introduce-consistency-checks-v2-0-4f96b3834779@gmail.com","threadId":"64068","inReplyTo":"20250819-228-reftable-introduce-consistency-checks-v1-0-8b8f6879fa9e@gmail.com","subject":"[PATCH v2 0/5] refs/reftable: add fsck checks","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-02T07:05:20Z","receivedAt":"2025-09-02T07:05:37Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This series adds the required infrastructure and also some fsck checks\nfor the reftable backend.\n\nSince the reftable backend is treated as a library within the Git\ncodebase, we don't want to spillover our internal fsck implementation\ninto the library. At the same time, the fsck checks need to access\ninternal structures of the reftable library which aren't exposed outside\nthe library.\n\nSo we solve this by adding a 'reftable/fsck.[ch]' which implements and\nexposes a checker for the reftable library and returns specific errors\nas defined by the library. We then add glue code within\n'refs/reftable-backend.c' to map these errors to errors which Git's fsck\nimplementation would understand. This allows us to separate concerns.\n\nThis series then adds some checks on the stack ('reftable/tables.list')\nlevel of reftable, namely:\n1. The table name is as per the spec\n2. The number of tables are consistent\n3. The tables.list has a newline at the end of file\n4. The table names follow correct index sequences\n\nI also plan to send in follow up series's which will implement further\nchecks and go into deeper layers (tables, block, references).\n\nSigned-off-by: Karthik Nayak \n---\nChanges in v2:\n- Ensured that 'struct reftable_fsck_info' is passed around as a\n pointer, this provides a smaller footprint (pointer size vs struct\n size).\n- Run FSCK checks for other worktrees too, even if one of them fails.\n- Separate messaging for table name vs table check and add additional\n test.\n- Use the relative path in messages used.\n- Small style and typo fixes.\n- Link to v1: https://lore.kernel.org/r/20250819-228-reftable-introduce-consistency-checks-v1-0-8b8f6879fa9e@gmail.com\n\n---\n Documentation/fsck-msgids.adoc | 15 +++-\n Makefile | 1 +\n fsck.h | 154 ++++++++++++++++++++-------------------\n meson.build | 1 +\n refs/reftable-backend.c | 66 +++++++++++++++--\n reftable/fsck.c | 134 ++++++++++++++++++++++++++++++++++\n reftable/reftable-fsck.h | 44 +++++++++++\n t/meson.build | 3 +-\n t/t0614-reftable-fsck.sh | 161 +++++++++++++++++++++++++++++++++++++++++\n 9 files changed, 494 insertions(+), 85 deletions(-)\n\nKarthik Nayak (5):\n fsck: order 'fsck_msg_type' alphabetically\n refs/reftable: add fsck check for checking the table name\n refs/reftable: add fsck check for number of tables\n refs/reftable: add fsck check for trailing newline\n refs/reftable: add fsck check for incorrect update index\n\nRange-diff versus v1:\n\n1: d1875fbbc7 = 1: c049cd428a fsck: order 'fsck_msg_type' alphabetically\n2: b63799aad1 ! 2: 1e46786745 refs/reftable: add fsck check for checking the table name\n @@ Commit message\n Also add 'badReftableTableName' as a corresponding error within Git. Add\n a test to check for this behavior.\n \n + While here, remove a unused header `#include \"../lockfile.h\"` from\n + 'refs/reftable-backend.c'.\n +\n Signed-off-by: Karthik Nayak \n \n ## Documentation/fsck-msgids.adoc ##\n @@ refs/reftable-backend.c: static int reftable_be_reflog_expire(struct ref_store *\n +\t\tfprintf_ln(stderr, \"%s\", _(msg));\n +}\n +\n -+static int reftable_fsck_error_handler(struct reftable_fsck_info info,\n ++static int reftable_fsck_error_handler(struct reftable_fsck_info *info,\n +\t\t\t\t void *cb_data)\n +{\n ++\tstruct fsck_ref_report report = { .path = info->path };\n +\tstruct fsck_options *o = cb_data;\n -+\tstruct fsck_ref_report report = { .path = info.path };\n +\tenum fsck_msg_id msg_id;\n +\n -+\tswitch (info.error) {\n ++\tswitch (info->error) {\n +\tcase REFTABLE_FSCK_ERROR_TABLE_NAME:\n +\t\tmsg_id = FSCK_MSG_BAD_REFTABLE_TABLE_NAME;\n +\t\tbreak;\n +\tdefault:\n -+\t\tBUG(\"unknown fsck error: %d\", info.error);\n ++\t\tBUG(\"unknown fsck error: %d\", info->error);\n +\t}\n +\n -+\treturn fsck_report_ref(o, &report, msg_id, \"%s\", info.msg);\n ++\treturn fsck_report_ref(o, &report, msg_id, \"%s\", info->msg);\n +}\n +\n +static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o,\n @@ refs/reftable-backend.c: static int reftable_be_reflog_expire(struct ref_store *\n +\tif (o->verbose)\n +\t\tfprintf_ln(stderr, _(\"Checking references consistency\"));\n +\n -+\tret = reftable_fsck_check(refs->main_backend.stack, reftable_fsck_error_handler,\n ++\tret |= reftable_fsck_check(refs->main_backend.stack, reftable_fsck_error_handler,\n +\t\t\t\t reftable_fsck_verbose_handler, o);\n -+\tif (!ret)\n -+\t\treturn ret;\n +\n +\tstrmap_for_each_entry(&refs->worktree_backends, &iter, entry) {\n +\t\tstruct reftable_backend *b = (struct reftable_backend *)entry->value;\n -+\t\tret = reftable_fsck_check(b->stack, reftable_fsck_error_handler,\n ++\t\tret |= reftable_fsck_check(b->stack, reftable_fsck_error_handler,\n +\t\t\t\t\t reftable_fsck_verbose_handler, o);\n -+\t\tif (!ret)\n -+\t\t\treturn ret;\n +\t}\n +\n +\treturn ret;\n @@ reftable/fsck.c (new)\n +\t\t\treftable_fsck_verbose_fn verbose_fn,\n +\t\t\tvoid *cb_data)\n +{\n ++\n +\tchar **names = NULL;\n +\tuint64_t min, max;\n +\tint err = 0;\n @@ reftable/fsck.c (new)\n +\t\tstruct reftable_fsck_info info = {\n +\t\t\t.error = REFTABLE_FSCK_ERROR_TABLE_NAME,\n +\t\t\t.path = names[i],\n -+\t\t\t.msg = \"invalid reftable name\"\n +\t\t};\n +\t\tuint32_t rnd;\n +\t\t/*\n @@ reftable/fsck.c (new)\n +\n +\t\tif (sscanf(names[i], \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x%5s\",\n +\t\t\t &min, &max, &rnd, tail) != 4) {\n -+\t\t\terr = report_fn(info, cb_data);\n ++\t\t\tinfo.msg = \"invalid reftable table name\";\n ++\t\t\terr = report_fn(&info, cb_data);\n ++\t\t\tcontinue;\n +\t\t}\n +\n +\t\tif (strcmp(tail, \".ref\")) {\n -+\t\t\terr = report_fn(info, cb_data);\n ++\t\t\tinfo.msg = \"invalid reftable table extension\";\n ++\t\t\terr = report_fn(&info, cb_data);\n +\t\t}\n +\t}\n +\n @@ reftable/reftable-fsck.h (new)\n +\tREFTABLE_FSCK_ERROR_TABLE_NAME = -1,\n +};\n +\n -+/* Represents an individual error encounctered during the FSCK checks. */\n ++/* Represents an individual error encountered during the FSCK checks. */\n +struct reftable_fsck_info {\n +\tenum reftable_fsck_error error;\n +\tconst char *msg;\n +\tconst char *path;\n +};\n +\n -+typedef int reftable_fsck_report_fn(struct reftable_fsck_info info,\n ++typedef int reftable_fsck_report_fn(struct reftable_fsck_info *info,\n +\t\t\t\t void *cb_data);\n +typedef void reftable_fsck_verbose_fn(const char *msg, void *cb_data);\n +\n @@ reftable/reftable-fsck.h (new)\n + *\n + * If an issue is encountered, the issue is reported to the callee via the\n + * provided 'report_fn'. If the issue is non-recoverable the flow will not\n -+ * conitnue. If it is recoverable, the flow will continue and further issues\n ++ * continue. If it is recoverable, the flow will continue and further issues\n + * will be reported as identified.\n + *\n + * The 'verbose_fn' will be invoked to provide verbose information about\n @@ t/t0614-reftable-fsck.sh (new)\n +\t\ttest_must_be_empty err &&\n +\n +\t\tTABLE_NAME=$(cat .git/reftable/tables.list | head -n1) &&\n ++\t\tsed \"1s/^/extra/\" .git/reftable/tables.list >.git/reftable/tables.list.tmp &&\n ++\t\tmv .git/reftable/tables.list.tmp .git/reftable/tables.list &&\n ++\t\tmv .git/reftable/${TABLE_NAME} .git/reftable/extra${TABLE_NAME} &&\n ++\n ++\t\ttest_must_fail git refs verify 2>err &&\n ++\t\tcat >expect <<-EOF &&\n ++\t\terror: extra${TABLE_NAME}: badReftableTableName: invalid reftable table name\n ++\t\tEOF\n ++\t\ttest_cmp expect err\n ++\t)\n ++'\n ++\n ++test_expect_success 'table name should be checked' '\n ++\ttest_when_finished \"rm -rf repo\" &&\n ++\tgit init repo &&\n ++\t(\n ++\t\tcd repo &&\n ++\t\tgit commit --allow-empty -m initial &&\n ++\n ++\t\tgit refs verify 2>err &&\n ++\t\ttest_must_be_empty err &&\n ++\n ++\t\tTABLE_NAME=$(cat .git/reftable/tables.list | head -n1) &&\n +\t\tsed \"1s/$/extra/\" .git/reftable/tables.list >.git/reftable/tables.list.tmp &&\n +\t\tmv .git/reftable/tables.list.tmp .git/reftable/tables.list &&\n +\t\tmv .git/reftable/${TABLE_NAME} .git/reftable/${TABLE_NAME}extra &&\n +\n +\t\ttest_must_fail git refs verify 2>err &&\n +\t\tcat >expect <<-EOF &&\n -+\t\terror: ${TABLE_NAME}extra: badReftableTableName: invalid reftable name\n ++\t\terror: ${TABLE_NAME}extra: badReftableTableName: invalid reftable table extension\n +\t\tEOF\n +\t\ttest_cmp expect err\n +\t)\n3: 4c6c99ded3 ! 3: 52fc14fdeb refs/reftable: add fsck check for number of tables\n @@ fsck.h: enum fsck_msg_type {\n \tFUNC(BAD_REF_FILETYPE, ERROR) \\\n \n ## refs/reftable-backend.c ##\n -@@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_fsck_info info,\n +@@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_fsck_info *info,\n \tcase REFTABLE_FSCK_ERROR_TABLE_NAME:\n \t\tmsg_id = FSCK_MSG_BAD_REFTABLE_TABLE_NAME;\n \t\tbreak;\n @@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_\n +\t\tmsg_id = FSCK_MSG_BAD_REFTABLE_STACK_COUNT;\n +\t\tbreak;\n \tdefault:\n - \t\tBUG(\"unknown fsck error: %d\", info.error);\n + \t\tBUG(\"unknown fsck error: %d\", info->error);\n \t}\n \n ## reftable/fsck.c ##\n @@ reftable/fsck.c: int reftable_fsck_check(struct reftable_stack *stack,\n +\tif (!reftable_fsck_valid_stack_count(stack)) {\n +\t\tstruct reftable_fsck_info info = {\n +\t\t\t.error = REFTABLE_FSCK_ERROR_STACK_COUNT,\n -+\t\t\t.path = stack->list_file,\n ++\t\t\t.path = \"reftable/tables.list\",\n +\t\t\t.msg = \"mismatch in number of tables\"\n +\t\t};\n +\n -+\t\terr = report_fn(info, cb_data);\n ++\t\terr = report_fn(&info, cb_data);\n +\t}\n +\n out:\n @@ reftable/reftable-fsck.h\n +\tREFTABLE_FSCK_ERROR_STACK_COUNT = -2,\n };\n \n - /* Represents an individual error encounctered during the FSCK checks. */\n + /* Represents an individual error encountered during the FSCK checks. */\n \n ## t/t0614-reftable-fsck.sh ##\n @@ t/t0614-reftable-fsck.sh: test_expect_success 'table name should be checked' '\n @@ t/t0614-reftable-fsck.sh: test_expect_success 'table name should be checked' '\n +\n +\t\ttest_must_fail git refs verify 2>err &&\n +\t\tcat >expect <<-EOF &&\n -+\t\terror: $(pwd)/.git/reftable/tables.list: badReftableStackCount: mismatch in number of tables\n ++\t\terror: reftable/tables.list: badReftableStackCount: mismatch in number of tables\n +\t\tEOF\n +\t\ttest_cmp expect err\n +\t)\n4: 7e8a14c77e ! 4: 4099878ceb refs/reftable: add fsck check for trailing newline\n @@ fsck.h: enum fsck_msg_type {\n \tFUNC(BAD_REF_FILETYPE, ERROR) \\\n \n ## refs/reftable-backend.c ##\n -@@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_fsck_info info,\n +@@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_fsck_info *info,\n \tcase REFTABLE_FSCK_ERROR_STACK_COUNT:\n \t\tmsg_id = FSCK_MSG_BAD_REFTABLE_STACK_COUNT;\n \t\tbreak;\n @@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_\n +\t\tmsg_id = FSCK_MSG_BAD_REFTABLE_STACK_LIST_NEWLINE;\n +\t\tbreak;\n \tdefault:\n - \t\tBUG(\"unknown fsck error: %d\", info.error);\n + \t\tBUG(\"unknown fsck error: %d\", info->error);\n \t}\n \n ## reftable/fsck.c ##\n @@ reftable/fsck.c: int reftable_fsck_check(struct reftable_stack *stack,\n +\tif (!reftable_fsck_stack_contains_newline(stack->list_file)) {\n +\t\tstruct reftable_fsck_info info = {\n +\t\t\t.error = REFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE,\n -+\t\t\t.path = stack->list_file,\n ++\t\t\t.path = \"reftable/tables.list\",\n +\t\t\t.msg = \"trailing newline missing in stack list\"\n +\t\t};\n +\n -+\t\terr = report_fn(info, cb_data);\n ++\t\terr = report_fn(&info, cb_data);\n +\t}\n +\n \tverbose_fn(\"Checking reftable tables count\", cb_data);\n @@ reftable/reftable-fsck.h: enum reftable_fsck_error {\n +\tREFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE = -3,\n };\n \n - /* Represents an individual error encounctered during the FSCK checks. */\n + /* Represents an individual error encountered during the FSCK checks. */\n \n ## t/t0614-reftable-fsck.sh ##\n @@ t/t0614-reftable-fsck.sh: test_expect_success 'table count should be checked' '\n @@ t/t0614-reftable-fsck.sh: test_expect_success 'table count should be checked' '\n +\n +\t\ttest_must_fail git refs verify 2>err &&\n +\t\tcat >expect <<-EOF &&\n -+\t\terror: $(pwd)/.git/reftable/tables.list: badReftableStackListNewline: trailing newline missing in stack list\n ++\t\terror: reftable/tables.list: badReftableStackListNewline: trailing newline missing in stack list\n +\t\tEOF\n +\t\ttest_cmp expect err\n +\t)\n5: 56ee4348d5 ! 5: e33345088b refs/reftable: add fsck check for incorrect update index\n @@ fsck.h: enum fsck_msg_type {\n \tFUNC(BAD_REF_NAME, ERROR) \\\n \n ## refs/reftable-backend.c ##\n -@@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_fsck_info info,\n +@@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_fsck_info *info,\n \tcase REFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE:\n \t\tmsg_id = FSCK_MSG_BAD_REFTABLE_STACK_LIST_NEWLINE;\n \t\tbreak;\n @@ refs/reftable-backend.c: static int reftable_fsck_error_handler(struct reftable_\n +\t\tmsg_id = FSCK_MSG_BAD_REFTABLE_UPDATE_INDEX;\n +\t\tbreak;\n \tdefault:\n - \t\tBUG(\"unknown fsck error: %d\", info.error);\n + \t\tBUG(\"unknown fsck error: %d\", info->error);\n \t}\n \n ## reftable/fsck.c ##\n @@ reftable/fsck.c: int reftable_fsck_check(struct reftable_stack *stack,\n \t\t\treftable_fsck_verbose_fn verbose_fn,\n \t\t\tvoid *cb_data)\n {\n +-\n +\tuint64_t min, max, prev_max = 0;\n \tchar **names = NULL;\n -\tuint64_t min, max;\n @@ reftable/fsck.c: int reftable_fsck_check(struct reftable_stack *stack,\n \n \tif (stack == NULL)\n @@ reftable/fsck.c: int reftable_fsck_check(struct reftable_stack *stack,\n - \t\t\terr = report_fn(info, cb_data);\n + \t\t\tcontinue;\n \t\t}\n \n +\t\tif (min != (prev_max + 1) || max < min) {\n @@ reftable/fsck.c: int reftable_fsck_check(struct reftable_stack *stack,\n +\t\t\t\t.msg = \"incorrect update index in table name\"\n +\t\t\t};\n +\n -+\t\t\terr = report_fn(info, cb_data);\n ++\t\t\terr = report_fn(&info, cb_data);\n +\t\t}\n +\n \t\tif (strcmp(tail, \".ref\")) {\n - \t\t\terr = report_fn(info, cb_data);\n + \t\t\tinfo.msg = \"invalid reftable table extension\";\n + \t\t\terr = report_fn(&info, cb_data);\n \t\t}\n +\n +\t\tprev_max = max;\n @@ reftable/reftable-fsck.h: enum reftable_fsck_error {\n +\tREFTABLE_FSCK_ERROR_UPDATE_INDEX = -4,\n };\n \n - /* Represents an individual error encounctered during the FSCK checks. */\n + /* Represents an individual error encountered during the FSCK checks. */\n \n ## t/t0614-reftable-fsck.sh ##\n @@ t/t0614-reftable-fsck.sh: test_expect_success 'stack list must contain trailing newline' '\n\n\nbase-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\nchange-id: 20250714-228-reftable-introduce-consistency-checks-379ded93c544\n\nThanks\n- Karthik\n\n"},{"id":"525292","messageId":"20250902-228-reftable-introduce-consistency-checks-v2-1-4f96b3834779@gmail.com","threadId":"64068","inReplyTo":"20250902-228-reftable-introduce-consistency-checks-v2-0-4f96b3834779@gmail.com","subject":"[PATCH v2 1/5] fsck: order 'fsck_msg_type' alphabetically","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-02T07:05:21Z","receivedAt":"2025-09-02T07:05:38Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The list of 'fsck_msg_type' seem to be alphabetically ordered, but there\nare a few small misses. Fix this by sorting the sub-sections of the\nlist to maintain alphabetical ordering. Also fix a clang-format issue\nwhere the escaped newlines are not aligned.\n\nWhile here, remove a duplicate instance of 'gitmodulesLarge' in the\n'fsck-msgids' documentation.\n\nSigned-off-by: Karthik Nayak \n---\n Documentation/fsck-msgids.adoc | 3 -\n fsck.h | 150 ++++++++++++++++++++---------------------\n 2 files changed, 75 insertions(+), 78 deletions(-)\n\ndiff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc\nindex 0ba4f9a27e..1c912615f9 100644\n--- a/Documentation/fsck-msgids.adoc\n+++ b/Documentation/fsck-msgids.adoc\n@@ -104,9 +104,6 @@\n `gitmodulesParse`::\n \t(INFO) Could not parse `.gitmodules` blob.\n \n-`gitmodulesLarge`;\n-\t(ERROR) `.gitmodules` blob is too large to parse.\n-\n `gitmodulesPath`::\n \t(ERROR) `.gitmodules` path is invalid.\n \ndiff --git a/fsck.h b/fsck.h\nindex dd7df3d5b3..559ad57807 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -20,82 +20,82 @@ enum fsck_msg_type {\n * two in sync.\n */\n \n-#define FOREACH_FSCK_MSG_ID(FUNC) \\\n-\t/* fatal errors */ \\\n-\tFUNC(NUL_IN_HEADER, FATAL) \\\n-\tFUNC(UNTERMINATED_HEADER, FATAL) \\\n-\t/* errors */ \\\n-\tFUNC(BAD_DATE, ERROR) \\\n-\tFUNC(BAD_DATE_OVERFLOW, ERROR) \\\n-\tFUNC(BAD_EMAIL, ERROR) \\\n-\tFUNC(BAD_NAME, ERROR) \\\n-\tFUNC(BAD_OBJECT_SHA1, ERROR) \\\n-\tFUNC(BAD_PACKED_REF_ENTRY, ERROR) \\\n-\tFUNC(BAD_PACKED_REF_HEADER, ERROR) \\\n-\tFUNC(BAD_PARENT_SHA1, ERROR) \\\n-\tFUNC(BAD_REF_CONTENT, ERROR) \\\n-\tFUNC(BAD_REF_FILETYPE, ERROR) \\\n-\tFUNC(BAD_REF_NAME, ERROR) \\\n-\tFUNC(BAD_REFERENT_NAME, ERROR) \\\n-\tFUNC(BAD_TIMEZONE, ERROR) \\\n-\tFUNC(BAD_TREE, ERROR) \\\n-\tFUNC(BAD_TREE_SHA1, ERROR) \\\n-\tFUNC(BAD_TYPE, ERROR) \\\n-\tFUNC(DUPLICATE_ENTRIES, ERROR) \\\n-\tFUNC(MISSING_AUTHOR, ERROR) \\\n-\tFUNC(MISSING_COMMITTER, ERROR) \\\n-\tFUNC(MISSING_EMAIL, ERROR) \\\n-\tFUNC(MISSING_NAME_BEFORE_EMAIL, ERROR) \\\n-\tFUNC(MISSING_OBJECT, ERROR) \\\n-\tFUNC(MISSING_SPACE_BEFORE_DATE, ERROR) \\\n-\tFUNC(MISSING_SPACE_BEFORE_EMAIL, ERROR) \\\n-\tFUNC(MISSING_TAG, ERROR) \\\n-\tFUNC(MISSING_TAG_ENTRY, ERROR) \\\n-\tFUNC(MISSING_TREE, ERROR) \\\n-\tFUNC(MISSING_TYPE, ERROR) \\\n-\tFUNC(MISSING_TYPE_ENTRY, ERROR) \\\n-\tFUNC(MULTIPLE_AUTHORS, ERROR) \\\n-\tFUNC(PACKED_REF_ENTRY_NOT_TERMINATED, ERROR) \\\n-\tFUNC(PACKED_REF_UNSORTED, ERROR) \\\n-\tFUNC(TREE_NOT_SORTED, ERROR) \\\n-\tFUNC(UNKNOWN_TYPE, ERROR) \\\n-\tFUNC(ZERO_PADDED_DATE, ERROR) \\\n-\tFUNC(GITMODULES_MISSING, ERROR) \\\n-\tFUNC(GITMODULES_BLOB, ERROR) \\\n-\tFUNC(GITMODULES_LARGE, ERROR) \\\n-\tFUNC(GITMODULES_NAME, ERROR) \\\n-\tFUNC(GITMODULES_SYMLINK, ERROR) \\\n-\tFUNC(GITMODULES_URL, ERROR) \\\n-\tFUNC(GITMODULES_PATH, ERROR) \\\n-\tFUNC(GITMODULES_UPDATE, ERROR) \\\n-\tFUNC(GITATTRIBUTES_MISSING, ERROR) \\\n-\tFUNC(GITATTRIBUTES_LARGE, ERROR) \\\n-\tFUNC(GITATTRIBUTES_LINE_LENGTH, ERROR) \\\n-\tFUNC(GITATTRIBUTES_BLOB, ERROR) \\\n-\t/* warnings */ \\\n-\tFUNC(EMPTY_NAME, WARN) \\\n-\tFUNC(FULL_PATHNAME, WARN) \\\n-\tFUNC(HAS_DOT, WARN) \\\n-\tFUNC(HAS_DOTDOT, WARN) \\\n-\tFUNC(HAS_DOTGIT, WARN) \\\n-\tFUNC(NULL_SHA1, WARN) \\\n-\tFUNC(ZERO_PADDED_FILEMODE, WARN) \\\n-\tFUNC(NUL_IN_COMMIT, WARN) \\\n-\tFUNC(LARGE_PATHNAME, WARN) \\\n+#define FOREACH_FSCK_MSG_ID(FUNC) \\\n+\t/* fatal errors */ \\\n+\tFUNC(NUL_IN_HEADER, FATAL) \\\n+\tFUNC(UNTERMINATED_HEADER, FATAL) \\\n+\t/* errors */ \\\n+\tFUNC(BAD_DATE, ERROR) \\\n+\tFUNC(BAD_DATE_OVERFLOW, ERROR) \\\n+\tFUNC(BAD_EMAIL, ERROR) \\\n+\tFUNC(BAD_NAME, ERROR) \\\n+\tFUNC(BAD_OBJECT_SHA1, ERROR) \\\n+\tFUNC(BAD_PACKED_REF_ENTRY, ERROR) \\\n+\tFUNC(BAD_PACKED_REF_HEADER, ERROR) \\\n+\tFUNC(BAD_PARENT_SHA1, ERROR) \\\n+\tFUNC(BAD_REFERENT_NAME, ERROR) \\\n+\tFUNC(BAD_REF_CONTENT, ERROR) \\\n+\tFUNC(BAD_REF_FILETYPE, ERROR) \\\n+\tFUNC(BAD_REF_NAME, ERROR) \\\n+\tFUNC(BAD_TIMEZONE, ERROR) \\\n+\tFUNC(BAD_TREE, ERROR) \\\n+\tFUNC(BAD_TREE_SHA1, ERROR) \\\n+\tFUNC(BAD_TYPE, ERROR) \\\n+\tFUNC(DUPLICATE_ENTRIES, ERROR) \\\n+\tFUNC(GITATTRIBUTES_BLOB, ERROR) \\\n+\tFUNC(GITATTRIBUTES_LARGE, ERROR) \\\n+\tFUNC(GITATTRIBUTES_LINE_LENGTH, ERROR) \\\n+\tFUNC(GITATTRIBUTES_MISSING, ERROR) \\\n+\tFUNC(GITMODULES_BLOB, ERROR) \\\n+\tFUNC(GITMODULES_LARGE, ERROR) \\\n+\tFUNC(GITMODULES_MISSING, ERROR) \\\n+\tFUNC(GITMODULES_NAME, ERROR) \\\n+\tFUNC(GITMODULES_PATH, ERROR) \\\n+\tFUNC(GITMODULES_SYMLINK, ERROR) \\\n+\tFUNC(GITMODULES_UPDATE, ERROR) \\\n+\tFUNC(GITMODULES_URL, ERROR) \\\n+\tFUNC(MISSING_AUTHOR, ERROR) \\\n+\tFUNC(MISSING_COMMITTER, ERROR) \\\n+\tFUNC(MISSING_EMAIL, ERROR) \\\n+\tFUNC(MISSING_NAME_BEFORE_EMAIL, ERROR) \\\n+\tFUNC(MISSING_OBJECT, ERROR) \\\n+\tFUNC(MISSING_SPACE_BEFORE_DATE, ERROR) \\\n+\tFUNC(MISSING_SPACE_BEFORE_EMAIL, ERROR) \\\n+\tFUNC(MISSING_TAG, ERROR) \\\n+\tFUNC(MISSING_TAG_ENTRY, ERROR) \\\n+\tFUNC(MISSING_TREE, ERROR) \\\n+\tFUNC(MISSING_TYPE, ERROR) \\\n+\tFUNC(MISSING_TYPE_ENTRY, ERROR) \\\n+\tFUNC(MULTIPLE_AUTHORS, ERROR) \\\n+\tFUNC(PACKED_REF_ENTRY_NOT_TERMINATED, ERROR) \\\n+\tFUNC(PACKED_REF_UNSORTED, ERROR) \\\n+\tFUNC(TREE_NOT_SORTED, ERROR) \\\n+\tFUNC(UNKNOWN_TYPE, ERROR) \\\n+\tFUNC(ZERO_PADDED_DATE, ERROR) \\\n+\t/* warnings */ \\\n+\tFUNC(EMPTY_NAME, WARN) \\\n+\tFUNC(FULL_PATHNAME, WARN) \\\n+\tFUNC(HAS_DOT, WARN) \\\n+\tFUNC(HAS_DOTDOT, WARN) \\\n+\tFUNC(HAS_DOTGIT, WARN) \\\n+\tFUNC(LARGE_PATHNAME, WARN) \\\n+\tFUNC(NULL_SHA1, WARN) \\\n+\tFUNC(NUL_IN_COMMIT, WARN) \\\n+\tFUNC(ZERO_PADDED_FILEMODE, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n-\tFUNC(BAD_FILEMODE, INFO) \\\n-\tFUNC(EMPTY_PACKED_REFS_FILE, INFO) \\\n-\tFUNC(GITMODULES_PARSE, INFO) \\\n-\tFUNC(GITIGNORE_SYMLINK, INFO) \\\n-\tFUNC(GITATTRIBUTES_SYMLINK, INFO) \\\n-\tFUNC(MAILMAP_SYMLINK, INFO) \\\n-\tFUNC(BAD_TAG_NAME, INFO) \\\n-\tFUNC(MISSING_TAGGER_ENTRY, INFO) \\\n-\tFUNC(SYMLINK_REF, INFO) \\\n-\tFUNC(REF_MISSING_NEWLINE, INFO) \\\n-\tFUNC(SYMREF_TARGET_IS_NOT_A_REF, INFO) \\\n-\tFUNC(TRAILING_REF_CONTENT, INFO) \\\n-\t/* ignored (elevated when requested) */ \\\n+\tFUNC(BAD_FILEMODE, INFO) \\\n+\tFUNC(BAD_TAG_NAME, INFO) \\\n+\tFUNC(EMPTY_PACKED_REFS_FILE, INFO) \\\n+\tFUNC(GITATTRIBUTES_SYMLINK, INFO) \\\n+\tFUNC(GITIGNORE_SYMLINK, INFO) \\\n+\tFUNC(GITMODULES_PARSE, INFO) \\\n+\tFUNC(MAILMAP_SYMLINK, INFO) \\\n+\tFUNC(MISSING_TAGGER_ENTRY, INFO) \\\n+\tFUNC(REF_MISSING_NEWLINE, INFO) \\\n+\tFUNC(SYMLINK_REF, INFO) \\\n+\tFUNC(SYMREF_TARGET_IS_NOT_A_REF, INFO) \\\n+\tFUNC(TRAILING_REF_CONTENT, INFO) \\\n+\t/* ignored (elevated when requested) */ \\\n \tFUNC(EXTRA_HEADER_ENTRY, IGNORE)\n \n #define MSG_ID(id, msg_type) FSCK_MSG_##id,\n\n-- \n2.50.1\n\n"},{"id":"525294","messageId":"20250902-228-reftable-introduce-consistency-checks-v2-2-4f96b3834779@gmail.com","threadId":"64068","inReplyTo":"20250902-228-reftable-introduce-consistency-checks-v2-0-4f96b3834779@gmail.com","subject":"[PATCH v2 2/5] refs/reftable: add fsck check for checking the table name","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-02T07:05:22Z","receivedAt":"2025-09-02T07:05:39Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `git refs verify` command is used to run fsck checks on the\nreference backends. This command is also invoked when users run 'git\nfsck'. While the files-backend has some fsck checks added, the reftable\nbackend lacks such checks. Let's add the required infrastructure and a\ncheck to test for the table names in the 'tables.list' of reftables.\n\nFor the infrastructure, since the reftable library is treated as an\nindependent library we should ensure that the library code works\nindependently without knowledge about Git's internals. To do this,\nadd both 'reftable/fsck.c' and 'reftable/reftable-fsck.h'. Which\nprovide an entry point 'reftable_fsck_check' for running fsck checks\nover a provided reftable stack. The callee provides the function with\ncallbacks to handle issue and information reporting.\n\nAdd glue code in 'refs/reftable-backend.c' which calls the reftable\nlibrary to perform the fsck checks. Here we also map the reftable errors\nto Git' fsck errors.\n\nIntroduce a check to validate table names for a given reftable stack.\nAlso add 'badReftableTableName' as a corresponding error within Git. Add\na test to check for this behavior.\n\nWhile here, remove a unused header `#include \"../lockfile.h\"` from\n'refs/reftable-backend.c'.\n\nSigned-off-by: Karthik Nayak \n---\n Documentation/fsck-msgids.adoc | 3 +++\n Makefile | 1 +\n fsck.h | 1 +\n meson.build | 1 +\n refs/reftable-backend.c | 57 ++++++++++++++++++++++++++++++++++++-----\n reftable/fsck.c | 53 ++++++++++++++++++++++++++++++++++++++\n reftable/reftable-fsck.h | 38 +++++++++++++++++++++++++++\n t/meson.build | 3 ++-\n t/t0614-reftable-fsck.sh | 58 ++++++++++++++++++++++++++++++++++++++++++\n 9 files changed, 208 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc\nindex 1c912615f9..784ddc0df5 100644\n--- a/Documentation/fsck-msgids.adoc\n+++ b/Documentation/fsck-msgids.adoc\n@@ -38,6 +38,9 @@\n `badReferentName`::\n \t(ERROR) The referent name of a symref is invalid.\n \n+`badReftableTableName`::\n+\t(ERROR) A reftable table has an invalid name.\n+\n `badTagName`::\n \t(INFO) A tag has an invalid format.\n \ndiff --git a/Makefile b/Makefile\nindex e11340c1ae..f2ddcc8d7c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2733,6 +2733,7 @@ REFTABLE_OBJS += reftable/error.o\n REFTABLE_OBJS += reftable/block.o\n REFTABLE_OBJS += reftable/blocksource.o\n REFTABLE_OBJS += reftable/iter.o\n+REFTABLE_OBJS += reftable/fsck.o\n REFTABLE_OBJS += reftable/merged.o\n REFTABLE_OBJS += reftable/pq.o\n REFTABLE_OBJS += reftable/record.o\ndiff --git a/fsck.h b/fsck.h\nindex 559ad57807..5901f944a1 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -34,6 +34,7 @@ enum fsck_msg_type {\n \tFUNC(BAD_PACKED_REF_HEADER, ERROR) \\\n \tFUNC(BAD_PARENT_SHA1, ERROR) \\\n \tFUNC(BAD_REFERENT_NAME, ERROR) \\\n+\tFUNC(BAD_REFTABLE_TABLE_NAME, ERROR) \\\n \tFUNC(BAD_REF_CONTENT, ERROR) \\\n \tFUNC(BAD_REF_FILETYPE, ERROR) \\\n \tFUNC(BAD_REF_NAME, ERROR) \\\ndiff --git a/meson.build b/meson.build\nindex 5dd299b496..82879fbfaa 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -452,6 +452,7 @@ libgit_sources = [\n 'reftable/error.c',\n 'reftable/block.c',\n 'reftable/blocksource.c',\n+ 'reftable/fsck.c',\n 'reftable/iter.c',\n 'reftable/merged.c',\n 'reftable/pq.c',\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 8dae1e1112..c38c6422f8 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -6,20 +6,21 @@\n #include \"../config.h\"\n #include \"../dir.h\"\n #include \"../environment.h\"\n+#include \"../fsck.h\"\n #include \"../gettext.h\"\n #include \"../hash.h\"\n #include \"../hex.h\"\n #include \"../iterator.h\"\n #include \"../ident.h\"\n-#include \"../lockfile.h\"\n #include \"../object.h\"\n #include \"../path.h\"\n #include \"../refs.h\"\n #include \"../reftable/reftable-basics.h\"\n-#include \"../reftable/reftable-stack.h\"\n-#include \"../reftable/reftable-record.h\"\n #include \"../reftable/reftable-error.h\"\n+#include \"../reftable/reftable-fsck.h\"\n #include \"../reftable/reftable-iterator.h\"\n+#include \"../reftable/reftable-record.h\"\n+#include \"../reftable/reftable-stack.h\"\n #include \"../repo-settings.h\"\n #include \"../setup.h\"\n #include \"../strmap.h\"\n@@ -2675,11 +2676,55 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,\n \treturn ret;\n }\n \n-static int reftable_be_fsck(struct ref_store *ref_store UNUSED,\n-\t\t\t struct fsck_options *o UNUSED,\n+static void reftable_fsck_verbose_handler(const char *msg, void *cb_data)\n+{\n+\tstruct fsck_options *o = cb_data;\n+\n+\tif (o->verbose)\n+\t\tfprintf_ln(stderr, \"%s\", _(msg));\n+}\n+\n+static int reftable_fsck_error_handler(struct reftable_fsck_info *info,\n+\t\t\t\t void *cb_data)\n+{\n+\tstruct fsck_ref_report report = { .path = info->path };\n+\tstruct fsck_options *o = cb_data;\n+\tenum fsck_msg_id msg_id;\n+\n+\tswitch (info->error) {\n+\tcase REFTABLE_FSCK_ERROR_TABLE_NAME:\n+\t\tmsg_id = FSCK_MSG_BAD_REFTABLE_TABLE_NAME;\n+\t\tbreak;\n+\tdefault:\n+\t\tBUG(\"unknown fsck error: %d\", info->error);\n+\t}\n+\n+\treturn fsck_report_ref(o, &report, msg_id, \"%s\", info->msg);\n+}\n+\n+static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o,\n \t\t\t struct worktree *wt UNUSED)\n {\n-\treturn 0;\n+\tstruct reftable_ref_store *refs;\n+\tstruct strmap_entry *entry;\n+\tstruct hashmap_iter iter;\n+\tint ret = 0;\n+\n+\trefs = reftable_be_downcast(ref_store, REF_STORE_READ, \"fsck\");\n+\n+\tif (o->verbose)\n+\t\tfprintf_ln(stderr, _(\"Checking references consistency\"));\n+\n+\tret |= reftable_fsck_check(refs->main_backend.stack, reftable_fsck_error_handler,\n+\t\t\t\t reftable_fsck_verbose_handler, o);\n+\n+\tstrmap_for_each_entry(&refs->worktree_backends, &iter, entry) {\n+\t\tstruct reftable_backend *b = (struct reftable_backend *)entry->value;\n+\t\tret |= reftable_fsck_check(b->stack, reftable_fsck_error_handler,\n+\t\t\t\t\t reftable_fsck_verbose_handler, o);\n+\t}\n+\n+\treturn ret;\n }\n \n struct ref_storage_be refs_be_reftable = {\ndiff --git a/reftable/fsck.c b/reftable/fsck.c\nnew file mode 100644\nindex 0000000000..4282b1413e\n--- /dev/null\n+++ b/reftable/fsck.c\n@@ -0,0 +1,53 @@\n+#include \"basics.h\"\n+#include \"reftable-fsck.h\"\n+#include \"stack.h\"\n+\n+int reftable_fsck_check(struct reftable_stack *stack,\n+\t\t\treftable_fsck_report_fn report_fn,\n+\t\t\treftable_fsck_verbose_fn verbose_fn,\n+\t\t\tvoid *cb_data)\n+{\n+\n+\tchar **names = NULL;\n+\tuint64_t min, max;\n+\tint err = 0;\n+\n+\tif (stack == NULL)\n+\t\tgoto out;\n+\n+\terr = read_lines(stack->list_file, &names);\n+\tif (err < 0)\n+\t\tgoto out;\n+\n+\tverbose_fn(\"Checking reftable table names\", cb_data);\n+\n+\tfor (size_t i = 0; names[i]; i++) {\n+\t\tstruct reftable_fsck_info info = {\n+\t\t\t.error = REFTABLE_FSCK_ERROR_TABLE_NAME,\n+\t\t\t.path = names[i],\n+\t\t};\n+\t\tuint32_t rnd;\n+\t\t/*\n+\t\t * We want to match the tail '.ref'. One extra byte to ensure\n+\t\t * that there is no unexpected extra character and one byte for\n+\t\t * the null terminator added by sscanf.\n+\t\t */\n+\t\tchar tail[6];\n+\n+\t\tif (sscanf(names[i], \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x%5s\",\n+\t\t\t &min, &max, &rnd, tail) != 4) {\n+\t\t\tinfo.msg = \"invalid reftable table name\";\n+\t\t\terr = report_fn(&info, cb_data);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (strcmp(tail, \".ref\")) {\n+\t\t\tinfo.msg = \"invalid reftable table extension\";\n+\t\t\terr = report_fn(&info, cb_data);\n+\t\t}\n+\t}\n+\n+out:\n+\tfree_names(names);\n+\treturn err;\n+}\ndiff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h\nnew file mode 100644\nindex 0000000000..4cf0053234\n--- /dev/null\n+++ b/reftable/reftable-fsck.h\n@@ -0,0 +1,38 @@\n+#ifndef REFTABLE_FSCK_H\n+#define REFTABLE_FSCK_H\n+\n+#include \"reftable-stack.h\"\n+\n+enum reftable_fsck_error {\n+\t/* Invalid table name */\n+\tREFTABLE_FSCK_ERROR_TABLE_NAME = -1,\n+};\n+\n+/* Represents an individual error encountered during the FSCK checks. */\n+struct reftable_fsck_info {\n+\tenum reftable_fsck_error error;\n+\tconst char *msg;\n+\tconst char *path;\n+};\n+\n+typedef int reftable_fsck_report_fn(struct reftable_fsck_info *info,\n+\t\t\t\t void *cb_data);\n+typedef void reftable_fsck_verbose_fn(const char *msg, void *cb_data);\n+\n+/*\n+ * Given a reftable stack, perform FSCK check on the stack.\n+ *\n+ * If an issue is encountered, the issue is reported to the callee via the\n+ * provided 'report_fn'. If the issue is non-recoverable the flow will not\n+ * continue. If it is recoverable, the flow will continue and further issues\n+ * will be reported as identified.\n+ *\n+ * The 'verbose_fn' will be invoked to provide verbose information about\n+ * the progress and state of the FSCK checks.\n+ */\n+int reftable_fsck_check(struct reftable_stack *stack,\n+\t\t\treftable_fsck_report_fn report_fn,\n+\t\t\treftable_fsck_verbose_fn verbose_fn,\n+\t\t\tvoid *cb_data);\n+\n+#endif /* REFTABLE_FSCK_H */\ndiff --git a/t/meson.build b/t/meson.build\nindex bbeba1a8d5..a8eb44eb30 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -145,6 +145,7 @@ integration_tests = [\n 't0611-reftable-httpd.sh',\n 't0612-reftable-jgit-compatibility.sh',\n 't0613-reftable-write-options.sh',\n+ 't0614-reftable-fsck.sh',\n 't1000-read-tree-m-3way.sh',\n 't1001-read-tree-m-2way.sh',\n 't1002-read-tree-m-u-2way.sh',\n@@ -1214,4 +1215,4 @@ if perl.found() and time.found()\n timeout: 0,\n )\n endforeach\n-endif\n\\ No newline at end of file\n+endif\ndiff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh\nnew file mode 100755\nindex 0000000000..81d30df2d7\n--- /dev/null\n+++ b/t/t0614-reftable-fsck.sh\n@@ -0,0 +1,58 @@\n+#!/bin/sh\n+\n+test_description='Test reftable backend consistency check'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+GIT_TEST_DEFAULT_REF_FORMAT=reftable\n+export GIT_TEST_DEFAULT_REF_FORMAT\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'table name should be checked' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit commit --allow-empty -m initial &&\n+\n+\t\tgit refs verify 2>err &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tTABLE_NAME=$(cat .git/reftable/tables.list | head -n1) &&\n+\t\tsed \"1s/^/extra/\" .git/reftable/tables.list >.git/reftable/tables.list.tmp &&\n+\t\tmv .git/reftable/tables.list.tmp .git/reftable/tables.list &&\n+\t\tmv .git/reftable/${TABLE_NAME} .git/reftable/extra${TABLE_NAME} &&\n+\n+\t\ttest_must_fail git refs verify 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: extra${TABLE_NAME}: badReftableTableName: invalid reftable table name\n+\t\tEOF\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n+test_expect_success 'table name should be checked' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit commit --allow-empty -m initial &&\n+\n+\t\tgit refs verify 2>err &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tTABLE_NAME=$(cat .git/reftable/tables.list | head -n1) &&\n+\t\tsed \"1s/$/extra/\" .git/reftable/tables.list >.git/reftable/tables.list.tmp &&\n+\t\tmv .git/reftable/tables.list.tmp .git/reftable/tables.list &&\n+\t\tmv .git/reftable/${TABLE_NAME} .git/reftable/${TABLE_NAME}extra &&\n+\n+\t\ttest_must_fail git refs verify 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: ${TABLE_NAME}extra: badReftableTableName: invalid reftable table extension\n+\t\tEOF\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n+test_done\n\n-- \n2.50.1\n\n"},{"id":"525295","messageId":"20250902-228-reftable-introduce-consistency-checks-v2-3-4f96b3834779@gmail.com","threadId":"64068","inReplyTo":"20250902-228-reftable-introduce-consistency-checks-v2-0-4f96b3834779@gmail.com","subject":"[PATCH v2 3/5] refs/reftable: add fsck check for number of tables","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-02T07:05:23Z","receivedAt":"2025-09-02T07:05:39Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce a reftable fsck check to check that the number of files in the\nreftable directory matches the number of files listed in 'tables.list'.\nWe do this by iterating over the files in the reftable directory and\ncounting all the files present excluding the 'tables.list'. This is also\nexposed over Git's fsck checks as a 'badReftableStackCount' error.\n\nSigned-off-by: Karthik Nayak \n---\n Documentation/fsck-msgids.adoc | 3 +++\n fsck.h | 1 +\n refs/reftable-backend.c | 3 +++\n reftable/fsck.c | 34 ++++++++++++++++++++++++++++++++++\n reftable/reftable-fsck.h | 2 ++\n t/t0614-reftable-fsck.sh | 20 ++++++++++++++++++++\n 6 files changed, 63 insertions(+)\n\ndiff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc\nindex 784ddc0df5..707e2fc50a 100644\n--- a/Documentation/fsck-msgids.adoc\n+++ b/Documentation/fsck-msgids.adoc\n@@ -38,6 +38,9 @@\n `badReferentName`::\n \t(ERROR) The referent name of a symref is invalid.\n \n+`badReftableStackCount`::\n+\t(ERROR) Mismatch in number of tables.\n+\n `badReftableTableName`::\n \t(ERROR) A reftable table has an invalid name.\n \ndiff --git a/fsck.h b/fsck.h\nindex 5901f944a1..256effc4f8 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -34,6 +34,7 @@ enum fsck_msg_type {\n \tFUNC(BAD_PACKED_REF_HEADER, ERROR) \\\n \tFUNC(BAD_PARENT_SHA1, ERROR) \\\n \tFUNC(BAD_REFERENT_NAME, ERROR) \\\n+\tFUNC(BAD_REFTABLE_STACK_COUNT, ERROR) \\\n \tFUNC(BAD_REFTABLE_TABLE_NAME, ERROR) \\\n \tFUNC(BAD_REF_CONTENT, ERROR) \\\n \tFUNC(BAD_REF_FILETYPE, ERROR) \\\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex c38c6422f8..59c39f9b52 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -2695,6 +2695,9 @@ static int reftable_fsck_error_handler(struct reftable_fsck_info *info,\n \tcase REFTABLE_FSCK_ERROR_TABLE_NAME:\n \t\tmsg_id = FSCK_MSG_BAD_REFTABLE_TABLE_NAME;\n \t\tbreak;\n+\tcase REFTABLE_FSCK_ERROR_STACK_COUNT:\n+\t\tmsg_id = FSCK_MSG_BAD_REFTABLE_STACK_COUNT;\n+\t\tbreak;\n \tdefault:\n \t\tBUG(\"unknown fsck error: %d\", info->error);\n \t}\ndiff --git a/reftable/fsck.c b/reftable/fsck.c\nindex 4282b1413e..20e6bfb0f1 100644\n--- a/reftable/fsck.c\n+++ b/reftable/fsck.c\n@@ -2,6 +2,28 @@\n #include \"reftable-fsck.h\"\n #include \"stack.h\"\n \n+static int reftable_fsck_valid_stack_count(struct reftable_stack *st)\n+{\n+\tDIR *dir = opendir(st->reftable_dir);\n+\tstruct dirent *d = NULL;\n+\tunsigned int count = 0;\n+\n+\tif (!dir)\n+\t\treturn 0;\n+\n+\twhile ((d = readdir(dir))) {\n+\t\tif (!strcmp(d->d_name, \"tables.list\"))\n+\t\t\tcontinue;\n+\n+\t\tif (d->d_type == DT_REG)\n+\t\t\tcount++;\n+\t}\n+\n+\tclosedir(dir);\n+\n+\treturn count == st->tables_len;\n+}\n+\n int reftable_fsck_check(struct reftable_stack *stack,\n \t\t\treftable_fsck_report_fn report_fn,\n \t\t\treftable_fsck_verbose_fn verbose_fn,\n@@ -47,6 +69,18 @@ int reftable_fsck_check(struct reftable_stack *stack,\n \t\t}\n \t}\n \n+\tverbose_fn(\"Checking reftable tables count\", cb_data);\n+\n+\tif (!reftable_fsck_valid_stack_count(stack)) {\n+\t\tstruct reftable_fsck_info info = {\n+\t\t\t.error = REFTABLE_FSCK_ERROR_STACK_COUNT,\n+\t\t\t.path = \"reftable/tables.list\",\n+\t\t\t.msg = \"mismatch in number of tables\"\n+\t\t};\n+\n+\t\terr = report_fn(&info, cb_data);\n+\t}\n+\n out:\n \tfree_names(names);\n \treturn err;\ndiff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h\nindex 4cf0053234..beba1bdd1a 100644\n--- a/reftable/reftable-fsck.h\n+++ b/reftable/reftable-fsck.h\n@@ -6,6 +6,8 @@\n enum reftable_fsck_error {\n \t/* Invalid table name */\n \tREFTABLE_FSCK_ERROR_TABLE_NAME = -1,\n+\t/* Incorrect number of tables present */\n+\tREFTABLE_FSCK_ERROR_STACK_COUNT = -2,\n };\n \n /* Represents an individual error encountered during the FSCK checks. */\ndiff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh\nindex 81d30df2d7..3a34a31890 100755\n--- a/t/t0614-reftable-fsck.sh\n+++ b/t/t0614-reftable-fsck.sh\n@@ -55,4 +55,24 @@ test_expect_success 'table name should be checked' '\n \t)\n '\n \n+test_expect_success 'table count should be checked' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit commit --allow-empty -m initial &&\n+\n+\t\tgit refs verify 2>err &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\ttouch .git/reftable/0x000000002812-0x000000002813-c830a596.ref &&\n+\n+\t\ttest_must_fail git refs verify 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: reftable/tables.list: badReftableStackCount: mismatch in number of tables\n+\t\tEOF\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n test_done\n\n-- \n2.50.1\n\n"},{"id":"525296","messageId":"20250902-228-reftable-introduce-consistency-checks-v2-4-4f96b3834779@gmail.com","threadId":"64068","inReplyTo":"20250902-228-reftable-introduce-consistency-checks-v2-0-4f96b3834779@gmail.com","subject":"[PATCH v2 4/5] refs/reftable: add fsck check for trailing newline","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-02T07:05:24Z","receivedAt":"2025-09-02T07:05:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce a fsck check for the reftable backend, which checks if the\n'tables.list' contains a newline. The reftable backend writes a trailing\nnewline when writing the 'tables.list', but it doesn't check for it when\nreading the file. A missing newline however indicates that the file was\nmanually tampered with, so let's raise this as an error to the user.\n\nSigned-off-by: Karthik Nayak \n---\n Documentation/fsck-msgids.adoc | 3 +++\n fsck.h | 1 +\n refs/reftable-backend.c | 3 +++\n reftable/fsck.c | 36 ++++++++++++++++++++++++++++++++++++\n reftable/reftable-fsck.h | 2 ++\n t/t0614-reftable-fsck.sh | 21 +++++++++++++++++++++\n 6 files changed, 66 insertions(+)\n\ndiff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc\nindex 707e2fc50a..1432b1de06 100644\n--- a/Documentation/fsck-msgids.adoc\n+++ b/Documentation/fsck-msgids.adoc\n@@ -41,6 +41,9 @@\n `badReftableStackCount`::\n \t(ERROR) Mismatch in number of tables.\n \n+`badReftableStackListNewline`::\n+\t(ERROR) Reftable stack list missing trailing newline.\n+\n `badReftableTableName`::\n \t(ERROR) A reftable table has an invalid name.\n \ndiff --git a/fsck.h b/fsck.h\nindex 256effc4f8..33432bae79 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -35,6 +35,7 @@ enum fsck_msg_type {\n \tFUNC(BAD_PARENT_SHA1, ERROR) \\\n \tFUNC(BAD_REFERENT_NAME, ERROR) \\\n \tFUNC(BAD_REFTABLE_STACK_COUNT, ERROR) \\\n+\tFUNC(BAD_REFTABLE_STACK_LIST_NEWLINE, ERROR) \\\n \tFUNC(BAD_REFTABLE_TABLE_NAME, ERROR) \\\n \tFUNC(BAD_REF_CONTENT, ERROR) \\\n \tFUNC(BAD_REF_FILETYPE, ERROR) \\\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 59c39f9b52..7331513b19 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -2698,6 +2698,9 @@ static int reftable_fsck_error_handler(struct reftable_fsck_info *info,\n \tcase REFTABLE_FSCK_ERROR_STACK_COUNT:\n \t\tmsg_id = FSCK_MSG_BAD_REFTABLE_STACK_COUNT;\n \t\tbreak;\n+\tcase REFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE:\n+\t\tmsg_id = FSCK_MSG_BAD_REFTABLE_STACK_LIST_NEWLINE;\n+\t\tbreak;\n \tdefault:\n \t\tBUG(\"unknown fsck error: %d\", info->error);\n \t}\ndiff --git a/reftable/fsck.c b/reftable/fsck.c\nindex 20e6bfb0f1..9a7f22c56b 100644\n--- a/reftable/fsck.c\n+++ b/reftable/fsck.c\n@@ -1,7 +1,31 @@\n #include \"basics.h\"\n+#include \"reftable-error.h\"\n #include \"reftable-fsck.h\"\n #include \"stack.h\"\n \n+static int reftable_fsck_stack_contains_newline(const char *list_file)\n+{\n+\tFILE *f = fopen(list_file, \"r\");\n+\tint c = 0;\n+\n+\tif (f == NULL) {\n+\t\tif (errno == ENOENT)\n+\t\t\treturn 0;\n+\t\treturn REFTABLE_IO_ERROR;\n+\t}\n+\n+\tif (fseek(f, 0, SEEK_END) == 0) {\n+\t\tlong size = ftell(f);\n+\t\tif (size <= 0)\n+\t\t\treturn REFTABLE_IO_ERROR;\n+\t\tfseek(f, -1, SEEK_END);\n+\t\tc = fgetc(f);\n+\t}\n+\tfclose(f);\n+\n+\treturn c == '\\n';\n+}\n+\n static int reftable_fsck_valid_stack_count(struct reftable_stack *st)\n {\n \tDIR *dir = opendir(st->reftable_dir);\n@@ -69,6 +93,18 @@ int reftable_fsck_check(struct reftable_stack *stack,\n \t\t}\n \t}\n \n+\tverbose_fn(\"Checking trailing newline in stack list\", cb_data);\n+\n+\tif (!reftable_fsck_stack_contains_newline(stack->list_file)) {\n+\t\tstruct reftable_fsck_info info = {\n+\t\t\t.error = REFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE,\n+\t\t\t.path = \"reftable/tables.list\",\n+\t\t\t.msg = \"trailing newline missing in stack list\"\n+\t\t};\n+\n+\t\terr = report_fn(&info, cb_data);\n+\t}\n+\n \tverbose_fn(\"Checking reftable tables count\", cb_data);\n \n \tif (!reftable_fsck_valid_stack_count(stack)) {\ndiff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h\nindex beba1bdd1a..17df661da8 100644\n--- a/reftable/reftable-fsck.h\n+++ b/reftable/reftable-fsck.h\n@@ -8,6 +8,8 @@ enum reftable_fsck_error {\n \tREFTABLE_FSCK_ERROR_TABLE_NAME = -1,\n \t/* Incorrect number of tables present */\n \tREFTABLE_FSCK_ERROR_STACK_COUNT = -2,\n+\t/* Newline missing at the end of the stack list */\n+\tREFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE = -3,\n };\n \n /* Represents an individual error encountered during the FSCK checks. */\ndiff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh\nindex 3a34a31890..3b119eae62 100755\n--- a/t/t0614-reftable-fsck.sh\n+++ b/t/t0614-reftable-fsck.sh\n@@ -75,4 +75,25 @@ test_expect_success 'table count should be checked' '\n \t)\n '\n \n+test_expect_success 'stack list must contain trailing newline' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit commit --allow-empty -m initial &&\n+\n+\t\tgit refs verify 2>err &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tprintf \"%s\" \"$(cat .git/reftable/tables.list)\" >.git/reftable/tables.list.tmp &&\n+\t\tmv .git/reftable/tables.list.tmp .git/reftable/tables.list &&\n+\n+\t\ttest_must_fail git refs verify 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: reftable/tables.list: badReftableStackListNewline: trailing newline missing in stack list\n+\t\tEOF\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n test_done\n\n-- \n2.50.1\n\n"},{"id":"525297","messageId":"20250902-228-reftable-introduce-consistency-checks-v2-5-4f96b3834779@gmail.com","threadId":"64068","inReplyTo":"20250902-228-reftable-introduce-consistency-checks-v2-0-4f96b3834779@gmail.com","subject":"[PATCH v2 5/5] refs/reftable: add fsck check for incorrect update index","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-02T07:05:25Z","receivedAt":"2025-09-02T07:05:41Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce a fsck check for the reftable backend, which checks if the\ntables in 'tables.list' contain sequential update index. The tables in\nthe reftable backend should contain sequential update index. This fsck\ncheck ensures that.\n\nWe must note that the reftable backend itself doesn't check to ensure\nthis and it also doesn't check to ensure that the index in the table\nname matches the index in the header or the table. The latter is not\nimplemented in this fsck check either and will be added in a future\npatch where we add fsck checks for internals of a table.\n\nSigned-off-by: Karthik Nayak \n---\n Documentation/fsck-msgids.adoc | 3 ++\n fsck.h | 1 +\n refs/reftable-backend.c | 3 ++\n reftable/fsck.c | 15 ++++++++--\n reftable/reftable-fsck.h | 2 ++\n t/t0614-reftable-fsck.sh | 62 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 84 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc\nindex 1432b1de06..982d51876c 100644\n--- a/Documentation/fsck-msgids.adoc\n+++ b/Documentation/fsck-msgids.adoc\n@@ -47,6 +47,9 @@\n `badReftableTableName`::\n \t(ERROR) A reftable table has an invalid name.\n \n+`badReftableUpdateIndex`::\n+\t(ERROR) Incorrect update index found for table.\n+\n `badTagName`::\n \t(INFO) A tag has an invalid format.\n \ndiff --git a/fsck.h b/fsck.h\nindex 33432bae79..60e9b84183 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -37,6 +37,7 @@ enum fsck_msg_type {\n \tFUNC(BAD_REFTABLE_STACK_COUNT, ERROR) \\\n \tFUNC(BAD_REFTABLE_STACK_LIST_NEWLINE, ERROR) \\\n \tFUNC(BAD_REFTABLE_TABLE_NAME, ERROR) \\\n+\tFUNC(BAD_REFTABLE_UPDATE_INDEX, ERROR) \\\n \tFUNC(BAD_REF_CONTENT, ERROR) \\\n \tFUNC(BAD_REF_FILETYPE, ERROR) \\\n \tFUNC(BAD_REF_NAME, ERROR) \\\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 7331513b19..519ade24b8 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -2701,6 +2701,9 @@ static int reftable_fsck_error_handler(struct reftable_fsck_info *info,\n \tcase REFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE:\n \t\tmsg_id = FSCK_MSG_BAD_REFTABLE_STACK_LIST_NEWLINE;\n \t\tbreak;\n+\tcase REFTABLE_FSCK_ERROR_UPDATE_INDEX:\n+\t\tmsg_id = FSCK_MSG_BAD_REFTABLE_UPDATE_INDEX;\n+\t\tbreak;\n \tdefault:\n \t\tBUG(\"unknown fsck error: %d\", info->error);\n \t}\ndiff --git a/reftable/fsck.c b/reftable/fsck.c\nindex 9a7f22c56b..5c6d842ac1 100644\n--- a/reftable/fsck.c\n+++ b/reftable/fsck.c\n@@ -53,9 +53,8 @@ int reftable_fsck_check(struct reftable_stack *stack,\n \t\t\treftable_fsck_verbose_fn verbose_fn,\n \t\t\tvoid *cb_data)\n {\n-\n+\tuint64_t min, max, prev_max = 0;\n \tchar **names = NULL;\n-\tuint64_t min, max;\n \tint err = 0;\n \n \tif (stack == NULL)\n@@ -87,10 +86,22 @@ int reftable_fsck_check(struct reftable_stack *stack,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (min != (prev_max + 1) || max < min) {\n+\t\t\tstruct reftable_fsck_info info = {\n+\t\t\t\t.error = REFTABLE_FSCK_ERROR_UPDATE_INDEX,\n+\t\t\t\t.path = names[i],\n+\t\t\t\t.msg = \"incorrect update index in table name\"\n+\t\t\t};\n+\n+\t\t\terr = report_fn(&info, cb_data);\n+\t\t}\n+\n \t\tif (strcmp(tail, \".ref\")) {\n \t\t\tinfo.msg = \"invalid reftable table extension\";\n \t\t\terr = report_fn(&info, cb_data);\n \t\t}\n+\n+\t\tprev_max = max;\n \t}\n \n \tverbose_fn(\"Checking trailing newline in stack list\", cb_data);\ndiff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h\nindex 17df661da8..0ab20a99b6 100644\n--- a/reftable/reftable-fsck.h\n+++ b/reftable/reftable-fsck.h\n@@ -10,6 +10,8 @@ enum reftable_fsck_error {\n \tREFTABLE_FSCK_ERROR_STACK_COUNT = -2,\n \t/* Newline missing at the end of the stack list */\n \tREFTABLE_FSCK_ERROR_STACK_LIST_MISSING_NEWLINE = -3,\n+\t/* Incorrect update index for table */\n+\tREFTABLE_FSCK_ERROR_UPDATE_INDEX = -4,\n };\n \n /* Represents an individual error encountered during the FSCK checks. */\ndiff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh\nindex 3b119eae62..1f37691b2e 100755\n--- a/t/t0614-reftable-fsck.sh\n+++ b/t/t0614-reftable-fsck.sh\n@@ -96,4 +96,66 @@ test_expect_success 'stack list must contain trailing newline' '\n \t)\n '\n \n+test_expect_success 'table update index should be sequential between tables' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit commit --allow-empty -m initial &&\n+\n+\t\t# Lock the existing table to disable auto-compaction\n+\t\tCUR_TABLE=$(cat .git/reftable/tables.list | tail -n1) &&\n+\t\ttouch .git/reftable/${CUR_TABLE}.lock &&\n+\t\tgit update-ref refs/heads/sample @ &&\n+\t\trm .git/reftable/${CUR_TABLE}.lock &&\n+\n+\t\tgit refs verify 2>err &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tTABLE_NAME=$(cat .git/reftable/tables.list | tail -n1) &&\n+\t\tNEW_TABLE_NAME=$(echo ${TABLE_NAME} | sed \"s/0003/0009/g\") &&\n+\n+\t\tsed \"2s/.*/${NEW_TABLE_NAME}/\" .git/reftable/tables.list >.git/reftable/tables.list.tmp &&\n+\t\tmv .git/reftable/tables.list.tmp .git/reftable/tables.list &&\n+\t\tmv .git/reftable/${TABLE_NAME} .git/reftable/${NEW_TABLE_NAME} &&\n+\n+\t\ttest_must_fail git refs verify 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: ${NEW_TABLE_NAME}: badReftableUpdateIndex: incorrect update index in table name\n+\t\tEOF\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n+test_expect_success 'table update index should be sequential within a table' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit commit --allow-empty -m initial &&\n+\n+\t\t# Lock the existing table to disable auto-compaction\n+\t\tCUR_TABLE=$(cat .git/reftable/tables.list | tail -n1) &&\n+\t\ttouch .git/reftable/${CUR_TABLE}.lock &&\n+\t\tgit update-ref refs/heads/sample @ &&\n+\t\trm .git/reftable/${CUR_TABLE}.lock &&\n+\n+\t\tgit refs verify 2>err &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\tTABLE_NAME=$(cat .git/reftable/tables.list | tail -n1) &&\n+\t\tNEW_TABLE_NAME=$(echo ${TABLE_NAME} | sed \"s/\\(.*\\)0003/\\10002/\") &&\n+\n+\t\tsed \"2s/.*/${NEW_TABLE_NAME}/\" .git/reftable/tables.list >.git/reftable/tables.list.tmp &&\n+\t\tmv .git/reftable/tables.list.tmp .git/reftable/tables.list &&\n+\t\tmv .git/reftable/${TABLE_NAME} .git/reftable/${NEW_TABLE_NAME} &&\n+\n+\t\ttest_must_fail git refs verify 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: ${NEW_TABLE_NAME}: badReftableUpdateIndex: incorrect update index in table name\n+\t\tEOF\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n test_done\n\n-- \n2.50.1\n\n"},{"id":"525382","messageId":"xmqqwm6gh4q3.fsf@gitster.g","threadId":"64068","inReplyTo":"20250902-228-reftable-introduce-consistency-checks-v2-1-4f96b3834779@gmail.com","subject":"Re: [PATCH v2 1/5] fsck: order 'fsck_msg_type' alphabetically","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-02T22:25:24Z","receivedAt":"2025-09-02T22:25:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> The list of 'fsck_msg_type' seem to be alphabetically ordered, but there\n> are a few small misses. Fix this by sorting the sub-sections of the\n> list to maintain alphabetical ordering. Also fix a clang-format issue\n> where the escaped newlines are not aligned.\n>\n> While here, remove a duplicate instance of 'gitmodulesLarge' in the\n> 'fsck-msgids' documentation.\n\n\"A few small misses\".\n\n> diff --git a/fsck.h b/fsck.h\n> index dd7df3d5b3..559ad57807 100644\n> --- a/fsck.h\n> +++ b/fsck.h\n> @@ -20,82 +20,82 @@ enum fsck_msg_type {\n> ...\n> -#define FOREACH_FSCK_MSG_ID(FUNC) \\\n> -\t/* fatal errors */ \\\n> -\tFUNC(NUL_IN_HEADER, FATAL) \\\n> -\tFUNC(UNTERMINATED_HEADER, FATAL) \\\n> ...\n> +#define FOREACH_FSCK_MSG_ID(FUNC) \\\n> +\t/* fatal errors */ \\\n> +\tFUNC(NUL_IN_HEADER, FATAL) \\\n> +\tFUNC(UNTERMINATED_HEADER, FATAL) \\\n> ...\n\nPlease undo these \"pad by spaces before backslash\"; otherwise we\ncannot tell which ones are \"a few small misses\".\n\nThanks.\n\n"},{"id":"525383","messageId":"xmqqseh4h446.fsf@gitster.g","threadId":"64068","inReplyTo":"20250902-228-reftable-introduce-consistency-checks-v2-4-4f96b3834779@gmail.com","subject":"Re: [PATCH v2 4/5] refs/reftable: add fsck check for trailing newline","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-02T22:38:33Z","receivedAt":"2025-09-02T22:38:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Introduce a fsck check for the reftable backend, which checks if the\n> 'tables.list' contains a newline. The reftable backend writes a trailing\n> newline when writing the 'tables.list', but it doesn't check for it when\n> reading the file. A missing newline however indicates that the file was\n> manually tampered with, so let's raise this as an error to the user.\n\nHmph, how does the code react to other kinds of \"manual tampering\"?\nFor example, if an empty line is inserted between two existing lines\n(or at the beginning of the file, for that matter), would the parser\ndetect it as a corrupt file and die?\n\nIf so, it makes me strongly suspect that we are better off enforcing\nthat the file does not end in an incomplete line at runtime and barf\njust the same way, instead of \"most of the anomalies that the write\ncodepath would never produce would cause error on the read codepath,\nbut only this one that the read codepath is happy with is caught by\nthe fsck\", which does not sound quite right.\n\n"},{"id":"525384","messageId":"xmqqo6rsh3xw.fsf@gitster.g","threadId":"64068","inReplyTo":"20250902-228-reftable-introduce-consistency-checks-v2-5-4f96b3834779@gmail.com","subject":"Re: [PATCH v2 5/5] refs/reftable: add fsck check for incorrect update index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-02T22:42:19Z","receivedAt":"2025-09-02T22:42:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Introduce a fsck check for the reftable backend, which checks if the\n> tables in 'tables.list' contain sequential update index. The tables in\n> the reftable backend should contain sequential update index. This fsck\n> check ensures that.\n>\n> We must note that the reftable backend itself doesn't check to ensure\n> this and it also doesn't check to ensure that the index in the table\n> name matches the index in the header or the table. The latter is not\n> implemented in this fsck check either and will be added in a future\n> patch where we add fsck checks for internals of a table.\n\nSimilar to the previous step, I am not sure why this should not be\nchecked at runtime and is flagged as an error.\n\nIn general, we do try to avoid retroactively tightening rules, but\nthe reftable is so new and not even the default. If we noticed that\nthe runtime has been overly loose, the time to tighten it is now,\nnot after even more installations use it.\n"},{"id":"525407","messageId":"aLf3MaKHZSQfnBlT@pks.im","threadId":"64068","inReplyTo":"20250902-228-reftable-introduce-consistency-checks-v2-2-4f96b3834779@gmail.com","subject":"Re: [PATCH v2 2/5] refs/reftable: add fsck check for checking the table name","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-03T08:07:13Z","receivedAt":"2025-09-03T08:07:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 02, 2025 at 09:05:22AM +0200, Karthik Nayak wrote:\n> The `git refs verify` command is used to run fsck checks on the\n> reference backends. This command is also invoked when users run 'git\n> fsck'. While the files-backend has some fsck checks added, the reftable\n> backend lacks such checks. Let's add the required infrastructure and a\n> check to test for the table names in the 'tables.list' of reftables.\n> \n> For the infrastructure, since the reftable library is treated as an\n> independent library we should ensure that the library code works\n> independently without knowledge about Git's internals. To do this,\n> add both 'reftable/fsck.c' and 'reftable/reftable-fsck.h'. Which\n> provide an entry point 'reftable_fsck_check' for running fsck checks\n> over a provided reftable stack. The callee provides the function with\n> callbacks to handle issue and information reporting.\n> \n> Add glue code in 'refs/reftable-backend.c' which calls the reftable\n> library to perform the fsck checks. Here we also map the reftable errors\n> to Git' fsck errors.\n> \n> Introduce a check to validate table names for a given reftable stack.\n> Also add 'badReftableTableName' as a corresponding error within Git. Add\n> a test to check for this behavior.\n> \n> While here, remove a unused header `#include \"../lockfile.h\"` from\n> 'refs/reftable-backend.c'.\n\nIt's quite a bunch of changes overall that could've been reasonably\nsplit up into multiple commits. E.g. one to introduce the reftable-side\nlogic, one to start calling it in Git, and one to drop the superfluous\nheader.\n\n> diff --git a/Makefile b/Makefile\n> index e11340c1ae..f2ddcc8d7c 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2733,6 +2733,7 @@ REFTABLE_OBJS += reftable/error.o\n> REFTABLE_OBJS += reftable/block.o\n> REFTABLE_OBJS += reftable/blocksource.o\n> REFTABLE_OBJS += reftable/iter.o\n> +REFTABLE_OBJS += reftable/fsck.o\n\n\"f\" is before \"i\" in the alphabet I'm accustomed to :) So let's retain\nlexicographic ordering here.\n\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index 8dae1e1112..c38c6422f8 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -2675,11 +2676,55 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,\n> \treturn ret;\n> }\n> \n> -static int reftable_be_fsck(struct ref_store *ref_store UNUSED,\n> -\t\t\t struct fsck_options *o UNUSED,\n> +static void reftable_fsck_verbose_handler(const char *msg, void *cb_data)\n> +{\n> +\tstruct fsck_options *o = cb_data;\n> +\n> +\tif (o->verbose)\n> +\t\tfprintf_ln(stderr, \"%s\", _(msg));\n> +}\n\nIs this `_()` marker correct here? There isn't really any reasonable way\nfor somebody to translate a variable with unknown contents. So shouldn't\nit only be the caller of `reftable_fsck_verbose_handler()` that should\nmark the string as translatable?\n\n> +static int reftable_fsck_error_handler(struct reftable_fsck_info *info,\n> +\t\t\t\t void *cb_data)\n> +{\n> +\tstruct fsck_ref_report report = { .path = info->path };\n> +\tstruct fsck_options *o = cb_data;\n> +\tenum fsck_msg_id msg_id;\n> +\n> +\tswitch (info->error) {\n> +\tcase REFTABLE_FSCK_ERROR_TABLE_NAME:\n> +\t\tmsg_id = FSCK_MSG_BAD_REFTABLE_TABLE_NAME;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tBUG(\"unknown fsck error: %d\", info->error);\n> +\t}\n> +\n> +\treturn fsck_report_ref(o, &report, msg_id, \"%s\", info->msg);\n> +}\n\nI think this function will become a bit unwieldy over time. We might\ninstead want to have an array that maps from reftable-specific to\nfsck-specific error code:\n\n static const fsck_msg_id[] = {\n [REFTABLE_FSCK_ERROR_TABLE_NAME] = FSCK_MSG_BAD_REFTABLE_TABLE_NAME,\n };\n\nSo in that case, all we'd have to do is to perform bounds checking in\nthe above function. And maybe verify that the developer didn't forget to\nfill in a new msg ID by checking that the derived message ID is non-zero.\n\n> +static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o,\n> \t\t\t struct worktree *wt UNUSED)\n> {\n> -\treturn 0;\n> +\tstruct reftable_ref_store *refs;\n> +\tstruct strmap_entry *entry;\n> +\tstruct hashmap_iter iter;\n> +\tint ret = 0;\n> +\n> +\trefs = reftable_be_downcast(ref_store, REF_STORE_READ, \"fsck\");\n> +\n> +\tif (o->verbose)\n> +\t\tfprintf_ln(stderr, _(\"Checking references consistency\"));\n\nThis line is duplicate across both backends, right? Maybe it's something\nthat we can do in the generic logic?\n\n> +\tret |= reftable_fsck_check(refs->main_backend.stack, reftable_fsck_error_handler,\n> +\t\t\t\t reftable_fsck_verbose_handler, o);\n> +\n> +\tstrmap_for_each_entry(&refs->worktree_backends, &iter, entry) {\n> +\t\tstruct reftable_backend *b = (struct reftable_backend *)entry->value;\n> +\t\tret |= reftable_fsck_check(b->stack, reftable_fsck_error_handler,\n> +\t\t\t\t\t reftable_fsck_verbose_handler, o);\n> +\t}\n> +\n> +\treturn ret;\n> }\n> \n> struct ref_storage_be refs_be_reftable = {\n\nLooks good.\n\n> diff --git a/reftable/fsck.c b/reftable/fsck.c\n> new file mode 100644\n> index 0000000000..4282b1413e\n> --- /dev/null\n> +++ b/reftable/fsck.c\n> @@ -0,0 +1,53 @@\n> +#include \"basics.h\"\n> +#include \"reftable-fsck.h\"\n> +#include \"stack.h\"\n> +\n> +int reftable_fsck_check(struct reftable_stack *stack,\n> +\t\t\treftable_fsck_report_fn report_fn,\n> +\t\t\treftable_fsck_verbose_fn verbose_fn,\n> +\t\t\tvoid *cb_data)\n> +{\n> +\n> +\tchar **names = NULL;\n> +\tuint64_t min, max;\n> +\tint err = 0;\n> +\n> +\tif (stack == NULL)\n> +\t\tgoto out;\n> +\n> +\terr = read_lines(stack->list_file, &names);\n> +\tif (err < 0)\n> +\t\tgoto out;\n> +\n> +\tverbose_fn(\"Checking reftable table names\", cb_data);\n> +\n> +\tfor (size_t i = 0; names[i]; i++) {\n> +\t\tstruct reftable_fsck_info info = {\n> +\t\t\t.error = REFTABLE_FSCK_ERROR_TABLE_NAME,\n> +\t\t\t.path = names[i],\n> +\t\t};\n> +\t\tuint32_t rnd;\n> +\t\t/*\n> +\t\t * We want to match the tail '.ref'. One extra byte to ensure\n> +\t\t * that there is no unexpected extra character and one byte for\n> +\t\t * the null terminator added by sscanf.\n> +\t\t */\n> +\t\tchar tail[6];\n> +\n> +\t\tif (sscanf(names[i], \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x%5s\",\n> +\t\t\t &min, &max, &rnd, tail) != 4) {\n> +\t\t\tinfo.msg = \"invalid reftable table name\";\n\nThis here is where the string should be translated.\n\n> +\t\t\terr = report_fn(&info, cb_data);\n> +\t\t\tcontinue;\n> +\t\t}\n\nI think sscanf is quite frowned-upon in the Git codebase. Maybe we\nshould manually parse through the string instead?\n\nFurthermore, I think we should move every single check into a separate\nfunction, similar to how the files backend does it. This ensures that\nchecks are self-contained and that it's way easier to add new checks\nover time.\n\nAnother angle: did you verify that reftables written by JGit follow this\nformat?\n\n> +\t\tif (strcmp(tail, \".ref\")) {\n> +\t\t\tinfo.msg = \"invalid reftable table extension\";\n\nSame here, this should be translated.\n\n> diff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h\n> new file mode 100644\n> index 0000000000..4cf0053234\n> --- /dev/null\n> +++ b/reftable/reftable-fsck.h\n> @@ -0,0 +1,38 @@\n> +#ifndef REFTABLE_FSCK_H\n> +#define REFTABLE_FSCK_H\n> +\n> +#include \"reftable-stack.h\"\n> +\n> +enum reftable_fsck_error {\n> +\t/* Invalid table name */\n> +\tREFTABLE_FSCK_ERROR_TABLE_NAME = -1,\n> +};\n\nWouldn't it be more natural to give these positive numbers?\n\n> +/* Represents an individual error encountered during the FSCK checks. */\n> +struct reftable_fsck_info {\n> +\tenum reftable_fsck_error error;\n> +\tconst char *msg;\n> +\tconst char *path;\n> +};\n\nI wonder whether it should be the reftable library that decides on the\nseverity of each generated finding.\n\n> +typedef int reftable_fsck_report_fn(struct reftable_fsck_info *info,\n> +\t\t\t\t void *cb_data);\n> +typedef void reftable_fsck_verbose_fn(const char *msg, void *cb_data);\n> +\n> +/*\n> + * Given a reftable stack, perform FSCK check on the stack.\n\ns/FSCK check/consistency checks/\n\n> + *\n> + * If an issue is encountered, the issue is reported to the callee via the\n> + * provided 'report_fn'. If the issue is non-recoverable the flow will not\n> + * continue. If it is recoverable, the flow will continue and further issues\n> + * will be reported as identified.\n> + *\n> + * The 'verbose_fn' will be invoked to provide verbose information about\n> + * the progress and state of the FSCK checks.\n\nSame here.\n\n> diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh\n> new file mode 100755\n> index 0000000000..81d30df2d7\n> --- /dev/null\n> +++ b/t/t0614-reftable-fsck.sh\n> @@ -0,0 +1,58 @@\n> +#!/bin/sh\n> +\n> +test_description='Test reftable backend consistency check'\n> +\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n\nTests shouldn't define these variables, but should dynamically figure\nout what the default branch name is as required, e.g. by using\ngit-symbolic-ref(1).\n\n> +GIT_TEST_DEFAULT_REF_FORMAT=reftable\n> +export GIT_TEST_DEFAULT_REF_FORMAT\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'table name should be checked' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tgit commit --allow-empty -m initial &&\n> +\n> +\t\tgit refs verify 2>err &&\n> +\t\ttest_must_be_empty err &&\n> +\n> +\t\tTABLE_NAME=$(cat .git/reftable/tables.list | head -n1) &&\n\nYou can drop the cat(1) invocation and directly say `head -n1 file`.\n\n> +\t\tsed \"1s/^/extra/\" .git/reftable/tables.list >.git/reftable/tables.list.tmp &&\n> +\t\tmv .git/reftable/tables.list.tmp .git/reftable/tables.list &&\n> +\t\tmv .git/reftable/${TABLE_NAME} .git/reftable/extra${TABLE_NAME} &&\n\nNo need for the curly braces around TABLE_NAME here and further down. It\nwould be nice to quote these strings though.\n\n> +\n> +\t\ttest_must_fail git refs verify 2>err &&\n> +\t\tcat >expect <<-EOF &&\n> +\t\terror: extra${TABLE_NAME}: badReftableTableName: invalid reftable table name\n> +\t\tEOF\n> +\t\ttest_cmp expect err\n> +\t)\n> +'\n> +\n> +test_expect_success 'table name should be checked' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tgit commit --allow-empty -m initial &&\n> +\n> +\t\tgit refs verify 2>err &&\n> +\t\ttest_must_be_empty err &&\n> +\n> +\t\tTABLE_NAME=$(cat .git/reftable/tables.list | head -n1) &&\n\nSame here wrt the extra invocation of cat(1).\n\nPatrick\n"},{"id":"525408","messageId":"aLf3NyodTZ38N8vY@pks.im","threadId":"64068","inReplyTo":"20250902-228-reftable-introduce-consistency-checks-v2-3-4f96b3834779@gmail.com","subject":"Re: [PATCH v2 3/5] refs/reftable: add fsck check for number of tables","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-03T08:07:19Z","receivedAt":"2025-09-03T08:07:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 02, 2025 at 09:05:23AM +0200, Karthik Nayak wrote:\n> Introduce a reftable fsck check to check that the number of files in the\n> reftable directory matches the number of files listed in 'tables.list'.\n> We do this by iterating over the files in the reftable directory and\n> counting all the files present excluding the 'tables.list'. This is also\n> exposed over Git's fsck checks as a 'badReftableStackCount' error.\n\nThis feels overly strict, as it can always be the case that a concurrent\nprocess is currently updating the stack. Furthermore, it's expected that\non Windows systems deletion of an old table may not work because the\nfile is still kept open by another process. The reftable library is\nprepared to handle this alright and will re-try deleting the table at a\nlater point in time.\n\nSo maybe a better check would be to verify that there are no files with\nunexpected names in the directory?\n\nPatrick\n"},{"id":"525409","messageId":"aLf3PFUkt49bvi1S@pks.im","threadId":"64068","inReplyTo":"xmqqseh4h446.fsf@gitster.g","subject":"Re: [PATCH v2 4/5] refs/reftable: add fsck check for trailing newline","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-09-03T08:07:24Z","receivedAt":"2025-09-03T08:07:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 02, 2025 at 03:38:33PM -0700, Junio C Hamano wrote:\n> Karthik Nayak writes:\n> \n> > Introduce a fsck check for the reftable backend, which checks if the\n> > 'tables.list' contains a newline. The reftable backend writes a trailing\n> > newline when writing the 'tables.list', but it doesn't check for it when\n> > reading the file. A missing newline however indicates that the file was\n> > manually tampered with, so let's raise this as an error to the user.\n> \n> Hmph, how does the code react to other kinds of \"manual tampering\"?\n> For example, if an empty line is inserted between two existing lines\n> (or at the beginning of the file, for that matter), would the parser\n> detect it as a corrupt file and die?\n> \n> If so, it makes me strongly suspect that we are better off enforcing\n> that the file does not end in an incomplete line at runtime and barf\n> just the same way, instead of \"most of the anomalies that the write\n> codepath would never produce would cause error on the read codepath,\n> but only this one that the read codepath is happy with is caught by\n> the fsck\", which does not sound quite right.\n\nFair, I'm also of the opinion that we should tighten the parser logic to\ndetect and reject any invalid files. For previous checks where we verify\nthat the table names are sane I think it's fair to live with it and warn\nabout those, as the actual names don't really matter. But as soon as we\nhit actually-broken formats I also think that we're better of rejecting\nthose altogether.\n\nPatrick\n"},{"id":"525447","messageId":"aLhx-HdIMCB3c2WK@ArchLinux","threadId":"64068","inReplyTo":"aLf3MaKHZSQfnBlT@pks.im","subject":"Re: [PATCH v2 2/5] refs/reftable: add fsck check for checking the table name","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-09-03T16:51:04Z","receivedAt":"2025-09-03T16:51:00Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Wed, Sep 03, 2025 at 10:07:13AM +0200, Patrick Steinhardt wrote:\n\n[snip]\n\n> > +static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o,\n> > \t\t\t struct worktree *wt UNUSED)\n> > {\n> > -\treturn 0;\n> > +\tstruct reftable_ref_store *refs;\n> > +\tstruct strmap_entry *entry;\n> > +\tstruct hashmap_iter iter;\n> > +\tint ret = 0;\n> > +\n> > +\trefs = reftable_be_downcast(ref_store, REF_STORE_READ, \"fsck\");\n> > +\n> > +\tif (o->verbose)\n> > +\t\tfprintf_ln(stderr, _(\"Checking references consistency\"));\n> \n> This line is duplicate across both backends, right? Maybe it's something\n> that we can do in the generic logic?\n> \n\nThat's right, it is duplicate. If we want to remove this, we need to do\nthis in the \"builtin/refs.c\". But I wonder whether we should do this in\nthe first place. Should we rather add more detailed information just\nlike the following code for packed backend?\n\n if (o->verbose)\n fprintf_ln(stderr, \"Checking packed-refs file %s\", refs->path);\n\nInstead of just using\n\n Checking references consistency\n\nCould we use\n\n Checking reftable references consistency\n\nHowever, I also feel strange about above, :)\n\n[snip]\n\n> > +/* Represents an individual error encountered during the FSCK checks. */\n> > +struct reftable_fsck_info {\n> > +\tenum reftable_fsck_error error;\n> > +\tconst char *msg;\n> > +\tconst char *path;\n> > +};\n> \n> I wonder whether it should be the reftable library that decides on the\n> severity of each generated finding.\n> \n\nThat's an interesting question. Let's inspect how Git handles the\nseverity. When defining the fsck message id, we need to specify its\nseverity like the following shows, this happens at compile time:\n\n FUNC(BAD_REFERENT_NAME, ERROR)\n\nAnd we could set the configuration \"fsck.[message id]=\" to change the\nfsck message severity.\n\nThen let's think if reftable library decides the severity. It means that\nwe need to use the API from reftable library to update\n\"fsck_option->msg_type\" at the runtime. And it is bad because the fsck\ninfrastructure would be highly coupled with the reftable library.\n\nSo, I don't think it's a good idea for reftable library to choose the\nseverity. Instead, reftable library should just provide users with error\ntypes and let the users decide the severity.\n\nThanks,\nJialuo\n"},{"id":"525799","messageId":"CAOLa=ZTuZ3fOr_wkQ7TsEVRKjKynei4uF18YHygpcafXT9Qjmw@mail.gmail.com","threadId":"64068","inReplyTo":"xmqqwm6gh4q3.fsf@gitster.g","subject":"Re: [PATCH v2 1/5] fsck: order 'fsck_msg_type' alphabetically","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-08T13:00:51Z","receivedAt":"2025-09-08T13:00:53Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> The list of 'fsck_msg_type' seem to be alphabetically ordered, but there\n>> are a few small misses. Fix this by sorting the sub-sections of the\n>> list to maintain alphabetical ordering. Also fix a clang-format issue\n>> where the escaped newlines are not aligned.\n>>\n>> While here, remove a duplicate instance of 'gitmodulesLarge' in the\n>> 'fsck-msgids' documentation.\n>\n> \"A few small misses\".\n>\n>> diff --git a/fsck.h b/fsck.h\n>> index dd7df3d5b3..559ad57807 100644\n>> --- a/fsck.h\n>> +++ b/fsck.h\n>> @@ -20,82 +20,82 @@ enum fsck_msg_type {\n>> ...\n>> -#define FOREACH_FSCK_MSG_ID(FUNC) \\\n>> -\t/* fatal errors */ \\\n>> -\tFUNC(NUL_IN_HEADER, FATAL) \\\n>> -\tFUNC(UNTERMINATED_HEADER, FATAL) \\\n>> ...\n>> +#define FOREACH_FSCK_MSG_ID(FUNC) \\\n>> +\t/* fatal errors */ \\\n>> +\tFUNC(NUL_IN_HEADER, FATAL) \\\n>> +\tFUNC(UNTERMINATED_HEADER, FATAL) \\\n>> ...\n>\n> Please undo these \"pad by spaces before backslash\"; otherwise we\n> cannot tell which ones are \"a few small misses\".\n>\n> Thanks.\n\nYeah, you're right, its much harder to review this way. Let me add in a\ncommit at the end to do the clang-formatting for this section, that way\nwe can drop it if it is too much noise.\n\n- Karthik\n"},{"id":"525925","messageId":"CAOLa=ZQoOX9y2_KwnAcyS-dVeQm1QnFdZocomt05BkxL9xDFuA@mail.gmail.com","threadId":"64068","inReplyTo":"aLf3MaKHZSQfnBlT@pks.im","subject":"Re: [PATCH v2 2/5] refs/reftable: add fsck check for checking the table name","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-09T08:42:08Z","receivedAt":"2025-09-09T08:42:09Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Tue, Sep 02, 2025 at 09:05:22AM +0200, Karthik Nayak wrote:\n>> The `git refs verify` command is used to run fsck checks on the\n>> reference backends. This command is also invoked when users run 'git\n>> fsck'. While the files-backend has some fsck checks added, the reftable\n>> backend lacks such checks. Let's add the required infrastructure and a\n>> check to test for the table names in the 'tables.list' of reftables.\n>>\n>> For the infrastructure, since the reftable library is treated as an\n>> independent library we should ensure that the library code works\n>> independently without knowledge about Git's internals. To do this,\n>> add both 'reftable/fsck.c' and 'reftable/reftable-fsck.h'. Which\n>> provide an entry point 'reftable_fsck_check' for running fsck checks\n>> over a provided reftable stack. The callee provides the function with\n>> callbacks to handle issue and information reporting.\n>>\n>> Add glue code in 'refs/reftable-backend.c' which calls the reftable\n>> library to perform the fsck checks. Here we also map the reftable errors\n>> to Git' fsck errors.\n>>\n>> Introduce a check to validate table names for a given reftable stack.\n>> Also add 'badReftableTableName' as a corresponding error within Git. Add\n>> a test to check for this behavior.\n>>\n>> While here, remove a unused header `#include \"../lockfile.h\"` from\n>> 'refs/reftable-backend.c'.\n>\n> It's quite a bunch of changes overall that could've been reasonably\n> split up into multiple commits. E.g. one to introduce the reftable-side\n> logic, one to start calling it in Git, and one to drop the superfluous\n> header.\n>\n\nI'm always hesitant to have small commits for some reason. Thanks for\ncalling out, I'll split it up.\n\nSidenote: I used `git history split` for this, and it was just perfect.\n\n>> diff --git a/Makefile b/Makefile\n>> index e11340c1ae..f2ddcc8d7c 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -2733,6 +2733,7 @@ REFTABLE_OBJS += reftable/error.o\n>> REFTABLE_OBJS += reftable/block.o\n>> REFTABLE_OBJS += reftable/blocksource.o\n>> REFTABLE_OBJS += reftable/iter.o\n>> +REFTABLE_OBJS += reftable/fsck.o\n>\n> \"f\" is before \"i\" in the alphabet I'm accustomed to :) So let's retain\n> lexicographic ordering here.\n>\n\nThis ordering was already broken, but that's no reason to break it more.\nLet' me fix it.\n\n>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n>> index 8dae1e1112..c38c6422f8 100644\n>> --- a/refs/reftable-backend.c\n>> +++ b/refs/reftable-backend.c\n>> @@ -2675,11 +2676,55 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,\n>> \treturn ret;\n>> }\n>>\n>> -static int reftable_be_fsck(struct ref_store *ref_store UNUSED,\n>> -\t\t\t struct fsck_options *o UNUSED,\n>> +static void reftable_fsck_verbose_handler(const char *msg, void *cb_data)\n>> +{\n>> +\tstruct fsck_options *o = cb_data;\n>> +\n>> +\tif (o->verbose)\n>> +\t\tfprintf_ln(stderr, \"%s\", _(msg));\n>> +}\n>\n> Is this `_()` marker correct here? There isn't really any reasonable way\n> for somebody to translate a variable with unknown contents. So shouldn't\n> it only be the caller of `reftable_fsck_verbose_handler()` that should\n> mark the string as translatable?\n>\n\nTrue, but this is a callback function called from within the reftable\nlibrary. I guess for now I can leave out the translation and we can\nthink about the best way to fix that later.\n\nWe need a more generic way to translate output strings originating from\nthe reftable library.\n\n>> +static int reftable_fsck_error_handler(struct reftable_fsck_info *info,\n>> +\t\t\t\t void *cb_data)\n>> +{\n>> +\tstruct fsck_ref_report report = { .path = info->path };\n>> +\tstruct fsck_options *o = cb_data;\n>> +\tenum fsck_msg_id msg_id;\n>> +\n>> +\tswitch (info->error) {\n>> +\tcase REFTABLE_FSCK_ERROR_TABLE_NAME:\n>> +\t\tmsg_id = FSCK_MSG_BAD_REFTABLE_TABLE_NAME;\n>> +\t\tbreak;\n>> +\tdefault:\n>> +\t\tBUG(\"unknown fsck error: %d\", info->error);\n>> +\t}\n>> +\n>> +\treturn fsck_report_ref(o, &report, msg_id, \"%s\", info->msg);\n>> +}\n>\n> I think this function will become a bit unwieldy over time. We might\n> instead want to have an array that maps from reftable-specific to\n> fsck-specific error code:\n>\n> static const fsck_msg_id[] = {\n> [REFTABLE_FSCK_ERROR_TABLE_NAME] = FSCK_MSG_BAD_REFTABLE_TABLE_NAME,\n> };\n>\n> So in that case, all we'd have to do is to perform bounds checking in\n> the above function. And maybe verify that the developer didn't forget to\n> fill in a new msg ID by checking that the derived message ID is non-zero.\n>\n\nYeah that sounds like a really good improvement, let me add that.\n\n>> +static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o,\n>> \t\t\t struct worktree *wt UNUSED)\n>> {\n>> -\treturn 0;\n>> +\tstruct reftable_ref_store *refs;\n>> +\tstruct strmap_entry *entry;\n>> +\tstruct hashmap_iter iter;\n>> +\tint ret = 0;\n>> +\n>> +\trefs = reftable_be_downcast(ref_store, REF_STORE_READ, \"fsck\");\n>> +\n>> +\tif (o->verbose)\n>> +\t\tfprintf_ln(stderr, _(\"Checking references consistency\"));\n>\n> This line is duplicate across both backends, right? Maybe it's something\n> that we can do in the generic logic?\n>\n\nYeah, we can. Will do.\n\n>> +\tret |= reftable_fsck_check(refs->main_backend.stack, reftable_fsck_error_handler,\n>> +\t\t\t\t reftable_fsck_verbose_handler, o);\n>> +\n>> +\tstrmap_for_each_entry(&refs->worktree_backends, &iter, entry) {\n>> +\t\tstruct reftable_backend *b = (struct reftable_backend *)entry->value;\n>> +\t\tret |= reftable_fsck_check(b->stack, reftable_fsck_error_handler,\n>> +\t\t\t\t\t reftable_fsck_verbose_handler, o);\n>> +\t}\n>> +\n>> +\treturn ret;\n>> }\n>>\n>> struct ref_storage_be refs_be_reftable = {\n>\n> Looks good.\n>\n>> diff --git a/reftable/fsck.c b/reftable/fsck.c\n>> new file mode 100644\n>> index 0000000000..4282b1413e\n>> --- /dev/null\n>> +++ b/reftable/fsck.c\n>> @@ -0,0 +1,53 @@\n>> +#include \"basics.h\"\n>> +#include \"reftable-fsck.h\"\n>> +#include \"stack.h\"\n>> +\n>> +int reftable_fsck_check(struct reftable_stack *stack,\n>> +\t\t\treftable_fsck_report_fn report_fn,\n>> +\t\t\treftable_fsck_verbose_fn verbose_fn,\n>> +\t\t\tvoid *cb_data)\n>> +{\n>> +\n>> +\tchar **names = NULL;\n>> +\tuint64_t min, max;\n>> +\tint err = 0;\n>> +\n>> +\tif (stack == NULL)\n>> +\t\tgoto out;\n>> +\n>> +\terr = read_lines(stack->list_file, &names);\n>> +\tif (err < 0)\n>> +\t\tgoto out;\n>> +\n>> +\tverbose_fn(\"Checking reftable table names\", cb_data);\n>> +\n>> +\tfor (size_t i = 0; names[i]; i++) {\n>> +\t\tstruct reftable_fsck_info info = {\n>> +\t\t\t.error = REFTABLE_FSCK_ERROR_TABLE_NAME,\n>> +\t\t\t.path = names[i],\n>> +\t\t};\n>> +\t\tuint32_t rnd;\n>> +\t\t/*\n>> +\t\t * We want to match the tail '.ref'. One extra byte to ensure\n>> +\t\t * that there is no unexpected extra character and one byte for\n>> +\t\t * the null terminator added by sscanf.\n>> +\t\t */\n>> +\t\tchar tail[6];\n>> +\n>> +\t\tif (sscanf(names[i], \"0x%012\" PRIx64 \"-0x%012\" PRIx64 \"-%08x%5s\",\n>> +\t\t\t &min, &max, &rnd, tail) != 4) {\n>> +\t\t\tinfo.msg = \"invalid reftable table name\";\n>\n> This here is where the string should be translated.\n>\n\nBut we don't have translation capabilities within the reftable lib no?\nOr am I mistaken?\n\n>> +\t\t\terr = report_fn(&info, cb_data);\n>> +\t\t\tcontinue;\n>> +\t\t}\n>\n> I think sscanf is quite frowned-upon in the Git codebase. Maybe we\n> should manually parse through the string instead?\n>\n\nThat would be cumbersome. This isn't user input data, so I thought this\nwould be okay. But let me do the change.\n\n> Furthermore, I think we should move every single check into a separate\n> function, similar to how the files backend does it. This ensures that\n> checks are self-contained and that it's way easier to add new checks\n> over time.\n>\n\nI think Shejialuo also mentioned this, let me do that.\n\n> Another angle: did you verify that reftables written by JGit follow this\n> format?\n>\n\nNo I haven't.\n\n>> +\t\tif (strcmp(tail, \".ref\")) {\n>> +\t\t\tinfo.msg = \"invalid reftable table extension\";\n>\n> Same here, this should be translated.\n>\n>> diff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h\n>> new file mode 100644\n>> index 0000000000..4cf0053234\n>> --- /dev/null\n>> +++ b/reftable/reftable-fsck.h\n>> @@ -0,0 +1,38 @@\n>> +#ifndef REFTABLE_FSCK_H\n>> +#define REFTABLE_FSCK_H\n>> +\n>> +#include \"reftable-stack.h\"\n>> +\n>> +enum reftable_fsck_error {\n>> +\t/* Invalid table name */\n>> +\tREFTABLE_FSCK_ERROR_TABLE_NAME = -1,\n>> +};\n>\n> Wouldn't it be more natural to give these positive numbers?\n>\n\nYes, that would be better and also fits in with the array suggstion you\nmade earlier.\n\n>> +/* Represents an individual error encountered during the FSCK checks. */\n>> +struct reftable_fsck_info {\n>> +\tenum reftable_fsck_error error;\n>> +\tconst char *msg;\n>> +\tconst char *path;\n>> +};\n>\n> I wonder whether it should be the reftable library that decides on the\n> severity of each generated finding.\n>\n\nI think that'd make sense. Let me add that in.\n\n>> +typedef int reftable_fsck_report_fn(struct reftable_fsck_info *info,\n>> +\t\t\t\t void *cb_data);\n>> +typedef void reftable_fsck_verbose_fn(const char *msg, void *cb_data);\n>> +\n>> +/*\n>> + * Given a reftable stack, perform FSCK check on the stack.\n>\n> s/FSCK check/consistency checks/\n>\n>> + *\n>> + * If an issue is encountered, the issue is reported to the callee via the\n>> + * provided 'report_fn'. If the issue is non-recoverable the flow will not\n>> + * continue. If it is recoverable, the flow will continue and further issues\n>> + * will be reported as identified.\n>> + *\n>> + * The 'verbose_fn' will be invoked to provide verbose information about\n>> + * the progress and state of the FSCK checks.\n>\n> Same here.\n>\n\nThanks, changed both.\n\n>> diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh\n>> new file mode 100755\n>> index 0000000000..81d30df2d7\n>> --- /dev/null\n>> +++ b/t/t0614-reftable-fsck.sh\n>> @@ -0,0 +1,58 @@\n>> +#!/bin/sh\n>> +\n>> +test_description='Test reftable backend consistency check'\n>> +\n>> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n>> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>\n> Tests shouldn't define these variables, but should dynamically figure\n> out what the default branch name is as required, e.g. by using\n> git-symbolic-ref(1).\n>\n\nYeah, makes sense. Will change it.\n\n>> +GIT_TEST_DEFAULT_REF_FORMAT=reftable\n>> +export GIT_TEST_DEFAULT_REF_FORMAT\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +test_expect_success 'table name should be checked' '\n>> +\ttest_when_finished \"rm -rf repo\" &&\n>> +\tgit init repo &&\n>> +\t(\n>> +\t\tcd repo &&\n>> +\t\tgit commit --allow-empty -m initial &&\n>> +\n>> +\t\tgit refs verify 2>err &&\n>> +\t\ttest_must_be_empty err &&\n>> +\n>> +\t\tTABLE_NAME=$(cat .git/reftable/tables.list | head -n1) &&\n>\n> You can drop the cat(1) invocation and directly say `head -n1 file`.\n>\n\nIndeed, thanks!\n\n>> +\t\tsed \"1s/^/extra/\" .git/reftable/tables.list >.git/reftable/tables.list.tmp &&\n>> +\t\tmv .git/reftable/tables.list.tmp .git/reftable/tables.list &&\n>> +\t\tmv .git/reftable/${TABLE_NAME} .git/reftable/extra${TABLE_NAME} &&\n>\n> No need for the curly braces around TABLE_NAME here and further down. It\n> would be nice to quote these strings though.\n>\n\nUnderstandable. I always prefer using them, since they make it much\neasier to read, even without the ambiguity issue.\n\n>> +\n>> +\t\ttest_must_fail git refs verify 2>err &&\n>> +\t\tcat >expect <<-EOF &&\n>> +\t\terror: extra${TABLE_NAME}: badReftableTableName: invalid reftable table name\n>> +\t\tEOF\n>> +\t\ttest_cmp expect err\n>> +\t)\n>> +'\n>> +\n>> +test_expect_success 'table name should be checked' '\n>> +\ttest_when_finished \"rm -rf repo\" &&\n>> +\tgit init repo &&\n>> +\t(\n>> +\t\tcd repo &&\n>> +\t\tgit commit --allow-empty -m initial &&\n>> +\n>> +\t\tgit refs verify 2>err &&\n>> +\t\ttest_must_be_empty err &&\n>> +\n>> +\t\tTABLE_NAME=$(cat .git/reftable/tables.list | head -n1) &&\n>\n> Same here wrt the extra invocation of cat(1).\n>\n\nWill change! Thanks.\n\n> Patrick\n"},{"id":"525963","messageId":"CAOLa=ZRzEY7VTcRTLz1u6=A5Mz0XFOhqNWK4s_wr1WOP==B1rg@mail.gmail.com","threadId":"64068","inReplyTo":"aLhx-HdIMCB3c2WK@ArchLinux","subject":"Re: [PATCH v2 2/5] refs/reftable: add fsck check for checking the table name","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-09T13:49:18Z","receivedAt":"2025-09-09T13:49:21Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"shejialuo writes:\n\n> On Wed, Sep 03, 2025 at 10:07:13AM +0200, Patrick Steinhardt wrote:\n>\n> [snip]\n>\n>> > +static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o,\n>> > \t\t\t struct worktree *wt UNUSED)\n>> > {\n>> > -\treturn 0;\n>> > +\tstruct reftable_ref_store *refs;\n>> > +\tstruct strmap_entry *entry;\n>> > +\tstruct hashmap_iter iter;\n>> > +\tint ret = 0;\n>> > +\n>> > +\trefs = reftable_be_downcast(ref_store, REF_STORE_READ, \"fsck\");\n>> > +\n>> > +\tif (o->verbose)\n>> > +\t\tfprintf_ln(stderr, _(\"Checking references consistency\"));\n>>\n>> This line is duplicate across both backends, right? Maybe it's something\n>> that we can do in the generic logic?\n>>\n>\n> That's right, it is duplicate. If we want to remove this, we need to do\n> this in the \"builtin/refs.c\". But I wonder whether we should do this in\n> the first place. Should we rather add more detailed information just\n> like the following code for packed backend?\n>\n> if (o->verbose)\n> fprintf_ln(stderr, \"Checking packed-refs file %s\", refs->path);\n>\n> Instead of just using\n>\n> Checking references consistency\n>\n> Could we use\n>\n> Checking reftable references consistency\n>\n> However, I also feel strange about above, :)\n>\n> [snip]\n>\n>> > +/* Represents an individual error encountered during the FSCK checks. */\n>> > +struct reftable_fsck_info {\n>> > +\tenum reftable_fsck_error error;\n>> > +\tconst char *msg;\n>> > +\tconst char *path;\n>> > +};\n>>\n>> I wonder whether it should be the reftable library that decides on the\n>> severity of each generated finding.\n>>\n>\n\n\nI think I did rush while agreeing to do this change and didn't realize\nthe complexity of it.\n\n> That's an interesting question. Let's inspect how Git handles the\n> severity. When defining the fsck message id, we need to specify its\n> severity like the following shows, this happens at compile time:\n>\n> FUNC(BAD_REFERENT_NAME, ERROR)\n>\n\nThis is used to create the enum of all values, but there is a\ncomplimentary structure `msg_id_info` which holds the mapping for each\nmessage id to its error category.\n\nBoth of these could be extended at compile time by including the errors\nfrom the reftable header. But to do this in a backend agnostic way, we'd\nhave to receive and re-expose it via `refs.h`.\n\n> And we could set the configuration \"fsck.[message id]=\" to change the\n> fsck message severity.\n>\n> Then let's think if reftable library decides the severity. It means that\n> we need to use the API from reftable library to update\n> \"fsck_option->msg_type\" at the runtime. And it is bad because the fsck\n> infrastructure would be highly coupled with the reftable library.\n>\n> So, I don't think it's a good idea for reftable library to choose the\n> severity. Instead, reftable library should just provide users with error\n> types and let the users decide the severity.\n>\n\nSo while there are ways to do it, it won't be simple/elegant and I'm not\nsure it'd be worth it.\n\n> Thanks,\n> Jialuo\n"},{"id":"526326","messageId":"CAOLa=ZR9nXkdXuMik2uq8+kpxQwmB72r7XZB0rnJSvKU7mKmkw@mail.gmail.com","threadId":"64068","inReplyTo":"aLf3NyodTZ38N8vY@pks.im","subject":"Re: [PATCH v2 3/5] refs/reftable: add fsck check for number of tables","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-15T09:27:43Z","receivedAt":"2025-09-15T09:27:45Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Tue, Sep 02, 2025 at 09:05:23AM +0200, Karthik Nayak wrote:\n>> Introduce a reftable fsck check to check that the number of files in the\n>> reftable directory matches the number of files listed in 'tables.list'.\n>> We do this by iterating over the files in the reftable directory and\n>> counting all the files present excluding the 'tables.list'. This is also\n>> exposed over Git's fsck checks as a 'badReftableStackCount' error.\n>\n> This feels overly strict, as it can always be the case that a concurrent\n> process is currently updating the stack. Furthermore, it's expected that\n> on Windows systems deletion of an old table may not work because the\n> file is still kept open by another process. The reftable library is\n> prepared to handle this alright and will re-try deleting the table at a\n> later point in time.\n>\n\nYeah that makes sense.\n\n> So maybe a better check would be to verify that there are no files with\n> unexpected names in the directory?\n>\n\nI was hoping to add structured consistency check in a layered format\n\n- Stack\n - For each `Table`\n - For each `Block`\n - For each `Ref`\n\nBut this wouldn't belong to that, since it isn't part of the stack. So,\nI'll keep the above structure and also add this to the stack level. So\nwe'll have\n\n- Stack\n - For each `Table`\n - For each `Block`\n - For each `Ref`\n - Other Stack level checks\n - Check other files in the repo\n\n> Patrick\n"},{"id":"526683","messageId":"CAOLa=ZTR0X+Za-8O04+6GzSUK83GUwaHTaWij3MA0vty2WUJUg@mail.gmail.com","threadId":"64068","inReplyTo":"xmqqo6rsh3xw.fsf@gitster.g","subject":"Re: [PATCH v2 5/5] refs/reftable: add fsck check for incorrect update index","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-09-18T08:11:24Z","receivedAt":"2025-09-18T08:11:26Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> Introduce a fsck check for the reftable backend, which checks if the\n>> tables in 'tables.list' contain sequential update index. The tables in\n>> the reftable backend should contain sequential update index. This fsck\n>> check ensures that.\n>>\n>> We must note that the reftable backend itself doesn't check to ensure\n>> this and it also doesn't check to ensure that the index in the table\n>> name matches the index in the header or the table. The latter is not\n>> implemented in this fsck check either and will be added in a future\n>> patch where we add fsck checks for internals of a table.\n>\n> Similar to the previous step, I am not sure why this should not be\n> checked at runtime and is flagged as an error.\n>\n> In general, we do try to avoid retroactively tightening rules, but\n> the reftable is so new and not even the default. If we noticed that\n> the runtime has been overly loose, the time to tighten it is now,\n> not after even more installations use it.\n\nI think your point is fair and I agree. I did consider it, but didn't\nwant to 'retroactively tightening rules'. But I think it is justified,\nmore so since we're not introducing any changes on the format, but just\nmore validation around it.\n\nI should've replied here sooner, but I didn't ever get to it. I found\nsome time to finally fix the comments from this series and will send in\na new version. I will be more diligent about it here on.\n\n- Karthik\n"}]}