[PATCH 00/17] Fixes and improvements for ref consistency checks

[PATCH 00/17] Fixes and improvements for ref consistency checks

[Patrick Steinhardt]

Hi,

this patch series contains a bunch of fixes and improvements for ref
consistency checks. It is structured as follows:

  - Patches 1 to 4 contain a couple of cleanups for the consistency
    checks done by the "files" backend.

  - Patches 5 to 7 introduce checks for root refs for the "files"
    backend.

  - Patches 9 to 14 introduce infrastructure for shared checks with the
    "files" and "reftable" backend.

  - Patches 15 to 17 move some ref consistency checks that were still
    driven by git-fsck(1) into `git refs verify`.

Thanks!

Patrick

---
Patrick Steinhardt (17):
      refs/files: simplify iterating through root refs
      refs/files: move fsck functions into global scope
      refs/files: remove `refs_check_dir` parameter
      refs/files: remove useless indirection
      refs/files: extract function to check single ref
      refs/files: improve error handling when verifying symrefs
      refs/files: perform consistency checks for root refs
      fsck: drop unused fields from `struct fsck_ref_report`
      refs/files: extract generic symref target checks
      refs/files: introduce function to perform normal ref checks
      refs/reftable: adapt includes to become consistent
      refs/reftable: extract function to retrieve backend for worktree
      refs/reftable: fix consistency checks with worktrees
      refs/reftable: introduce generic checks for refs
      builtin/fsck: move generic object ID checks into `refs_fsck()`
      builtin/fsck: move generic HEAD check into `refs_fsck()`
      builtin/fsck: drop `fsck_head_link()`

 Documentation/fsck-msgids.adoc |   6 ++
 builtin/fsck.c                 |  46 +--------
 fsck.c                         |   5 -
 fsck.h                         |   4 +-
 refs.c                         |  43 ++++++++
 refs.h                         |  18 ++++
 refs/files-backend.c           | 230 ++++++++++++++++++++++++-----------------
 refs/reftable-backend.c        | 167 ++++++++++++++++++++++--------
 t/t0602-reffiles-fsck.sh       |  30 ++++++
 t/t0614-reftable-fsck.sh       |  44 ++++++++
 t/t1450-fsck.sh                |  10 +-
 11 files changed, 416 insertions(+), 187 deletions(-)


---
base-commit: d529f3a197364881746f558e5652f0236131eb86
change-id: 20260109-pks-refs-verify-fixes-1e47872317cf

[PATCH 01/17] refs/files: simplify iterating through root refs

[Patrick Steinhardt]

When iterating through root refs we first need to determine the
directory in which the refs live. This is done by retrieving the root of
the loose refs via `refs->loose->root->name`, and putting it through
`files_ref_path()` to derive the final path.

This is somewhat redundant though: the root name of the loose files
cache is always going to be the empty string. As such, we always end up
passing that empty string to `files_ref_path()` as the ref hierarchy we
want to start. And this actually makes sense: `files_ref_path()` already
computes the location of the root directory, so of course we need to
pass the empty string for the ref hierarchy itself. So going via the
loose ref cache to figure out that the root of a ref hierarchy is empty
is only causing confusion.

But next to the added confusion, it can also lead to a segfault. The
loose ref cache is populated lazily, so it may not always be set. It
seems to be sheer luck that this is a condition we do not currently hit.
The right thing to do would be to call `get_loose_ref_cache()`, which
knows to populate the cache if required.

Simplify the code and fix the potential segfault by simply removing the
indirection via the loose ref cache completely.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6f6f76a8d8..297739f203 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -354,13 +354,11 @@ static int for_each_root_ref(struct files_ref_store *refs,
 			     void *cb_data)
 {
 	struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT;
-	const char *dirname = refs->loose->root->name;
 	struct dirent *de;
-	size_t dirnamelen;
 	int ret;
 	DIR *d;
 
-	files_ref_path(refs, &path, dirname);
+	files_ref_path(refs, &path, "");
 
 	d = opendir(path.buf);
 	if (!d) {
@@ -368,9 +366,6 @@ static int for_each_root_ref(struct files_ref_store *refs,
 		return -1;
 	}
 
-	strbuf_addstr(&refname, dirname);
-	dirnamelen = refname.len;
-
 	while ((de = readdir(d)) != NULL) {
 		unsigned char dtype;
 
@@ -378,6 +373,8 @@ static int for_each_root_ref(struct files_ref_store *refs,
 			continue;
 		if (ends_with(de->d_name, ".lock"))
 			continue;
+
+		strbuf_reset(&refname);
 		strbuf_addstr(&refname, de->d_name);
 
 		dtype = get_dtype(de, &path, 1);
@@ -386,8 +383,6 @@ static int for_each_root_ref(struct files_ref_store *refs,
 			if (ret)
 				goto done;
 		}
-
-		strbuf_setlen(&refname, dirnamelen);
 	}
 
 	ret = 0;

-- 
2.52.0.542.g9473a8513b.dirty

Re: [PATCH 01/17] refs/files: simplify iterating through root refs

[shejialuo]

On Fri, Jan 09, 2026 at 01:39:30PM +0100, Patrick Steinhardt wrote:
> When iterating through root refs we first need to determine the
> directory in which the refs live. This is done by retrieving the root of
> the loose refs via `refs->loose->root->name`, and putting it through
> `files_ref_path()` to derive the final path.
> 
> This is somewhat redundant though: the root name of the loose files
> cache is always going to be the empty string. As such, we always end up
> passing that empty string to `files_ref_path()` as the ref hierarchy we
> want to start. And this actually makes sense: `files_ref_path()` already
> computes the location of the root directory, so of course we need to
> pass the empty string for the ref hierarchy itself. So going via the
> loose ref cache to figure out that the root of a ref hierarchy is empty
> is only causing confusion.

Make sense, in `refs/ref-cache.c` we would call the following to create
the root loose cache:

    ret->root = create_dir_entry(ret, "", 0)

It would always be empty.

Thanks,
Jialuo

[PATCH 02/17] refs/files: move fsck functions into global scope

[Patrick Steinhardt]

When performing consistency checks we pass the functions that perform
the verification down the calling stack. This is somewhat unnecessary
though, as the set of functions doesn't ever change.

Simplify the code by moving the array into global scope and remove the
parameter.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 297739f203..feba3ee58b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3890,11 +3890,16 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
 	return ret;
 }
 
+static const files_fsck_refs_fn fsck_refs_fn[]= {
+	files_fsck_refs_name,
+	files_fsck_refs_content,
+	NULL,
+};
+
 static int files_fsck_refs_dir(struct ref_store *ref_store,
 			       struct fsck_options *o,
 			       const char *refs_check_dir,
-			       struct worktree *wt,
-			       files_fsck_refs_fn *fsck_refs_fn)
+			       struct worktree *wt)
 {
 	struct strbuf refname = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
@@ -3955,13 +3960,7 @@ static int files_fsck_refs(struct ref_store *ref_store,
 			   struct fsck_options *o,
 			   struct worktree *wt)
 {
-	files_fsck_refs_fn fsck_refs_fn[]= {
-		files_fsck_refs_name,
-		files_fsck_refs_content,
-		NULL,
-	};
-
-	return files_fsck_refs_dir(ref_store, o, "refs", wt, fsck_refs_fn);
+	return files_fsck_refs_dir(ref_store, o, "refs", wt);
 }
 
 static int files_fsck(struct ref_store *ref_store,

-- 
2.52.0.542.g9473a8513b.dirty

[PATCH 03/17] refs/files: remove `refs_check_dir` parameter

[Patrick Steinhardt]

The parameter `refs_check_dir` determines which directory we want to
check references for. But as we always want to check the complete
refs hierarchy, this parameter is always set to "refs".

Drop the parameter and hardcode it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index feba3ee58b..0a104c7bf6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3898,7 +3898,6 @@ static const files_fsck_refs_fn fsck_refs_fn[]= {
 
 static int files_fsck_refs_dir(struct ref_store *ref_store,
 			       struct fsck_options *o,
-			       const char *refs_check_dir,
 			       struct worktree *wt)
 {
 	struct strbuf refname = STRBUF_INIT;
@@ -3907,7 +3906,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
 	int iter_status;
 	int ret = 0;
 
-	strbuf_addf(&sb, "%s/%s", ref_store->gitdir, refs_check_dir);
+	strbuf_addf(&sb, "%s/refs", ref_store->gitdir);
 
 	iter = dir_iterator_begin(sb.buf, 0);
 	if (!iter) {
@@ -3927,8 +3926,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
 
 			if (!is_main_worktree(wt))
 				strbuf_addf(&refname, "worktrees/%s/", wt->id);
-			strbuf_addf(&refname, "%s/%s", refs_check_dir,
-				    iter->relative_path);
+			strbuf_addf(&refname, "refs/%s", iter->relative_path);
 
 			if (o->verbose)
 				fprintf_ln(stderr, "Checking %s", refname.buf);
@@ -3960,7 +3958,7 @@ static int files_fsck_refs(struct ref_store *ref_store,
 			   struct fsck_options *o,
 			   struct worktree *wt)
 {
-	return files_fsck_refs_dir(ref_store, o, "refs", wt);
+	return files_fsck_refs_dir(ref_store, o, wt);
 }
 
 static int files_fsck(struct ref_store *ref_store,

-- 
2.52.0.542.g9473a8513b.dirty

[PATCH 04/17] refs/files: remove useless indirection

[Patrick Steinhardt]

The function `files_fsck_refs()` only has a single callsite and forwards
all of its arguments as-is, so it's basically a useless indirection.
Inline the function call.

While at it, also remove the bitwise or that we have for return values.
We don't really want to or them at all, but rather just want to return
an error in case either of the functions has failed.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0a104c7bf6..4cbee23dad 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3954,22 +3954,20 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
 	return ret;
 }
 
-static int files_fsck_refs(struct ref_store *ref_store,
-			   struct fsck_options *o,
-			   struct worktree *wt)
-{
-	return files_fsck_refs_dir(ref_store, o, wt);
-}
-
 static int files_fsck(struct ref_store *ref_store,
 		      struct fsck_options *o,
 		      struct worktree *wt)
 {
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_READ, "fsck");
+	int ret = 0;
 
-	return files_fsck_refs(ref_store, o, wt) |
-	       refs->packed_ref_store->be->fsck(refs->packed_ref_store, o, wt);
+	if (files_fsck_refs_dir(ref_store, o, wt) < 0)
+		ret = -1;
+	if (refs->packed_ref_store->be->fsck(refs->packed_ref_store, o, wt) < 0)
+		ret = -1;
+
+	return ret;
 }
 
 struct ref_storage_be refs_be_files = {

-- 
2.52.0.542.g9473a8513b.dirty

[PATCH 05/17] refs/files: extract function to check single ref

[Patrick Steinhardt]

When checking the consistency of references we create a directory
iterator and then verify each single reference in a loop. The logic to
perform the actual checks is embedded into that loop, which makes it
hard to reuse. But In a subsequent commit we're about to introduce a
second path that wants to verify references.

Prepare for this by extracting the logic to check a single reference
into a standalone function.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 80 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 29 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4cbee23dad..9972221f9f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3715,7 +3715,8 @@ static int files_ref_store_remove_on_disk(struct ref_store *ref_store,
 typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
 				  struct fsck_options *o,
 				  const char *refname,
-				  struct dir_iterator *iter);
+				  const char *path,
+				  int mode);
 
 static int files_fsck_symref_target(struct fsck_options *o,
 				    struct fsck_ref_report *report,
@@ -3772,7 +3773,8 @@ static int files_fsck_symref_target(struct fsck_options *o,
 static int files_fsck_refs_content(struct ref_store *ref_store,
 				   struct fsck_options *o,
 				   const char *target_name,
-				   struct dir_iterator *iter)
+				   const char *path,
+				   int mode)
 {
 	struct strbuf ref_content = STRBUF_INIT;
 	struct strbuf abs_gitdir = STRBUF_INIT;
@@ -3786,7 +3788,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 
 	report.path = target_name;
 
-	if (S_ISLNK(iter->st.st_mode)) {
+	if (S_ISLNK(mode)) {
 		const char *relative_referent_path = NULL;
 
 		ret = fsck_report_ref(o, &report,
@@ -3798,7 +3800,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 		if (!is_dir_sep(abs_gitdir.buf[abs_gitdir.len - 1]))
 			strbuf_addch(&abs_gitdir, '/');
 
-		strbuf_add_real_path(&ref_content, iter->path.buf);
+		strbuf_add_real_path(&ref_content, path);
 		skip_prefix(ref_content.buf, abs_gitdir.buf,
 			    &relative_referent_path);
 
@@ -3811,7 +3813,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 		goto cleanup;
 	}
 
-	if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) {
+	if (strbuf_read_file(&ref_content, path, 0) < 0) {
 		/*
 		 * Ref file could be removed by another concurrent process. We should
 		 * ignore this error and continue to the next ref.
@@ -3819,7 +3821,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 		if (errno == ENOENT)
 			goto cleanup;
 
-		ret = error_errno(_("cannot read ref file '%s'"), iter->path.buf);
+		ret = error_errno(_("cannot read ref file '%s'"), path);
 		goto cleanup;
 	}
 
@@ -3861,16 +3863,20 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
 				struct fsck_options *o,
 				const char *refname,
-				struct dir_iterator *iter)
+				const char *path,
+				int mode UNUSED)
 {
 	struct strbuf sb = STRBUF_INIT;
+	const char *filename;
 	int ret = 0;
 
+	filename = basename((char *) path);
+
 	/*
 	 * Ignore the files ending with ".lock" as they may be lock files
 	 * However, do not allow bare ".lock" files.
 	 */
-	if (iter->basename[0] != '.' && ends_with(iter->basename, ".lock"))
+	if (filename[0] != '.' && ends_with(filename, ".lock"))
 		goto cleanup;
 
 	/*
@@ -3896,6 +3902,35 @@ static const files_fsck_refs_fn fsck_refs_fn[]= {
 	NULL,
 };
 
+static int files_fsck_ref(struct ref_store *ref_store,
+			  struct fsck_options *o,
+			  const char *refname,
+			  const char *path,
+			  int mode)
+{
+	int ret = 0;
+
+	if (o->verbose)
+		fprintf_ln(stderr, "Checking %s", refname);
+
+	if (!S_ISREG(mode) && !S_ISLNK(mode)) {
+		struct fsck_ref_report report = { .path = refname };
+
+		if (fsck_report_ref(o, &report,
+				    FSCK_MSG_BAD_REF_FILETYPE,
+				    "unexpected file type"))
+			ret = -1;
+		goto out;
+	}
+
+	for (size_t i = 0; fsck_refs_fn[i]; i++)
+		if (fsck_refs_fn[i](ref_store, o, refname, path, mode))
+			ret = -1;
+
+out:
+	return ret;
+}
+
 static int files_fsck_refs_dir(struct ref_store *ref_store,
 			       struct fsck_options *o,
 			       struct worktree *wt)
@@ -3918,30 +3953,17 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
 	}
 
 	while ((iter_status = dir_iterator_advance(iter)) == ITER_OK) {
-		if (S_ISDIR(iter->st.st_mode)) {
+		if (S_ISDIR(iter->st.st_mode))
 			continue;
-		} else if (S_ISREG(iter->st.st_mode) ||
-			   S_ISLNK(iter->st.st_mode)) {
-			strbuf_reset(&refname);
-
-			if (!is_main_worktree(wt))
-				strbuf_addf(&refname, "worktrees/%s/", wt->id);
-			strbuf_addf(&refname, "refs/%s", iter->relative_path);
 
-			if (o->verbose)
-				fprintf_ln(stderr, "Checking %s", refname.buf);
+		strbuf_reset(&refname);
+		if (!is_main_worktree(wt))
+			strbuf_addf(&refname, "worktrees/%s/", wt->id);
+		strbuf_addf(&refname, "refs/%s", iter->relative_path);
 
-			for (size_t i = 0; fsck_refs_fn[i]; i++) {
-				if (fsck_refs_fn[i](ref_store, o, refname.buf, iter))
-					ret = -1;
-			}
-		} else {
-			struct fsck_ref_report report = { .path = iter->basename };
-			if (fsck_report_ref(o, &report,
-					    FSCK_MSG_BAD_REF_FILETYPE,
-					    "unexpected file type"))
-				ret = -1;
-		}
+		if (files_fsck_ref(ref_store, o, refname.buf,
+				   iter->path.buf, iter->st.st_mode) < 0)
+			ret = -1;
 	}
 
 	if (iter_status != ITER_DONE)

-- 
2.52.0.542.g9473a8513b.dirty

[PATCH 06/17] refs/files: improve error handling when verifying symrefs

[Patrick Steinhardt]

The error handling when verifying symbolic refs is a bit on the wild
side:

  - `fsck_report_ref()` can be told to ignore specific errors. If an
    error has been ignored and a previous check raised an unignored
    error, then assigning `ret = fsck_report_ref()` will cause us to
    swallow the previous error.

  - When the target reference is not valid we bail out early without
    checking for other errors.

Fix both of these issues by consistently or'ing the return value and not
bailing out early.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 9972221f9f..abc2165339 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3737,17 +3737,15 @@ static int files_fsck_symref_target(struct fsck_options *o,
 	if (!is_referent_root &&
 	    !starts_with(referent->buf, "refs/") &&
 	    !starts_with(referent->buf, "worktrees/")) {
-		ret = fsck_report_ref(o, report,
-				      FSCK_MSG_SYMREF_TARGET_IS_NOT_A_REF,
-				      "points to non-ref target '%s'", referent->buf);
-
+		ret |= fsck_report_ref(o, report,
+				       FSCK_MSG_SYMREF_TARGET_IS_NOT_A_REF,
+				       "points to non-ref target '%s'", referent->buf);
 	}
 
 	if (!is_referent_root && check_refname_format(referent->buf, 0)) {
-		ret = fsck_report_ref(o, report,
-				      FSCK_MSG_BAD_REFERENT_NAME,
-				      "points to invalid refname '%s'", referent->buf);
-		goto out;
+		ret |= fsck_report_ref(o, report,
+				       FSCK_MSG_BAD_REFERENT_NAME,
+				       "points to invalid refname '%s'", referent->buf);
 	}
 
 	if (symbolic_link)
@@ -3755,19 +3753,19 @@ static int files_fsck_symref_target(struct fsck_options *o,
 
 	if (referent->len == orig_len ||
 	    (referent->len < orig_len && orig_last_byte != '\n')) {
-		ret = fsck_report_ref(o, report,
-				      FSCK_MSG_REF_MISSING_NEWLINE,
-				      "misses LF at the end");
+		ret |= fsck_report_ref(o, report,
+				       FSCK_MSG_REF_MISSING_NEWLINE,
+				       "misses LF at the end");
 	}
 
 	if (referent->len != orig_len && referent->len != orig_len - 1) {
-		ret = fsck_report_ref(o, report,
-				      FSCK_MSG_TRAILING_REF_CONTENT,
-				      "has trailing whitespaces or newlines");
+		ret |= fsck_report_ref(o, report,
+				       FSCK_MSG_TRAILING_REF_CONTENT,
+				       "has trailing whitespaces or newlines");
 	}
 
 out:
-	return ret;
+	return ret ? -1 : 0;
 }
 
 static int files_fsck_refs_content(struct ref_store *ref_store,

-- 
2.52.0.542.g9473a8513b.dirty

Re: [PATCH 06/17] refs/files: improve error handling when verifying symrefs

[shejialuo]

On Fri, Jan 09, 2026 at 01:39:35PM +0100, Patrick Steinhardt wrote:
> The error handling when verifying symbolic refs is a bit on the wild
> side:
> 
>   - `fsck_report_ref()` can be told to ignore specific errors. If an
>     error has been ignored and a previous check raised an unignored
>     error, then assigning `ret = fsck_report_ref()` will cause us to
>     swallow the previous error.
> 

Make sense, I think I haven't thought about this carefully when I wrote
the code. I totally ignored the case. Thanks for catching this.

[PATCH 07/17] refs/files: perform consistency checks for root refs

[Patrick Steinhardt]

While the "files" backend already knows to perform consistency checks
for the "refs/" hierarchy, it doesn't verify any of its root refs. Plug
this omission.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c     | 52 +++++++++++++++++++++++++++++++++++++++++++++---
 t/t0602-reffiles-fsck.sh | 30 ++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index abc2165339..0ff047d0df 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3877,9 +3877,9 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
 	if (filename[0] != '.' && ends_with(filename, ".lock"))
 		goto cleanup;
 
-	/*
-	 * This works right now because we never check the root refs.
-	 */
+	if (is_root_ref(refname))
+		goto cleanup;
+
 	if (check_refname_format(refname, 0)) {
 		struct fsck_ref_report report = { 0 };
 
@@ -3974,19 +3974,65 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
 	return ret;
 }
 
+struct files_fsck_root_ref_data {
+	struct files_ref_store *refs;
+	struct fsck_options *o;
+	struct worktree *wt;
+	struct strbuf refname;
+	struct strbuf path;
+	bool errors_found;
+};
+
+static int files_fsck_root_ref(const char *refname, void *cb_data)
+{
+	struct files_fsck_root_ref_data *data = cb_data;
+	struct stat st;
+
+	strbuf_reset(&data->refname);
+	if (!is_main_worktree(data->wt))
+		strbuf_addf(&data->refname, "worktrees/%s/", data->wt->id);
+	strbuf_addstr(&data->refname, refname);
+
+	strbuf_reset(&data->path);
+	strbuf_addf(&data->path, "%s/%s", data->refs->gitcommondir, data->refname.buf);
+
+	if (stat(data->path.buf, &st)) {
+		if (errno == ENOENT)
+			return 0;
+		return error_errno("failed to read ref: '%s'", data->path.buf);
+	}
+
+	return files_fsck_ref(&data->refs->base, data->o, data->refname.buf,
+			      data->path.buf, st.st_mode);
+}
+
 static int files_fsck(struct ref_store *ref_store,
 		      struct fsck_options *o,
 		      struct worktree *wt)
 {
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_READ, "fsck");
+	struct files_fsck_root_ref_data data = {
+		.refs = refs,
+		.o = o,
+		.wt = wt,
+		.refname = STRBUF_INIT,
+		.path = STRBUF_INIT,
+	};
 	int ret = 0;
 
 	if (files_fsck_refs_dir(ref_store, o, wt) < 0)
 		ret = -1;
+
+	if (for_each_root_ref(refs, files_fsck_root_ref, &data) < 0 ||
+	    data.errors_found)
+		ret = -1;
+
 	if (refs->packed_ref_store->be->fsck(refs->packed_ref_store, o, wt) < 0)
 		ret = -1;
 
+	strbuf_release(&data.refname);
+	strbuf_release(&data.path);
 	return ret;
 }
 
diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
index 0ef483659d..479f3d528e 100755
--- a/t/t0602-reffiles-fsck.sh
+++ b/t/t0602-reffiles-fsck.sh
@@ -905,4 +905,34 @@ test_expect_success '--[no-]references option should apply to fsck' '
 	)
 '
 
+test_expect_success 'complains about broken root ref' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		echo "ref: refs/../HEAD" >.git/HEAD &&
+		test_must_fail git refs verify 2>err &&
+		cat >expect <<-EOF &&
+		error: HEAD: badReferentName: points to invalid refname ${SQ}refs/../HEAD${SQ}
+		EOF
+		test_cmp expect err
+	)
+'
+
+test_expect_success 'complains about broken root ref in worktree' '
+	test_when_finished "rm -rf repo worktree" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit initial &&
+		git worktree add ../worktree &&
+		echo "ref: refs/../HEAD" >.git/worktrees/worktree/HEAD &&
+		test_must_fail git refs verify 2>err &&
+		cat >expect <<-EOF &&
+		error: worktrees/worktree/HEAD: badReferentName: points to invalid refname ${SQ}refs/../HEAD${SQ}
+		EOF
+		test_cmp expect err
+	)
+'
+
 test_done

-- 
2.52.0.542.g9473a8513b.dirty

Re: [PATCH 07/17] refs/files: perform consistency checks for root refs

[shejialuo]

On Fri, Jan 09, 2026 at 01:39:36PM +0100, Patrick Steinhardt wrote:
>  static int files_fsck(struct ref_store *ref_store,
>  		      struct fsck_options *o,
>  		      struct worktree *wt)
>  {
>  	struct files_ref_store *refs =
>  		files_downcast(ref_store, REF_STORE_READ, "fsck");
> +	struct files_fsck_root_ref_data data = {
> +		.refs = refs,
> +		.o = o,
> +		.wt = wt,
> +		.refname = STRBUF_INIT,
> +		.path = STRBUF_INIT,
> +	};
>  	int ret = 0;
>  
>  	if (files_fsck_refs_dir(ref_store, o, wt) < 0)
>  		ret = -1;
> +
> +	if (for_each_root_ref(refs, files_fsck_root_ref, &data) < 0 ||
> +	    data.errors_found)

I am wondering where we update this filed in `files_fsck_root_ref`. It
seems that we never do this in this commit. I think we should delete
this filed in `files_fsck_root_ref_data` and add this field back when we
do need this to avoid confusion.

Thanks,
Jialuo

Re: [PATCH 07/17] refs/files: perform consistency checks for root refs

[Patrick Steinhardt]

On Sat, Jan 10, 2026 at 08:47:31PM +0800, shejialuo wrote:
> On Fri, Jan 09, 2026 at 01:39:36PM +0100, Patrick Steinhardt wrote:
> >  static int files_fsck(struct ref_store *ref_store,
> >  		      struct fsck_options *o,
> >  		      struct worktree *wt)
> >  {
> >  	struct files_ref_store *refs =
> >  		files_downcast(ref_store, REF_STORE_READ, "fsck");
> > +	struct files_fsck_root_ref_data data = {
> > +		.refs = refs,
> > +		.o = o,
> > +		.wt = wt,
> > +		.refname = STRBUF_INIT,
> > +		.path = STRBUF_INIT,
> > +	};
> >  	int ret = 0;
> >  
> >  	if (files_fsck_refs_dir(ref_store, o, wt) < 0)
> >  		ret = -1;
> > +
> > +	if (for_each_root_ref(refs, files_fsck_root_ref, &data) < 0 ||
> > +	    data.errors_found)
> 
> I am wondering where we update this filed in `files_fsck_root_ref`. It
> seems that we never do this in this commit. I think we should delete
> this filed in `files_fsck_root_ref_data` and add this field back when we
> do need this to avoid confusion.

Oh, you're right. I think I did use it in an earlier iteration, but
don't seem to do anymore. Will fix.

Patrick

[PATCH 08/17] fsck: drop unused fields from `struct fsck_ref_report`

[Patrick Steinhardt]

The `struct fsck_ref_report` has a couple fields that are intended to
improve the error reporting for broken ref reports by showing which
object ID or target reference the ref points to. These fields are never
set though and are thus essentially unused.

Remove them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 fsck.c | 5 -----
 fsck.h | 2 --
 2 files changed, 7 deletions(-)

diff --git a/fsck.c b/fsck.c
index fae18d8561..813d927d57 100644
--- a/fsck.c
+++ b/fsck.c
@@ -1310,11 +1310,6 @@ int fsck_refs_error_function(struct fsck_options *options UNUSED,
 
 	strbuf_addstr(&sb, report->path);
 
-	if (report->oid)
-		strbuf_addf(&sb, " -> (%s)", oid_to_hex(report->oid));
-	else if (report->referent)
-		strbuf_addf(&sb, " -> (%s)", report->referent);
-
 	if (msg_type == FSCK_WARN)
 		warning("%s: %s", sb.buf, message);
 	else
diff --git a/fsck.h b/fsck.h
index 336917c045..bfe0d9c6d2 100644
--- a/fsck.h
+++ b/fsck.h
@@ -162,8 +162,6 @@ struct fsck_object_report {
 
 struct fsck_ref_report {
 	const char *path;
-	const struct object_id *oid;
-	const char *referent;
 };
 
 struct fsck_options {

-- 
2.52.0.542.g9473a8513b.dirty

[PATCH 09/17] refs/files: extract generic symref target checks

[Patrick Steinhardt]

The consistency checks for the "files" backend contain a couple of
verifications for symrefs that verify generic properties of the target
reference. These properties need to hold for every backend, no matter
whether it's using the "files" or "reftable" backend.

Reimplementing these checks for every single backend doesn't really make
sense. Extract it into a generic `refs_fsck_symref()` function that can
be used my other backends, as well. The "reftable" backend will be wired
up in a subsequent commit.

While at it, improve the consistency checks so that we don't complain
about refs pointing to a non-ref target in case the target refname
format does not verify. Otherwise it's very likely that we'll generate
both error messages, which feels somewhat redundant in this case.

Note that the function has a couple of `UNUSED` parameters. These will
become referenced in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c               | 21 ++++++++++++++++++++
 refs.h               | 10 ++++++++++
 refs/files-backend.c | 54 ++++++++++++++++++++--------------------------------
 3 files changed, 52 insertions(+), 33 deletions(-)

diff --git a/refs.c b/refs.c
index e06e0cb072..739bf9fefc 100644
--- a/refs.c
+++ b/refs.c
@@ -320,6 +320,27 @@ int check_refname_format(const char *refname, int flags)
 	return check_or_sanitize_refname(refname, flags, NULL);
 }
 
+int refs_fsck_symref(struct ref_store *refs UNUSED, struct fsck_options *o,
+		     struct fsck_ref_report *report,
+		     const char *refname UNUSED, const char *target)
+{
+	if (is_root_ref(target))
+		return 0;
+
+	if (check_refname_format(target, 0) &&
+	    fsck_report_ref(o, report, FSCK_MSG_BAD_REFERENT_NAME,
+			    "points to invalid refname '%s'", target))
+		return -1;
+
+	if (!starts_with(target, "refs/") &&
+	    !starts_with(target, "worktrees/") &&
+	    fsck_report_ref(o, report, FSCK_MSG_SYMREF_TARGET_IS_NOT_A_REF,
+			    "points to non-ref target '%s'", target))
+		return -1;
+
+	return 0;
+}
+
 int refs_fsck(struct ref_store *refs, struct fsck_options *o,
 	      struct worktree *wt)
 {
diff --git a/refs.h b/refs.h
index d9051bbb04..d91fcb2d2f 100644
--- a/refs.h
+++ b/refs.h
@@ -653,6 +653,16 @@ int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_dat
  */
 int check_refname_format(const char *refname, int flags);
 
+struct fsck_ref_report;
+
+/*
+ * Perform generic checks for a specific symref target. This function is
+ * expected to be called by the ref backends for every symbolic ref.
+ */
+int refs_fsck_symref(struct ref_store *refs, struct fsck_options *o,
+		     struct fsck_ref_report *report,
+		     const char *refname, const char *target);
+
 /*
  * Check the reference database for consistency. Return 0 if refs and
  * reflogs are consistent, and non-zero otherwise. The errors will be
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0ff047d0df..72c1db849e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3718,53 +3718,39 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
 				  const char *path,
 				  int mode);
 
-static int files_fsck_symref_target(struct fsck_options *o,
+static int files_fsck_symref_target(struct ref_store *ref_store,
+				    struct fsck_options *o,
 				    struct fsck_ref_report *report,
+				    const char *refname,
 				    struct strbuf *referent,
 				    unsigned int symbolic_link)
 {
-	int is_referent_root;
 	char orig_last_byte;
 	size_t orig_len;
 	int ret = 0;
 
 	orig_len = referent->len;
 	orig_last_byte = referent->buf[orig_len - 1];
-	if (!symbolic_link)
-		strbuf_rtrim(referent);
-
-	is_referent_root = is_root_ref(referent->buf);
-	if (!is_referent_root &&
-	    !starts_with(referent->buf, "refs/") &&
-	    !starts_with(referent->buf, "worktrees/")) {
-		ret |= fsck_report_ref(o, report,
-				       FSCK_MSG_SYMREF_TARGET_IS_NOT_A_REF,
-				       "points to non-ref target '%s'", referent->buf);
-	}
 
-	if (!is_referent_root && check_refname_format(referent->buf, 0)) {
-		ret |= fsck_report_ref(o, report,
-				       FSCK_MSG_BAD_REFERENT_NAME,
-				       "points to invalid refname '%s'", referent->buf);
-	}
+	if (!symbolic_link) {
+		strbuf_rtrim(referent);
 
-	if (symbolic_link)
-		goto out;
+		if (referent->len == orig_len ||
+		    (referent->len < orig_len && orig_last_byte != '\n')) {
+			ret |= fsck_report_ref(o, report,
+					       FSCK_MSG_REF_MISSING_NEWLINE,
+					       "misses LF at the end");
+		}
 
-	if (referent->len == orig_len ||
-	    (referent->len < orig_len && orig_last_byte != '\n')) {
-		ret |= fsck_report_ref(o, report,
-				       FSCK_MSG_REF_MISSING_NEWLINE,
-				       "misses LF at the end");
+		if (referent->len != orig_len && referent->len != orig_len - 1) {
+			ret |= fsck_report_ref(o, report,
+					       FSCK_MSG_TRAILING_REF_CONTENT,
+					       "has trailing whitespaces or newlines");
+		}
 	}
 
-	if (referent->len != orig_len && referent->len != orig_len - 1) {
-		ret |= fsck_report_ref(o, report,
-				       FSCK_MSG_TRAILING_REF_CONTENT,
-				       "has trailing whitespaces or newlines");
-	}
+	ret |= refs_fsck_symref(ref_store, o, report, refname, referent->buf);
 
-out:
 	return ret ? -1 : 0;
 }
 
@@ -3807,7 +3793,8 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 		else
 			strbuf_addbuf(&referent, &ref_content);
 
-		ret |= files_fsck_symref_target(o, &report, &referent, 1);
+		ret |= files_fsck_symref_target(ref_store, o, &report,
+						target_name, &referent, 1);
 		goto cleanup;
 	}
 
@@ -3847,7 +3834,8 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 			goto cleanup;
 		}
 	} else {
-		ret = files_fsck_symref_target(o, &report, &referent, 0);
+		ret = files_fsck_symref_target(ref_store, o, &report,
+					       target_name, &referent, 0);
 		goto cleanup;
 	}
 

-- 
2.52.0.542.g9473a8513b.dirty

Re: [PATCH 09/17] refs/files: extract generic symref target checks

[shejialuo]

On Fri, Jan 09, 2026 at 01:39:38PM +0100, Patrick Steinhardt wrote:
> The consistency checks for the "files" backend contain a couple of
> verifications for symrefs that verify generic properties of the target
> reference. These properties need to hold for every backend, no matter
> whether it's using the "files" or "reftable" backend.
> 
> Reimplementing these checks for every single backend doesn't really make
> sense. Extract it into a generic `refs_fsck_symref()` function that can
> be used my other backends, as well. The "reftable" backend will be wired
> up in a subsequent commit.
> 

s/my/by

> While at it, improve the consistency checks so that we don't complain
> about refs pointing to a non-ref target in case the target refname
> format does not verify. Otherwise it's very likely that we'll generate
> both error messages, which feels somewhat redundant in this case.
> 

Make sense, we should fail early in this case.

> Note that the function has a couple of `UNUSED` parameters. These will
> become referenced in a subsequent commit.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs.c               | 21 ++++++++++++++++++++
>  refs.h               | 10 ++++++++++
>  refs/files-backend.c | 54 ++++++++++++++++++++--------------------------------
>  3 files changed, 52 insertions(+), 33 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index e06e0cb072..739bf9fefc 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -320,6 +320,27 @@ int check_refname_format(const char *refname, int flags)
>  	return check_or_sanitize_refname(refname, flags, NULL);
>  }
>  
> +int refs_fsck_symref(struct ref_store *refs UNUSED, struct fsck_options *o,
> +		     struct fsck_ref_report *report,
> +		     const char *refname UNUSED, const char *target)
> +{
> +	if (is_root_ref(target))
> +		return 0;
> +
> +	if (check_refname_format(target, 0) &&
> +	    fsck_report_ref(o, report, FSCK_MSG_BAD_REFERENT_NAME,
> +			    "points to invalid refname '%s'", target))
> +		return -1;
> +
> +	if (!starts_with(target, "refs/") &&
> +	    !starts_with(target, "worktrees/") &&
> +	    fsck_report_ref(o, report, FSCK_MSG_SYMREF_TARGET_IS_NOT_A_REF,
> +			    "points to non-ref target '%s'", target))
> +		return -1;
> +
> +	return 0;
> +}
> +
>  int refs_fsck(struct ref_store *refs, struct fsck_options *o,
>  	      struct worktree *wt)
>  {
> diff --git a/refs.h b/refs.h
> index d9051bbb04..d91fcb2d2f 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -653,6 +653,16 @@ int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_dat
>   */
>  int check_refname_format(const char *refname, int flags);
>  
> +struct fsck_ref_report;
> +
> +/*
> + * Perform generic checks for a specific symref target. This function is
> + * expected to be called by the ref backends for every symbolic ref.
> + */
> +int refs_fsck_symref(struct ref_store *refs, struct fsck_options *o,
> +		     struct fsck_ref_report *report,
> +		     const char *refname, const char *target);
> +
>  /*
>   * Check the reference database for consistency. Return 0 if refs and
>   * reflogs are consistent, and non-zero otherwise. The errors will be
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 0ff047d0df..72c1db849e 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3718,53 +3718,39 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
>  				  const char *path,
>  				  int mode);
>  
> -static int files_fsck_symref_target(struct fsck_options *o,
> +static int files_fsck_symref_target(struct ref_store *ref_store,
> +				    struct fsck_options *o,
>  				    struct fsck_ref_report *report,
> +				    const char *refname,
>  				    struct strbuf *referent,
>  				    unsigned int symbolic_link)


Nit: as we touch this function, maybe we could change `unsigned int
symbolic_link` to be `bool symbolic_link`.

>  {
> -	int is_referent_root;
>  	char orig_last_byte;
>  	size_t orig_len;
>  	int ret = 0;
>  
>  	orig_len = referent->len;
>  	orig_last_byte = referent->buf[orig_len - 1];
> -	if (!symbolic_link)
> -		strbuf_rtrim(referent);
> -
> -	is_referent_root = is_root_ref(referent->buf);
> -	if (!is_referent_root &&
> -	    !starts_with(referent->buf, "refs/") &&
> -	    !starts_with(referent->buf, "worktrees/")) {
> -		ret |= fsck_report_ref(o, report,
> -				       FSCK_MSG_SYMREF_TARGET_IS_NOT_A_REF,
> -				       "points to non-ref target '%s'", referent->buf);
> -	}
>  
> -	if (!is_referent_root && check_refname_format(referent->buf, 0)) {
> -		ret |= fsck_report_ref(o, report,
> -				       FSCK_MSG_BAD_REFERENT_NAME,
> -				       "points to invalid refname '%s'", referent->buf);
> -	}
> +	if (!symbolic_link) {
> +		strbuf_rtrim(referent);
>  
> -	if (symbolic_link)
> -		goto out;
> +		if (referent->len == orig_len ||
> +		    (referent->len < orig_len && orig_last_byte != '\n')) {
> +			ret |= fsck_report_ref(o, report,
> +					       FSCK_MSG_REF_MISSING_NEWLINE,
> +					       "misses LF at the end");
> +		}
>  
> -	if (referent->len == orig_len ||
> -	    (referent->len < orig_len && orig_last_byte != '\n')) {
> -		ret |= fsck_report_ref(o, report,
> -				       FSCK_MSG_REF_MISSING_NEWLINE,
> -				       "misses LF at the end");
> +		if (referent->len != orig_len && referent->len != orig_len - 1) {
> +			ret |= fsck_report_ref(o, report,
> +					       FSCK_MSG_TRAILING_REF_CONTENT,
> +					       "has trailing whitespaces or newlines");
> +		}
>  	}
>  
> -	if (referent->len != orig_len && referent->len != orig_len - 1) {
> -		ret |= fsck_report_ref(o, report,
> -				       FSCK_MSG_TRAILING_REF_CONTENT,
> -				       "has trailing whitespaces or newlines");
> -	}
> +	ret |= refs_fsck_symref(ref_store, o, report, refname, referent->buf);
>  
> -out:
>  	return ret ? -1 : 0;
>  }
>  
> @@ -3807,7 +3793,8 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
>  		else
>  			strbuf_addbuf(&referent, &ref_content);
>  
> -		ret |= files_fsck_symref_target(o, &report, &referent, 1);
> +		ret |= files_fsck_symref_target(ref_store, o, &report,
> +						target_name, &referent, 1);

Nit: we might change 1 to be `true`.

>  		goto cleanup;
>  	}
>  
> @@ -3847,7 +3834,8 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
>  			goto cleanup;
>  		}
>  	} else {
> -		ret = files_fsck_symref_target(o, &report, &referent, 0);
> +		ret = files_fsck_symref_target(ref_store, o, &report,
> +					       target_name, &referent, 0);
>  		goto cleanup;
>  	}
>  
> 
> -- 
> 2.52.0.542.g9473a8513b.dirty
> 

Thanks,
Jialuo

Re: [PATCH 09/17] refs/files: extract generic symref target checks

[Patrick Steinhardt]

On Sat, Jan 10, 2026 at 08:59:10PM +0800, shejialuo wrote:
> On Fri, Jan 09, 2026 at 01:39:38PM +0100, Patrick Steinhardt wrote:
> > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > index 0ff047d0df..72c1db849e 100644
> > --- a/refs/files-backend.c
> > +++ b/refs/files-backend.c
> > @@ -3718,53 +3718,39 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
> >  				  const char *path,
> >  				  int mode);
> >  
> > -static int files_fsck_symref_target(struct fsck_options *o,
> > +static int files_fsck_symref_target(struct ref_store *ref_store,
> > +				    struct fsck_options *o,
> >  				    struct fsck_ref_report *report,
> > +				    const char *refname,
> >  				    struct strbuf *referent,
> >  				    unsigned int symbolic_link)
> 
> 
> Nit: as we touch this function, maybe we could change `unsigned int
> symbolic_link` to be `bool symbolic_link`.

I'd prefer to not have this while-at-it change. The benefit isn't clear
enough to actually change it, and it would distract from the actual
changes a bit.

Patrick

[PATCH 10/17] refs/files: introduce function to perform normal ref checks

[Patrick Steinhardt]

In a subsequent commit we'll introduce new generic checks for direct
refs. These checks will be independent of the actual backend.

Introduce a new function `refs_fsck_ref()` that will be used for this
purpose. At the current point in time it's still empty, but it will get
populated in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c               | 7 +++++++
 refs.h               | 8 ++++++++
 refs/files-backend.c | 2 ++
 3 files changed, 17 insertions(+)

diff --git a/refs.c b/refs.c
index 739bf9fefc..4fc1317cb3 100644
--- a/refs.c
+++ b/refs.c
@@ -320,6 +320,13 @@ int check_refname_format(const char *refname, int flags)
 	return check_or_sanitize_refname(refname, flags, NULL);
 }
 
+int refs_fsck_ref(struct ref_store *refs UNUSED, struct fsck_options *o UNUSED,
+		  struct fsck_ref_report *report UNUSED,
+		  const char *refname UNUSED, const struct object_id *oid UNUSED)
+{
+	return 0;
+}
+
 int refs_fsck_symref(struct ref_store *refs UNUSED, struct fsck_options *o,
 		     struct fsck_ref_report *report,
 		     const char *refname UNUSED, const char *target)
diff --git a/refs.h b/refs.h
index d91fcb2d2f..61c56cca36 100644
--- a/refs.h
+++ b/refs.h
@@ -655,6 +655,14 @@ int check_refname_format(const char *refname, int flags);
 
 struct fsck_ref_report;
 
+/*
+ * Perform generic checks for a specific symref target. This function is
+ * expected to be called by the ref backends for every symbolic ref.
+ */
+int refs_fsck_ref(struct ref_store *refs, struct fsck_options *o,
+		  struct fsck_ref_report *report,
+		  const char *refname, const struct object_id *oid);
+
 /*
  * Perform generic checks for a specific symref target. This function is
  * expected to be called by the ref backends for every symbolic ref.
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 72c1db849e..e59794f5da 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3833,6 +3833,8 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 					      "has trailing garbage: '%s'", trailing);
 			goto cleanup;
 		}
+
+		ret = refs_fsck_ref(ref_store, o, &report, target_name, &oid);
 	} else {
 		ret = files_fsck_symref_target(ref_store, o, &report,
 					       target_name, &referent, 0);

-- 
2.52.0.542.g9473a8513b.dirty

Re: [PATCH 10/17] refs/files: introduce function to perform normal ref checks

[shejialuo]

On Fri, Jan 09, 2026 at 01:39:39PM +0100, Patrick Steinhardt wrote:
> In a subsequent commit we'll introduce new generic checks for direct
> refs. These checks will be independent of the actual backend.
> 
> Introduce a new function `refs_fsck_ref()` that will be used for this
> purpose. At the current point in time it's still empty, but it will get
> populated in a subsequent commit.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs.c               | 7 +++++++
>  refs.h               | 8 ++++++++
>  refs/files-backend.c | 2 ++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/refs.c b/refs.c
> index 739bf9fefc..4fc1317cb3 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -320,6 +320,13 @@ int check_refname_format(const char *refname, int flags)
>  	return check_or_sanitize_refname(refname, flags, NULL);
>  }
>  
> +int refs_fsck_ref(struct ref_store *refs UNUSED, struct fsck_options *o UNUSED,
> +		  struct fsck_ref_report *report UNUSED,
> +		  const char *refname UNUSED, const struct object_id *oid UNUSED)
> +{
> +	return 0;
> +}
> +
>  int refs_fsck_symref(struct ref_store *refs UNUSED, struct fsck_options *o,
>  		     struct fsck_ref_report *report,
>  		     const char *refname UNUSED, const char *target)
> diff --git a/refs.h b/refs.h
> index d91fcb2d2f..61c56cca36 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -655,6 +655,14 @@ int check_refname_format(const char *refname, int flags);
>  
>  struct fsck_ref_report;
>  
> +/*
> + * Perform generic checks for a specific symref target. This function is
> + * expected to be called by the ref backends for every symbolic ref.
> + */

I think above comment is the same as `refs_fsck_symref`, I think we
should update to say that we perform generic checks for a ref instead of
a specific symref target.

Thanks,
Jialuo

Re: [PATCH 10/17] refs/files: introduce function to perform normal ref checks

[Patrick Steinhardt]

On Sat, Jan 10, 2026 at 09:12:15PM +0800, shejialuo wrote:
> On Fri, Jan 09, 2026 at 01:39:39PM +0100, Patrick Steinhardt wrote:
> > diff --git a/refs.h b/refs.h
> > index d91fcb2d2f..61c56cca36 100644
> > --- a/refs.h
> > +++ b/refs.h
> > @@ -655,6 +655,14 @@ int check_refname_format(const char *refname, int flags);
> >  
> >  struct fsck_ref_report;
> >  
> > +/*
> > + * Perform generic checks for a specific symref target. This function is
> > + * expected to be called by the ref backends for every symbolic ref.
> > + */
> 
> I think above comment is the same as `refs_fsck_symref`, I think we
> should update to say that we perform generic checks for a ref instead of
> a specific symref target.

Indeed, a classical copy-paste error. Thanks!

Patrick

[PATCH 11/17] refs/reftable: adapt includes to become consistent

[Patrick Steinhardt]

Adapt the includes to be sorted and to use include paths that are
relative to the "refs/" directory.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 4319a4eacb..d61790cf65 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -10,9 +10,10 @@
 #include "../gettext.h"
 #include "../hash.h"
 #include "../hex.h"
-#include "../iterator.h"
 #include "../ident.h"
+#include "../iterator.h"
 #include "../object.h"
+#include "../parse.h"
 #include "../path.h"
 #include "../refs.h"
 #include "../reftable/reftable-basics.h"
@@ -26,7 +27,6 @@
 #include "../strmap.h"
 #include "../trace2.h"
 #include "../write-or-die.h"
-#include "parse.h"
 #include "refs-internal.h"
 
 /*

-- 
2.52.0.542.g9473a8513b.dirty

[PATCH 12/17] refs/reftable: extract function to retrieve backend for worktree

[Patrick Steinhardt]

Pull out the logic to retrieve a backend for a given worktree. This
function will be used in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c | 70 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 27 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index d61790cf65..dda961a32b 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -172,6 +172,37 @@ static struct reftable_ref_store *reftable_be_downcast(struct ref_store *ref_sto
 	return refs;
 }
 
+static int backend_for_worktree(struct reftable_backend **out,
+				struct reftable_ref_store *store,
+				const char *worktree_name)
+{
+	struct strbuf worktree_dir = STRBUF_INIT;
+	int ret;
+
+	*out = strmap_get(&store->worktree_backends, worktree_name);
+	if (*out) {
+		ret = 0;
+		goto out;
+	}
+
+	strbuf_addf(&worktree_dir, "%s/worktrees/%s/reftable",
+		    store->base.repo->commondir, worktree_name);
+
+	CALLOC_ARRAY(*out, 1);
+	store->err = ret = reftable_backend_init(*out, worktree_dir.buf,
+						 &store->write_options);
+	if (ret < 0) {
+		free(*out);
+		goto out;
+	}
+
+	strmap_put(&store->worktree_backends, worktree_name, *out);
+
+out:
+	strbuf_release(&worktree_dir);
+	return ret;
+}
+
 /*
  * Some refs are global to the repository (refs/heads/{*}), while others are
  * local to the worktree (eg. HEAD, refs/bisect/{*}). We solve this by having
@@ -191,19 +222,19 @@ static int backend_for(struct reftable_backend **out,
 		       const char **rewritten_ref,
 		       int reload)
 {
-	struct reftable_backend *be;
 	const char *wtname;
 	int wtname_len;
+	int ret;
 
 	if (!refname) {
-		be = &store->main_backend;
+		*out = &store->main_backend;
+		ret = 0;
 		goto out;
 	}
 
 	switch (parse_worktree_ref(refname, &wtname, &wtname_len, rewritten_ref)) {
 	case REF_WORKTREE_OTHER: {
 		static struct strbuf wtname_buf = STRBUF_INIT;
-		struct strbuf wt_dir = STRBUF_INIT;
 
 		/*
 		 * We're using a static buffer here so that we don't need to
@@ -223,20 +254,8 @@ static int backend_for(struct reftable_backend **out,
 		 * already and error out when trying to write a reference via
 		 * both stacks.
 		 */
-		be = strmap_get(&store->worktree_backends, wtname_buf.buf);
-		if (!be) {
-			strbuf_addf(&wt_dir, "%s/worktrees/%s/reftable",
-				    store->base.repo->commondir, wtname_buf.buf);
+		ret = backend_for_worktree(out, store, wtname_buf.buf);
 
-			CALLOC_ARRAY(be, 1);
-			store->err = reftable_backend_init(be, wt_dir.buf,
-							   &store->write_options);
-			assert(store->err != REFTABLE_API_ERROR);
-
-			strmap_put(&store->worktree_backends, wtname_buf.buf, be);
-		}
-
-		strbuf_release(&wt_dir);
 		goto out;
 	}
 	case REF_WORKTREE_CURRENT:
@@ -245,27 +264,24 @@ static int backend_for(struct reftable_backend **out,
 		 * main worktree. We thus return the main stack in that case.
 		 */
 		if (!store->worktree_backend.stack)
-			be = &store->main_backend;
+			*out = &store->main_backend;
 		else
-			be = &store->worktree_backend;
+			*out = &store->worktree_backend;
+		ret = 0;
 		goto out;
 	case REF_WORKTREE_MAIN:
 	case REF_WORKTREE_SHARED:
-		be = &store->main_backend;
+		*out = &store->main_backend;
+		ret = 0;
 		goto out;
 	default:
 		BUG("unhandled worktree reference type");
 	}
 
 out:
-	if (reload) {
-		int ret = reftable_stack_reload(be->stack);
-		if (ret)
-			return ret;
-	}
-	*out = be;
-
-	return 0;
+	if (reload && !ret)
+		ret = reftable_stack_reload((*out)->stack);
+	return ret;
 }
 
 static int should_write_log(struct reftable_ref_store *refs, const char *refname)

-- 
2.52.0.542.g9473a8513b.dirty

[PATCH 13/17] refs/reftable: fix consistency checks with worktrees

[Patrick Steinhardt]

The ref consistency checks are driven via `cmd_refs_verify()`. That
function loops through all worktrees (including the main worktree) and
then checks the ref store for each of them individually. It follows that
the backend is expected to only verify refs that belong to the specified
worktree.

While the "files" backend handles this correctly, the "reftable" backend
doesn't. In fact, it completely ignores the passed worktree and instead
verifies refs of _all_ worktrees. The consequence is that we'll end up
every ref store N times, where N is the number of worktrees.

Or rather, that would be the case if we actually iterated through the
worktree reftable stacks correctly. But we use `strmap_for_each_entry()`
to iterate through the stacks, but the map is in fact not even properly
populated. So instead of checking stacks N^2 times, we actually only end
up checking the reftable stack of the main worktree.

Fix this bug by only verifying the stack of the passed-in worktree and
constructing the backends via `backend_for_worktree()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c  | 29 ++++++++++++++---------------
 t/t0614-reftable-fsck.sh | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 15 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index dda961a32b..6361b27015 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -26,6 +26,7 @@
 #include "../setup.h"
 #include "../strmap.h"
 #include "../trace2.h"
+#include "../worktree.h"
 #include "../write-or-die.h"
 #include "refs-internal.h"
 
@@ -2762,25 +2763,23 @@ static int reftable_fsck_error_handler(struct reftable_fsck_info *info,
 }
 
 static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o,
-			    struct worktree *wt UNUSED)
+			    struct worktree *wt)
 {
-	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);
+	struct reftable_ref_store *refs =
+		reftable_be_downcast(ref_store, REF_STORE_READ, "fsck");
+	struct reftable_backend *backend;
 
-	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 (is_main_worktree(wt)) {
+		backend = &refs->main_backend;
+	} else {
+		int ret = backend_for_worktree(&backend, refs, wt->id);
+		if (ret < 0)
+			return error(_("reftable stack for worktree '%s' is broken"),
+				     wt->id);
 	}
 
-	return ret;
+	return reftable_fsck_check(backend->stack, reftable_fsck_error_handler,
+				   reftable_fsck_verbose_handler, o);
 }
 
 struct ref_storage_be refs_be_reftable = {
diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh
index 677eb9143c..4757eb5931 100755
--- a/t/t0614-reftable-fsck.sh
+++ b/t/t0614-reftable-fsck.sh
@@ -55,4 +55,36 @@ for TABLE_NAME in "foo-bar-e4d12d59.ref" \
 	'
 done
 
+test_expect_success 'worktree stacks can be verified' '
+	test_when_finished "rm -rf repo worktree" &&
+	git init repo &&
+	test_commit -C repo initial &&
+	git -C repo worktree add ../worktree &&
+
+	git -C worktree refs verify 2>err &&
+	test_must_be_empty err &&
+
+	REFTABLE_DIR=$(git -C worktree rev-parse --git-dir)/reftable &&
+	EXISTING_TABLE=$(head -n1 "$REFTABLE_DIR/tables.list") &&
+	mv "$REFTABLE_DIR/$EXISTING_TABLE" "$REFTABLE_DIR/broken.ref" &&
+
+	for d in repo worktree
+	do
+		echo "broken.ref" >"$REFTABLE_DIR/tables.list" &&
+		git -C "$d" refs verify 2>err &&
+		cat >expect <<-EOF &&
+		warning: broken.ref: badReftableTableName: invalid reftable table name
+		EOF
+		test_cmp expect err &&
+
+		echo garbage >"$REFTABLE_DIR/tables.list" &&
+		test_must_fail git -C "$d" refs verify 2>err &&
+		cat >expect <<-EOF &&
+		error: reftable stack for worktree ${SQ}worktree${SQ} is broken
+		EOF
+		test_cmp expect err || return 1
+
+	done
+'
+
 test_done

-- 
2.52.0.542.g9473a8513b.dirty

[PATCH 14/17] refs/reftable: introduce generic checks for refs

[Patrick Steinhardt]

In a preceding commit we have extracted generic checks for both direct
and symbolic refs that apply for all backends. Wire up those checks for
the "reftable" backend.

Note that this is done by iterating through all refs manually with the
low-level reftable ref iterator. We explicitly don't want to use the
higher-level iterator that is exposed to users of the reftable backend
as that iterator may swallow for example broken refs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c  | 82 ++++++++++++++++++++++++++++++++++++++++++++----
 t/t0614-reftable-fsck.sh | 12 +++++++
 2 files changed, 88 insertions(+), 6 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 6361b27015..fe74af73af 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -2767,19 +2767,89 @@ static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o,
 {
 	struct reftable_ref_store *refs =
 		reftable_be_downcast(ref_store, REF_STORE_READ, "fsck");
+	struct reftable_ref_iterator *iter = NULL;
+	struct reftable_ref_record ref = { 0 };
+	struct fsck_ref_report report = { 0 };
+	struct strbuf refname = STRBUF_INIT;
 	struct reftable_backend *backend;
+	int ret, errors = 0;
 
 	if (is_main_worktree(wt)) {
 		backend = &refs->main_backend;
 	} else {
-		int ret = backend_for_worktree(&backend, refs, wt->id);
-		if (ret < 0)
-			return error(_("reftable stack for worktree '%s' is broken"),
-				     wt->id);
+		ret = backend_for_worktree(&backend, refs, wt->id);
+		if (ret < 0) {
+			ret = error(_("reftable stack for worktree '%s' is broken"),
+				    wt->id);
+			goto out;
+		}
+	}
+
+	errors |= reftable_fsck_check(backend->stack, reftable_fsck_error_handler,
+				      reftable_fsck_verbose_handler, o);
+
+	iter = ref_iterator_for_stack(refs, backend->stack, "", NULL, 0);
+	if (!iter) {
+		ret = error(_("could not create iterator for worktree '%s'"), wt->id);
+		goto out;
+	}
+
+	while (1) {
+		ret = reftable_iterator_next_ref(&iter->iter, &ref);
+		if (ret > 0)
+			break;
+		if (ret < 0) {
+			ret = error(_("could not read record for worktree '%s'"), wt->id);
+			goto out;
+		}
+
+		strbuf_reset(&refname);
+		if (!is_main_worktree(wt))
+			strbuf_addf(&refname, "worktrees/%s/", wt->id);
+		strbuf_addstr(&refname, ref.refname);
+		report.path = refname.buf;
+
+		switch (ref.value_type) {
+		case REFTABLE_REF_VAL1:
+		case REFTABLE_REF_VAL2: {
+			struct object_id oid;
+			unsigned hash_id;
+
+			switch (reftable_stack_hash_id(backend->stack)) {
+			case REFTABLE_HASH_SHA1:
+				hash_id = GIT_HASH_SHA1;
+				break;
+			case REFTABLE_HASH_SHA256:
+				hash_id = GIT_HASH_SHA256;
+				break;
+			default:
+				BUG("unhandled hash ID %d",
+				    reftable_stack_hash_id(backend->stack));
+			}
+
+			oidread(&oid, reftable_ref_record_val1(&ref),
+				&hash_algos[hash_id]);
+
+			errors |= refs_fsck_ref(ref_store, o, &report, ref.refname, &oid);
+			break;
+		}
+		case REFTABLE_REF_SYMREF:
+			errors |= refs_fsck_symref(ref_store, o, &report, ref.refname,
+						   ref.value.symref);
+			break;
+		default:
+			BUG("unhandled reference value type %d", ref.value_type);
+		}
 	}
 
-	return reftable_fsck_check(backend->stack, reftable_fsck_error_handler,
-				   reftable_fsck_verbose_handler, o);
+	ret = errors ? -1 : 0;
+
+out:
+	if (iter)
+		ref_iterator_free(&iter->base);
+	reftable_ref_record_release(&ref);
+	strbuf_release(&refname);
+	return ret;
 }
 
 struct ref_storage_be refs_be_reftable = {
diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh
index 4757eb5931..d24b87f961 100755
--- a/t/t0614-reftable-fsck.sh
+++ b/t/t0614-reftable-fsck.sh
@@ -87,4 +87,16 @@ test_expect_success 'worktree stacks can be verified' '
 	done
 '
 
+test_expect_success 'invalid symref gets reported' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	test_commit -C repo initial &&
+	git -C repo symbolic-ref refs/heads/symref garbage &&
+	test_must_fail git -C repo refs verify 2>err &&
+	cat >expect <<-EOF &&
+	error: refs/heads/symref: badReferentName: points to invalid refname ${SQ}garbage${SQ}
+	EOF
+	test_cmp expect err
+'
+
 test_done

-- 
2.52.0.542.g9473a8513b.dirty

[PATCH 15/17] builtin/fsck: move generic object ID checks into `refs_fsck()`

[Patrick Steinhardt]

While most of the logic that verifies the consistency of refs is
driven by `refs_fsck()`, we still have a small handful of checks in
`fsck_head_link()`. These checks don't use the git-fsck(1) reporting
infrastructure, and as such it's impossible to for example disable
some of those checks.

One such check detects refs that point to the all-zeroes object ID.
Extract this check into the generic `refs_fsck_ref()` function that is
used by both the "files" and "reftable" backends.

Note that this will cause us to not return an error code from
`fsck_head_link()` anymore in case this error was detected. This is fine
though: the only caller of this function does not check the error code
anyway. To demonstrate this, adapt the function to drop its return value
altogether. The function will be removed in a subsequent commit anyway.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/fsck-msgids.adoc |  3 +++
 builtin/fsck.c                 | 41 +++++++++++++++--------------------------
 fsck.h                         |  1 +
 refs.c                         | 11 ++++++++---
 t/t1450-fsck.sh                |  6 +++---
 5 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc
index acac9683af..76609321f6 100644
--- a/Documentation/fsck-msgids.adoc
+++ b/Documentation/fsck-msgids.adoc
@@ -41,6 +41,9 @@
 `badRefName`::
 	(ERROR) A ref has an invalid format.
 
+`badRefOid`::
+	(ERROR) A ref points to an invalid object ID.
+
 `badReferentName`::
 	(ERROR) The referent name of a symref is invalid.
 
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 4979bc795e..4dd4d74d1e 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -564,9 +564,9 @@ static int fsck_handle_ref(const struct reference *ref, void *cb_data UNUSED)
 	return 0;
 }
 
-static int fsck_head_link(const char *head_ref_name,
-			  const char **head_points_at,
-			  struct object_id *head_oid);
+static void fsck_head_link(const char *head_ref_name,
+			   const char **head_points_at,
+			   struct object_id *head_oid);
 
 static void get_default_heads(void)
 {
@@ -713,12 +713,10 @@ static void fsck_source(struct odb_source *source)
 	stop_progress(&progress);
 }
 
-static int fsck_head_link(const char *head_ref_name,
-			  const char **head_points_at,
-			  struct object_id *head_oid)
+static void fsck_head_link(const char *head_ref_name,
+			   const char **head_points_at,
+			   struct object_id *head_oid)
 {
-	int null_is_error = 0;
-
 	if (verbose)
 		fprintf_ln(stderr, _("Checking %s link"), head_ref_name);
 
@@ -727,27 +725,18 @@ static int fsck_head_link(const char *head_ref_name,
 						  NULL);
 	if (!*head_points_at) {
 		errors_found |= ERROR_REFS;
-		return error(_("invalid %s"), head_ref_name);
+		error(_("invalid %s"), head_ref_name);
+		return;
 	}
-	if (!strcmp(*head_points_at, head_ref_name))
-		/* detached HEAD */
-		null_is_error = 1;
-	else if (!starts_with(*head_points_at, "refs/heads/")) {
+	if (strcmp(*head_points_at, head_ref_name) &&
+	    !starts_with(*head_points_at, "refs/heads/")) {
 		errors_found |= ERROR_REFS;
-		return error(_("%s points to something strange (%s)"),
-			     head_ref_name, *head_points_at);
-	}
-	if (is_null_oid(head_oid)) {
-		if (null_is_error) {
-			errors_found |= ERROR_REFS;
-			return error(_("%s: detached HEAD points at nothing"),
-				     head_ref_name);
-		}
-		fprintf_ln(stderr,
-			   _("notice: %s points to an unborn branch (%s)"),
-			   head_ref_name, *head_points_at + 11);
+		error(_("%s points to something strange (%s)"),
+		      head_ref_name, *head_points_at);
+		return;
 	}
-	return 0;
+
+	return;
 }
 
 static int fsck_cache_tree(struct cache_tree *it, const char *index_path)
diff --git a/fsck.h b/fsck.h
index bfe0d9c6d2..1f472b7daa 100644
--- a/fsck.h
+++ b/fsck.h
@@ -39,6 +39,7 @@ enum fsck_msg_type {
 	FUNC(BAD_REF_CONTENT, ERROR) \
 	FUNC(BAD_REF_FILETYPE, ERROR) \
 	FUNC(BAD_REF_NAME, ERROR) \
+	FUNC(BAD_REF_OID, ERROR) \
 	FUNC(BAD_TIMEZONE, ERROR) \
 	FUNC(BAD_TREE, ERROR) \
 	FUNC(BAD_TREE_SHA1, ERROR) \
diff --git a/refs.c b/refs.c
index 4fc1317cb3..c3528862c6 100644
--- a/refs.c
+++ b/refs.c
@@ -320,10 +320,15 @@ int check_refname_format(const char *refname, int flags)
 	return check_or_sanitize_refname(refname, flags, NULL);
 }
 
-int refs_fsck_ref(struct ref_store *refs UNUSED, struct fsck_options *o UNUSED,
-		  struct fsck_ref_report *report UNUSED,
-		  const char *refname UNUSED, const struct object_id *oid UNUSED)
+int refs_fsck_ref(struct ref_store *refs UNUSED, struct fsck_options *o,
+		  struct fsck_ref_report *report,
+		  const char *refname UNUSED, const struct object_id *oid)
 {
+	if (is_null_oid(oid))
+		return fsck_report_ref(o, report, FSCK_MSG_BAD_REF_OID,
+				       "points to invalid object ID '%s'",
+				       oid_to_hex(oid));
+
 	return 0;
 }
 
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index c4b651c2dc..900c1b2eb2 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -105,7 +105,7 @@ test_expect_success REFFILES 'HEAD link pointing at a funny object' '
 	echo $ZERO_OID >.git/HEAD &&
 	# avoid corrupt/broken HEAD from interfering with repo discovery
 	test_must_fail env GIT_DIR=.git git fsck 2>out &&
-	test_grep "detached HEAD points" out
+	test_grep "HEAD: badRefOid: points to invalid object ID ${SQ}$ZERO_OID${SQ}" out
 '
 
 test_expect_success 'HEAD link pointing at a funny place' '
@@ -123,7 +123,7 @@ test_expect_success REFFILES 'HEAD link pointing at a funny object (from differe
 	echo $ZERO_OID >.git/HEAD &&
 	# avoid corrupt/broken HEAD from interfering with repo discovery
 	test_must_fail git -C wt fsck 2>out &&
-	test_grep "main-worktree/HEAD: detached HEAD points" out
+	test_grep "HEAD: badRefOid: points to invalid object ID ${SQ}$ZERO_OID${SQ}" out
 '
 
 test_expect_success REFFILES 'other worktree HEAD link pointing at a funny object' '
@@ -131,7 +131,7 @@ test_expect_success REFFILES 'other worktree HEAD link pointing at a funny objec
 	git worktree add other &&
 	echo $ZERO_OID >.git/worktrees/other/HEAD &&
 	test_must_fail git fsck 2>out &&
-	test_grep "worktrees/other/HEAD: detached HEAD points" out
+	test_grep "worktrees/other/HEAD: badRefOid: points to invalid object ID ${SQ}$ZERO_OID${SQ}" out
 '
 
 test_expect_success 'other worktree HEAD link pointing at missing object' '

-- 
2.52.0.542.g9473a8513b.dirty

[PATCH 16/17] builtin/fsck: move generic HEAD check into `refs_fsck()`

[Patrick Steinhardt]

Move the check that detects "HEAD" refs that do not point at a branch
into `refs_fsck()`. This follows the same motivation as the preceding
commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/fsck-msgids.adoc |  3 +++
 builtin/fsck.c                 |  7 -------
 fsck.h                         |  1 +
 refs.c                         | 12 +++++++++++-
 t/t0602-reffiles-fsck.sh       |  8 ++++----
 t/t1450-fsck.sh                |  4 ++--
 6 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc
index 76609321f6..6a4db3a991 100644
--- a/Documentation/fsck-msgids.adoc
+++ b/Documentation/fsck-msgids.adoc
@@ -13,6 +13,9 @@
 `badGpgsig`::
 	(ERROR) A tag contains a bad (truncated) signature (e.g., `gpgsig`) header.
 
+`badHeadTarget`::
+	(ERROR) The `HEAD` ref is a symref that does not refer to a branch.
+
 `badHeaderContinuation`::
 	(ERROR) A continuation header (such as for `gpgsig`) is unexpectedly truncated.
 
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 4dd4d74d1e..5dda441f45 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -728,13 +728,6 @@ static void fsck_head_link(const char *head_ref_name,
 		error(_("invalid %s"), head_ref_name);
 		return;
 	}
-	if (strcmp(*head_points_at, head_ref_name) &&
-	    !starts_with(*head_points_at, "refs/heads/")) {
-		errors_found |= ERROR_REFS;
-		error(_("%s points to something strange (%s)"),
-		      head_ref_name, *head_points_at);
-		return;
-	}
 
 	return;
 }
diff --git a/fsck.h b/fsck.h
index 1f472b7daa..65ecbb7fe1 100644
--- a/fsck.h
+++ b/fsck.h
@@ -30,6 +30,7 @@ enum fsck_msg_type {
 	FUNC(BAD_DATE_OVERFLOW, ERROR) \
 	FUNC(BAD_EMAIL, ERROR) \
 	FUNC(BAD_GPGSIG, ERROR) \
+	FUNC(BAD_HEAD_TARGET, ERROR) \
 	FUNC(BAD_NAME, ERROR) \
 	FUNC(BAD_OBJECT_SHA1, ERROR) \
 	FUNC(BAD_PACKED_REF_ENTRY, ERROR) \
diff --git a/refs.c b/refs.c
index c3528862c6..a772d371cd 100644
--- a/refs.c
+++ b/refs.c
@@ -334,8 +334,18 @@ int refs_fsck_ref(struct ref_store *refs UNUSED, struct fsck_options *o,
 
 int refs_fsck_symref(struct ref_store *refs UNUSED, struct fsck_options *o,
 		     struct fsck_ref_report *report,
-		     const char *refname UNUSED, const char *target)
+		     const char *refname, const char *target)
 {
+	const char *stripped_refname;
+
+	parse_worktree_ref(refname, NULL, NULL, &stripped_refname);
+
+	if (!strcmp(stripped_refname, "HEAD") &&
+	    !starts_with(target, "refs/heads/") &&
+	    fsck_report_ref(o, report, FSCK_MSG_BAD_HEAD_TARGET,
+			    "HEAD points to non-branch '%s'", target))
+		return -1;
+
 	if (is_root_ref(target))
 		return 0;
 
diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
index 479f3d528e..3c1f553b81 100755
--- a/t/t0602-reffiles-fsck.sh
+++ b/t/t0602-reffiles-fsck.sh
@@ -910,10 +910,10 @@ test_expect_success 'complains about broken root ref' '
 	git init repo &&
 	(
 		cd repo &&
-		echo "ref: refs/../HEAD" >.git/HEAD &&
+		echo "ref: refs/heads/../HEAD" >.git/HEAD &&
 		test_must_fail git refs verify 2>err &&
 		cat >expect <<-EOF &&
-		error: HEAD: badReferentName: points to invalid refname ${SQ}refs/../HEAD${SQ}
+		error: HEAD: badReferentName: points to invalid refname ${SQ}refs/heads/../HEAD${SQ}
 		EOF
 		test_cmp expect err
 	)
@@ -926,10 +926,10 @@ test_expect_success 'complains about broken root ref in worktree' '
 		cd repo &&
 		test_commit initial &&
 		git worktree add ../worktree &&
-		echo "ref: refs/../HEAD" >.git/worktrees/worktree/HEAD &&
+		echo "ref: refs/heads/../HEAD" >.git/worktrees/worktree/HEAD &&
 		test_must_fail git refs verify 2>err &&
 		cat >expect <<-EOF &&
-		error: worktrees/worktree/HEAD: badReferentName: points to invalid refname ${SQ}refs/../HEAD${SQ}
+		error: worktrees/worktree/HEAD: badReferentName: points to invalid refname ${SQ}refs/heads/../HEAD${SQ}
 		EOF
 		test_cmp expect err
 	)
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 900c1b2eb2..3fae05f9d9 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -113,7 +113,7 @@ test_expect_success 'HEAD link pointing at a funny place' '
 	test-tool ref-store main create-symref HEAD refs/funny/place &&
 	# avoid corrupt/broken HEAD from interfering with repo discovery
 	test_must_fail env GIT_DIR=.git git fsck 2>out &&
-	test_grep "HEAD points to something strange" out
+	test_grep "HEAD: badHeadTarget: HEAD points to non-branch ${SQ}refs/funny/place${SQ}" out
 '
 
 test_expect_success REFFILES 'HEAD link pointing at a funny object (from different wt)' '
@@ -148,7 +148,7 @@ test_expect_success 'other worktree HEAD link pointing at a funny place' '
 	git worktree add other &&
 	git -C other symbolic-ref HEAD refs/funny/place &&
 	test_must_fail git fsck 2>out &&
-	test_grep "worktrees/other/HEAD points to something strange" out
+	test_grep "worktrees/other/HEAD: badHeadTarget: HEAD points to non-branch ${SQ}refs/funny/place${SQ}" out
 '
 
 test_expect_success 'commit with multiple signatures is okay' '

-- 
2.52.0.542.g9473a8513b.dirty

Re: [PATCH 16/17] builtin/fsck: move generic HEAD check into `refs_fsck()`

[shejialuo]

On Fri, Jan 09, 2026 at 01:39:45PM +0100, Patrick Steinhardt wrote:
> Move the check that detects "HEAD" refs that do not point at a branch
> into `refs_fsck()`. This follows the same motivation as the preceding
> commit.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  Documentation/fsck-msgids.adoc |  3 +++
>  builtin/fsck.c                 |  7 -------
>  fsck.h                         |  1 +
>  refs.c                         | 12 +++++++++++-
>  t/t0602-reffiles-fsck.sh       |  8 ++++----
>  t/t1450-fsck.sh                |  4 ++--
>  6 files changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc
> index 76609321f6..6a4db3a991 100644
> --- a/Documentation/fsck-msgids.adoc
> +++ b/Documentation/fsck-msgids.adoc
> @@ -13,6 +13,9 @@
>  `badGpgsig`::
>  	(ERROR) A tag contains a bad (truncated) signature (e.g., `gpgsig`) header.
>  
> +`badHeadTarget`::
> +	(ERROR) The `HEAD` ref is a symref that does not refer to a branch.
> +
>  `badHeaderContinuation`::
>  	(ERROR) A continuation header (such as for `gpgsig`) is unexpectedly truncated.
>  
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 4dd4d74d1e..5dda441f45 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -728,13 +728,6 @@ static void fsck_head_link(const char *head_ref_name,
>  		error(_("invalid %s"), head_ref_name);
>  		return;
>  	}
> -	if (strcmp(*head_points_at, head_ref_name) &&
> -	    !starts_with(*head_points_at, "refs/heads/")) {
> -		errors_found |= ERROR_REFS;
> -		error(_("%s points to something strange (%s)"),
> -		      head_ref_name, *head_points_at);
> -		return;
> -	}
>  
>  	return;
>  }
> diff --git a/fsck.h b/fsck.h
> index 1f472b7daa..65ecbb7fe1 100644
> --- a/fsck.h
> +++ b/fsck.h
> @@ -30,6 +30,7 @@ enum fsck_msg_type {
>  	FUNC(BAD_DATE_OVERFLOW, ERROR) \
>  	FUNC(BAD_EMAIL, ERROR) \
>  	FUNC(BAD_GPGSIG, ERROR) \
> +	FUNC(BAD_HEAD_TARGET, ERROR) \
>  	FUNC(BAD_NAME, ERROR) \
>  	FUNC(BAD_OBJECT_SHA1, ERROR) \
>  	FUNC(BAD_PACKED_REF_ENTRY, ERROR) \
> diff --git a/refs.c b/refs.c
> index c3528862c6..a772d371cd 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -334,8 +334,18 @@ int refs_fsck_ref(struct ref_store *refs UNUSED, struct fsck_options *o,
>  
>  int refs_fsck_symref(struct ref_store *refs UNUSED, struct fsck_options *o,
>  		     struct fsck_ref_report *report,
> -		     const char *refname UNUSED, const char *target)
> +		     const char *refname, const char *target)
>  {
> +	const char *stripped_refname;
> +
> +	parse_worktree_ref(refname, NULL, NULL, &stripped_refname);
> +
> +	if (!strcmp(stripped_refname, "HEAD") &&
> +	    !starts_with(target, "refs/heads/") &&

We would first check whether the current ref is `HEAD`. And I am
wondering whether we have some common APIs. And I find the similar logic
in `reglog.c::is_head` like the following shows:

    static int is_head(const char *refname)
    {
            const char *stripped_refname;
            parse_worktree_ref(refname, NULL, NULL, &stripped_refname);
            return !strcmp(stripped_refname, "HEAD");
    }

I think we might just extract this common logic to avoid introducing
repetition.

Thanks,
Jialuo

Re: [PATCH 16/17] builtin/fsck: move generic HEAD check into `refs_fsck()`

[Patrick Steinhardt]

On Sat, Jan 10, 2026 at 09:31:07PM +0800, shejialuo wrote:
> On Fri, Jan 09, 2026 at 01:39:45PM +0100, Patrick Steinhardt wrote:
> > diff --git a/refs.c b/refs.c
> > index c3528862c6..a772d371cd 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -334,8 +334,18 @@ int refs_fsck_ref(struct ref_store *refs UNUSED, struct fsck_options *o,
> >  
> >  int refs_fsck_symref(struct ref_store *refs UNUSED, struct fsck_options *o,
> >  		     struct fsck_ref_report *report,
> > -		     const char *refname UNUSED, const char *target)
> > +		     const char *refname, const char *target)
> >  {
> > +	const char *stripped_refname;
> > +
> > +	parse_worktree_ref(refname, NULL, NULL, &stripped_refname);
> > +
> > +	if (!strcmp(stripped_refname, "HEAD") &&
> > +	    !starts_with(target, "refs/heads/") &&
> 
> We would first check whether the current ref is `HEAD`. And I am
> wondering whether we have some common APIs. And I find the similar logic
> in `reglog.c::is_head` like the following shows:
> 
>     static int is_head(const char *refname)
>     {
>             const char *stripped_refname;
>             parse_worktree_ref(refname, NULL, NULL, &stripped_refname);
>             return !strcmp(stripped_refname, "HEAD");
>     }
> 
> I think we might just extract this common logic to avoid introducing
> repetition.

Hm. We could, but I'm a tiny bit worried about just calling it
`is_head()`. It might be surprising to some callers that there isn't
only one "HEAD", but that this would also recognize worktree HEADs. If
somebody just goes like "I wanna know whether I've got HEAD" they might
not think about that at all.

So given that the complexity is comparatively low I'd prefer to keep
this as-is for now. On the other hand, if you've got some proposal for
how to make this interface not confusing I'm very open to that :)

Thanks!

Patrick

Re: [PATCH 16/17] builtin/fsck: move generic HEAD check into `refs_fsck()`

[shejialuo]

On Mon, Jan 12, 2026 at 09:18:01AM +0100, Patrick Steinhardt wrote:
> On Sat, Jan 10, 2026 at 09:31:07PM +0800, shejialuo wrote:
> > On Fri, Jan 09, 2026 at 01:39:45PM +0100, Patrick Steinhardt wrote:
> > > diff --git a/refs.c b/refs.c
> > > index c3528862c6..a772d371cd 100644
> > > --- a/refs.c
> > > +++ b/refs.c
> > > @@ -334,8 +334,18 @@ int refs_fsck_ref(struct ref_store *refs UNUSED, struct fsck_options *o,
> > >  
> > >  int refs_fsck_symref(struct ref_store *refs UNUSED, struct fsck_options *o,
> > >  		     struct fsck_ref_report *report,
> > > -		     const char *refname UNUSED, const char *target)
> > > +		     const char *refname, const char *target)
> > >  {
> > > +	const char *stripped_refname;
> > > +
> > > +	parse_worktree_ref(refname, NULL, NULL, &stripped_refname);
> > > +
> > > +	if (!strcmp(stripped_refname, "HEAD") &&
> > > +	    !starts_with(target, "refs/heads/") &&
> > 
> > We would first check whether the current ref is `HEAD`. And I am
> > wondering whether we have some common APIs. And I find the similar logic
> > in `reglog.c::is_head` like the following shows:
> > 
> >     static int is_head(const char *refname)
> >     {
> >             const char *stripped_refname;
> >             parse_worktree_ref(refname, NULL, NULL, &stripped_refname);
> >             return !strcmp(stripped_refname, "HEAD");
> >     }
> > 
> > I think we might just extract this common logic to avoid introducing
> > repetition.
> 
> Hm. We could, but I'm a tiny bit worried about just calling it
> `is_head()`. It might be surprising to some callers that there isn't
> only one "HEAD", but that this would also recognize worktree HEADs. If
> somebody just goes like "I wanna know whether I've got HEAD" they might
> not think about that at all.
> 

Make sense.

> So given that the complexity is comparatively low I'd prefer to keep
> this as-is for now. On the other hand, if you've got some proposal for
> how to make this interface not confusing I'm very open to that :)
> 

Yeah, I cannot give some better idea, either. Let's keep this as-is :)

> Thanks!
> 
> Patrick

[PATCH 17/17] builtin/fsck: drop `fsck_head_link()`

[Patrick Steinhardt]

The function `fsck_head_link()` was historically used to perform a
couple of consistency checks for refs. (Almost) all of these checks have
now been moved into the refs subsystem. There's only a single check
remaining that verifies whether `refs_resolve_ref_unsafe()` returns a
`NULL` pointer. This may happen in a couple of cases:

  - When `refs_is_safe()` declares the ref to be unsafe. We already have
    checks for this as we verify refnames with `check_refname_format()`.

  - When the ref doesn't exist. A repository without "HEAD" is
    completely broken though, and we would notice this error ahead of
    time already.

  - In case the caller passes `RESOLVE_REF_READING` and the ref is a
    symref that doesn't resolve. We don't pass this flag though.

As such, this check doesn't cover anything anymore that isn't already
covered by `refs_fsck()`. Drop it, which also allows us to inline the
call to `refs_resolve_ref_unsafe()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c | 28 ++++------------------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 5dda441f45..f104b7af0e 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -564,10 +564,6 @@ static int fsck_handle_ref(const struct reference *ref, void *cb_data UNUSED)
 	return 0;
 }
 
-static void fsck_head_link(const char *head_ref_name,
-			   const char **head_points_at,
-			   struct object_id *head_oid);
-
 static void get_default_heads(void)
 {
 	struct worktree **worktrees, **p;
@@ -583,7 +579,10 @@ static void get_default_heads(void)
 		struct strbuf refname = STRBUF_INIT;
 
 		strbuf_worktree_ref(wt, &refname, "HEAD");
-		fsck_head_link(refname.buf, &head_points_at, &head_oid);
+
+		head_points_at = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
+							 refname.buf, 0, &head_oid, NULL);
+
 		if (head_points_at && !is_null_oid(&head_oid)) {
 			struct reference ref = {
 				.name = refname.buf,
@@ -713,25 +712,6 @@ static void fsck_source(struct odb_source *source)
 	stop_progress(&progress);
 }
 
-static void fsck_head_link(const char *head_ref_name,
-			   const char **head_points_at,
-			   struct object_id *head_oid)
-{
-	if (verbose)
-		fprintf_ln(stderr, _("Checking %s link"), head_ref_name);
-
-	*head_points_at = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
-						  head_ref_name, 0, head_oid,
-						  NULL);
-	if (!*head_points_at) {
-		errors_found |= ERROR_REFS;
-		error(_("invalid %s"), head_ref_name);
-		return;
-	}
-
-	return;
-}
-
 static int fsck_cache_tree(struct cache_tree *it, const char *index_path)
 {
 	int i;

-- 
2.52.0.542.g9473a8513b.dirty

Re: [PATCH 00/17] Fixes and improvements for ref consistency checks

[shejialuo]

On Fri, Jan 09, 2026 at 01:39:29PM +0100, Patrick Steinhardt wrote:
> Hi,
> 
> this patch series contains a bunch of fixes and improvements for ref
> consistency checks. It is structured as follows:
> 
>   - Patches 1 to 4 contain a couple of cleanups for the consistency
>     checks done by the "files" backend.
> 
>   - Patches 5 to 7 introduce checks for root refs for the "files"
>     backend.
> 
>   - Patches 9 to 14 introduce infrastructure for shared checks with the
>     "files" and "reftable" backend.
> 
>   - Patches 15 to 17 move some ref consistency checks that were still
>     driven by git-fsck(1) into `git refs verify`.
> 
> Thanks!
> 
> Patrick

I left some comments. In conclusion, I very appreciate the direction to
share the common logic for both "files" backend and "reftable" backend.
And also, we could check the correctness of `HEAD` to make the ref
subsystem self-contained.

Thanks,
Jialuo

Re: [PATCH 00/17] Fixes and improvements for ref consistency checks

[Patrick Steinhardt]

On Sat, Jan 10, 2026 at 09:37:14PM +0800, shejialuo wrote:
> On Fri, Jan 09, 2026 at 01:39:29PM +0100, Patrick Steinhardt wrote:
> > Hi,
> > 
> > this patch series contains a bunch of fixes and improvements for ref
> > consistency checks. It is structured as follows:
> > 
> >   - Patches 1 to 4 contain a couple of cleanups for the consistency
> >     checks done by the "files" backend.
> > 
> >   - Patches 5 to 7 introduce checks for root refs for the "files"
> >     backend.
> > 
> >   - Patches 9 to 14 introduce infrastructure for shared checks with the
> >     "files" and "reftable" backend.
> > 
> >   - Patches 15 to 17 move some ref consistency checks that were still
> >     driven by git-fsck(1) into `git refs verify`.
> > 
> > Thanks!
> > 
> > Patrick
> 
> I left some comments. In conclusion, I very appreciate the direction to
> share the common logic for both "files" backend and "reftable" backend.
> And also, we could check the correctness of `HEAD` to make the ref
> subsystem self-contained.

Thanks for your review!

Patrick

[PATCH v2 00/17] Fixes and improvements for ref consistency checks

[Patrick Steinhardt]

Hi,

this patch series contains a bunch of fixes and improvements for ref
consistency checks. It is structured as follows:

  - Patches 1 to 4 contain a couple of cleanups for the consistency
    checks done by the "files" backend.

  - Patches 5 to 7 introduce checks for root refs for the "files"
    backend.

  - Patches 9 to 14 introduce infrastructure for shared checks with the
    "files" and "reftable" backend.

  - Patches 15 to 17 move some ref consistency checks that were still
    driven by git-fsck(1) into `git refs verify`.

Changes in v2:
  - Remove unused `errors_found` field.
  - Fix a commit message typo.
  - Fix a copy-paste error in a function comment.
  - Link to v1: https://lore.kernel.org/r/20260109-pks-refs-verify-fixes-v1-0-3587dba18294@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (17):
      refs/files: simplify iterating through root refs
      refs/files: move fsck functions into global scope
      refs/files: remove `refs_check_dir` parameter
      refs/files: remove useless indirection
      refs/files: extract function to check single ref
      refs/files: improve error handling when verifying symrefs
      refs/files: perform consistency checks for root refs
      fsck: drop unused fields from `struct fsck_ref_report`
      refs/files: extract generic symref target checks
      refs/files: introduce function to perform normal ref checks
      refs/reftable: adapt includes to become consistent
      refs/reftable: extract function to retrieve backend for worktree
      refs/reftable: fix consistency checks with worktrees
      refs/reftable: introduce generic checks for refs
      builtin/fsck: move generic object ID checks into `refs_fsck()`
      builtin/fsck: move generic HEAD check into `refs_fsck()`
      builtin/fsck: drop `fsck_head_link()`

 Documentation/fsck-msgids.adoc |   6 ++
 builtin/fsck.c                 |  46 +--------
 fsck.c                         |   5 -
 fsck.h                         |   4 +-
 refs.c                         |  43 ++++++++
 refs.h                         |  18 ++++
 refs/files-backend.c           | 228 ++++++++++++++++++++++++-----------------
 refs/reftable-backend.c        | 167 ++++++++++++++++++++++--------
 t/t0602-reffiles-fsck.sh       |  30 ++++++
 t/t0614-reftable-fsck.sh       |  44 ++++++++
 t/t1450-fsck.sh                |  10 +-
 11 files changed, 414 insertions(+), 187 deletions(-)

Range-diff versus v1:

 1:  201451626d =  1:  21531efb05 refs/files: simplify iterating through root refs
 2:  88252f2b99 =  2:  861bd57d6e refs/files: move fsck functions into global scope
 3:  56d8ce2c85 =  3:  e06b8bdd23 refs/files: remove `refs_check_dir` parameter
 4:  ddf450134c =  4:  92992a522e refs/files: remove useless indirection
 5:  2d3ebf80fd =  5:  904fecf80e refs/files: extract function to check single ref
 6:  316dafeff8 =  6:  b5f5e86f1f refs/files: improve error handling when verifying symrefs
 7:  94a9b3d58b !  7:  d1abff98f8 refs/files: perform consistency checks for root refs
    @@ refs/files-backend.c: static int files_fsck_refs_dir(struct ref_store *ref_store
     +	struct worktree *wt;
     +	struct strbuf refname;
     +	struct strbuf path;
    -+	bool errors_found;
     +};
     +
     +static int files_fsck_root_ref(const char *refname, void *cb_data)
    @@ refs/files-backend.c: static int files_fsck_refs_dir(struct ref_store *ref_store
      	if (files_fsck_refs_dir(ref_store, o, wt) < 0)
      		ret = -1;
     +
    -+	if (for_each_root_ref(refs, files_fsck_root_ref, &data) < 0 ||
    -+	    data.errors_found)
    ++	if (for_each_root_ref(refs, files_fsck_root_ref, &data) < 0)
     +		ret = -1;
     +
      	if (refs->packed_ref_store->be->fsck(refs->packed_ref_store, o, wt) < 0)
 8:  a773fd65e9 =  8:  0b0e0e0033 fsck: drop unused fields from `struct fsck_ref_report`
 9:  12cf39ce8c !  9:  4bf249e530 refs/files: extract generic symref target checks
    @@ Commit message
     
         Reimplementing these checks for every single backend doesn't really make
         sense. Extract it into a generic `refs_fsck_symref()` function that can
    -    be used my other backends, as well. The "reftable" backend will be wired
    +    be used by other backends, as well. The "reftable" backend will be wired
         up in a subsequent commit.
     
         While at it, improve the consistency checks so that we don't complain
10:  9ae96c0acb ! 10:  5bd34fb53c refs/files: introduce function to perform normal ref checks
    @@ refs.h: int check_refname_format(const char *refname, int flags);
      struct fsck_ref_report;
      
     +/*
    -+ * Perform generic checks for a specific symref target. This function is
    ++ * Perform generic checks for a specific direct ref. This function is
     + * expected to be called by the ref backends for every symbolic ref.
     + */
     +int refs_fsck_ref(struct ref_store *refs, struct fsck_options *o,
11:  c2b0a1f517 = 11:  ca62b50abc refs/reftable: adapt includes to become consistent
12:  608b689d9e = 12:  66b5d6c981 refs/reftable: extract function to retrieve backend for worktree
13:  d39733206f = 13:  d31b7fb348 refs/reftable: fix consistency checks with worktrees
14:  37b8d22941 = 14:  eb960e66f2 refs/reftable: introduce generic checks for refs
15:  72b81062d2 = 15:  d0e2e3fe33 builtin/fsck: move generic object ID checks into `refs_fsck()`
16:  07a2403bc7 = 16:  029d02dd8a builtin/fsck: move generic HEAD check into `refs_fsck()`
17:  e944a0e430 = 17:  99eb06f153 builtin/fsck: drop `fsck_head_link()`

---
base-commit: d529f3a197364881746f558e5652f0236131eb86
change-id: 20260109-pks-refs-verify-fixes-1e47872317cf

[PATCH v2 01/17] refs/files: simplify iterating through root refs

[Patrick Steinhardt]

When iterating through root refs we first need to determine the
directory in which the refs live. This is done by retrieving the root of
the loose refs via `refs->loose->root->name`, and putting it through
`files_ref_path()` to derive the final path.

This is somewhat redundant though: the root name of the loose files
cache is always going to be the empty string. As such, we always end up
passing that empty string to `files_ref_path()` as the ref hierarchy we
want to start. And this actually makes sense: `files_ref_path()` already
computes the location of the root directory, so of course we need to
pass the empty string for the ref hierarchy itself. So going via the
loose ref cache to figure out that the root of a ref hierarchy is empty
is only causing confusion.

But next to the added confusion, it can also lead to a segfault. The
loose ref cache is populated lazily, so it may not always be set. It
seems to be sheer luck that this is a condition we do not currently hit.
The right thing to do would be to call `get_loose_ref_cache()`, which
knows to populate the cache if required.

Simplify the code and fix the potential segfault by simply removing the
indirection via the loose ref cache completely.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6f6f76a8d8..297739f203 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -354,13 +354,11 @@ static int for_each_root_ref(struct files_ref_store *refs,
 			     void *cb_data)
 {
 	struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT;
-	const char *dirname = refs->loose->root->name;
 	struct dirent *de;
-	size_t dirnamelen;
 	int ret;
 	DIR *d;
 
-	files_ref_path(refs, &path, dirname);
+	files_ref_path(refs, &path, "");
 
 	d = opendir(path.buf);
 	if (!d) {
@@ -368,9 +366,6 @@ static int for_each_root_ref(struct files_ref_store *refs,
 		return -1;
 	}
 
-	strbuf_addstr(&refname, dirname);
-	dirnamelen = refname.len;
-
 	while ((de = readdir(d)) != NULL) {
 		unsigned char dtype;
 
@@ -378,6 +373,8 @@ static int for_each_root_ref(struct files_ref_store *refs,
 			continue;
 		if (ends_with(de->d_name, ".lock"))
 			continue;
+
+		strbuf_reset(&refname);
 		strbuf_addstr(&refname, de->d_name);
 
 		dtype = get_dtype(de, &path, 1);
@@ -386,8 +383,6 @@ static int for_each_root_ref(struct files_ref_store *refs,
 			if (ret)
 				goto done;
 		}
-
-		strbuf_setlen(&refname, dirnamelen);
 	}
 
 	ret = 0;

-- 
2.52.0.590.g1f87b77810.dirty

Re: [PATCH v2 01/17] refs/files: simplify iterating through root refs

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3063 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> When iterating through root refs we first need to determine the
> directory in which the refs live. This is done by retrieving the root of
> the loose refs via `refs->loose->root->name`, and putting it through
> `files_ref_path()` to derive the final path.
>
> This is somewhat redundant though: the root name of the loose files
> cache is always going to be the empty string. As such, we always end up
> passing that empty string to `files_ref_path()` as the ref hierarchy we
> want to start. And this actually makes sense: `files_ref_path()` already
> computes the location of the root directory, so of course we need to
> pass the empty string for the ref hierarchy itself. So going via the
> loose ref cache to figure out that the root of a ref hierarchy is empty
> is only causing confusion.
>
> But next to the added confusion, it can also lead to a segfault. The
> loose ref cache is populated lazily, so it may not always be set. It
> seems to be sheer luck that this is a condition we do not currently hit.
> The right thing to do would be to call `get_loose_ref_cache()`, which
> knows to populate the cache if required.
>
> Simplify the code and fix the potential segfault by simply removing the
> indirection via the loose ref cache completely.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/files-backend.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 6f6f76a8d8..297739f203 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -354,13 +354,11 @@ static int for_each_root_ref(struct files_ref_store *refs,
>  			     void *cb_data)
>  {
>  	struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT;
> -	const char *dirname = refs->loose->root->name;
>  	struct dirent *de;
> -	size_t dirnamelen;
>  	int ret;
>  	DIR *d;
>
> -	files_ref_path(refs, &path, dirname);
> +	files_ref_path(refs, &path, "");
>

Since refs->loose->root->name is always `""`, we directly pass that
instead. Makes sense.

>  	d = opendir(path.buf);
>  	if (!d) {
> @@ -368,9 +366,6 @@ static int for_each_root_ref(struct files_ref_store *refs,
>  		return -1;
>  	}
>
> -	strbuf_addstr(&refname, dirname);
> -	dirnamelen = refname.len;
> -

This too is unnecessary since the len here is 0.

>  	while ((de = readdir(d)) != NULL) {
>  		unsigned char dtype;
>
> @@ -378,6 +373,8 @@ static int for_each_root_ref(struct files_ref_store *refs,
>  			continue;
>  		if (ends_with(de->d_name, ".lock"))
>  			continue;
> +
> +		strbuf_reset(&refname);
>  		strbuf_addstr(&refname, de->d_name);
>
>  		dtype = get_dtype(de, &path, 1);
> @@ -386,8 +383,6 @@ static int for_each_root_ref(struct files_ref_store *refs,
>  			if (ret)
>  				goto done;
>  		}
> -
> -		strbuf_setlen(&refname, dirnamelen);

Earlier we were setting the length to 0, but thats the same as
strbuf_reset(), so we do that now. This gets rid of the `dirnamelen`
varible. Looks good.

>  	}
>
>  	ret = 0;
>
> --
> 2.52.0.590.g1f87b77810.dirty

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v2 02/17] refs/files: move fsck functions into global scope

[Patrick Steinhardt]

When performing consistency checks we pass the functions that perform
the verification down the calling stack. This is somewhat unnecessary
though, as the set of functions doesn't ever change.

Simplify the code by moving the array into global scope and remove the
parameter.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 297739f203..feba3ee58b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3890,11 +3890,16 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
 	return ret;
 }
 
+static const files_fsck_refs_fn fsck_refs_fn[]= {
+	files_fsck_refs_name,
+	files_fsck_refs_content,
+	NULL,
+};
+
 static int files_fsck_refs_dir(struct ref_store *ref_store,
 			       struct fsck_options *o,
 			       const char *refs_check_dir,
-			       struct worktree *wt,
-			       files_fsck_refs_fn *fsck_refs_fn)
+			       struct worktree *wt)
 {
 	struct strbuf refname = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
@@ -3955,13 +3960,7 @@ static int files_fsck_refs(struct ref_store *ref_store,
 			   struct fsck_options *o,
 			   struct worktree *wt)
 {
-	files_fsck_refs_fn fsck_refs_fn[]= {
-		files_fsck_refs_name,
-		files_fsck_refs_content,
-		NULL,
-	};
-
-	return files_fsck_refs_dir(ref_store, o, "refs", wt, fsck_refs_fn);
+	return files_fsck_refs_dir(ref_store, o, "refs", wt);
 }
 
 static int files_fsck(struct ref_store *ref_store,

-- 
2.52.0.590.g1f87b77810.dirty

[PATCH v2 03/17] refs/files: remove `refs_check_dir` parameter

[Patrick Steinhardt]

The parameter `refs_check_dir` determines which directory we want to
check references for. But as we always want to check the complete
refs hierarchy, this parameter is always set to "refs".

Drop the parameter and hardcode it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index feba3ee58b..0a104c7bf6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3898,7 +3898,6 @@ static const files_fsck_refs_fn fsck_refs_fn[]= {
 
 static int files_fsck_refs_dir(struct ref_store *ref_store,
 			       struct fsck_options *o,
-			       const char *refs_check_dir,
 			       struct worktree *wt)
 {
 	struct strbuf refname = STRBUF_INIT;
@@ -3907,7 +3906,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
 	int iter_status;
 	int ret = 0;
 
-	strbuf_addf(&sb, "%s/%s", ref_store->gitdir, refs_check_dir);
+	strbuf_addf(&sb, "%s/refs", ref_store->gitdir);
 
 	iter = dir_iterator_begin(sb.buf, 0);
 	if (!iter) {
@@ -3927,8 +3926,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
 
 			if (!is_main_worktree(wt))
 				strbuf_addf(&refname, "worktrees/%s/", wt->id);
-			strbuf_addf(&refname, "%s/%s", refs_check_dir,
-				    iter->relative_path);
+			strbuf_addf(&refname, "refs/%s", iter->relative_path);
 
 			if (o->verbose)
 				fprintf_ln(stderr, "Checking %s", refname.buf);
@@ -3960,7 +3958,7 @@ static int files_fsck_refs(struct ref_store *ref_store,
 			   struct fsck_options *o,
 			   struct worktree *wt)
 {
-	return files_fsck_refs_dir(ref_store, o, "refs", wt);
+	return files_fsck_refs_dir(ref_store, o, wt);
 }
 
 static int files_fsck(struct ref_store *ref_store,

-- 
2.52.0.590.g1f87b77810.dirty

[PATCH v2 04/17] refs/files: remove useless indirection

[Patrick Steinhardt]

The function `files_fsck_refs()` only has a single callsite and forwards
all of its arguments as-is, so it's basically a useless indirection.
Inline the function call.

While at it, also remove the bitwise or that we have for return values.
We don't really want to or them at all, but rather just want to return
an error in case either of the functions has failed.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0a104c7bf6..4cbee23dad 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3954,22 +3954,20 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
 	return ret;
 }
 
-static int files_fsck_refs(struct ref_store *ref_store,
-			   struct fsck_options *o,
-			   struct worktree *wt)
-{
-	return files_fsck_refs_dir(ref_store, o, wt);
-}
-
 static int files_fsck(struct ref_store *ref_store,
 		      struct fsck_options *o,
 		      struct worktree *wt)
 {
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_READ, "fsck");
+	int ret = 0;
 
-	return files_fsck_refs(ref_store, o, wt) |
-	       refs->packed_ref_store->be->fsck(refs->packed_ref_store, o, wt);
+	if (files_fsck_refs_dir(ref_store, o, wt) < 0)
+		ret = -1;
+	if (refs->packed_ref_store->be->fsck(refs->packed_ref_store, o, wt) < 0)
+		ret = -1;
+
+	return ret;
 }
 
 struct ref_storage_be refs_be_files = {

-- 
2.52.0.590.g1f87b77810.dirty

Re: [PATCH v2 04/17] refs/files: remove useless indirection

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> The function `files_fsck_refs()` only has a single callsite and forwards
> all of its arguments as-is, so it's basically a useless indirection.
> Inline the function call.
>
> While at it, also remove the bitwise or that we have for return values.
> We don't really want to or them at all, but rather just want to return
> an error in case either of the functions has failed.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/files-backend.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 0a104c7bf6..4cbee23dad 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3954,22 +3954,20 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
>  	return ret;
>  }
>
> -static int files_fsck_refs(struct ref_store *ref_store,
> -			   struct fsck_options *o,
> -			   struct worktree *wt)
> -{
> -	return files_fsck_refs_dir(ref_store, o, wt);
> -}
> -
>  static int files_fsck(struct ref_store *ref_store,
>  		      struct fsck_options *o,
>  		      struct worktree *wt)
>  {
>  	struct files_ref_store *refs =
>  		files_downcast(ref_store, REF_STORE_READ, "fsck");
> +	int ret = 0;
>
> -	return files_fsck_refs(ref_store, o, wt) |
> -	       refs->packed_ref_store->be->fsck(refs->packed_ref_store, o, wt);
> +	if (files_fsck_refs_dir(ref_store, o, wt) < 0)
> +		ret = -1;
> +	if (refs->packed_ref_store->be->fsck(refs->packed_ref_store, o, wt) < 0)
> +		ret = -1;
> +

I wonder if this should have been a logical or instead of the bitwise
or, but then we directly return so even that wouldn't work. This looks
good! Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v2 05/17] refs/files: extract function to check single ref

[Patrick Steinhardt]

When checking the consistency of references we create a directory
iterator and then verify each single reference in a loop. The logic to
perform the actual checks is embedded into that loop, which makes it
hard to reuse. But In a subsequent commit we're about to introduce a
second path that wants to verify references.

Prepare for this by extracting the logic to check a single reference
into a standalone function.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 80 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 29 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4cbee23dad..9972221f9f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3715,7 +3715,8 @@ static int files_ref_store_remove_on_disk(struct ref_store *ref_store,
 typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
 				  struct fsck_options *o,
 				  const char *refname,
-				  struct dir_iterator *iter);
+				  const char *path,
+				  int mode);
 
 static int files_fsck_symref_target(struct fsck_options *o,
 				    struct fsck_ref_report *report,
@@ -3772,7 +3773,8 @@ static int files_fsck_symref_target(struct fsck_options *o,
 static int files_fsck_refs_content(struct ref_store *ref_store,
 				   struct fsck_options *o,
 				   const char *target_name,
-				   struct dir_iterator *iter)
+				   const char *path,
+				   int mode)
 {
 	struct strbuf ref_content = STRBUF_INIT;
 	struct strbuf abs_gitdir = STRBUF_INIT;
@@ -3786,7 +3788,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 
 	report.path = target_name;
 
-	if (S_ISLNK(iter->st.st_mode)) {
+	if (S_ISLNK(mode)) {
 		const char *relative_referent_path = NULL;
 
 		ret = fsck_report_ref(o, &report,
@@ -3798,7 +3800,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 		if (!is_dir_sep(abs_gitdir.buf[abs_gitdir.len - 1]))
 			strbuf_addch(&abs_gitdir, '/');
 
-		strbuf_add_real_path(&ref_content, iter->path.buf);
+		strbuf_add_real_path(&ref_content, path);
 		skip_prefix(ref_content.buf, abs_gitdir.buf,
 			    &relative_referent_path);
 
@@ -3811,7 +3813,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 		goto cleanup;
 	}
 
-	if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) {
+	if (strbuf_read_file(&ref_content, path, 0) < 0) {
 		/*
 		 * Ref file could be removed by another concurrent process. We should
 		 * ignore this error and continue to the next ref.
@@ -3819,7 +3821,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 		if (errno == ENOENT)
 			goto cleanup;
 
-		ret = error_errno(_("cannot read ref file '%s'"), iter->path.buf);
+		ret = error_errno(_("cannot read ref file '%s'"), path);
 		goto cleanup;
 	}
 
@@ -3861,16 +3863,20 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
 				struct fsck_options *o,
 				const char *refname,
-				struct dir_iterator *iter)
+				const char *path,
+				int mode UNUSED)
 {
 	struct strbuf sb = STRBUF_INIT;
+	const char *filename;
 	int ret = 0;
 
+	filename = basename((char *) path);
+
 	/*
 	 * Ignore the files ending with ".lock" as they may be lock files
 	 * However, do not allow bare ".lock" files.
 	 */
-	if (iter->basename[0] != '.' && ends_with(iter->basename, ".lock"))
+	if (filename[0] != '.' && ends_with(filename, ".lock"))
 		goto cleanup;
 
 	/*
@@ -3896,6 +3902,35 @@ static const files_fsck_refs_fn fsck_refs_fn[]= {
 	NULL,
 };
 
+static int files_fsck_ref(struct ref_store *ref_store,
+			  struct fsck_options *o,
+			  const char *refname,
+			  const char *path,
+			  int mode)
+{
+	int ret = 0;
+
+	if (o->verbose)
+		fprintf_ln(stderr, "Checking %s", refname);
+
+	if (!S_ISREG(mode) && !S_ISLNK(mode)) {
+		struct fsck_ref_report report = { .path = refname };
+
+		if (fsck_report_ref(o, &report,
+				    FSCK_MSG_BAD_REF_FILETYPE,
+				    "unexpected file type"))
+			ret = -1;
+		goto out;
+	}
+
+	for (size_t i = 0; fsck_refs_fn[i]; i++)
+		if (fsck_refs_fn[i](ref_store, o, refname, path, mode))
+			ret = -1;
+
+out:
+	return ret;
+}
+
 static int files_fsck_refs_dir(struct ref_store *ref_store,
 			       struct fsck_options *o,
 			       struct worktree *wt)
@@ -3918,30 +3953,17 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
 	}
 
 	while ((iter_status = dir_iterator_advance(iter)) == ITER_OK) {
-		if (S_ISDIR(iter->st.st_mode)) {
+		if (S_ISDIR(iter->st.st_mode))
 			continue;
-		} else if (S_ISREG(iter->st.st_mode) ||
-			   S_ISLNK(iter->st.st_mode)) {
-			strbuf_reset(&refname);
-
-			if (!is_main_worktree(wt))
-				strbuf_addf(&refname, "worktrees/%s/", wt->id);
-			strbuf_addf(&refname, "refs/%s", iter->relative_path);
 
-			if (o->verbose)
-				fprintf_ln(stderr, "Checking %s", refname.buf);
+		strbuf_reset(&refname);
+		if (!is_main_worktree(wt))
+			strbuf_addf(&refname, "worktrees/%s/", wt->id);
+		strbuf_addf(&refname, "refs/%s", iter->relative_path);
 
-			for (size_t i = 0; fsck_refs_fn[i]; i++) {
-				if (fsck_refs_fn[i](ref_store, o, refname.buf, iter))
-					ret = -1;
-			}
-		} else {
-			struct fsck_ref_report report = { .path = iter->basename };
-			if (fsck_report_ref(o, &report,
-					    FSCK_MSG_BAD_REF_FILETYPE,
-					    "unexpected file type"))
-				ret = -1;
-		}
+		if (files_fsck_ref(ref_store, o, refname.buf,
+				   iter->path.buf, iter->st.st_mode) < 0)
+			ret = -1;
 	}
 
 	if (iter_status != ITER_DONE)

-- 
2.52.0.590.g1f87b77810.dirty

[PATCH v2 06/17] refs/files: improve error handling when verifying symrefs

[Patrick Steinhardt]

The error handling when verifying symbolic refs is a bit on the wild
side:

  - `fsck_report_ref()` can be told to ignore specific errors. If an
    error has been ignored and a previous check raised an unignored
    error, then assigning `ret = fsck_report_ref()` will cause us to
    swallow the previous error.

  - When the target reference is not valid we bail out early without
    checking for other errors.

Fix both of these issues by consistently or'ing the return value and not
bailing out early.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 9972221f9f..abc2165339 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3737,17 +3737,15 @@ static int files_fsck_symref_target(struct fsck_options *o,
 	if (!is_referent_root &&
 	    !starts_with(referent->buf, "refs/") &&
 	    !starts_with(referent->buf, "worktrees/")) {
-		ret = fsck_report_ref(o, report,
-				      FSCK_MSG_SYMREF_TARGET_IS_NOT_A_REF,
-				      "points to non-ref target '%s'", referent->buf);
-
+		ret |= fsck_report_ref(o, report,
+				       FSCK_MSG_SYMREF_TARGET_IS_NOT_A_REF,
+				       "points to non-ref target '%s'", referent->buf);
 	}
 
 	if (!is_referent_root && check_refname_format(referent->buf, 0)) {
-		ret = fsck_report_ref(o, report,
-				      FSCK_MSG_BAD_REFERENT_NAME,
-				      "points to invalid refname '%s'", referent->buf);
-		goto out;
+		ret |= fsck_report_ref(o, report,
+				       FSCK_MSG_BAD_REFERENT_NAME,
+				       "points to invalid refname '%s'", referent->buf);
 	}
 
 	if (symbolic_link)
@@ -3755,19 +3753,19 @@ static int files_fsck_symref_target(struct fsck_options *o,
 
 	if (referent->len == orig_len ||
 	    (referent->len < orig_len && orig_last_byte != '\n')) {
-		ret = fsck_report_ref(o, report,
-				      FSCK_MSG_REF_MISSING_NEWLINE,
-				      "misses LF at the end");
+		ret |= fsck_report_ref(o, report,
+				       FSCK_MSG_REF_MISSING_NEWLINE,
+				       "misses LF at the end");
 	}
 
 	if (referent->len != orig_len && referent->len != orig_len - 1) {
-		ret = fsck_report_ref(o, report,
-				      FSCK_MSG_TRAILING_REF_CONTENT,
-				      "has trailing whitespaces or newlines");
+		ret |= fsck_report_ref(o, report,
+				       FSCK_MSG_TRAILING_REF_CONTENT,
+				       "has trailing whitespaces or newlines");
 	}
 
 out:
-	return ret;
+	return ret ? -1 : 0;
 }
 
 static int files_fsck_refs_content(struct ref_store *ref_store,

-- 
2.52.0.590.g1f87b77810.dirty

[PATCH v2 07/17] refs/files: perform consistency checks for root refs

[Patrick Steinhardt]

While the "files" backend already knows to perform consistency checks
for the "refs/" hierarchy, it doesn't verify any of its root refs. Plug
this omission.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/files-backend.c     | 50 +++++++++++++++++++++++++++++++++++++++++++++---
 t/t0602-reffiles-fsck.sh | 30 +++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index abc2165339..9ae80b700a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3877,9 +3877,9 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
 	if (filename[0] != '.' && ends_with(filename, ".lock"))
 		goto cleanup;
 
-	/*
-	 * This works right now because we never check the root refs.
-	 */
+	if (is_root_ref(refname))
+		goto cleanup;
+
 	if (check_refname_format(refname, 0)) {
 		struct fsck_ref_report report = { 0 };
 
@@ -3974,19 +3974,63 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
 	return ret;
 }
 
+struct files_fsck_root_ref_data {
+	struct files_ref_store *refs;
+	struct fsck_options *o;
+	struct worktree *wt;
+	struct strbuf refname;
+	struct strbuf path;
+};
+
+static int files_fsck_root_ref(const char *refname, void *cb_data)
+{
+	struct files_fsck_root_ref_data *data = cb_data;
+	struct stat st;
+
+	strbuf_reset(&data->refname);
+	if (!is_main_worktree(data->wt))
+		strbuf_addf(&data->refname, "worktrees/%s/", data->wt->id);
+	strbuf_addstr(&data->refname, refname);
+
+	strbuf_reset(&data->path);
+	strbuf_addf(&data->path, "%s/%s", data->refs->gitcommondir, data->refname.buf);
+
+	if (stat(data->path.buf, &st)) {
+		if (errno == ENOENT)
+			return 0;
+		return error_errno("failed to read ref: '%s'", data->path.buf);
+	}
+
+	return files_fsck_ref(&data->refs->base, data->o, data->refname.buf,
+			      data->path.buf, st.st_mode);
+}
+
 static int files_fsck(struct ref_store *ref_store,
 		      struct fsck_options *o,
 		      struct worktree *wt)
 {
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_READ, "fsck");
+	struct files_fsck_root_ref_data data = {
+		.refs = refs,
+		.o = o,
+		.wt = wt,
+		.refname = STRBUF_INIT,
+		.path = STRBUF_INIT,
+	};
 	int ret = 0;
 
 	if (files_fsck_refs_dir(ref_store, o, wt) < 0)
 		ret = -1;
+
+	if (for_each_root_ref(refs, files_fsck_root_ref, &data) < 0)
+		ret = -1;
+
 	if (refs->packed_ref_store->be->fsck(refs->packed_ref_store, o, wt) < 0)
 		ret = -1;
 
+	strbuf_release(&data.refname);
+	strbuf_release(&data.path);
 	return ret;
 }
 
diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
index 0ef483659d..479f3d528e 100755
--- a/t/t0602-reffiles-fsck.sh
+++ b/t/t0602-reffiles-fsck.sh
@@ -905,4 +905,34 @@ test_expect_success '--[no-]references option should apply to fsck' '
 	)
 '
 
+test_expect_success 'complains about broken root ref' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		echo "ref: refs/../HEAD" >.git/HEAD &&
+		test_must_fail git refs verify 2>err &&
+		cat >expect <<-EOF &&
+		error: HEAD: badReferentName: points to invalid refname ${SQ}refs/../HEAD${SQ}
+		EOF
+		test_cmp expect err
+	)
+'
+
+test_expect_success 'complains about broken root ref in worktree' '
+	test_when_finished "rm -rf repo worktree" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit initial &&
+		git worktree add ../worktree &&
+		echo "ref: refs/../HEAD" >.git/worktrees/worktree/HEAD &&
+		test_must_fail git refs verify 2>err &&
+		cat >expect <<-EOF &&
+		error: worktrees/worktree/HEAD: badReferentName: points to invalid refname ${SQ}refs/../HEAD${SQ}
+		EOF
+		test_cmp expect err
+	)
+'
+
 test_done

-- 
2.52.0.590.g1f87b77810.dirty

[PATCH v2 08/17] fsck: drop unused fields from `struct fsck_ref_report`

[Patrick Steinhardt]

The `struct fsck_ref_report` has a couple fields that are intended to
improve the error reporting for broken ref reports by showing which
object ID or target reference the ref points to. These fields are never
set though and are thus essentially unused.

Remove them.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 fsck.c | 5 -----
 fsck.h | 2 --
 2 files changed, 7 deletions(-)

diff --git a/fsck.c b/fsck.c
index fae18d8561..813d927d57 100644
--- a/fsck.c
+++ b/fsck.c
@@ -1310,11 +1310,6 @@ int fsck_refs_error_function(struct fsck_options *options UNUSED,
 
 	strbuf_addstr(&sb, report->path);
 
-	if (report->oid)
-		strbuf_addf(&sb, " -> (%s)", oid_to_hex(report->oid));
-	else if (report->referent)
-		strbuf_addf(&sb, " -> (%s)", report->referent);
-
 	if (msg_type == FSCK_WARN)
 		warning("%s: %s", sb.buf, message);
 	else
diff --git a/fsck.h b/fsck.h
index 336917c045..bfe0d9c6d2 100644
--- a/fsck.h
+++ b/fsck.h
@@ -162,8 +162,6 @@ struct fsck_object_report {
 
 struct fsck_ref_report {
 	const char *path;
-	const struct object_id *oid;
-	const char *referent;
 };
 
 struct fsck_options {

-- 
2.52.0.590.g1f87b77810.dirty

[PATCH v2 09/17] refs/files: extract generic symref target checks

[Patrick Steinhardt]

The consistency checks for the "files" backend contain a couple of
verifications for symrefs that verify generic properties of the target
reference. These properties need to hold for every backend, no matter
whether it's using the "files" or "reftable" backend.

Reimplementing these checks for every single backend doesn't really make
sense. Extract it into a generic `refs_fsck_symref()` function that can
be used by other backends, as well. The "reftable" backend will be wired
up in a subsequent commit.

While at it, improve the consistency checks so that we don't complain
about refs pointing to a non-ref target in case the target refname
format does not verify. Otherwise it's very likely that we'll generate
both error messages, which feels somewhat redundant in this case.

Note that the function has a couple of `UNUSED` parameters. These will
become referenced in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c               | 21 ++++++++++++++++++++
 refs.h               | 10 ++++++++++
 refs/files-backend.c | 54 ++++++++++++++++++++--------------------------------
 3 files changed, 52 insertions(+), 33 deletions(-)

diff --git a/refs.c b/refs.c
index e06e0cb072..739bf9fefc 100644
--- a/refs.c
+++ b/refs.c
@@ -320,6 +320,27 @@ int check_refname_format(const char *refname, int flags)
 	return check_or_sanitize_refname(refname, flags, NULL);
 }
 
+int refs_fsck_symref(struct ref_store *refs UNUSED, struct fsck_options *o,
+		     struct fsck_ref_report *report,
+		     const char *refname UNUSED, const char *target)
+{
+	if (is_root_ref(target))
+		return 0;
+
+	if (check_refname_format(target, 0) &&
+	    fsck_report_ref(o, report, FSCK_MSG_BAD_REFERENT_NAME,
+			    "points to invalid refname '%s'", target))
+		return -1;
+
+	if (!starts_with(target, "refs/") &&
+	    !starts_with(target, "worktrees/") &&
+	    fsck_report_ref(o, report, FSCK_MSG_SYMREF_TARGET_IS_NOT_A_REF,
+			    "points to non-ref target '%s'", target))
+		return -1;
+
+	return 0;
+}
+
 int refs_fsck(struct ref_store *refs, struct fsck_options *o,
 	      struct worktree *wt)
 {
diff --git a/refs.h b/refs.h
index d9051bbb04..d91fcb2d2f 100644
--- a/refs.h
+++ b/refs.h
@@ -653,6 +653,16 @@ int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_dat
  */
 int check_refname_format(const char *refname, int flags);
 
+struct fsck_ref_report;
+
+/*
+ * Perform generic checks for a specific symref target. This function is
+ * expected to be called by the ref backends for every symbolic ref.
+ */
+int refs_fsck_symref(struct ref_store *refs, struct fsck_options *o,
+		     struct fsck_ref_report *report,
+		     const char *refname, const char *target);
+
 /*
  * Check the reference database for consistency. Return 0 if refs and
  * reflogs are consistent, and non-zero otherwise. The errors will be
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 9ae80b700a..687c26ddcb 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3718,53 +3718,39 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
 				  const char *path,
 				  int mode);
 
-static int files_fsck_symref_target(struct fsck_options *o,
+static int files_fsck_symref_target(struct ref_store *ref_store,
+				    struct fsck_options *o,
 				    struct fsck_ref_report *report,
+				    const char *refname,
 				    struct strbuf *referent,
 				    unsigned int symbolic_link)
 {
-	int is_referent_root;
 	char orig_last_byte;
 	size_t orig_len;
 	int ret = 0;
 
 	orig_len = referent->len;
 	orig_last_byte = referent->buf[orig_len - 1];
-	if (!symbolic_link)
-		strbuf_rtrim(referent);
-
-	is_referent_root = is_root_ref(referent->buf);
-	if (!is_referent_root &&
-	    !starts_with(referent->buf, "refs/") &&
-	    !starts_with(referent->buf, "worktrees/")) {
-		ret |= fsck_report_ref(o, report,
-				       FSCK_MSG_SYMREF_TARGET_IS_NOT_A_REF,
-				       "points to non-ref target '%s'", referent->buf);
-	}
 
-	if (!is_referent_root && check_refname_format(referent->buf, 0)) {
-		ret |= fsck_report_ref(o, report,
-				       FSCK_MSG_BAD_REFERENT_NAME,
-				       "points to invalid refname '%s'", referent->buf);
-	}
+	if (!symbolic_link) {
+		strbuf_rtrim(referent);
 
-	if (symbolic_link)
-		goto out;
+		if (referent->len == orig_len ||
+		    (referent->len < orig_len && orig_last_byte != '\n')) {
+			ret |= fsck_report_ref(o, report,
+					       FSCK_MSG_REF_MISSING_NEWLINE,
+					       "misses LF at the end");
+		}
 
-	if (referent->len == orig_len ||
-	    (referent->len < orig_len && orig_last_byte != '\n')) {
-		ret |= fsck_report_ref(o, report,
-				       FSCK_MSG_REF_MISSING_NEWLINE,
-				       "misses LF at the end");
+		if (referent->len != orig_len && referent->len != orig_len - 1) {
+			ret |= fsck_report_ref(o, report,
+					       FSCK_MSG_TRAILING_REF_CONTENT,
+					       "has trailing whitespaces or newlines");
+		}
 	}
 
-	if (referent->len != orig_len && referent->len != orig_len - 1) {
-		ret |= fsck_report_ref(o, report,
-				       FSCK_MSG_TRAILING_REF_CONTENT,
-				       "has trailing whitespaces or newlines");
-	}
+	ret |= refs_fsck_symref(ref_store, o, report, refname, referent->buf);
 
-out:
 	return ret ? -1 : 0;
 }
 
@@ -3807,7 +3793,8 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 		else
 			strbuf_addbuf(&referent, &ref_content);
 
-		ret |= files_fsck_symref_target(o, &report, &referent, 1);
+		ret |= files_fsck_symref_target(ref_store, o, &report,
+						target_name, &referent, 1);
 		goto cleanup;
 	}
 
@@ -3847,7 +3834,8 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 			goto cleanup;
 		}
 	} else {
-		ret = files_fsck_symref_target(o, &report, &referent, 0);
+		ret = files_fsck_symref_target(ref_store, o, &report,
+					       target_name, &referent, 0);
 		goto cleanup;
 	}
 

-- 
2.52.0.590.g1f87b77810.dirty

[PATCH v2 10/17] refs/files: introduce function to perform normal ref checks

[Patrick Steinhardt]

In a subsequent commit we'll introduce new generic checks for direct
refs. These checks will be independent of the actual backend.

Introduce a new function `refs_fsck_ref()` that will be used for this
purpose. At the current point in time it's still empty, but it will get
populated in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c               | 7 +++++++
 refs.h               | 8 ++++++++
 refs/files-backend.c | 2 ++
 3 files changed, 17 insertions(+)

diff --git a/refs.c b/refs.c
index 739bf9fefc..4fc1317cb3 100644
--- a/refs.c
+++ b/refs.c
@@ -320,6 +320,13 @@ int check_refname_format(const char *refname, int flags)
 	return check_or_sanitize_refname(refname, flags, NULL);
 }
 
+int refs_fsck_ref(struct ref_store *refs UNUSED, struct fsck_options *o UNUSED,
+		  struct fsck_ref_report *report UNUSED,
+		  const char *refname UNUSED, const struct object_id *oid UNUSED)
+{
+	return 0;
+}
+
 int refs_fsck_symref(struct ref_store *refs UNUSED, struct fsck_options *o,
 		     struct fsck_ref_report *report,
 		     const char *refname UNUSED, const char *target)
diff --git a/refs.h b/refs.h
index d91fcb2d2f..f0abfa1d93 100644
--- a/refs.h
+++ b/refs.h
@@ -655,6 +655,14 @@ int check_refname_format(const char *refname, int flags);
 
 struct fsck_ref_report;
 
+/*
+ * Perform generic checks for a specific direct ref. This function is
+ * expected to be called by the ref backends for every symbolic ref.
+ */
+int refs_fsck_ref(struct ref_store *refs, struct fsck_options *o,
+		  struct fsck_ref_report *report,
+		  const char *refname, const struct object_id *oid);
+
 /*
  * Perform generic checks for a specific symref target. This function is
  * expected to be called by the ref backends for every symbolic ref.
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 687c26ddcb..240d3c3b26 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3833,6 +3833,8 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 					      "has trailing garbage: '%s'", trailing);
 			goto cleanup;
 		}
+
+		ret = refs_fsck_ref(ref_store, o, &report, target_name, &oid);
 	} else {
 		ret = files_fsck_symref_target(ref_store, o, &report,
 					       target_name, &referent, 0);

-- 
2.52.0.590.g1f87b77810.dirty

Re: [PATCH v2 10/17] refs/files: introduce function to perform normal ref checks

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 372 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> In a subsequent commit we'll introduce new generic checks for direct
> refs. These checks will be independent of the actual backend.

I don't think we've used the terminology 'direct refs' before. Took
me a second to understand. We generally use 'regular refs', but that
includes symrefs, so I think this does make sense.

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v2 10/17] refs/files: introduce function to perform normal ref checks

[Patrick Steinhardt]

On Mon, Jan 12, 2026 at 06:42:04AM -0500, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > In a subsequent commit we'll introduce new generic checks for direct
> > refs. These checks will be independent of the actual backend.
> 
> I don't think we've used the terminology 'direct refs' before. Took
> me a second to understand. We generally use 'regular refs', but that
> includes symrefs, so I think this does make sense.

Yeah, I didn't really know what to call these other than "direct refs".
We could instead say "non-symbolic refs", but that also feels kind of
awkward. So I guess this is good enough...?

Patrick

Re: [PATCH v2 10/17] refs/files: introduce function to perform normal ref checks

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Jan 12, 2026 at 06:42:04AM -0500, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> 
>> > In a subsequent commit we'll introduce new generic checks for direct
>> > refs. These checks will be independent of the actual backend.
>> 
>> I don't think we've used the terminology 'direct refs' before. Took
>> me a second to understand. We generally use 'regular refs', but that
>> includes symrefs, so I think this does make sense.
>
> Yeah, I didn't really know what to call these other than "direct refs".
> We could instead say "non-symbolic refs", but that also feels kind of
> awkward. So I guess this is good enough...?

The latter is understandable, if awkward.  The former is not.

Thanks.

Re: [PATCH v2 10/17] refs/files: introduce function to perform normal ref checks

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>> On Mon, Jan 12, 2026 at 06:42:04AM -0500, Karthik Nayak wrote:
>>> Patrick Steinhardt <ps@pks.im> writes:
>>> 
>>> > In a subsequent commit we'll introduce new generic checks for direct
>>> > refs. These checks will be independent of the actual backend.
>>> 
>>> I don't think we've used the terminology 'direct refs' before. Took
>>> me a second to understand. We generally use 'regular refs', but that
>>> includes symrefs, so I think this does make sense.
>>
>> Yeah, I didn't really know what to call these other than "direct refs".
>> We could instead say "non-symbolic refs", but that also feels kind of
>> awkward. So I guess this is good enough...?
>
> The latter is understandable, if awkward.  The former is not.

Well, I failed to elaborate why I think "the former is not".

The former would have been, if we were calling HEAD as "indirect
ref", instead of "symbolic ref".  But we use the latter, hence
"direct ref" is much less understandable than "non-symbolic ref".

Thanks.

Re: [PATCH v2 10/17] refs/files: introduce function to perform normal ref checks

[Patrick Steinhardt]

On Mon, Jan 12, 2026 at 06:37:05AM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> >> On Mon, Jan 12, 2026 at 06:42:04AM -0500, Karthik Nayak wrote:
> >>> Patrick Steinhardt <ps@pks.im> writes:
> >>> 
> >>> > In a subsequent commit we'll introduce new generic checks for direct
> >>> > refs. These checks will be independent of the actual backend.
> >>> 
> >>> I don't think we've used the terminology 'direct refs' before. Took
> >>> me a second to understand. We generally use 'regular refs', but that
> >>> includes symrefs, so I think this does make sense.
> >>
> >> Yeah, I didn't really know what to call these other than "direct refs".
> >> We could instead say "non-symbolic refs", but that also feels kind of
> >> awkward. So I guess this is good enough...?
> >
> > The latter is understandable, if awkward.  The former is not.
> 
> Well, I failed to elaborate why I think "the former is not".
> 
> The former would have been, if we were calling HEAD as "indirect
> ref", instead of "symbolic ref".  But we use the latter, hence
> "direct ref" is much less understandable than "non-symbolic ref".

Fair enough. I've queued this change locally and will send it out with
the next iteration. Thanks!

Patrick

[PATCH v2 11/17] refs/reftable: adapt includes to become consistent

[Patrick Steinhardt]

Adapt the includes to be sorted and to use include paths that are
relative to the "refs/" directory.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 4319a4eacb..d61790cf65 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -10,9 +10,10 @@
 #include "../gettext.h"
 #include "../hash.h"
 #include "../hex.h"
-#include "../iterator.h"
 #include "../ident.h"
+#include "../iterator.h"
 #include "../object.h"
+#include "../parse.h"
 #include "../path.h"
 #include "../refs.h"
 #include "../reftable/reftable-basics.h"
@@ -26,7 +27,6 @@
 #include "../strmap.h"
 #include "../trace2.h"
 #include "../write-or-die.h"
-#include "parse.h"
 #include "refs-internal.h"
 
 /*

-- 
2.52.0.590.g1f87b77810.dirty

[PATCH v2 12/17] refs/reftable: extract function to retrieve backend for worktree

[Patrick Steinhardt]

Pull out the logic to retrieve a backend for a given worktree. This
function will be used in a subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c | 70 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 27 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index d61790cf65..dda961a32b 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -172,6 +172,37 @@ static struct reftable_ref_store *reftable_be_downcast(struct ref_store *ref_sto
 	return refs;
 }
 
+static int backend_for_worktree(struct reftable_backend **out,
+				struct reftable_ref_store *store,
+				const char *worktree_name)
+{
+	struct strbuf worktree_dir = STRBUF_INIT;
+	int ret;
+
+	*out = strmap_get(&store->worktree_backends, worktree_name);
+	if (*out) {
+		ret = 0;
+		goto out;
+	}
+
+	strbuf_addf(&worktree_dir, "%s/worktrees/%s/reftable",
+		    store->base.repo->commondir, worktree_name);
+
+	CALLOC_ARRAY(*out, 1);
+	store->err = ret = reftable_backend_init(*out, worktree_dir.buf,
+						 &store->write_options);
+	if (ret < 0) {
+		free(*out);
+		goto out;
+	}
+
+	strmap_put(&store->worktree_backends, worktree_name, *out);
+
+out:
+	strbuf_release(&worktree_dir);
+	return ret;
+}
+
 /*
  * Some refs are global to the repository (refs/heads/{*}), while others are
  * local to the worktree (eg. HEAD, refs/bisect/{*}). We solve this by having
@@ -191,19 +222,19 @@ static int backend_for(struct reftable_backend **out,
 		       const char **rewritten_ref,
 		       int reload)
 {
-	struct reftable_backend *be;
 	const char *wtname;
 	int wtname_len;
+	int ret;
 
 	if (!refname) {
-		be = &store->main_backend;
+		*out = &store->main_backend;
+		ret = 0;
 		goto out;
 	}
 
 	switch (parse_worktree_ref(refname, &wtname, &wtname_len, rewritten_ref)) {
 	case REF_WORKTREE_OTHER: {
 		static struct strbuf wtname_buf = STRBUF_INIT;
-		struct strbuf wt_dir = STRBUF_INIT;
 
 		/*
 		 * We're using a static buffer here so that we don't need to
@@ -223,20 +254,8 @@ static int backend_for(struct reftable_backend **out,
 		 * already and error out when trying to write a reference via
 		 * both stacks.
 		 */
-		be = strmap_get(&store->worktree_backends, wtname_buf.buf);
-		if (!be) {
-			strbuf_addf(&wt_dir, "%s/worktrees/%s/reftable",
-				    store->base.repo->commondir, wtname_buf.buf);
+		ret = backend_for_worktree(out, store, wtname_buf.buf);
 
-			CALLOC_ARRAY(be, 1);
-			store->err = reftable_backend_init(be, wt_dir.buf,
-							   &store->write_options);
-			assert(store->err != REFTABLE_API_ERROR);
-
-			strmap_put(&store->worktree_backends, wtname_buf.buf, be);
-		}
-
-		strbuf_release(&wt_dir);
 		goto out;
 	}
 	case REF_WORKTREE_CURRENT:
@@ -245,27 +264,24 @@ static int backend_for(struct reftable_backend **out,
 		 * main worktree. We thus return the main stack in that case.
 		 */
 		if (!store->worktree_backend.stack)
-			be = &store->main_backend;
+			*out = &store->main_backend;
 		else
-			be = &store->worktree_backend;
+			*out = &store->worktree_backend;
+		ret = 0;
 		goto out;
 	case REF_WORKTREE_MAIN:
 	case REF_WORKTREE_SHARED:
-		be = &store->main_backend;
+		*out = &store->main_backend;
+		ret = 0;
 		goto out;
 	default:
 		BUG("unhandled worktree reference type");
 	}
 
 out:
-	if (reload) {
-		int ret = reftable_stack_reload(be->stack);
-		if (ret)
-			return ret;
-	}
-	*out = be;
-
-	return 0;
+	if (reload && !ret)
+		ret = reftable_stack_reload((*out)->stack);
+	return ret;
 }
 
 static int should_write_log(struct reftable_ref_store *refs, const char *refname)

-- 
2.52.0.590.g1f87b77810.dirty

[PATCH v2 13/17] refs/reftable: fix consistency checks with worktrees

[Patrick Steinhardt]

The ref consistency checks are driven via `cmd_refs_verify()`. That
function loops through all worktrees (including the main worktree) and
then checks the ref store for each of them individually. It follows that
the backend is expected to only verify refs that belong to the specified
worktree.

While the "files" backend handles this correctly, the "reftable" backend
doesn't. In fact, it completely ignores the passed worktree and instead
verifies refs of _all_ worktrees. The consequence is that we'll end up
every ref store N times, where N is the number of worktrees.

Or rather, that would be the case if we actually iterated through the
worktree reftable stacks correctly. But we use `strmap_for_each_entry()`
to iterate through the stacks, but the map is in fact not even properly
populated. So instead of checking stacks N^2 times, we actually only end
up checking the reftable stack of the main worktree.

Fix this bug by only verifying the stack of the passed-in worktree and
constructing the backends via `backend_for_worktree()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c  | 29 ++++++++++++++---------------
 t/t0614-reftable-fsck.sh | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 15 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index dda961a32b..6361b27015 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -26,6 +26,7 @@
 #include "../setup.h"
 #include "../strmap.h"
 #include "../trace2.h"
+#include "../worktree.h"
 #include "../write-or-die.h"
 #include "refs-internal.h"
 
@@ -2762,25 +2763,23 @@ static int reftable_fsck_error_handler(struct reftable_fsck_info *info,
 }
 
 static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o,
-			    struct worktree *wt UNUSED)
+			    struct worktree *wt)
 {
-	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);
+	struct reftable_ref_store *refs =
+		reftable_be_downcast(ref_store, REF_STORE_READ, "fsck");
+	struct reftable_backend *backend;
 
-	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 (is_main_worktree(wt)) {
+		backend = &refs->main_backend;
+	} else {
+		int ret = backend_for_worktree(&backend, refs, wt->id);
+		if (ret < 0)
+			return error(_("reftable stack for worktree '%s' is broken"),
+				     wt->id);
 	}
 
-	return ret;
+	return reftable_fsck_check(backend->stack, reftable_fsck_error_handler,
+				   reftable_fsck_verbose_handler, o);
 }
 
 struct ref_storage_be refs_be_reftable = {
diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh
index 677eb9143c..4757eb5931 100755
--- a/t/t0614-reftable-fsck.sh
+++ b/t/t0614-reftable-fsck.sh
@@ -55,4 +55,36 @@ for TABLE_NAME in "foo-bar-e4d12d59.ref" \
 	'
 done
 
+test_expect_success 'worktree stacks can be verified' '
+	test_when_finished "rm -rf repo worktree" &&
+	git init repo &&
+	test_commit -C repo initial &&
+	git -C repo worktree add ../worktree &&
+
+	git -C worktree refs verify 2>err &&
+	test_must_be_empty err &&
+
+	REFTABLE_DIR=$(git -C worktree rev-parse --git-dir)/reftable &&
+	EXISTING_TABLE=$(head -n1 "$REFTABLE_DIR/tables.list") &&
+	mv "$REFTABLE_DIR/$EXISTING_TABLE" "$REFTABLE_DIR/broken.ref" &&
+
+	for d in repo worktree
+	do
+		echo "broken.ref" >"$REFTABLE_DIR/tables.list" &&
+		git -C "$d" refs verify 2>err &&
+		cat >expect <<-EOF &&
+		warning: broken.ref: badReftableTableName: invalid reftable table name
+		EOF
+		test_cmp expect err &&
+
+		echo garbage >"$REFTABLE_DIR/tables.list" &&
+		test_must_fail git -C "$d" refs verify 2>err &&
+		cat >expect <<-EOF &&
+		error: reftable stack for worktree ${SQ}worktree${SQ} is broken
+		EOF
+		test_cmp expect err || return 1
+
+	done
+'
+
 test_done

-- 
2.52.0.590.g1f87b77810.dirty

Re: [PATCH v2 13/17] refs/reftable: fix consistency checks with worktrees

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> The ref consistency checks are driven via `cmd_refs_verify()`. That
> function loops through all worktrees (including the main worktree) and
> then checks the ref store for each of them individually. It follows that
> the backend is expected to only verify refs that belong to the specified
> worktree.
>
> While the "files" backend handles this correctly, the "reftable" backend
> doesn't. In fact, it completely ignores the passed worktree and instead
> verifies refs of _all_ worktrees. The consequence is that we'll end up
> every ref store N times, where N is the number of worktrees.
>
> Or rather, that would be the case if we actually iterated through the
> worktree reftable stacks correctly. But we use `strmap_for_each_entry()`
> to iterate through the stacks, but the map is in fact not even properly
> populated. So instead of checking stacks N^2 times, we actually only end
> up checking the reftable stack of the main worktree.
>
> Fix this bug by only verifying the stack of the passed-in worktree and
> constructing the backends via `backend_for_worktree()`.
>

Ah, I was the author of this, I didn't know that the fsck function gets
called per worktree, so thanks for fixing it!

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v2 14/17] refs/reftable: introduce generic checks for refs

[Patrick Steinhardt]

In a preceding commit we have extracted generic checks for both direct
and symbolic refs that apply for all backends. Wire up those checks for
the "reftable" backend.

Note that this is done by iterating through all refs manually with the
low-level reftable ref iterator. We explicitly don't want to use the
higher-level iterator that is exposed to users of the reftable backend
as that iterator may swallow for example broken refs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs/reftable-backend.c  | 82 ++++++++++++++++++++++++++++++++++++++++++++----
 t/t0614-reftable-fsck.sh | 12 +++++++
 2 files changed, 88 insertions(+), 6 deletions(-)

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 6361b27015..fe74af73af 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -2767,19 +2767,89 @@ static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o,
 {
 	struct reftable_ref_store *refs =
 		reftable_be_downcast(ref_store, REF_STORE_READ, "fsck");
+	struct reftable_ref_iterator *iter = NULL;
+	struct reftable_ref_record ref = { 0 };
+	struct fsck_ref_report report = { 0 };
+	struct strbuf refname = STRBUF_INIT;
 	struct reftable_backend *backend;
+	int ret, errors = 0;
 
 	if (is_main_worktree(wt)) {
 		backend = &refs->main_backend;
 	} else {
-		int ret = backend_for_worktree(&backend, refs, wt->id);
-		if (ret < 0)
-			return error(_("reftable stack for worktree '%s' is broken"),
-				     wt->id);
+		ret = backend_for_worktree(&backend, refs, wt->id);
+		if (ret < 0) {
+			ret = error(_("reftable stack for worktree '%s' is broken"),
+				    wt->id);
+			goto out;
+		}
+	}
+
+	errors |= reftable_fsck_check(backend->stack, reftable_fsck_error_handler,
+				      reftable_fsck_verbose_handler, o);
+
+	iter = ref_iterator_for_stack(refs, backend->stack, "", NULL, 0);
+	if (!iter) {
+		ret = error(_("could not create iterator for worktree '%s'"), wt->id);
+		goto out;
+	}
+
+	while (1) {
+		ret = reftable_iterator_next_ref(&iter->iter, &ref);
+		if (ret > 0)
+			break;
+		if (ret < 0) {
+			ret = error(_("could not read record for worktree '%s'"), wt->id);
+			goto out;
+		}
+
+		strbuf_reset(&refname);
+		if (!is_main_worktree(wt))
+			strbuf_addf(&refname, "worktrees/%s/", wt->id);
+		strbuf_addstr(&refname, ref.refname);
+		report.path = refname.buf;
+
+		switch (ref.value_type) {
+		case REFTABLE_REF_VAL1:
+		case REFTABLE_REF_VAL2: {
+			struct object_id oid;
+			unsigned hash_id;
+
+			switch (reftable_stack_hash_id(backend->stack)) {
+			case REFTABLE_HASH_SHA1:
+				hash_id = GIT_HASH_SHA1;
+				break;
+			case REFTABLE_HASH_SHA256:
+				hash_id = GIT_HASH_SHA256;
+				break;
+			default:
+				BUG("unhandled hash ID %d",
+				    reftable_stack_hash_id(backend->stack));
+			}
+
+			oidread(&oid, reftable_ref_record_val1(&ref),
+				&hash_algos[hash_id]);
+
+			errors |= refs_fsck_ref(ref_store, o, &report, ref.refname, &oid);
+			break;
+		}
+		case REFTABLE_REF_SYMREF:
+			errors |= refs_fsck_symref(ref_store, o, &report, ref.refname,
+						   ref.value.symref);
+			break;
+		default:
+			BUG("unhandled reference value type %d", ref.value_type);
+		}
 	}
 
-	return reftable_fsck_check(backend->stack, reftable_fsck_error_handler,
-				   reftable_fsck_verbose_handler, o);
+	ret = errors ? -1 : 0;
+
+out:
+	if (iter)
+		ref_iterator_free(&iter->base);
+	reftable_ref_record_release(&ref);
+	strbuf_release(&refname);
+	return ret;
 }
 
 struct ref_storage_be refs_be_reftable = {
diff --git a/t/t0614-reftable-fsck.sh b/t/t0614-reftable-fsck.sh
index 4757eb5931..d24b87f961 100755
--- a/t/t0614-reftable-fsck.sh
+++ b/t/t0614-reftable-fsck.sh
@@ -87,4 +87,16 @@ test_expect_success 'worktree stacks can be verified' '
 	done
 '
 
+test_expect_success 'invalid symref gets reported' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	test_commit -C repo initial &&
+	git -C repo symbolic-ref refs/heads/symref garbage &&
+	test_must_fail git -C repo refs verify 2>err &&
+	cat >expect <<-EOF &&
+	error: refs/heads/symref: badReferentName: points to invalid refname ${SQ}garbage${SQ}
+	EOF
+	test_cmp expect err
+'
+
 test_done

-- 
2.52.0.590.g1f87b77810.dirty

[PATCH v2 15/17] builtin/fsck: move generic object ID checks into `refs_fsck()`

[Patrick Steinhardt]

While most of the logic that verifies the consistency of refs is
driven by `refs_fsck()`, we still have a small handful of checks in
`fsck_head_link()`. These checks don't use the git-fsck(1) reporting
infrastructure, and as such it's impossible to for example disable
some of those checks.

One such check detects refs that point to the all-zeroes object ID.
Extract this check into the generic `refs_fsck_ref()` function that is
used by both the "files" and "reftable" backends.

Note that this will cause us to not return an error code from
`fsck_head_link()` anymore in case this error was detected. This is fine
though: the only caller of this function does not check the error code
anyway. To demonstrate this, adapt the function to drop its return value
altogether. The function will be removed in a subsequent commit anyway.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/fsck-msgids.adoc |  3 +++
 builtin/fsck.c                 | 41 +++++++++++++++--------------------------
 fsck.h                         |  1 +
 refs.c                         | 11 ++++++++---
 t/t1450-fsck.sh                |  6 +++---
 5 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc
index acac9683af..76609321f6 100644
--- a/Documentation/fsck-msgids.adoc
+++ b/Documentation/fsck-msgids.adoc
@@ -41,6 +41,9 @@
 `badRefName`::
 	(ERROR) A ref has an invalid format.
 
+`badRefOid`::
+	(ERROR) A ref points to an invalid object ID.
+
 `badReferentName`::
 	(ERROR) The referent name of a symref is invalid.
 
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 4979bc795e..4dd4d74d1e 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -564,9 +564,9 @@ static int fsck_handle_ref(const struct reference *ref, void *cb_data UNUSED)
 	return 0;
 }
 
-static int fsck_head_link(const char *head_ref_name,
-			  const char **head_points_at,
-			  struct object_id *head_oid);
+static void fsck_head_link(const char *head_ref_name,
+			   const char **head_points_at,
+			   struct object_id *head_oid);
 
 static void get_default_heads(void)
 {
@@ -713,12 +713,10 @@ static void fsck_source(struct odb_source *source)
 	stop_progress(&progress);
 }
 
-static int fsck_head_link(const char *head_ref_name,
-			  const char **head_points_at,
-			  struct object_id *head_oid)
+static void fsck_head_link(const char *head_ref_name,
+			   const char **head_points_at,
+			   struct object_id *head_oid)
 {
-	int null_is_error = 0;
-
 	if (verbose)
 		fprintf_ln(stderr, _("Checking %s link"), head_ref_name);
 
@@ -727,27 +725,18 @@ static int fsck_head_link(const char *head_ref_name,
 						  NULL);
 	if (!*head_points_at) {
 		errors_found |= ERROR_REFS;
-		return error(_("invalid %s"), head_ref_name);
+		error(_("invalid %s"), head_ref_name);
+		return;
 	}
-	if (!strcmp(*head_points_at, head_ref_name))
-		/* detached HEAD */
-		null_is_error = 1;
-	else if (!starts_with(*head_points_at, "refs/heads/")) {
+	if (strcmp(*head_points_at, head_ref_name) &&
+	    !starts_with(*head_points_at, "refs/heads/")) {
 		errors_found |= ERROR_REFS;
-		return error(_("%s points to something strange (%s)"),
-			     head_ref_name, *head_points_at);
-	}
-	if (is_null_oid(head_oid)) {
-		if (null_is_error) {
-			errors_found |= ERROR_REFS;
-			return error(_("%s: detached HEAD points at nothing"),
-				     head_ref_name);
-		}
-		fprintf_ln(stderr,
-			   _("notice: %s points to an unborn branch (%s)"),
-			   head_ref_name, *head_points_at + 11);
+		error(_("%s points to something strange (%s)"),
+		      head_ref_name, *head_points_at);
+		return;
 	}
-	return 0;
+
+	return;
 }
 
 static int fsck_cache_tree(struct cache_tree *it, const char *index_path)
diff --git a/fsck.h b/fsck.h
index bfe0d9c6d2..1f472b7daa 100644
--- a/fsck.h
+++ b/fsck.h
@@ -39,6 +39,7 @@ enum fsck_msg_type {
 	FUNC(BAD_REF_CONTENT, ERROR) \
 	FUNC(BAD_REF_FILETYPE, ERROR) \
 	FUNC(BAD_REF_NAME, ERROR) \
+	FUNC(BAD_REF_OID, ERROR) \
 	FUNC(BAD_TIMEZONE, ERROR) \
 	FUNC(BAD_TREE, ERROR) \
 	FUNC(BAD_TREE_SHA1, ERROR) \
diff --git a/refs.c b/refs.c
index 4fc1317cb3..c3528862c6 100644
--- a/refs.c
+++ b/refs.c
@@ -320,10 +320,15 @@ int check_refname_format(const char *refname, int flags)
 	return check_or_sanitize_refname(refname, flags, NULL);
 }
 
-int refs_fsck_ref(struct ref_store *refs UNUSED, struct fsck_options *o UNUSED,
-		  struct fsck_ref_report *report UNUSED,
-		  const char *refname UNUSED, const struct object_id *oid UNUSED)
+int refs_fsck_ref(struct ref_store *refs UNUSED, struct fsck_options *o,
+		  struct fsck_ref_report *report,
+		  const char *refname UNUSED, const struct object_id *oid)
 {
+	if (is_null_oid(oid))
+		return fsck_report_ref(o, report, FSCK_MSG_BAD_REF_OID,
+				       "points to invalid object ID '%s'",
+				       oid_to_hex(oid));
+
 	return 0;
 }
 
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index c4b651c2dc..900c1b2eb2 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -105,7 +105,7 @@ test_expect_success REFFILES 'HEAD link pointing at a funny object' '
 	echo $ZERO_OID >.git/HEAD &&
 	# avoid corrupt/broken HEAD from interfering with repo discovery
 	test_must_fail env GIT_DIR=.git git fsck 2>out &&
-	test_grep "detached HEAD points" out
+	test_grep "HEAD: badRefOid: points to invalid object ID ${SQ}$ZERO_OID${SQ}" out
 '
 
 test_expect_success 'HEAD link pointing at a funny place' '
@@ -123,7 +123,7 @@ test_expect_success REFFILES 'HEAD link pointing at a funny object (from differe
 	echo $ZERO_OID >.git/HEAD &&
 	# avoid corrupt/broken HEAD from interfering with repo discovery
 	test_must_fail git -C wt fsck 2>out &&
-	test_grep "main-worktree/HEAD: detached HEAD points" out
+	test_grep "HEAD: badRefOid: points to invalid object ID ${SQ}$ZERO_OID${SQ}" out
 '
 
 test_expect_success REFFILES 'other worktree HEAD link pointing at a funny object' '
@@ -131,7 +131,7 @@ test_expect_success REFFILES 'other worktree HEAD link pointing at a funny objec
 	git worktree add other &&
 	echo $ZERO_OID >.git/worktrees/other/HEAD &&
 	test_must_fail git fsck 2>out &&
-	test_grep "worktrees/other/HEAD: detached HEAD points" out
+	test_grep "worktrees/other/HEAD: badRefOid: points to invalid object ID ${SQ}$ZERO_OID${SQ}" out
 '
 
 test_expect_success 'other worktree HEAD link pointing at missing object' '

-- 
2.52.0.590.g1f87b77810.dirty

[PATCH v2 16/17] builtin/fsck: move generic HEAD check into `refs_fsck()`

[Patrick Steinhardt]

Move the check that detects "HEAD" refs that do not point at a branch
into `refs_fsck()`. This follows the same motivation as the preceding
commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/fsck-msgids.adoc |  3 +++
 builtin/fsck.c                 |  7 -------
 fsck.h                         |  1 +
 refs.c                         | 12 +++++++++++-
 t/t0602-reffiles-fsck.sh       |  8 ++++----
 t/t1450-fsck.sh                |  4 ++--
 6 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc
index 76609321f6..6a4db3a991 100644
--- a/Documentation/fsck-msgids.adoc
+++ b/Documentation/fsck-msgids.adoc
@@ -13,6 +13,9 @@
 `badGpgsig`::
 	(ERROR) A tag contains a bad (truncated) signature (e.g., `gpgsig`) header.
 
+`badHeadTarget`::
+	(ERROR) The `HEAD` ref is a symref that does not refer to a branch.
+
 `badHeaderContinuation`::
 	(ERROR) A continuation header (such as for `gpgsig`) is unexpectedly truncated.
 
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 4dd4d74d1e..5dda441f45 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -728,13 +728,6 @@ static void fsck_head_link(const char *head_ref_name,
 		error(_("invalid %s"), head_ref_name);
 		return;
 	}
-	if (strcmp(*head_points_at, head_ref_name) &&
-	    !starts_with(*head_points_at, "refs/heads/")) {
-		errors_found |= ERROR_REFS;
-		error(_("%s points to something strange (%s)"),
-		      head_ref_name, *head_points_at);
-		return;
-	}
 
 	return;
 }
diff --git a/fsck.h b/fsck.h
index 1f472b7daa..65ecbb7fe1 100644
--- a/fsck.h
+++ b/fsck.h
@@ -30,6 +30,7 @@ enum fsck_msg_type {
 	FUNC(BAD_DATE_OVERFLOW, ERROR) \
 	FUNC(BAD_EMAIL, ERROR) \
 	FUNC(BAD_GPGSIG, ERROR) \
+	FUNC(BAD_HEAD_TARGET, ERROR) \
 	FUNC(BAD_NAME, ERROR) \
 	FUNC(BAD_OBJECT_SHA1, ERROR) \
 	FUNC(BAD_PACKED_REF_ENTRY, ERROR) \
diff --git a/refs.c b/refs.c
index c3528862c6..a772d371cd 100644
--- a/refs.c
+++ b/refs.c
@@ -334,8 +334,18 @@ int refs_fsck_ref(struct ref_store *refs UNUSED, struct fsck_options *o,
 
 int refs_fsck_symref(struct ref_store *refs UNUSED, struct fsck_options *o,
 		     struct fsck_ref_report *report,
-		     const char *refname UNUSED, const char *target)
+		     const char *refname, const char *target)
 {
+	const char *stripped_refname;
+
+	parse_worktree_ref(refname, NULL, NULL, &stripped_refname);
+
+	if (!strcmp(stripped_refname, "HEAD") &&
+	    !starts_with(target, "refs/heads/") &&
+	    fsck_report_ref(o, report, FSCK_MSG_BAD_HEAD_TARGET,
+			    "HEAD points to non-branch '%s'", target))
+		return -1;
+
 	if (is_root_ref(target))
 		return 0;
 
diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
index 479f3d528e..3c1f553b81 100755
--- a/t/t0602-reffiles-fsck.sh
+++ b/t/t0602-reffiles-fsck.sh
@@ -910,10 +910,10 @@ test_expect_success 'complains about broken root ref' '
 	git init repo &&
 	(
 		cd repo &&
-		echo "ref: refs/../HEAD" >.git/HEAD &&
+		echo "ref: refs/heads/../HEAD" >.git/HEAD &&
 		test_must_fail git refs verify 2>err &&
 		cat >expect <<-EOF &&
-		error: HEAD: badReferentName: points to invalid refname ${SQ}refs/../HEAD${SQ}
+		error: HEAD: badReferentName: points to invalid refname ${SQ}refs/heads/../HEAD${SQ}
 		EOF
 		test_cmp expect err
 	)
@@ -926,10 +926,10 @@ test_expect_success 'complains about broken root ref in worktree' '
 		cd repo &&
 		test_commit initial &&
 		git worktree add ../worktree &&
-		echo "ref: refs/../HEAD" >.git/worktrees/worktree/HEAD &&
+		echo "ref: refs/heads/../HEAD" >.git/worktrees/worktree/HEAD &&
 		test_must_fail git refs verify 2>err &&
 		cat >expect <<-EOF &&
-		error: worktrees/worktree/HEAD: badReferentName: points to invalid refname ${SQ}refs/../HEAD${SQ}
+		error: worktrees/worktree/HEAD: badReferentName: points to invalid refname ${SQ}refs/heads/../HEAD${SQ}
 		EOF
 		test_cmp expect err
 	)
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 900c1b2eb2..3fae05f9d9 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -113,7 +113,7 @@ test_expect_success 'HEAD link pointing at a funny place' '
 	test-tool ref-store main create-symref HEAD refs/funny/place &&
 	# avoid corrupt/broken HEAD from interfering with repo discovery
 	test_must_fail env GIT_DIR=.git git fsck 2>out &&
-	test_grep "HEAD points to something strange" out
+	test_grep "HEAD: badHeadTarget: HEAD points to non-branch ${SQ}refs/funny/place${SQ}" out
 '
 
 test_expect_success REFFILES 'HEAD link pointing at a funny object (from different wt)' '
@@ -148,7 +148,7 @@ test_expect_success 'other worktree HEAD link pointing at a funny place' '
 	git worktree add other &&
 	git -C other symbolic-ref HEAD refs/funny/place &&
 	test_must_fail git fsck 2>out &&
-	test_grep "worktrees/other/HEAD points to something strange" out
+	test_grep "worktrees/other/HEAD: badHeadTarget: HEAD points to non-branch ${SQ}refs/funny/place${SQ}" out
 '
 
 test_expect_success 'commit with multiple signatures is okay' '

-- 
2.52.0.590.g1f87b77810.dirty

[PATCH v2 17/17] builtin/fsck: drop `fsck_head_link()`

[Patrick Steinhardt]

The function `fsck_head_link()` was historically used to perform a
couple of consistency checks for refs. (Almost) all of these checks have
now been moved into the refs subsystem. There's only a single check
remaining that verifies whether `refs_resolve_ref_unsafe()` returns a
`NULL` pointer. This may happen in a couple of cases:

  - When `refs_is_safe()` declares the ref to be unsafe. We already have
    checks for this as we verify refnames with `check_refname_format()`.

  - When the ref doesn't exist. A repository without "HEAD" is
    completely broken though, and we would notice this error ahead of
    time already.

  - In case the caller passes `RESOLVE_REF_READING` and the ref is a
    symref that doesn't resolve. We don't pass this flag though.

As such, this check doesn't cover anything anymore that isn't already
covered by `refs_fsck()`. Drop it, which also allows us to inline the
call to `refs_resolve_ref_unsafe()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c | 28 ++++------------------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 5dda441f45..f104b7af0e 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -564,10 +564,6 @@ static int fsck_handle_ref(const struct reference *ref, void *cb_data UNUSED)
 	return 0;
 }
 
-static void fsck_head_link(const char *head_ref_name,
-			   const char **head_points_at,
-			   struct object_id *head_oid);
-
 static void get_default_heads(void)
 {
 	struct worktree **worktrees, **p;
@@ -583,7 +579,10 @@ static void get_default_heads(void)
 		struct strbuf refname = STRBUF_INIT;
 
 		strbuf_worktree_ref(wt, &refname, "HEAD");
-		fsck_head_link(refname.buf, &head_points_at, &head_oid);
+
+		head_points_at = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
+							 refname.buf, 0, &head_oid, NULL);
+
 		if (head_points_at && !is_null_oid(&head_oid)) {
 			struct reference ref = {
 				.name = refname.buf,
@@ -713,25 +712,6 @@ static void fsck_source(struct odb_source *source)
 	stop_progress(&progress);
 }
 
-static void fsck_head_link(const char *head_ref_name,
-			   const char **head_points_at,
-			   struct object_id *head_oid)
-{
-	if (verbose)
-		fprintf_ln(stderr, _("Checking %s link"), head_ref_name);
-
-	*head_points_at = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
-						  head_ref_name, 0, head_oid,
-						  NULL);
-	if (!*head_points_at) {
-		errors_found |= ERROR_REFS;
-		error(_("invalid %s"), head_ref_name);
-		return;
-	}
-
-	return;
-}
-
 static int fsck_cache_tree(struct cache_tree *it, const char *index_path)
 {
 	int i;

-- 
2.52.0.590.g1f87b77810.dirty

Re: [PATCH v2 00/17] Fixes and improvements for ref consistency checks

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 729 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Hi,
>
> this patch series contains a bunch of fixes and improvements for ref
> consistency checks. It is structured as follows:
>
>   - Patches 1 to 4 contain a couple of cleanups for the consistency
>     checks done by the "files" backend.
>
>   - Patches 5 to 7 introduce checks for root refs for the "files"
>     backend.
>
>   - Patches 9 to 14 introduce infrastructure for shared checks with the
>     "files" and "reftable" backend.
>
>   - Patches 15 to 17 move some ref consistency checks that were still
>     driven by git-fsck(1) into `git refs verify`.
>

I reviewed the series and it already looks good, thanks for fixing some
of the broken parts and cleaning up.

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v2 00/17] Fixes and improvements for ref consistency checks

[Patrick Steinhardt]

On Mon, Jan 12, 2026 at 06:50:17AM -0500, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Hi,
> >
> > this patch series contains a bunch of fixes and improvements for ref
> > consistency checks. It is structured as follows:
> >
> >   - Patches 1 to 4 contain a couple of cleanups for the consistency
> >     checks done by the "files" backend.
> >
> >   - Patches 5 to 7 introduce checks for root refs for the "files"
> >     backend.
> >
> >   - Patches 9 to 14 introduce infrastructure for shared checks with the
> >     "files" and "reftable" backend.
> >
> >   - Patches 15 to 17 move some ref consistency checks that were still
> >     driven by git-fsck(1) into `git refs verify`.
> >
> 
> I reviewed the series and it already looks good, thanks for fixing some
> of the broken parts and cleaning up.

Thanks for your review!

Patrick

Re: [PATCH v2 00/17] Fixes and improvements for ref consistency checks

[shejialuo]

On Mon, Jan 12, 2026 at 10:02:49AM +0100, Patrick Steinhardt wrote:
> Hi,
> 
> this patch series contains a bunch of fixes and improvements for ref
> consistency checks. It is structured as follows:
> 
>   - Patches 1 to 4 contain a couple of cleanups for the consistency
>     checks done by the "files" backend.
> 
>   - Patches 5 to 7 introduce checks for root refs for the "files"
>     backend.
> 
>   - Patches 9 to 14 introduce infrastructure for shared checks with the
>     "files" and "reftable" backend.
> 
>   - Patches 15 to 17 move some ref consistency checks that were still
>     driven by git-fsck(1) into `git refs verify`.
> 
> Changes in v2:
>   - Remove unused `errors_found` field.
>   - Fix a commit message typo.
>   - Fix a copy-paste error in a function comment.
>   - Link to v1: https://lore.kernel.org/r/20260109-pks-refs-verify-fixes-v1-0-3587dba18294@pks.im
> 
> Thanks!
> 
> Patrick
> 

The range-diff looks good to me.

Thanks,
Jialuo

Re: [PATCH v2 00/17] Fixes and improvements for ref consistency checks

[Patrick Steinhardt]

On Thu, Jan 15, 2026 at 08:56:07PM +0800, shejialuo wrote:
> On Mon, Jan 12, 2026 at 10:02:49AM +0100, Patrick Steinhardt wrote:
> > Hi,
> > 
> > this patch series contains a bunch of fixes and improvements for ref
> > consistency checks. It is structured as follows:
> > 
> >   - Patches 1 to 4 contain a couple of cleanups for the consistency
> >     checks done by the "files" backend.
> > 
> >   - Patches 5 to 7 introduce checks for root refs for the "files"
> >     backend.
> > 
> >   - Patches 9 to 14 introduce infrastructure for shared checks with the
> >     "files" and "reftable" backend.
> > 
> >   - Patches 15 to 17 move some ref consistency checks that were still
> >     driven by git-fsck(1) into `git refs verify`.
> > 
> > Changes in v2:
> >   - Remove unused `errors_found` field.
> >   - Fix a commit message typo.
> >   - Fix a copy-paste error in a function comment.
> >   - Link to v1: https://lore.kernel.org/r/20260109-pks-refs-verify-fixes-v1-0-3587dba18294@pks.im
> > 
> > Thanks!
> > 
> > Patrick
> > 
> 
> The range-diff looks good to me.

Thanks!

Patrick