From: Karthik Nayak Date: Mon, 01 Sep 2025 13:33:24 GMT Subject: Re: [PATCH 2/5] refs/reftable: add fsck check for checking the table name Message-ID: In-Reply-To: shejialuo writes: > On Tue, Aug 19, 2025 at 02:21:01PM +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 > > A design question here, we name the "fsck.c" for the source code but for > the header, we use "reftable-fsck.h", it is a little strange. Why not > just "fsck.h" instead of "reftable-fsck.h". > Since the reftable code is treated as an external library, all 'reftable-.*.h' headers are treated as headers which expose APIs for the libraries users. We would have defined 'reftable/fsck.h' if there were internal users of the 'fsck.c' code. But there are none. >> diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc >> index 1c912615f9..784ddc0df5 100644 >> --- a/Documentation/fsck-msgids.adoc >> +++ b/Documentation/fsck-msgids.adoc >> @@ -38,6 +38,9 @@ >> `badReferentName`:: >> (ERROR) The referent name of a symref is invalid. >> >> +`badReftableTableName`:: >> + (ERROR) A reftable table has an invalid name. >> + > > When reading this, I feel a little strange. `Reftable` already indicates > it is a table. Should we simply say like the following: > > A reftable has an invalid table name > I'm not sure about this, since 'reftable' refers to the reference backend and the 'table' refers to an individual table within the 'reftable' format. I would say both are important. CC'ing Patrick here for a second opinion. >> `badTagName`:: >> (INFO) A tag has an invalid format. >> >> diff --git a/Makefile b/Makefile >> index e11340c1ae..f2ddcc8d7c 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -2733,6 +2733,7 @@ REFTABLE_OBJS += reftable/error.o >> REFTABLE_OBJS += reftable/block.o >> REFTABLE_OBJS += reftable/blocksource.o >> REFTABLE_OBJS += reftable/iter.o >> +REFTABLE_OBJS += reftable/fsck.o >> REFTABLE_OBJS += reftable/merged.o >> REFTABLE_OBJS += reftable/pq.o >> REFTABLE_OBJS += reftable/record.o >> diff --git a/fsck.h b/fsck.h >> index 559ad57807..5901f944a1 100644 >> --- a/fsck.h >> +++ b/fsck.h >> @@ -34,6 +34,7 @@ enum fsck_msg_type { >> FUNC(BAD_PACKED_REF_HEADER, ERROR) \ >> FUNC(BAD_PARENT_SHA1, ERROR) \ >> FUNC(BAD_REFERENT_NAME, ERROR) \ >> + FUNC(BAD_REFTABLE_TABLE_NAME, ERROR) \ >> FUNC(BAD_REF_CONTENT, ERROR) \ >> FUNC(BAD_REF_FILETYPE, ERROR) \ >> FUNC(BAD_REF_NAME, ERROR) \ >> diff --git a/meson.build b/meson.build >> index 5dd299b496..82879fbfaa 100644 >> --- a/meson.build >> +++ b/meson.build >> @@ -452,6 +452,7 @@ libgit_sources = [ >> 'reftable/error.c', >> 'reftable/block.c', >> 'reftable/blocksource.c', >> + 'reftable/fsck.c', >> 'reftable/iter.c', >> 'reftable/merged.c', >> 'reftable/pq.c', >> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c >> index 8dae1e1112..ccd12052f2 100644 >> --- a/refs/reftable-backend.c >> +++ b/refs/reftable-backend.c >> @@ -6,20 +6,21 @@ >> #include "../config.h" >> #include "../dir.h" >> #include "../environment.h" >> +#include "../fsck.h" >> #include "../gettext.h" >> #include "../hash.h" >> #include "../hex.h" >> #include "../iterator.h" >> #include "../ident.h" >> -#include "../lockfile.h" > > Here, we delete this header file. Is the reason that we don't need this > header file anymore? > Yes, it wasn't needed in the first place, let me add a comment in the commit message. >> #include "../object.h" >> #include "../path.h" >> #include "../refs.h" >> #include "../reftable/reftable-basics.h" >> -#include "../reftable/reftable-stack.h" >> -#include "../reftable/reftable-record.h" >> #include "../reftable/reftable-error.h" >> +#include "../reftable/reftable-fsck.h" >> #include "../reftable/reftable-iterator.h" >> +#include "../reftable/reftable-record.h" >> +#include "../reftable/reftable-stack.h" >> #include "../repo-settings.h" >> #include "../setup.h" >> #include "../strmap.h" >> @@ -2675,11 +2676,59 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, >> return ret; >> } >> >> -static int reftable_be_fsck(struct ref_store *ref_store UNUSED, >> - struct fsck_options *o UNUSED, >> +static void reftable_fsck_verbose_handler(const char *msg, void *cb_data) >> +{ >> + struct fsck_options *o = cb_data; >> + >> + if (o->verbose) >> + fprintf_ln(stderr, "%s", _(msg)); >> +} >> + >> +static int reftable_fsck_error_handler(struct reftable_fsck_info info, > > A design question: why do we need to pass the value "info" instead of > pointer? > I didn't see a reason to make it a pointer. But it does make it more efficient when the struct size increases. Let me change it! > >> + void *cb_data) >> +{ >> + struct fsck_options *o = cb_data; >> + struct fsck_ref_report report = { .path = info.path }; > > Let's make it reverse-christmas-tree ordering. > Will change! >> +static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o, >> struct worktree *wt UNUSED) >> { >> - return 0; >> + struct reftable_ref_store *refs; >> + struct strmap_entry *entry; >> + struct hashmap_iter iter; >> + int ret = 0; >> + >> + refs = reftable_be_downcast(ref_store, REF_STORE_READ, "fsck"); >> + >> + if (o->verbose) >> + fprintf_ln(stderr, _("Checking references consistency")); >> + >> + ret = reftable_fsck_check(refs->main_backend.stack, reftable_fsck_error_handler, >> + reftable_fsck_verbose_handler, o); >> + if (!ret) >> + return ret; >> + > > From my understanding, if we find that there is any trouble in the main > worktree reftable backend, we would just abort the check. Should we > continue to check the linked worktrees? > I think that makes sense. Let me make that change. >> diff --git a/reftable/fsck.c b/reftable/fsck.c >> new file mode 100644 >> index 0000000000..22ec3c26e9 >> --- /dev/null >> +++ b/reftable/fsck.c >> @@ -0,0 +1,50 @@ >> +#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], >> + .msg = "invalid reftable name" >> + }; > > Should we define this data structure outside of the loop? It's > unnecessary here as we could change ".path" and ".msg" dynamically in > the loop. > I don't think it'd make much difference for reftables, since tables are geometrically packed. But I don't feel strongly, so I'll make the change. >> + 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) { >> + err = report_fn(info, cb_data); > > I think we could just pass pointer to avoid unnecessary copy operations. > Besides that, I think here we report two different kinds of problem. But > we would give report the user always the same message `invalid reftable > name`. This is too vague. > Not sure what you mean by 'unnecessary copy operations', could you elaborate? > I think we'd better set different messages for different problems. > Fair enough, let me modify that. [snip] >> diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh >> new file mode 100755 >> index 0000000000..0d11871b1c >> --- /dev/null >> +++ b/t/t0614-reftable-fsck.sh >> @@ -0,0 +1,35 @@ >> +#!/bin/sh >> + >> +test_description='Test reftable backend consistency check' >> + >> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main >> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME >> +GIT_TEST_DEFAULT_REF_FORMAT=reftable >> +export GIT_TEST_DEFAULT_REF_FORMAT >> + >> +. ./test-lib.sh >> + >> +test_expect_success 'table name should be checked' ' >> + test_when_finished "rm -rf repo" && >> + git init repo && >> + ( >> + cd repo && >> + git commit --allow-empty -m initial && >> + >> + git refs verify 2>err && >> + test_must_be_empty err && >> + >> + TABLE_NAME=$(cat .git/reftable/tables.list | head -n1) && >> + sed "1s/$/extra/" .git/reftable/tables.list >.git/reftable/tables.list.tmp && >> + mv .git/reftable/tables.list.tmp .git/reftable/tables.list && >> + mv .git/reftable/${TABLE_NAME} .git/reftable/${TABLE_NAME}extra && >> + >> + test_must_fail git refs verify 2>err && >> + cat >expect <<-EOF && >> + error: ${TABLE_NAME}extra: badReftableTableName: invalid reftable name >> + EOF >> + test_cmp expect err >> + ) >> +' > > We would check two kinds of errors, should we add two tests instead of > only this one. > Yeah, makes sense, will add! >> + >> +test_done >> >> -- >> 2.50.1 >> > > Thanks, > Jialuo Thanks for the review.