From: Karthik Nayak Date: Mon, 06 Oct 2025 14:22:58 GMT Subject: [PATCH v5 0/7] refs/reftable: add consistency checks Message-ID: <20251006-228-reftable-introduce-consistency-checks-v5-0-f196d386214f@gmail.com> In-Reply-To: <20250819-228-reftable-introduce-consistency-checks-v1-0-8b8f6879fa9e@gmail.com> The reference subsystems allows for adding backend specific consistency checks. These checks are run as part of 'git refs verify'. While the files backend has some consistency checks added, the reftable backend currently has none. This series first tightens the reftable backend to make it a little more strict and then also adds the required infrastructure and some simple consistency checks. Since the reftable backend is treated as a library within the Git codebase, we don't want to spillover our internal fsck implementation into the library. At the same time, the fsck checks need to access internal structures of the reftable library which aren't exposed outside the library. So we solve this by adding a 'reftable/fsck.[ch]' which implements and exposes a checker for the reftable library and returns specific errors as defined by the library. We then add glue code within 'refs/reftable-backend.c' to map these errors to errors which Git's fsck implementation would understand. This allows us to separate concerns. We add the following consistency checks: 1. Check for validating the reftable table name. This is treated as a warning since the reftable specification only suggests a table name but doesn't enforce it. Also there is a difference in the table name used in Git vs that in jGit. We tighten the reftable backend by raising a REFTABLE_FORMAT_ERROR error when: 1. The 'tables.list' file doesn't have a trailing newline. --- Changes in v5: - Added documentation around the return value of 'parse_names()'. - Added a test to validate that 'git refs verify' doesn't barf against a clean working repository with multiple reftable tables. - Link to v4: https://lore.kernel.org/all/20250926-228-reftable-introduce-consistency-checks-v4-0-c96fd8551c0d@gmail.com Changes in v4: - The biggest change is to iterate over the tables in a reftable stack for consistency checks instead of all files inside the REFTABLE_DIR. This avoids all race conditions. Also, since we only check the tables in a stack, it no longer makes sense to check file type. - The discussion about update indices was concluded that tables indices in a stack must be strictly monotonically increasing. While modifying the code to do the same. I realized that we already have this check in 'reftable_addition_add()' where we check while adding a new table to the stack: `wr->min_update_index < add->next_update_index`. So I've dropped this patch from the series. - Change parse_names() to accept the output string array as an argument and return an error instead. This makes the flow a little easier to understand. - Link to v3: https://lore.kernel.org/r/20250918-228-reftable-introduce-consistency-checks-v3-0-271af03eb34d@gmail.com Changes in v3: - I took a long hiatus from this topic, mostly due to other priorities. This has been rebased on top of '92c87bdc40 (The eighth batch, 2025-09-12)' since there were conflicts. - Junio suggested that two of the consistency checks (trailing newlines, sequential update indices for tables in stack) should actually be checked during runtime. I have made that change in this version. - I've cleaned up the code and modularized the 'reftable/fsck.c' code. - Invalid table name emits a warning, since the reftable spec doesn't enforce it but only makes a suggestion. - Broken down the commits to make it easier to review. - Link to v2: https://lore.kernel.org/r/20250902-228-reftable-introduce-consistency-checks-v2-0-4f96b3834779@gmail.com Changes in v2: - Ensured that 'struct reftable_fsck_info' is passed around as a pointer, this provides a smaller footprint (pointer size vs struct size). - Run FSCK checks for other worktrees too, even if one of them fails. - Separate messaging for table name vs table check and add additional test. - Use the relative path in messages used. - Small style and typo fixes. - Link to v1: https://lore.kernel.org/r/20250819-228-reftable-introduce-consistency-checks-v1-0-8b8f6879fa9e@gmail.com --- Documentation/fsck-msgids.adoc | 6 +-- Makefile | 3 +- fsck.h | 39 +++++++-------- meson.build | 1 + refs.c | 4 ++ refs/debug.c | 1 - refs/files-backend.c | 3 -- refs/reftable-backend.c | 58 ++++++++++++++++++++--- reftable/basics.c | 37 ++++++++++----- reftable/basics.h | 7 +-- reftable/fsck.c | 100 +++++++++++++++++++++++++++++++++++++++ reftable/reftable-fsck.h | 40 ++++++++++++++++ reftable/stack.c | 7 +-- t/meson.build | 1 + t/t0614-reftable-fsck.sh | 58 +++++++++++++++++++++++ t/unit-tests/u-reftable-basics.c | 24 ++++++++-- 16 files changed, 330 insertions(+), 59 deletions(-) Karthik Nayak (7): refs: remove unused headers refs: move consistency check msg to generic layer reftable: check for trailing newline in 'tables.list' Documentation/fsck-msgids: remove duplicate msg id fsck: order 'fsck_msg_type' alphabetically reftable: add code to facilitate consistency checks refs/reftable: add fsck check for checking the table name Range-diff versus v4: 1: 4e40ab1ff7 < -: ---------- refs/reftable: add consistency checks 2: b91194e060 = 1: 6e3766330b refs: remove unused headers 3: d48afbf588 = 2: e93c0deaf7 refs: move consistency check msg to generic layer 4: cd7ca2a585 ! 3: 7a282473a1 reftable: check for trailing newline in 'tables.list' @@ reftable/basics.h: void free_names(char **a); - * without terminating '\0'. Empty names are discarded. Returns a `NULL` - * pointer when allocations fail. + * without terminating '\0'. Empty names are discarded. ++ * ++ * Returns 0 on success, a reftable error code on error. */ -char **parse_names(char *buf, int size); +int parse_names(char *buf, int size, char ***out); 5: e3e0c0b4ae = 4: 4b47088232 Documentation/fsck-msgids: remove duplicate msg id 6: 24a8d93adc = 5: 112ae21321 fsck: order 'fsck_msg_type' alphabetically 7: d83d763be1 = 6: 3d1fc18260 reftable: add code to facilitate consistency checks 8: d86ecd5bed ! 7: 2b628e3623 refs/reftable: add fsck check for checking the table name @@ Commit message So treat non-conformant file names as warnings. - While adding the fsck header to 'refs/reftable-backend.c', order the - list of headers. + While adding the fsck header to 'refs/reftable-backend.c', modify the + list to maintain lexicographical ordering. Signed-off-by: Karthik Nayak @@ t/t0614-reftable-fsck.sh (new) + +. ./test-lib.sh + ++test_expect_success "no errors reported on a well formed repository" ' ++ test_when_finished "rm -rf repo" && ++ git init repo && ++ ( ++ cd repo && ++ git commit --allow-empty -m initial && ++ ++ for i in $(test_seq 20) ++ do ++ git update-ref branch-$i HEAD || return 1 ++ done && ++ ++ # The repository should end up with multiple tables. ++ test_line_count ">" 1 .git/reftable/tables.list && ++ ++ git refs verify 2>err && ++ test_must_be_empty err ++ ) ++' ++ +for TABLE_NAME in "foo-bar-e4d12d59.ref" \ + "0x00000000zzzz-0x00000000zzzz-e4d12d59.ref" \ + "0x000000000001-0x000000000002-e4d12d59.abc" \ base-commit: a483264b01b977f3e65a4419103c21e6af7412a2 change-id: 20250714-228-reftable-introduce-consistency-checks-379ded93c544 Thanks - Karthik