# [PATCH v4 1/7] refs: remove unused headers

14 messages from 2025-09-26 to 2025-10-06. Participants: Karthik Nayak, Junio C Hamano, Patrick Steinhardt.
Thread: https://gitlist.dev/t/64212

## Karthik Nayak, 2025-09-26 07:25

Subject: [PATCH v4 1/7] refs: remove unused headers
Message-ID: <20250926-228-reftable-introduce-consistency-checks-v4-1-c96fd8551c0d@gmail.com>
URL: https://gitlist.dev/e/20250926-228-reftable-introduce-consistency-checks-v4-1-c96fd8551c0d%40gmail.com
In-Reply-To: <20250926-228-reftable-introduce-consistency-checks-v4-0-c96fd8551c0d@gmail.com>

```
In the 'refs/' namespace, some of the included header files are not
needed, let's remove them.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/debug.c            | 1 -
 refs/files-backend.c    | 1 -
 refs/reftable-backend.c | 1 -
 3 files changed, 3 deletions(-)

diff --git a/refs/debug.c b/refs/debug.c
index 1cb955961e..697adbd0dc 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -1,7 +1,6 @@
 #include "git-compat-util.h"
 #include "hex.h"
 #include "refs-internal.h"
-#include "string-list.h"
 #include "trace.h"
 
 static struct trace_key trace_refs = TRACE_KEY_INIT(REFS);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1b3bf26add..d4fb033417 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -20,7 +20,6 @@
 #include "../dir-iterator.h"
 #include "../lockfile.h"
 #include "../object.h"
-#include "../object-file.h"
 #include "../path.h"
 #include "../dir.h"
 #include "../chdir-notify.h"
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 9e889da2ff..2152349cb9 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -11,7 +11,6 @@
 #include "../hex.h"
 #include "../iterator.h"
 #include "../ident.h"
-#include "../lockfile.h"
 #include "../object.h"
 #include "../path.h"
 #include "../refs.h"

-- 
2.51.0


```

## Karthik Nayak, 2025-09-26 07:25

Subject: [PATCH v4 0/7] refs/reftable: add consistency checks
Message-ID: <20250926-228-reftable-introduce-consistency-checks-v4-0-c96fd8551c0d@gmail.com>
URL: https://gitlist.dev/e/20250926-228-reftable-introduce-consistency-checks-v4-0-c96fd8551c0d%40gmail.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 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                |   5 +-
 reftable/fsck.c                  | 100 +++++++++++++++++++++++++++++++++++++++
 reftable/reftable-fsck.h         |  40 ++++++++++++++++
 reftable/stack.c                 |   7 +--
 t/meson.build                    |   1 +
 t/t0614-reftable-fsck.sh         |  38 +++++++++++++++
 t/unit-tests/u-reftable-basics.c |  24 ++++++++--
 16 files changed, 308 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 v3:

1:  4522c10e6e = 1:  b91194e060 refs: remove unused headers
2:  40a83fc6fa = 2:  d48afbf588 refs: move consistency check  msg to generic layer
3:  df401e46f7 ! 3:  cd7ca2a585 reftable: check for trailing newline in 'tables.list'
    @@ Metadata
      ## Commit message ##
         reftable: check for trailing newline in 'tables.list'
     
    -    In the reftable format, the 'tables.list' file contains a newline
    -    separated list of tables. While we parse this file, we do not check or
    -    care about trailing newlines. Tighten the parser in `parse_names()` to
    -    return an appropriate error if there is no trailing newline.
    +    In the reftable format, the 'tables.list' file contains a
    +    newline separated list of tables. While we parse this file, we do not
    +    check or care about the last newline. Tighten the parser in
    +    `parse_names()` to return an appropriate error if the last newline is
    +    missing.
     
    -    This requires modification to `parse_names()` to accept a third argument
    -    which will hold the error value.
    +    This requires modification to `parse_names()` to now return the error
    +    while accepting the output as a third argument.
     
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
    @@ reftable/basics.c: size_t names_length(const char **names)
      }
      
     -char **parse_names(char *buf, int size)
    -+char **parse_names(char *buf, int size, int *err)
    ++int parse_names(char *buf, int size, char ***out)
      {
      	char **names = NULL;
      	size_t names_cap = 0;
    -@@ reftable/basics.c: char **parse_names(char *buf, int size)
    + 	size_t names_len = 0;
    + 	char *p = buf;
    + 	char *end = buf + size;
    ++	int err = 0;
      
      	while (p < end) {
      		char *next = strchr(p, '\n');
     -		if (next && next < end) {
    +-			*next = 0;
     +		if (!next) {
    -+			*err = REFTABLE_FORMAT_ERROR;
    ++			err = REFTABLE_FORMAT_ERROR;
     +			goto done;
     +		} else if (next < end) {
    - 			*next = 0;
    ++			*next = '\0';
      		} else {
      			next = end;
      		}
    @@ reftable/basics.c: char **parse_names(char *buf, int size)
     -						names_cap))
     -				goto err;
     +						names_cap)) {
    -+				*err = REFTABLE_OUT_OF_MEMORY_ERROR;
    ++				err = REFTABLE_OUT_OF_MEMORY_ERROR;
     +				goto done;
     +			}
      
    @@ reftable/basics.c: char **parse_names(char *buf, int size)
     -			if (!names[names_len++])
     -				goto err;
     +			if (!names[names_len++]) {
    -+				*err = REFTABLE_OUT_OF_MEMORY_ERROR;
    ++				err = REFTABLE_OUT_OF_MEMORY_ERROR;
     +				goto done;
     +			}
      		}
    @@ reftable/basics.c: char **parse_names(char *buf, int size)
     -	if (REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap))
     -		goto err;
     +	if (REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap)) {
    -+		*err = REFTABLE_OUT_OF_MEMORY_ERROR;
    ++		err = REFTABLE_OUT_OF_MEMORY_ERROR;
     +		goto done;
     +	}
      	names[names_len] = NULL;
      
    - 	return names;
    - 
    +-	return names;
    +-
     -err:
    ++	*out = names;
    ++	return 0;
     +done:
      	for (size_t i = 0; i < names_len; i++)
      		reftable_free(names[i]);
      	reftable_free(names);
    +-	return NULL;
    ++	return err;
    + }
    + 
    + int names_equal(const char **a, const char **b)
     
      ## reftable/basics.h ##
     @@ reftable/basics.h: void free_names(char **a);
    @@ 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.
    -+ *
    -+ * Errors are assigned to the `err` variable.
       */
     -char **parse_names(char *buf, int size);
    -+char **parse_names(char *buf, int size, int *err);
    ++int parse_names(char *buf, int size, char ***out);
      
      /* compares two NULL-terminated arrays of strings. */
      int names_equal(const char **a, const char **b);
    @@ reftable/stack.c: static int fd_read_lines(int fd, char ***namesp)
     -	*namesp = parse_names(buf, size);
     -	if (!*namesp) {
     -		err = REFTABLE_OUT_OF_MEMORY_ERROR;
    -+	*namesp = parse_names(buf, size, &err);
    -+	if (!*namesp)
    - 		goto done;
    +-		goto done;
     -	}
    - 
    +-
    ++	err = parse_names(buf, size, namesp);
      done:
      	reftable_free(buf);
    + 	return err;
     
      ## t/unit-tests/u-reftable-basics.c ##
     @@ t/unit-tests/u-reftable-basics.c: license that can be found in the LICENSE file or at
    @@ t/unit-tests/u-reftable-basics.c: void test_reftable_basics__names_equal(void)
     -	char in2[] = "a\nb\nc";
     -	char **out = parse_names(in1, strlen(in1));
     +	char in2[] = "a\nb\nc\n";
    -+	int err = 0;
    -+	char **out = parse_names(in1, strlen(in1), &err);
    ++	char **out = NULL;
    ++	int err = parse_names(in1, strlen(in1), &out);
     +	cl_assert(err == 0);
      	cl_assert(out != NULL);
      	cl_assert_equal_s(out[0], "line");
    @@ t/unit-tests/u-reftable-basics.c: void test_reftable_basics__names_equal(void)
      	free_names(out);
      
     -	out = parse_names(in2, strlen(in2));
    -+	out = parse_names(in2, strlen(in2), &err);
    ++	out = NULL;
    ++	err = parse_names(in2, strlen(in2), &out);
     +	cl_assert(err == 0);
      	cl_assert(out != NULL);
      	cl_assert_equal_s(out[0], "a");
    @@ t/unit-tests/u-reftable-basics.c: void test_reftable_basics__parse_names(void)
     +void test_reftable_basics__parse_names_missing_newline(void)
     +{
     +	char in1[] = "line\nline2";
    -+	int err = 0;
    -+	char **out = parse_names(in1, strlen(in1), &err);
    ++	char **out = NULL;
    ++	int err = parse_names(in1, strlen(in1), &out);
     +	cl_assert(err == REFTABLE_FORMAT_ERROR);
     +	cl_assert(out == NULL);
     +}
    @@ t/unit-tests/u-reftable-basics.c: void test_reftable_basics__parse_names(void)
      {
      	char in[] = "a\n\nb\n";
     -	char **out = parse_names(in, strlen(in));
    -+	int err = 0;
    -+	char **out = parse_names(in, strlen(in), &err);
    -+	cl_assert(err ==  0);
    ++	char **out = NULL;
    ++	int err = parse_names(in, strlen(in), &out);
    ++	cl_assert(err == 0);
      	cl_assert(out != NULL);
      	cl_assert_equal_s(out[0], "a");
      	/* simply '\n' should be dropped as empty string */
4:  435707f26c < -:  ---------- reftable: ensure tables in a stack use sequential update indices
5:  ac6275ab87 = 4:  e3e0c0b4ae Documentation/fsck-msgids: remove duplicate msg id
6:  6c02925af1 = 5:  24a8d93adc fsck: order 'fsck_msg_type' alphabetically
7:  1ada7bc89c ! 6:  d83d763be1 reftable: add code to facilitate consistency checks
    @@ Commit message
         stack. The callee provides the function with callbacks to handle issue
         and information reporting.
     
    -    The added check, goes over all files in the reftable directory and
    -    validates that they have the expected file type and a valid name. It
    -    raises specific errors for both.
    +    The added check, goes over all tables in the reftable stack validates
    +    that they have a valid name. It not, it raises an error.
     
         While here, move 'reftable/error.o' in the Makefile to retain
         lexicographic ordering.
    @@ reftable/fsck.c (new)
     @@
     +#include "basics.h"
     +#include "reftable-fsck.h"
    ++#include "reftable-table.h"
     +#include "stack.h"
     +
    -+static bool valid_table_name(const char *name, uint64_t *min_update_index,
    -+			     uint64_t *max_update_index)
    ++static bool table_has_valid_name(const char *name)
     +{
     +	const char *ptr = name;
     +	char *endptr;
    @@ reftable/fsck.c (new)
     +	/* strtoull doesn't set errno on success */
     +	errno = 0;
     +
    -+	*min_update_index = strtoull(ptr, &endptr, 16);
    -+	if (errno == EINVAL)
    ++	strtoull(ptr, &endptr, 16);
    ++	if (errno)
     +		return false;
     +	ptr = endptr;
     +
    -+	if (strncmp(ptr, "-", 1))
    ++	if (*ptr != '-')
     +		return false;
     +	ptr++;
     +
    -+	*max_update_index = strtoull(ptr, &endptr, 16);
    -+	if (errno == EINVAL)
    ++	strtoull(ptr, &endptr, 16);
    ++	if (errno)
     +		return false;
     +	ptr = endptr;
     +
    @@ reftable/fsck.c (new)
     +	ptr++;
     +
     +	strtoul(ptr, &endptr, 16);
    -+	if (errno == EINVAL)
    ++	if (errno)
     +		return false;
     +	ptr = endptr;
     +
    @@ reftable/fsck.c (new)
     +	return true;
     +}
     +
    -+static int stack_check_all_files_in_dir(struct reftable_stack *stack,
    -+					reftable_fsck_report_fn report_fn,
    -+					void *cb_data)
    ++typedef int (*table_check_fn)(struct reftable_table *table,
    ++			      reftable_fsck_report_fn report_fn,
    ++			      void *cb_data);
    ++
    ++static int table_check_name(struct reftable_table *table,
    ++			    reftable_fsck_report_fn report_fn,
    ++			    void *cb_data)
     +{
    -+	DIR *dir = opendir(stack->reftable_dir);
    -+	struct reftable_fsck_info info;
    -+	struct dirent *d = NULL;
    -+	uint64_t min, max;
    -+	int err = 0;
    ++	if (!table_has_valid_name(table->name)) {
    ++		struct reftable_fsck_info info;
    ++
    ++		info.error = REFTABLE_FSCK_ERROR_TABLE_NAME;
    ++		info.msg = "invalid reftable table name";
    ++		info.path = table->name;
     +
    -+	if (!dir)
    -+		return 0;
    -+
    -+	while ((d = readdir(dir))) {
    -+		if (!strcmp(d->d_name, "tables.list"))
    -+			continue;
    -+
    -+		if ((d->d_name[0] == '.' &&
    -+		     (d->d_name[1] == '\0' ||
    -+		      (d->d_name[1] == '.' && d->d_name[2] == '\0'))))
    -+			continue;
    -+
    -+		if (d->d_type == DT_REG) {
    -+			if (!valid_table_name(d->d_name, &min, &max)) {
    -+				info.error = REFTABLE_FSCK_ERROR_TABLE_NAME;
    -+				info.msg = "file with invalid table name";
    -+				info.path = d->d_name;
    -+
    -+				err |= report_fn(&info, cb_data);
    -+			}
    -+		} else {
    -+			info.error = REFTABLE_FSCK_ERROR_INVALID_FILE_TYPE;
    -+			info.msg = "file with unexpected type";
    -+			info.path = d->d_name;
    -+
    -+			err |= report_fn(&info, cb_data);
    -+		}
    ++		return report_fn(&info, cb_data);
     +	}
     +
    -+	closedir(dir);
    -+	return err;
    ++	return 0;
     +}
     +
    -+static int stack_checks(struct reftable_stack *stack,
    ++static int table_checks(struct reftable_table *table,
     +			reftable_fsck_report_fn report_fn,
    ++			reftable_fsck_verbose_fn verbose_fn UNUSED,
     +			void *cb_data)
     +{
    -+	struct reftable_buf msg = REFTABLE_BUF_INIT;
    -+	char **names = NULL;
    ++	table_check_fn table_check_fns[] = {
    ++		table_check_name,
    ++		NULL,
    ++	};
     +	int err = 0;
     +
    -+	if (stack == NULL)
    -+		goto out;
    ++	for (size_t i = 0; table_check_fns[i]; i++)
    ++		err |= table_check_fns[i](table, report_fn, cb_data);
     +
    -+	err |= stack_check_all_files_in_dir(stack, report_fn, cb_data);
    -+
    -+out:
    -+	free_names(names);
    -+	reftable_buf_release(&msg);
     +	return err;
     +}
     +
    @@ reftable/fsck.c (new)
     +			reftable_fsck_verbose_fn verbose_fn,
     +			void *cb_data)
     +{
    -+	verbose_fn("Checking reftable: stack checks", cb_data);
    -+	return stack_checks(stack, report_fn, cb_data);
    ++	struct reftable_buf msg = REFTABLE_BUF_INIT;
    ++	int err = 0;
    ++
    ++	for (size_t i = 0; i < stack->tables_len; i++) {
    ++		reftable_buf_reset(&msg);
    ++		reftable_buf_addstr(&msg, "Checking table: ");
    ++		reftable_buf_addstr(&msg, stack->tables[i]->name);
    ++		verbose_fn(msg.buf, cb_data);
    ++
    ++		err |= table_checks(stack->tables[i], report_fn, verbose_fn, cb_data);
    ++	}
    ++
    ++	reftable_buf_release(&msg);
    ++	return err;
     +}
     
      ## reftable/reftable-fsck.h (new) ##
    @@ reftable/reftable-fsck.h (new)
     +#include "reftable-stack.h"
     +
     +enum reftable_fsck_error {
    -+	/* Non regular file in the reftable directory */
    -+	REFTABLE_FSCK_ERROR_INVALID_FILE_TYPE = 0,
     +	/* Invalid table name */
    -+	REFTABLE_FSCK_ERROR_TABLE_NAME,
    ++	REFTABLE_FSCK_ERROR_TABLE_NAME = 0,
     +	/* Used for bounds checking, must be last */
    -+	REFTABLE_FSCK_MAX_VALUE
    ++	REFTABLE_FSCK_MAX_VALUE,
     +};
     +
     +/* Represents an individual error encountered during the FSCK checks. */
8:  77be84e23f ! 7:  c49b7887d8 refs/reftable: add fsck check for checking the table name
    @@ Commit message
           ${min_update_index}-${max_update_index}-${random}.ref as a naming
           convention.
     
    -    So treat non-conformant file names as warnings. Introduce another check
    -    to check for file types, non-expected filetypes will be treated as
    -    errors.
    +    So treat non-conformant file names as warnings.
    +
    +    While adding the fsck header to 'refs/reftable-backend.c', modify the
    +    list to maintain lexicographical ordering.
     
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
    @@ Documentation/fsck-msgids.adoc
      `badReferentName`::
      	(ERROR) The referent name of a symref is invalid.
      
    -+`badReftableFiletype`::
    -+	(ERROR) File with unexpected type in reftable directory.
    -+
     +`badReftableTableName`::
     +	(WARN) A reftable table has an invalid name.
     +
    @@ Documentation/fsck-msgids.adoc
      
     
      ## fsck.h ##
    -@@ fsck.h: enum fsck_msg_type {
    - 	FUNC(BAD_PACKED_REF_HEADER, ERROR) \
    - 	FUNC(BAD_PARENT_SHA1, ERROR) \
    - 	FUNC(BAD_REFERENT_NAME, ERROR) \
    -+	FUNC(BAD_REFTABLE_FILETYPE, ERROR) \
    - 	FUNC(BAD_REF_CONTENT, ERROR) \
    - 	FUNC(BAD_REF_FILETYPE, ERROR) \
    - 	FUNC(BAD_REF_NAME, ERROR) \
     @@ fsck.h: enum fsck_msg_type {
      	FUNC(UNKNOWN_TYPE, ERROR) \
      	FUNC(ZERO_PADDED_DATE, ERROR) \
    @@ refs/reftable-backend.c: static int reftable_be_reflog_expire(struct ref_store *
     +}
     +
     +static const enum fsck_msg_id fsck_msg_id_map[] = {
    -+	[REFTABLE_FSCK_ERROR_INVALID_FILE_TYPE] = FSCK_MSG_BAD_REFTABLE_FILETYPE,
     +	[REFTABLE_FSCK_ERROR_TABLE_NAME] = FSCK_MSG_BAD_REFTABLE_TABLE_NAME,
     +};
     +
    @@ refs/reftable-backend.c: static int reftable_be_reflog_expire(struct ref_store *
     +	enum fsck_msg_id msg_id;
     +
     +	if (info->error < 0 || info->error >= REFTABLE_FSCK_MAX_VALUE)
    -+		BUG("unknown fsck error: %d", info->error);
    ++		BUG("unknown fsck error: %d", (int)info->error);
     +
     +	msg_id = fsck_msg_id_map[info->error];
     +
     +	if (!msg_id)
    -+		BUG("fsck_msg_id value missing for reftable error: %d", info->error);
    ++		BUG("fsck_msg_id value missing for reftable error: %d", (int)info->error);
     +
     +	return fsck_report_ref(o, &report, msg_id, "%s", info->msg);
     +}
    @@ t/t0614-reftable-fsck.sh (new)
     +			git refs verify 2>err &&
     +			test_must_be_empty err &&
     +
    -+			touch ".git/reftable/$TABLE_NAME" &&
    ++			EXISTING_TABLE=$(head -n1 .git/reftable/tables.list) &&
    ++			mv ".git/reftable/$EXISTING_TABLE" ".git/reftable/$TABLE_NAME" &&
    ++			sed "s/${EXISTING_TABLE}/${TABLE_NAME}/g" .git/reftable/tables.list > tables.list &&
    ++			mv tables.list .git/reftable/tables.list &&
     +
     +			git refs verify 2>err &&
     +			cat >expect <<-EOF &&
    -+			warning: ${TABLE_NAME}: badReftableTableName: file with invalid table name
    ++			warning: ${TABLE_NAME}: badReftableTableName: invalid reftable table name
     +			EOF
     +			test_cmp expect err
     +		)
     +	'
     +done
     +
    -+test_expect_success "invalid file type 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 &&
    -+
    -+		mkdir ".git/reftable/foo" &&
    -+
    -+		test_must_fail git refs verify 2>err &&
    -+		cat >expect <<-EOF &&
    -+		error: foo: badReftableFiletype: file with unexpected type
    -+		EOF
    -+		test_cmp expect err
    -+	)
    -+'
    -+
     +test_done


base-commit: a483264b01b977f3e65a4419103c21e6af7412a2
change-id: 20250714-228-reftable-introduce-consistency-checks-379ded93c544

Thanks
- Karthik


```

## Karthik Nayak, 2025-09-26 07:25

Subject: [PATCH v4 2/7] refs: move consistency check msg to generic layer
Message-ID: <20250926-228-reftable-introduce-consistency-checks-v4-2-c96fd8551c0d@gmail.com>
URL: https://gitlist.dev/e/20250926-228-reftable-introduce-consistency-checks-v4-2-c96fd8551c0d%40gmail.com
In-Reply-To: <20250926-228-reftable-introduce-consistency-checks-v4-0-c96fd8551c0d@gmail.com>

```
The files-backend prints a message before the consistency checks run.
Move this to the generic layer so both the files and reftable backend
can benefit from this message.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c               | 4 ++++
 refs/files-backend.c | 2 --
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 4ff55cf24f..4a7c394226 100644
--- a/refs.c
+++ b/refs.c
@@ -32,6 +32,7 @@
 #include "commit.h"
 #include "wildmatch.h"
 #include "ident.h"
+#include "fsck.h"
 
 /*
  * List of all available backends
@@ -323,6 +324,9 @@ int check_refname_format(const char *refname, int flags)
 int refs_fsck(struct ref_store *refs, struct fsck_options *o,
 	      struct worktree *wt)
 {
+	if (o->verbose)
+		fprintf_ln(stderr, _("Checking references consistency"));
+
 	return refs->be->fsck(refs, o, wt);
 }
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index d4fb033417..603b1343d8 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3906,8 +3906,6 @@ static int files_fsck_refs(struct ref_store *ref_store,
 		NULL,
 	};
 
-	if (o->verbose)
-		fprintf_ln(stderr, _("Checking references consistency"));
 	return files_fsck_refs_dir(ref_store, o, "refs", wt, fsck_refs_fn);
 }
 

-- 
2.51.0


```

## Karthik Nayak, 2025-09-26 07:25

Subject: [PATCH v4 3/7] reftable: check for trailing newline in 'tables.list'
Message-ID: <20250926-228-reftable-introduce-consistency-checks-v4-3-c96fd8551c0d@gmail.com>
URL: https://gitlist.dev/e/20250926-228-reftable-introduce-consistency-checks-v4-3-c96fd8551c0d%40gmail.com
In-Reply-To: <20250926-228-reftable-introduce-consistency-checks-v4-0-c96fd8551c0d@gmail.com>

```
In the reftable format, the 'tables.list' file contains a
newline separated list of tables. While we parse this file, we do not
check or care about the last newline. Tighten the parser in
`parse_names()` to return an appropriate error if the last newline is
missing.

This requires modification to `parse_names()` to now return the error
while accepting the output as a third argument.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 reftable/basics.c                | 37 ++++++++++++++++++++++++-------------
 reftable/basics.h                |  5 ++---
 reftable/stack.c                 |  7 +------
 t/unit-tests/u-reftable-basics.c | 24 ++++++++++++++++++++----
 4 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/reftable/basics.c b/reftable/basics.c
index 9988ebd635..e969927b61 100644
--- a/reftable/basics.c
+++ b/reftable/basics.c
@@ -195,44 +195,55 @@ size_t names_length(const char **names)
 	return p - names;
 }
 
-char **parse_names(char *buf, int size)
+int parse_names(char *buf, int size, char ***out)
 {
 	char **names = NULL;
 	size_t names_cap = 0;
 	size_t names_len = 0;
 	char *p = buf;
 	char *end = buf + size;
+	int err = 0;
 
 	while (p < end) {
 		char *next = strchr(p, '\n');
-		if (next && next < end) {
-			*next = 0;
+		if (!next) {
+			err = REFTABLE_FORMAT_ERROR;
+			goto done;
+		} else if (next < end) {
+			*next = '\0';
 		} else {
 			next = end;
 		}
+
 		if (p < next) {
 			if (REFTABLE_ALLOC_GROW(names, names_len + 1,
-						names_cap))
-				goto err;
+						names_cap)) {
+				err = REFTABLE_OUT_OF_MEMORY_ERROR;
+				goto done;
+			}
 
 			names[names_len] = reftable_strdup(p);
-			if (!names[names_len++])
-				goto err;
+			if (!names[names_len++]) {
+				err = REFTABLE_OUT_OF_MEMORY_ERROR;
+				goto done;
+			}
 		}
 		p = next + 1;
 	}
 
-	if (REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap))
-		goto err;
+	if (REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap)) {
+		err = REFTABLE_OUT_OF_MEMORY_ERROR;
+		goto done;
+	}
 	names[names_len] = NULL;
 
-	return names;
-
-err:
+	*out = names;
+	return 0;
+done:
 	for (size_t i = 0; i < names_len; i++)
 		reftable_free(names[i]);
 	reftable_free(names);
-	return NULL;
+	return err;
 }
 
 int names_equal(const char **a, const char **b)
diff --git a/reftable/basics.h b/reftable/basics.h
index 7d22f96261..693db9524f 100644
--- a/reftable/basics.h
+++ b/reftable/basics.h
@@ -167,10 +167,9 @@ void free_names(char **a);
 
 /*
  * Parse a newline separated list of names. `size` is the length of the buffer,
- * without terminating '\0'. Empty names are discarded. Returns a `NULL`
- * pointer when allocations fail.
+ * without terminating '\0'. Empty names are discarded.
  */
-char **parse_names(char *buf, int size);
+int parse_names(char *buf, int size, char ***out);
 
 /* compares two NULL-terminated arrays of strings. */
 int names_equal(const char **a, const char **b);
diff --git a/reftable/stack.c b/reftable/stack.c
index f91ce50bcd..65d89820bd 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -109,12 +109,7 @@ static int fd_read_lines(int fd, char ***namesp)
 	}
 	buf[size] = 0;
 
-	*namesp = parse_names(buf, size);
-	if (!*namesp) {
-		err = REFTABLE_OUT_OF_MEMORY_ERROR;
-		goto done;
-	}
-
+	err = parse_names(buf, size, namesp);
 done:
 	reftable_free(buf);
 	return err;
diff --git a/t/unit-tests/u-reftable-basics.c b/t/unit-tests/u-reftable-basics.c
index a0471083e7..73566ed0eb 100644
--- a/t/unit-tests/u-reftable-basics.c
+++ b/t/unit-tests/u-reftable-basics.c
@@ -9,6 +9,7 @@ license that can be found in the LICENSE file or at
 #include "unit-test.h"
 #include "lib-reftable.h"
 #include "reftable/basics.h"
+#include "reftable/reftable-error.h"
 
 struct integer_needle_lesseq_args {
 	int needle;
@@ -79,14 +80,18 @@ void test_reftable_basics__names_equal(void)
 void test_reftable_basics__parse_names(void)
 {
 	char in1[] = "line\n";
-	char in2[] = "a\nb\nc";
-	char **out = parse_names(in1, strlen(in1));
+	char in2[] = "a\nb\nc\n";
+	char **out = NULL;
+	int err = parse_names(in1, strlen(in1), &out);
+	cl_assert(err == 0);
 	cl_assert(out != NULL);
 	cl_assert_equal_s(out[0], "line");
 	cl_assert(!out[1]);
 	free_names(out);
 
-	out = parse_names(in2, strlen(in2));
+	out = NULL;
+	err = parse_names(in2, strlen(in2), &out);
+	cl_assert(err == 0);
 	cl_assert(out != NULL);
 	cl_assert_equal_s(out[0], "a");
 	cl_assert_equal_s(out[1], "b");
@@ -95,10 +100,21 @@ void test_reftable_basics__parse_names(void)
 	free_names(out);
 }
 
+void test_reftable_basics__parse_names_missing_newline(void)
+{
+	char in1[] = "line\nline2";
+	char **out = NULL;
+	int err = parse_names(in1, strlen(in1), &out);
+	cl_assert(err == REFTABLE_FORMAT_ERROR);
+	cl_assert(out == NULL);
+}
+
 void test_reftable_basics__parse_names_drop_empty_string(void)
 {
 	char in[] = "a\n\nb\n";
-	char **out = parse_names(in, strlen(in));
+	char **out = NULL;
+	int err = parse_names(in, strlen(in), &out);
+	cl_assert(err == 0);
 	cl_assert(out != NULL);
 	cl_assert_equal_s(out[0], "a");
 	/* simply '\n' should be dropped as empty string */

-- 
2.51.0


```

## Karthik Nayak, 2025-09-26 07:25

Subject: [PATCH v4 4/7] Documentation/fsck-msgids: remove duplicate msg id
Message-ID: <20250926-228-reftable-introduce-consistency-checks-v4-4-c96fd8551c0d@gmail.com>
URL: https://gitlist.dev/e/20250926-228-reftable-introduce-consistency-checks-v4-4-c96fd8551c0d%40gmail.com
In-Reply-To: <20250926-228-reftable-introduce-consistency-checks-v4-0-c96fd8551c0d@gmail.com>

```
The `gitmodulesLarge` is repeated twice. Remove the second duplicate.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/fsck-msgids.adoc | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc
index 0ba4f9a27e..1c912615f9 100644
--- a/Documentation/fsck-msgids.adoc
+++ b/Documentation/fsck-msgids.adoc
@@ -104,9 +104,6 @@
 `gitmodulesParse`::
 	(INFO) Could not parse `.gitmodules` blob.
 
-`gitmodulesLarge`;
-	(ERROR) `.gitmodules` blob is too large to parse.
-
 `gitmodulesPath`::
 	(ERROR) `.gitmodules` path is invalid.
 

-- 
2.51.0


```

## Karthik Nayak, 2025-09-26 07:25

Subject: [PATCH v4 5/7] fsck: order 'fsck_msg_type' alphabetically
Message-ID: <20250926-228-reftable-introduce-consistency-checks-v4-5-c96fd8551c0d@gmail.com>
URL: https://gitlist.dev/e/20250926-228-reftable-introduce-consistency-checks-v4-5-c96fd8551c0d%40gmail.com
In-Reply-To: <20250926-228-reftable-introduce-consistency-checks-v4-0-c96fd8551c0d@gmail.com>

```
The list of 'fsck_msg_type' seem to be alphabetically ordered, but there
are a few small misses. Fix this by sorting the sub-sections of the
list to maintain alphabetical ordering.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 fsck.h | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/fsck.h b/fsck.h
index dd7df3d5b3..6b0db235e0 100644
--- a/fsck.h
+++ b/fsck.h
@@ -33,15 +33,27 @@ enum fsck_msg_type {
 	FUNC(BAD_PACKED_REF_ENTRY, ERROR) \
 	FUNC(BAD_PACKED_REF_HEADER, ERROR) \
 	FUNC(BAD_PARENT_SHA1, ERROR) \
+	FUNC(BAD_REFERENT_NAME, ERROR) \
 	FUNC(BAD_REF_CONTENT, ERROR) \
 	FUNC(BAD_REF_FILETYPE, ERROR) \
 	FUNC(BAD_REF_NAME, ERROR) \
-	FUNC(BAD_REFERENT_NAME, ERROR) \
 	FUNC(BAD_TIMEZONE, ERROR) \
 	FUNC(BAD_TREE, ERROR) \
 	FUNC(BAD_TREE_SHA1, ERROR) \
 	FUNC(BAD_TYPE, ERROR) \
 	FUNC(DUPLICATE_ENTRIES, ERROR) \
+	FUNC(GITATTRIBUTES_BLOB, ERROR) \
+	FUNC(GITATTRIBUTES_LARGE, ERROR) \
+	FUNC(GITATTRIBUTES_LINE_LENGTH, ERROR) \
+	FUNC(GITATTRIBUTES_MISSING, ERROR) \
+	FUNC(GITMODULES_BLOB, ERROR) \
+	FUNC(GITMODULES_LARGE, ERROR) \
+	FUNC(GITMODULES_MISSING, ERROR) \
+	FUNC(GITMODULES_NAME, ERROR) \
+	FUNC(GITMODULES_PATH, ERROR) \
+	FUNC(GITMODULES_SYMLINK, ERROR) \
+	FUNC(GITMODULES_UPDATE, ERROR) \
+	FUNC(GITMODULES_URL, ERROR) \
 	FUNC(MISSING_AUTHOR, ERROR) \
 	FUNC(MISSING_COMMITTER, ERROR) \
 	FUNC(MISSING_EMAIL, ERROR) \
@@ -60,39 +72,27 @@ enum fsck_msg_type {
 	FUNC(TREE_NOT_SORTED, ERROR) \
 	FUNC(UNKNOWN_TYPE, ERROR) \
 	FUNC(ZERO_PADDED_DATE, ERROR) \
-	FUNC(GITMODULES_MISSING, ERROR) \
-	FUNC(GITMODULES_BLOB, ERROR) \
-	FUNC(GITMODULES_LARGE, ERROR) \
-	FUNC(GITMODULES_NAME, ERROR) \
-	FUNC(GITMODULES_SYMLINK, ERROR) \
-	FUNC(GITMODULES_URL, ERROR) \
-	FUNC(GITMODULES_PATH, ERROR) \
-	FUNC(GITMODULES_UPDATE, ERROR) \
-	FUNC(GITATTRIBUTES_MISSING, ERROR) \
-	FUNC(GITATTRIBUTES_LARGE, ERROR) \
-	FUNC(GITATTRIBUTES_LINE_LENGTH, ERROR) \
-	FUNC(GITATTRIBUTES_BLOB, ERROR) \
 	/* warnings */ \
 	FUNC(EMPTY_NAME, WARN) \
 	FUNC(FULL_PATHNAME, WARN) \
 	FUNC(HAS_DOT, WARN) \
 	FUNC(HAS_DOTDOT, WARN) \
 	FUNC(HAS_DOTGIT, WARN) \
+	FUNC(LARGE_PATHNAME, WARN) \
 	FUNC(NULL_SHA1, WARN) \
-	FUNC(ZERO_PADDED_FILEMODE, WARN) \
 	FUNC(NUL_IN_COMMIT, WARN) \
-	FUNC(LARGE_PATHNAME, WARN) \
+	FUNC(ZERO_PADDED_FILEMODE, WARN) \
 	/* infos (reported as warnings, but ignored by default) */ \
 	FUNC(BAD_FILEMODE, INFO) \
+	FUNC(BAD_TAG_NAME, INFO) \
 	FUNC(EMPTY_PACKED_REFS_FILE, INFO) \
-	FUNC(GITMODULES_PARSE, INFO) \
-	FUNC(GITIGNORE_SYMLINK, INFO) \
 	FUNC(GITATTRIBUTES_SYMLINK, INFO) \
+	FUNC(GITIGNORE_SYMLINK, INFO) \
+	FUNC(GITMODULES_PARSE, INFO) \
 	FUNC(MAILMAP_SYMLINK, INFO) \
-	FUNC(BAD_TAG_NAME, INFO) \
 	FUNC(MISSING_TAGGER_ENTRY, INFO) \
-	FUNC(SYMLINK_REF, INFO) \
 	FUNC(REF_MISSING_NEWLINE, INFO) \
+	FUNC(SYMLINK_REF, INFO) \
 	FUNC(SYMREF_TARGET_IS_NOT_A_REF, INFO) \
 	FUNC(TRAILING_REF_CONTENT, INFO) \
 	/* ignored (elevated when requested) */ \

-- 
2.51.0


```

## Karthik Nayak, 2025-09-26 07:25

Subject: [PATCH v4 6/7] reftable: add code to facilitate consistency checks
Message-ID: <20250926-228-reftable-introduce-consistency-checks-v4-6-c96fd8551c0d@gmail.com>
URL: https://gitlist.dev/e/20250926-228-reftable-introduce-consistency-checks-v4-6-c96fd8551c0d%40gmail.com
In-Reply-To: <20250926-228-reftable-introduce-consistency-checks-v4-0-c96fd8551c0d@gmail.com>

```
The `git refs verify` command is used to run consistency 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 files present in the reftable directory.

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.

The added check, goes over all tables in the reftable stack validates
that they have a valid name. It not, it raises an error.

While here, move 'reftable/error.o' in the Makefile to retain
lexicographic ordering.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Makefile                 |   3 +-
 meson.build              |   1 +
 reftable/fsck.c          | 100 +++++++++++++++++++++++++++++++++++++++++++++++
 reftable/reftable-fsck.h |  40 +++++++++++++++++++
 4 files changed, 143 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 4c95affadb..03fbaf2b21 100644
--- a/Makefile
+++ b/Makefile
@@ -2732,9 +2732,10 @@ XDIFF_OBJS += xdiff/xutils.o
 xdiff-objs: $(XDIFF_OBJS)
 
 REFTABLE_OBJS += reftable/basics.o
-REFTABLE_OBJS += reftable/error.o
 REFTABLE_OBJS += reftable/block.o
 REFTABLE_OBJS += reftable/blocksource.o
+REFTABLE_OBJS += reftable/error.o
+REFTABLE_OBJS += reftable/fsck.o
 REFTABLE_OBJS += reftable/iter.o
 REFTABLE_OBJS += reftable/merged.o
 REFTABLE_OBJS += reftable/pq.o
diff --git a/meson.build b/meson.build
index b3dfcc0497..8914252910 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/reftable/fsck.c b/reftable/fsck.c
new file mode 100644
index 0000000000..26b9115b14
--- /dev/null
+++ b/reftable/fsck.c
@@ -0,0 +1,100 @@
+#include "basics.h"
+#include "reftable-fsck.h"
+#include "reftable-table.h"
+#include "stack.h"
+
+static bool table_has_valid_name(const char *name)
+{
+	const char *ptr = name;
+	char *endptr;
+
+	/* strtoull doesn't set errno on success */
+	errno = 0;
+
+	strtoull(ptr, &endptr, 16);
+	if (errno)
+		return false;
+	ptr = endptr;
+
+	if (*ptr != '-')
+		return false;
+	ptr++;
+
+	strtoull(ptr, &endptr, 16);
+	if (errno)
+		return false;
+	ptr = endptr;
+
+	if (*ptr != '-')
+		return false;
+	ptr++;
+
+	strtoul(ptr, &endptr, 16);
+	if (errno)
+		return false;
+	ptr = endptr;
+
+	if (strcmp(ptr, ".ref") && strcmp(ptr, ".log"))
+		return false;
+
+	return true;
+}
+
+typedef int (*table_check_fn)(struct reftable_table *table,
+			      reftable_fsck_report_fn report_fn,
+			      void *cb_data);
+
+static int table_check_name(struct reftable_table *table,
+			    reftable_fsck_report_fn report_fn,
+			    void *cb_data)
+{
+	if (!table_has_valid_name(table->name)) {
+		struct reftable_fsck_info info;
+
+		info.error = REFTABLE_FSCK_ERROR_TABLE_NAME;
+		info.msg = "invalid reftable table name";
+		info.path = table->name;
+
+		return report_fn(&info, cb_data);
+	}
+
+	return 0;
+}
+
+static int table_checks(struct reftable_table *table,
+			reftable_fsck_report_fn report_fn,
+			reftable_fsck_verbose_fn verbose_fn UNUSED,
+			void *cb_data)
+{
+	table_check_fn table_check_fns[] = {
+		table_check_name,
+		NULL,
+	};
+	int err = 0;
+
+	for (size_t i = 0; table_check_fns[i]; i++)
+		err |= table_check_fns[i](table, report_fn, cb_data);
+
+	return err;
+}
+
+int reftable_fsck_check(struct reftable_stack *stack,
+			reftable_fsck_report_fn report_fn,
+			reftable_fsck_verbose_fn verbose_fn,
+			void *cb_data)
+{
+	struct reftable_buf msg = REFTABLE_BUF_INIT;
+	int err = 0;
+
+	for (size_t i = 0; i < stack->tables_len; i++) {
+		reftable_buf_reset(&msg);
+		reftable_buf_addstr(&msg, "Checking table: ");
+		reftable_buf_addstr(&msg, stack->tables[i]->name);
+		verbose_fn(msg.buf, cb_data);
+
+		err |= table_checks(stack->tables[i], report_fn, verbose_fn, cb_data);
+	}
+
+	reftable_buf_release(&msg);
+	return err;
+}
diff --git a/reftable/reftable-fsck.h b/reftable/reftable-fsck.h
new file mode 100644
index 0000000000..007a392cf9
--- /dev/null
+++ b/reftable/reftable-fsck.h
@@ -0,0 +1,40 @@
+#ifndef REFTABLE_FSCK_H
+#define REFTABLE_FSCK_H
+
+#include "reftable-stack.h"
+
+enum reftable_fsck_error {
+	/* Invalid table name */
+	REFTABLE_FSCK_ERROR_TABLE_NAME = 0,
+	/* Used for bounds checking, must be last */
+	REFTABLE_FSCK_MAX_VALUE,
+};
+
+/* Represents an individual error encountered 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);
+typedef void reftable_fsck_verbose_fn(const char *msg, void *cb_data);
+
+/*
+ * Given a reftable stack, perform consistency checks 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
+ * 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 consistency 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 */

-- 
2.51.0


```

## Karthik Nayak, 2025-09-26 07:25

Subject: [PATCH v4 7/7] refs/reftable: add fsck check for checking the table name
Message-ID: <20250926-228-reftable-introduce-consistency-checks-v4-7-c96fd8551c0d@gmail.com>
URL: https://gitlist.dev/e/20250926-228-reftable-introduce-consistency-checks-v4-7-c96fd8551c0d%40gmail.com
In-Reply-To: <20250926-228-reftable-introduce-consistency-checks-v4-0-c96fd8551c0d@gmail.com>

```
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. The
reftable specification mentions:

  It suggested to use
  ${min_update_index}-${max_update_index}-${random}.ref as a naming
  convention.

So treat non-conformant file names as warnings.

While adding the fsck header to 'refs/reftable-backend.c', modify the
list to maintain lexicographical ordering.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/fsck-msgids.adoc |  3 +++
 fsck.h                         |  1 +
 refs/reftable-backend.c        | 57 ++++++++++++++++++++++++++++++++++++++----
 t/meson.build                  |  1 +
 t/t0614-reftable-fsck.sh       | 38 ++++++++++++++++++++++++++++
 5 files changed, 95 insertions(+), 5 deletions(-)

diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc
index 1c912615f9..81f11ba125 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`::
+	(WARN) A reftable table has an invalid name.
+
 `badTagName`::
 	(INFO) A tag has an invalid format.
 
diff --git a/fsck.h b/fsck.h
index 6b0db235e0..759df97655 100644
--- a/fsck.h
+++ b/fsck.h
@@ -73,6 +73,7 @@ enum fsck_msg_type {
 	FUNC(UNKNOWN_TYPE, ERROR) \
 	FUNC(ZERO_PADDED_DATE, ERROR) \
 	/* warnings */ \
+	FUNC(BAD_REFTABLE_TABLE_NAME, WARN) \
 	FUNC(EMPTY_NAME, WARN) \
 	FUNC(FULL_PATHNAME, WARN) \
 	FUNC(HAS_DOT, WARN) \
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 2152349cb9..b106fd8b53 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -6,6 +6,7 @@
 #include "../config.h"
 #include "../dir.h"
 #include "../environment.h"
+#include "../fsck.h"
 #include "../gettext.h"
 #include "../hash.h"
 #include "../hex.h"
@@ -15,10 +16,11 @@
 #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"
@@ -2707,11 +2709,56 @@ 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 const enum fsck_msg_id fsck_msg_id_map[] = {
+	[REFTABLE_FSCK_ERROR_TABLE_NAME] = FSCK_MSG_BAD_REFTABLE_TABLE_NAME,
+};
+
+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;
+
+	if (info->error < 0 || info->error >= REFTABLE_FSCK_MAX_VALUE)
+		BUG("unknown fsck error: %d", (int)info->error);
+
+	msg_id = fsck_msg_id_map[info->error];
+
+	if (!msg_id)
+		BUG("fsck_msg_id value missing for reftable error: %d", (int)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");
+
+	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 = {
diff --git a/t/meson.build b/t/meson.build
index 7974795fe4..ec1fc0b2a1 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -146,6 +146,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',
diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh
new file mode 100755
index 0000000000..250d244e66
--- /dev/null
+++ b/t/t0614-reftable-fsck.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='Test reftable backend consistency check'
+
+GIT_TEST_DEFAULT_REF_FORMAT=reftable
+export GIT_TEST_DEFAULT_REF_FORMAT
+
+. ./test-lib.sh
+
+for TABLE_NAME in "foo-bar-e4d12d59.ref" \
+	"0x00000000zzzz-0x00000000zzzz-e4d12d59.ref" \
+	"0x000000000001-0x000000000002-e4d12d59.abc" \
+	"0x000000000001-0x000000000002-e4d12d59.refabc"; do
+	test_expect_success "table name $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 &&
+
+			EXISTING_TABLE=$(head -n1 .git/reftable/tables.list) &&
+			mv ".git/reftable/$EXISTING_TABLE" ".git/reftable/$TABLE_NAME" &&
+			sed "s/${EXISTING_TABLE}/${TABLE_NAME}/g" .git/reftable/tables.list > tables.list &&
+			mv tables.list .git/reftable/tables.list &&
+
+			git refs verify 2>err &&
+			cat >expect <<-EOF &&
+			warning: ${TABLE_NAME}: badReftableTableName: invalid reftable table name
+			EOF
+			test_cmp expect err
+		)
+	'
+done
+
+test_done

-- 
2.51.0


```

## Junio C Hamano, 2025-09-26 21:08

Subject: Re: [PATCH v4 0/7] refs/reftable: add consistency checks
Message-ID: <xmqq1pnsc46y.fsf@gitster.g>
URL: https://gitlist.dev/e/xmqq1pnsc46y.fsf%40gitster.g
In-Reply-To: <20250926-228-reftable-introduce-consistency-checks-v4-0-c96fd8551c0d@gmail.com>

```
Karthik Nayak <karthik.188@gmail.com> writes:

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

Nice.

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

Great.  Reading over patches and noticing that it is not needed is
the best kind of proofreading ;-)

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

Wonderful.

Will queue.  Thanks.

```

## Patrick Steinhardt, 2025-10-02 11:44

Subject: Re: [PATCH v4 3/7] reftable: check for trailing newline in 'tables.list'
Message-ID: <aN5lmEx7Q57YEyHH@pks.im>
URL: https://gitlist.dev/e/aN5lmEx7Q57YEyHH%40pks.im
In-Reply-To: <20250926-228-reftable-introduce-consistency-checks-v4-3-c96fd8551c0d@gmail.com>

```
On Fri, Sep 26, 2025 at 09:25:46AM +0200, Karthik Nayak wrote:
> diff --git a/reftable/basics.c b/reftable/basics.c
> index 9988ebd635..e969927b61 100644
> --- a/reftable/basics.c
> +++ b/reftable/basics.c
> @@ -195,44 +195,55 @@ size_t names_length(const char **names)
>  	return p - names;
>  }
>  
> -char **parse_names(char *buf, int size)
> +int parse_names(char *buf, int size, char ***out)
>  {

Yup, this changed function signature reads a lot nicer to me now and is
more in line with our usual coding style.

> diff --git a/reftable/basics.h b/reftable/basics.h
> index 7d22f96261..693db9524f 100644
> --- a/reftable/basics.h
> +++ b/reftable/basics.h
> @@ -167,10 +167,9 @@ void free_names(char **a);
>  
>  /*
>   * Parse a newline separated list of names. `size` is the length of the buffer,
> - * without terminating '\0'. Empty names are discarded. Returns a `NULL`
> - * pointer when allocations fail.
> + * without terminating '\0'. Empty names are discarded.
>   */
> -char **parse_names(char *buf, int size);
> +int parse_names(char *buf, int size, char ***out);

Tiny nit, not worth a reroll: we may still want to document that a
return value of 0 means success, and that it otherwise returns a
reftable error code.

Patrick

```

## Patrick Steinhardt, 2025-10-02 11:44

Subject: Re: [PATCH v4 6/7] reftable: add code to facilitate consistency checks
Message-ID: <aN5lngvE39JGwGSx@pks.im>
URL: https://gitlist.dev/e/aN5lngvE39JGwGSx%40pks.im
In-Reply-To: <20250926-228-reftable-introduce-consistency-checks-v4-6-c96fd8551c0d@gmail.com>

```
On Fri, Sep 26, 2025 at 09:25:49AM +0200, Karthik Nayak wrote:
> diff --git a/reftable/fsck.c b/reftable/fsck.c
> new file mode 100644
> index 0000000000..26b9115b14
> --- /dev/null
> +++ b/reftable/fsck.c
> @@ -0,0 +1,100 @@
[snip]
> +static int table_checks(struct reftable_table *table,
> +			reftable_fsck_report_fn report_fn,
> +			reftable_fsck_verbose_fn verbose_fn UNUSED,
> +			void *cb_data)
> +{
> +	table_check_fn table_check_fns[] = {
> +		table_check_name,
> +		NULL,
> +	};
> +	int err = 0;
> +
> +	for (size_t i = 0; table_check_fns[i]; i++)
> +		err |= table_check_fns[i](table, report_fn, cb_data);
> +
> +	return err;
> +}

Okay, good. We now only verify individual table names part of the stack,
and don't scan the directory anymore. Furthermore, it is easy to add
more tests by adding to the function array.

Patrick

```

## Patrick Steinhardt, 2025-10-02 11:44

Subject: Re: [PATCH v4 7/7] refs/reftable: add fsck check for checking the table name
Message-ID: <aN5lrhCLQFnw7qUB@pks.im>
URL: https://gitlist.dev/e/aN5lrhCLQFnw7qUB%40pks.im
In-Reply-To: <20250926-228-reftable-introduce-consistency-checks-v4-7-c96fd8551c0d@gmail.com>

```
On Fri, Sep 26, 2025 at 09:25:50AM +0200, Karthik Nayak wrote:
> diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh
> new file mode 100755
> index 0000000000..250d244e66
> --- /dev/null
> +++ b/t/t0614-reftable-fsck.sh
> @@ -0,0 +1,38 @@
> +#!/bin/sh
> +
> +test_description='Test reftable backend consistency check'
> +
> +GIT_TEST_DEFAULT_REF_FORMAT=reftable
> +export GIT_TEST_DEFAULT_REF_FORMAT
> +
> +. ./test-lib.sh
> +
> +for TABLE_NAME in "foo-bar-e4d12d59.ref" \
> +	"0x00000000zzzz-0x00000000zzzz-e4d12d59.ref" \
> +	"0x000000000001-0x000000000002-e4d12d59.abc" \
> +	"0x000000000001-0x000000000002-e4d12d59.refabc"; do
> +	test_expect_success "table name $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 &&
> +
> +			EXISTING_TABLE=$(head -n1 .git/reftable/tables.list) &&
> +			mv ".git/reftable/$EXISTING_TABLE" ".git/reftable/$TABLE_NAME" &&
> +			sed "s/${EXISTING_TABLE}/${TABLE_NAME}/g" .git/reftable/tables.list > tables.list &&
> +			mv tables.list .git/reftable/tables.list &&
> +
> +			git refs verify 2>err &&
> +			cat >expect <<-EOF &&
> +			warning: ${TABLE_NAME}: badReftableTableName: invalid reftable table name
> +			EOF
> +			test_cmp expect err
> +		)
> +	'
> +done
> +
> +test_done

Nit: we don't have any test that verifies that `git refs verify` doesn't
complain with a well-formed stack.

Other than that this series looks good to me, thanks! I think we might
want to have one final reroll, but once that's out I think this should
be ready to be merged down.

Patrick

```

## Karthik Nayak, 2025-10-06 12:02

Subject: Re: [PATCH v4 3/7] reftable: check for trailing newline in 'tables.list'
Message-ID: <CAOLa=ZT48BV5Xs8R0Mwi1tpMHKC1jrR8AGScHHAq37xU4_UpOw@mail.gmail.com>
URL: https://gitlist.dev/e/CAOLa%3DZT48BV5Xs8R0Mwi1tpMHKC1jrR8AGScHHAq37xU4_UpOw%40mail.gmail.com
In-Reply-To: <aN5lmEx7Q57YEyHH@pks.im>

```
Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Sep 26, 2025 at 09:25:46AM +0200, Karthik Nayak wrote:
>> diff --git a/reftable/basics.c b/reftable/basics.c
>> index 9988ebd635..e969927b61 100644
>> --- a/reftable/basics.c
>> +++ b/reftable/basics.c
>> @@ -195,44 +195,55 @@ size_t names_length(const char **names)
>>  	return p - names;
>>  }
>>
>> -char **parse_names(char *buf, int size)
>> +int parse_names(char *buf, int size, char ***out)
>>  {
>
> Yup, this changed function signature reads a lot nicer to me now and is
> more in line with our usual coding style.
>

I have to agree with that. It would be so much nicer, if we could return
(value, err) in C. Anyways this seems a lot more consistent.

>> diff --git a/reftable/basics.h b/reftable/basics.h
>> index 7d22f96261..693db9524f 100644
>> --- a/reftable/basics.h
>> +++ b/reftable/basics.h
>> @@ -167,10 +167,9 @@ void free_names(char **a);
>>
>>  /*
>>   * Parse a newline separated list of names. `size` is the length of the buffer,
>> - * without terminating '\0'. Empty names are discarded. Returns a `NULL`
>> - * pointer when allocations fail.
>> + * without terminating '\0'. Empty names are discarded.
>>   */
>> -char **parse_names(char *buf, int size);
>> +int parse_names(char *buf, int size, char ***out);
>
> Tiny nit, not worth a reroll: we may still want to document that a
> return value of 0 means success, and that it otherwise returns a
> reftable error code.
>

Yes, I'll add that in my re-roll.

> Patrick

```

## Karthik Nayak, 2025-10-06 12:05

Subject: Re: [PATCH v4 7/7] refs/reftable: add fsck check for checking the table name
Message-ID: <CAOLa=ZRU01MQCLwWx4zEZNRtqNBvUJnMo8euE+J86q2a9adSwA@mail.gmail.com>
URL: https://gitlist.dev/e/CAOLa%3DZRU01MQCLwWx4zEZNRtqNBvUJnMo8euE%2BJ86q2a9adSwA%40mail.gmail.com
In-Reply-To: <aN5lrhCLQFnw7qUB@pks.im>

```
Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Sep 26, 2025 at 09:25:50AM +0200, Karthik Nayak wrote:
>> diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh
>> new file mode 100755
>> index 0000000000..250d244e66
>> --- /dev/null
>> +++ b/t/t0614-reftable-fsck.sh
>> @@ -0,0 +1,38 @@
>> +#!/bin/sh
>> +
>> +test_description='Test reftable backend consistency check'
>> +
>> +GIT_TEST_DEFAULT_REF_FORMAT=reftable
>> +export GIT_TEST_DEFAULT_REF_FORMAT
>> +
>> +. ./test-lib.sh
>> +
>> +for TABLE_NAME in "foo-bar-e4d12d59.ref" \
>> +	"0x00000000zzzz-0x00000000zzzz-e4d12d59.ref" \
>> +	"0x000000000001-0x000000000002-e4d12d59.abc" \
>> +	"0x000000000001-0x000000000002-e4d12d59.refabc"; do
>> +	test_expect_success "table name $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 &&
>> +
>> +			EXISTING_TABLE=$(head -n1 .git/reftable/tables.list) &&
>> +			mv ".git/reftable/$EXISTING_TABLE" ".git/reftable/$TABLE_NAME" &&
>> +			sed "s/${EXISTING_TABLE}/${TABLE_NAME}/g" .git/reftable/tables.list > tables.list &&
>> +			mv tables.list .git/reftable/tables.list &&
>> +
>> +			git refs verify 2>err &&
>> +			cat >expect <<-EOF &&
>> +			warning: ${TABLE_NAME}: badReftableTableName: invalid reftable table name
>> +			EOF
>> +			test_cmp expect err
>> +		)
>> +	'
>> +done
>> +
>> +test_done
>
> Nit: we don't have any test that verifies that `git refs verify` doesn't
> complain with a well-formed stack.

The above test does run `git refs verify` on the repository before
modifying the 'tables.list' file. Do you mean a stack with > 1 tables? I
think that would be worthwhile. Let me do that.

>
> Other than that this series looks good to me, thanks! I think we might
> want to have one final reroll, but once that's out I think this should
> be ready to be merged down.
>
> Patrick

Really appreciate the quick and thorough reviews.

Thanks,
Karthik

```