git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
shejialuo <shejialuo@gmail.com>
Date
Aug 26, 2025, 16:21 UTC
Message-ID
<aK3fHRMFiRBYNiJE@ArchLinux>
In-Reply-To
<20250819-228-reftable-introduce-consistency-checks-v1-2-8b8f6879fa9e@gmail.com>
On Tue, Aug 19, 2025 at 02:21:01PM +0200, Karthik Nayak wrote:
Show 10 quoted lines
> 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".

Show 36 quoted lines
> 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.
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  Documentation/fsck-msgids.adoc |  3 +++
>  Makefile                       |  1 +
>  fsck.h                         |  1 +
>  meson.build                    |  1 +
>  refs/reftable-backend.c        | 61 +++++++++++++++++++++++++++++++++++++-----
>  reftable/fsck.c                | 50 ++++++++++++++++++++++++++++++++++
>  reftable/reftable-fsck.h       | 38 ++++++++++++++++++++++++++
>  t/meson.build                  |  3 ++-
>  t/t0614-reftable-fsck.sh       | 35 ++++++++++++++++++++++++
>  9 files changed, 186 insertions(+), 7 deletions(-)
> 
> 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
Show 54 quoted lines
>  `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?

Show 29 quoted lines
>  #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?

> +				       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.
Show 32 quoted lines
> +	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);
> +}
> +
> +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?

Show 46 quoted lines
> +	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);
> +		if (!ret)
> +			return ret;
> +	}
> +
> +	return ret;
>  }
>  
>  struct ref_storage_be refs_be_reftable = {
> 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.

Show 11 quoted lines
> +		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.

I think we'd better set different messages for different problems.
Show 36 quoted lines
> +		}
> +
> +		if (strcmp(tail, ".ref")) {
> +			err = report_fn(info, cb_data);
> +		}
> +	}
> +
> +out:
> +	free_names(names);
> +	return err;
> +}
> diff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h
> new file mode 100644
> index 0000000000..087430d979
> --- /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,
> +};
> +
> +/* Represents an individual error encounctered during the FSCK checks. */
> +struct reftable_fsck_info {
> +	enum reftable_fsck_error error;
> +	const char *msg;
> +	const char *path;
> +};
> +
> +typedef int reftable_fsck_report_fn(struct reftable_fsck_info info,
> +				    void *cb_data);

As I have explained above, we should use `struct reftable_fsck_info *info` instead of `struct reftable_fsck_info info`.

Show 77 quoted lines
> +typedef void reftable_fsck_verbose_fn(const char *msg, void *cb_data);
> +
> +/*
> + * Given a reftable stack, perform FSCK check on the stack.
> + *
> + * 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
> + * conitnue. 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.
> + */
> +int reftable_fsck_check(struct reftable_stack *stack,
> +			reftable_fsck_report_fn report_fn,
> +			reftable_fsck_verbose_fn verbose_fn,
> +			void *cb_data);
> +
> +#endif /* REFTABLE_FSCK_H */
> diff --git a/t/meson.build b/t/meson.build
> index bbeba1a8d5..a8eb44eb30 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -145,6 +145,7 @@ integration_tests = [
>    't0611-reftable-httpd.sh',
>    't0612-reftable-jgit-compatibility.sh',
>    't0613-reftable-write-options.sh',
> +  't0614-reftable-fsck.sh',
>    't1000-read-tree-m-3way.sh',
>    't1001-read-tree-m-2way.sh',
>    't1002-read-tree-m-u-2way.sh',
> @@ -1214,4 +1215,4 @@ if perl.found() and time.found()
>        timeout: 0,
>      )
>    endforeach
> -endif
> \ No newline at end of file
> +endif
> 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.

Show 6 quoted lines
> +
> +test_done
> 
> -- 
> 2.50.1
>

Thanks, Jialuo

Previous: Karthik NayakNext: Karthik Nayak
Message 4 of 64 in “refs/reftable: add fsck checks”
  1. 0/5 refs/reftable: add fsck checksKarthik Nayak, Aug 19, 2025
  2. 1/5 fsck: order 'fsck_msg_type' alphabeticallyKarthik Nayak, Aug 19, 2025
  3. 2/5 refs/reftable: add fsck check for checking the table nameKarthik Nayak, Aug 19, 2025
  4. shejialuoAug 26, 2025
  5. Karthik NayakSep 1, 2025
  6. shejialuoSep 3, 2025
  7. 3/5 refs/reftable: add fsck check for number of tablesKarthik Nayak, Aug 19, 2025
  8. shejialuoAug 26, 2025
  9. Karthik NayakSep 1, 2025
  10. shejialuoAug 26, 2025
  11. Karthik NayakSep 1, 2025
  12. 4/5 refs/reftable: add fsck check for trailing newlineKarthik Nayak, Aug 19, 2025
  13. 5/5 refs/reftable: add fsck check for incorrect update indexKarthik Nayak, Aug 19, 2025
  14. shejialuoAug 26, 2025
  15. Karthik NayakSep 1, 2025
  16. 0/8 refs/reftable: add consistency checksKarthik Nayak, Sep 18, 2025
  17. 1/8 refs: remove unused headersKarthik Nayak, Sep 18, 2025
  18. 2/8 refs: move consistency check msg to generic layerKarthik Nayak, Sep 18, 2025
  19. 3/8 reftable: check for trailing newline in 'tables.list'Karthik Nayak, Sep 18, 2025
  20. Junio C HamanoSep 18, 2025
  21. Karthik NayakSep 23, 2025
  22. Patrick SteinhardtSep 24, 2025
  23. Karthik NayakSep 24, 2025
  24. Kristoffer HaugsbakkSep 24, 2025
  25. Karthik NayakSep 24, 2025
  26. 5/8 Documentation/fsck-msgids: remove duplicate msg idKarthik Nayak, Sep 18, 2025
  27. 4/8 reftable: ensure tables in a stack use sequential update indicesKarthik Nayak, Sep 18, 2025
  28. Patrick SteinhardtSep 24, 2025
  29. Karthik NayakSep 24, 2025
  30. Junio C HamanoSep 24, 2025
  31. Karthik NayakSep 24, 2025
  32. Patrick SteinhardtSep 25, 2025
  33. Junio C HamanoSep 25, 2025
  34. 6/8 fsck: order 'fsck_msg_type' alphabeticallyKarthik Nayak, Sep 18, 2025
  35. 7/8 reftable: add code to facilitate consistency checksKarthik Nayak, Sep 18, 2025
  36. Patrick SteinhardtSep 24, 2025
  37. Karthik NayakSep 24, 2025
  38. Patrick SteinhardtSep 25, 2025
  39. 8/8 refs/reftable: add fsck check for checking the table nameKarthik Nayak, Sep 18, 2025
  40. Patrick SteinhardtSep 24, 2025
  41. Karthik NayakSep 24, 2025
  42. 0/7 refs/reftable: add consistency checksKarthik Nayak, Oct 6, 2025
  43. 1/7 refs: remove unused headersKarthik Nayak, Oct 6, 2025
  44. 2/7 refs: move consistency check msg to generic layerKarthik Nayak, Oct 6, 2025
  45. 3/7 reftable: check for trailing newline in 'tables.list'Karthik Nayak, Oct 6, 2025
  46. 4/7 Documentation/fsck-msgids: remove duplicate msg idKarthik Nayak, Oct 6, 2025
  47. 5/7 fsck: order 'fsck_msg_type' alphabeticallyKarthik Nayak, Oct 6, 2025
  48. 6/7 reftable: add code to facilitate consistency checksKarthik Nayak, Oct 6, 2025
  49. 7/7 refs/reftable: add fsck check for checking the table nameKarthik Nayak, Oct 6, 2025
  50. Jeff KingOct 7, 2025
  51. Karthik NayakOct 7, 2025
  52. Junio C HamanoOct 6, 2025
  53. Karthik NayakOct 7, 2025
  54. Junio C HamanoOct 7, 2025
  55. 0/7 refs/reftable: add consistency checksKarthik Nayak, Oct 7, 2025
  56. 1/7 refs: remove unused headersKarthik Nayak, Oct 7, 2025
  57. 2/7 refs: move consistency check msg to generic layerKarthik Nayak, Oct 7, 2025
  58. 3/7 reftable: check for trailing newline in 'tables.list'Karthik Nayak, Oct 7, 2025
  59. 4/7 Documentation/fsck-msgids: remove duplicate msg idKarthik Nayak, Oct 7, 2025
  60. 5/7 fsck: order 'fsck_msg_type' alphabeticallyKarthik Nayak, Oct 7, 2025
  61. 6/7 reftable: add code to facilitate consistency checksKarthik Nayak, Oct 7, 2025
  62. 7/7 refs/reftable: add fsck check for checking the table nameKarthik Nayak, Oct 7, 2025
  63. Patrick SteinhardtOct 7, 2025
  64. Junio C HamanoOct 7, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.