On Tue, Sep 02, 2025 at 09:05:22AM +0200, Karthik Nayak wrote:
> The `git refs verify` command is used to run fsck checks on the
> reference backends. This command is also invoked when users run 'git
> fsck'. While the files-backend has some fsck checks added, the reftable
> backend lacks such checks. Let's add the required infrastructure and a
> check to test for the table names in the 'tables.list' of reftables.
>
> For the infrastructure, since the reftable library is treated as an
> independent library we should ensure that the library code works
> independently without knowledge about Git's internals. To do this,
> add both 'reftable/fsck.c' and 'reftable/reftable-fsck.h'. Which
> provide an entry point 'reftable_fsck_check' for running fsck checks
> over a provided reftable stack. The callee provides the function with
> callbacks to handle issue and information reporting.
>
> Add glue code in 'refs/reftable-backend.c' which calls the reftable
> library to perform the fsck checks. Here we also map the reftable errors
> to Git' fsck errors.
>
> Introduce a check to validate table names for a given reftable stack.
> Also add 'badReftableTableName' as a corresponding error within Git. Add
> a test to check for this behavior.
>
> While here, remove a unused header `#include "../lockfile.h"` from
> 'refs/reftable-backend.c'.
It's quite a bunch of changes overall that could've been reasonably
split up into multiple commits. E.g. one to introduce the reftable-side
logic, one to start calling it in Git, and one to drop the superfluous
header.
> diff --git a/Makefile b/Makefile
> index e11340c1ae..f2ddcc8d7c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2733,6 +2733,7 @@ REFTABLE_OBJS += reftable/error.o
> REFTABLE_OBJS += reftable/block.o
> REFTABLE_OBJS += reftable/blocksource.o
> REFTABLE_OBJS += reftable/iter.o
> +REFTABLE_OBJS += reftable/fsck.o
"f" is before "i" in the alphabet I'm accustomed to :) So let's retain
lexicographic ordering here.
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 8dae1e1112..c38c6422f8 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -2675,11 +2676,55 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,
> return ret;
> }
>
> -static int reftable_be_fsck(struct ref_store *ref_store UNUSED,
> - struct fsck_options *o UNUSED,
> +static void reftable_fsck_verbose_handler(const char *msg, void *cb_data)
> +{
> + struct fsck_options *o = cb_data;
> +
> + if (o->verbose)
> + fprintf_ln(stderr, "%s", _(msg));
> +}
Is this `_()` marker correct here? There isn't really any reasonable way
for somebody to translate a variable with unknown contents. So shouldn't
it only be the caller of `reftable_fsck_verbose_handler()` that should
mark the string as translatable?
> +static int reftable_fsck_error_handler(struct reftable_fsck_info *info,
> + void *cb_data)
> +{
> + struct fsck_ref_report report = { .path = info->path };
> + struct fsck_options *o = cb_data;
> + enum fsck_msg_id msg_id;
> +
> + switch (info->error) {
> + case REFTABLE_FSCK_ERROR_TABLE_NAME:
> + msg_id = FSCK_MSG_BAD_REFTABLE_TABLE_NAME;
> + break;
> + default:
> + BUG("unknown fsck error: %d", info->error);
> + }
> +
> + return fsck_report_ref(o, &report, msg_id, "%s", info->msg);
> +}
I think this function will become a bit unwieldy over time. We might
instead want to have an array that maps from reftable-specific to
fsck-specific error code:
static const fsck_msg_id[] = {
[REFTABLE_FSCK_ERROR_TABLE_NAME] = FSCK_MSG_BAD_REFTABLE_TABLE_NAME,
};
So in that case, all we'd have to do is to perform bounds checking in
the above function. And maybe verify that the developer didn't forget to
fill in a new msg ID by checking that the derived message ID is non-zero.
> +static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o,
> struct worktree *wt UNUSED)
> {
> - return 0;
> + struct reftable_ref_store *refs;
> + struct strmap_entry *entry;
> + struct hashmap_iter iter;
> + int ret = 0;
> +
> + refs = reftable_be_downcast(ref_store, REF_STORE_READ, "fsck");
> +
> + if (o->verbose)
> + fprintf_ln(stderr, _("Checking references consistency"));
This line is duplicate across both backends, right? Maybe it's something
that we can do in the generic logic?
> + ret |= reftable_fsck_check(refs->main_backend.stack, reftable_fsck_error_handler,
> + reftable_fsck_verbose_handler, o);
> +
> + strmap_for_each_entry(&refs->worktree_backends, &iter, entry) {
> + struct reftable_backend *b = (struct reftable_backend *)entry->value;
> + ret |= reftable_fsck_check(b->stack, reftable_fsck_error_handler,
> + reftable_fsck_verbose_handler, o);
> + }
> +
> + return ret;
> }
>
> struct ref_storage_be refs_be_reftable = {
Looks good.
> diff --git a/reftable/fsck.c b/reftable/fsck.c
> new file mode 100644
> index 0000000000..4282b1413e
> --- /dev/null
> +++ b/reftable/fsck.c
> @@ -0,0 +1,53 @@
> +#include "basics.h"
> +#include "reftable-fsck.h"
> +#include "stack.h"
> +
> +int reftable_fsck_check(struct reftable_stack *stack,
> + reftable_fsck_report_fn report_fn,
> + reftable_fsck_verbose_fn verbose_fn,
> + void *cb_data)
> +{
> +
> + char **names = NULL;
> + uint64_t min, max;
> + int err = 0;
> +
> + if (stack == NULL)
> + goto out;
> +
> + err = read_lines(stack->list_file, &names);
> + if (err < 0)
> + goto out;
> +
> + verbose_fn("Checking reftable table names", cb_data);
> +
> + for (size_t i = 0; names[i]; i++) {
> + struct reftable_fsck_info info = {
> + .error = REFTABLE_FSCK_ERROR_TABLE_NAME,
> + .path = names[i],
> + };
> + uint32_t rnd;
> + /*
> + * We want to match the tail '.ref'. One extra byte to ensure
> + * that there is no unexpected extra character and one byte for
> + * the null terminator added by sscanf.
> + */
> + char tail[6];
> +
> + if (sscanf(names[i], "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x%5s",
> + &min, &max, &rnd, tail) != 4) {
> + info.msg = "invalid reftable table name";
This here is where the string should be translated.
> + err = report_fn(&info, cb_data);
> + continue;
> + }
I think sscanf is quite frowned-upon in the Git codebase. Maybe we
should manually parse through the string instead?
Furthermore, I think we should move every single check into a separate
function, similar to how the files backend does it. This ensures that
checks are self-contained and that it's way easier to add new checks
over time.
Another angle: did you verify that reftables written by JGit follow this
format?
> + if (strcmp(tail, ".ref")) {
> + info.msg = "invalid reftable table extension";
Same here, this should be translated.
> diff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h
> new file mode 100644
> index 0000000000..4cf0053234
> --- /dev/null
> +++ b/reftable/reftable-fsck.h
> @@ -0,0 +1,38 @@
> +#ifndef REFTABLE_FSCK_H
> +#define REFTABLE_FSCK_H
> +
> +#include "reftable-stack.h"
> +
> +enum reftable_fsck_error {
> + /* Invalid table name */
> + REFTABLE_FSCK_ERROR_TABLE_NAME = -1,
> +};
Wouldn't it be more natural to give these positive numbers?
> +/* Represents an individual error encountered during the FSCK checks. */
> +struct reftable_fsck_info {
> + enum reftable_fsck_error error;
> + const char *msg;
> + const char *path;
> +};
I wonder whether it should be the reftable library that decides on the
severity of each generated finding.
> +typedef int reftable_fsck_report_fn(struct reftable_fsck_info *info,
> + void *cb_data);
> +typedef void reftable_fsck_verbose_fn(const char *msg, void *cb_data);
> +
> +/*
> + * Given a reftable stack, perform FSCK check on the stack.
s/FSCK check/consistency checks/
> + *
> + * If an issue is encountered, the issue is reported to the callee via the
> + * provided 'report_fn'. If the issue is non-recoverable the flow will not
> + * continue. If it is recoverable, the flow will continue and further issues
> + * will be reported as identified.
> + *
> + * The 'verbose_fn' will be invoked to provide verbose information about
> + * the progress and state of the FSCK checks.
Same here.
> diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh
> new file mode 100755
> index 0000000000..81d30df2d7
> --- /dev/null
> +++ b/t/t0614-reftable-fsck.sh
> @@ -0,0 +1,58 @@
> +#!/bin/sh
> +
> +test_description='Test reftable backend consistency check'
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
Tests shouldn't define these variables, but should dynamically figure
out what the default branch name is as required, e.g. by using
git-symbolic-ref(1).
> +GIT_TEST_DEFAULT_REF_FORMAT=reftable
> +export GIT_TEST_DEFAULT_REF_FORMAT
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'table name should be checked' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + (
> + cd repo &&
> + git commit --allow-empty -m initial &&
> +
> + git refs verify 2>err &&
> + test_must_be_empty err &&
> +
> + TABLE_NAME=$(cat .git/reftable/tables.list | head -n1) &&
You can drop the cat(1) invocation and directly say `head -n1 file`.
> + sed "1s/^/extra/" .git/reftable/tables.list >.git/reftable/tables.list.tmp &&
> + mv .git/reftable/tables.list.tmp .git/reftable/tables.list &&
> + mv .git/reftable/${TABLE_NAME} .git/reftable/extra${TABLE_NAME} &&
No need for the curly braces around TABLE_NAME here and further down. It
would be nice to quote these strings though.
> +
> + test_must_fail git refs verify 2>err &&
> + cat >expect <<-EOF &&
> + error: extra${TABLE_NAME}: badReftableTableName: invalid reftable table name
> + EOF
> + test_cmp expect err
> + )
> +'
> +
> +test_expect_success 'table name should be checked' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + (
> + cd repo &&
> + git commit --allow-empty -m initial &&
> +
> + git refs verify 2>err &&
> + test_must_be_empty err &&
> +
> + TABLE_NAME=$(cat .git/reftable/tables.list | head -n1) &&
Same here wrt the extra invocation of cat(1).
Patrick
