Re: [PATCH v2 2/5] refs/reftable: add fsck check for checking the table name

> 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.
>

>> +		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).
>