Merge branch 'ps/ref-consistency-checks'

Update code paths that check data integrity around refs subsystem.
cf. <CAOLa=ZShPP3BPXa=YnC-vuX4zF=pUTFdUidZwOdna8bfVTNM9w@mail.gmail.com>

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

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.
 
@@ -41,6 +44,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.
 

builtin/fsck.c

@@ -596,10 +596,6 @@ 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 snapshot_refs(struct snapshot *snap, int argc, const char **argv)
 {
 	struct worktree **worktrees, **p;
@@ -636,7 +632,10 @@ static void snapshot_refs(struct snapshot *snap, int argc, const char **argv)
 		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,
@@ -803,43 +802,6 @@ 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)
-{
-	int null_is_error = 0;
-
-	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;
-		return error(_("invalid %s"), head_ref_name);
-	}
-	if (!strcmp(*head_points_at, head_ref_name))
-		/* detached HEAD */
-		null_is_error = 1;
-	else if (!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);
-	}
-	return 0;
-}
-
 static int fsck_cache_tree(struct cache_tree *it, const char *index_path)
 {
 	int i;

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

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) \
@@ -39,6 +40,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) \
@@ -162,8 +164,6 @@ struct fsck_object_report {
 
 struct fsck_ref_report {
 	const char *path;
-	const struct object_id *oid;
-	const char *referent;
 };
 
 struct fsck_options {

refs.c

@@ -320,6 +320,49 @@ 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,
+		  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;
+}
+
+int refs_fsck_symref(struct ref_store *refs UNUSED, struct fsck_options *o,
+		     struct fsck_ref_report *report,
+		     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;
+
+	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)
 {

refs.h

@@ -653,6 +653,24 @@ 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 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.
+ */
+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

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;
@@ -3720,64 +3715,50 @@ 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,
+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)
+
+	if (!symbolic_link) {
 		strbuf_rtrim(referent);
 
-	is_referent_root = is_root_ref(referent->buf);
+		if (referent->len == orig_len ||
-	if (!is_referent_root &&
+		    (referent->len < orig_len && orig_last_byte != '\n')) {
-	    !starts_with(referent->buf, "refs/") &&
+			ret |= fsck_report_ref(o, report,
-	    !starts_with(referent->buf, "worktrees/")) {
+					       FSCK_MSG_REF_MISSING_NEWLINE,
-		ret = fsck_report_ref(o, report,
+					       "misses LF at the end");
-				      FSCK_MSG_SYMREF_TARGET_IS_NOT_A_REF,
+		}
-				      "points to non-ref target '%s'", referent->buf);
 
+		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 (!is_referent_root && check_refname_format(referent->buf, 0)) {
+	ret |= refs_fsck_symref(ref_store, o, report, refname, referent->buf);
-		ret = fsck_report_ref(o, report,
-				      FSCK_MSG_BAD_REFERENT_NAME,
-				      "points to invalid refname '%s'", referent->buf);
-		goto out;
-	}
 
-	if (symbolic_link)
+	return ret ? -1 : 0;
-		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 - 1) {
-		ret = fsck_report_ref(o, report,
-				      FSCK_MSG_TRAILING_REF_CONTENT,
-				      "has trailing whitespaces or newlines");
-	}
-
-out:
-	return ret;
 }
 
 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;
@@ -3791,7 +3772,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,
@@ -3803,7 +3784,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);
 
@@ -3812,11 +3793,12 @@ 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;
 	}
 
-	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.
@@ -3824,7 +3806,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;
 	}
 
@@ -3851,8 +3833,11 @@ 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(o, &report, &referent, 0);
+		ret = files_fsck_symref_target(ref_store, o, &report,
+					       target_name, &referent, 0);
 		goto cleanup;
 	}
 
@@ -3866,21 +3851,25 @@ cleanup:
 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;
+
+	if (is_root_ref(refname))
 		goto cleanup;
 
-	/*
-	 * This works right now because we never check the root refs.
-	 */
 	if (check_refname_format(refname, 0)) {
 		struct fsck_ref_report report = { 0 };
 
@@ -3895,11 +3884,44 @@ cleanup:
 	return ret;
 }
 
+static const files_fsck_refs_fn fsck_refs_fn[]= {
+	files_fsck_refs_name,
+	files_fsck_refs_content,
+	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,
-			       const char *refs_check_dir,
+			       struct worktree *wt)
-			       struct worktree *wt,
-			       files_fsck_refs_fn *fsck_refs_fn)
 {
 	struct strbuf refname = STRBUF_INIT;
 	struct strbuf sb = STRBUF_INIT;
@@ -3907,7 +3929,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) {
@@ -3919,31 +3941,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_reset(&refname);
-				strbuf_addf(&refname, "worktrees/%s/", wt->id);
+		if (!is_main_worktree(wt))
-			strbuf_addf(&refname, "%s/%s", refs_check_dir,
+			strbuf_addf(&refname, "worktrees/%s/", wt->id);
-				    iter->relative_path);
+		strbuf_addf(&refname, "refs/%s", iter->relative_path);
 
-			if (o->verbose)
+		if (files_fsck_ref(ref_store, o, refname.buf,
-				fprintf_ln(stderr, "Checking %s", refname.buf);
+				   iter->path.buf, iter->st.st_mode) < 0)
-
+			ret = -1;
-			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 (iter_status != ITER_DONE)
@@ -3956,17 +3964,35 @@ out:
 	return ret;
 }
 
-static int files_fsck_refs(struct ref_store *ref_store,
+struct files_fsck_root_ref_data {
-			   struct fsck_options *o,
+	struct files_ref_store *refs;
-			   struct worktree *wt)
+	struct fsck_options *o;
-{
+	struct worktree *wt;
-	files_fsck_refs_fn fsck_refs_fn[]= {
+	struct strbuf refname;
-		files_fsck_refs_name,
+	struct strbuf path;
-		files_fsck_refs_content,
+};
-		NULL,
-	};
 
-	return files_fsck_refs_dir(ref_store, o, "refs", wt, fsck_refs_fn);
+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,
@@ -3975,9 +4001,27 @@ static int files_fsck(struct ref_store *ref_store,
 {
 	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;
 
-	return files_fsck_refs(ref_store, o, wt) |
+	if (files_fsck_refs_dir(ref_store, o, wt) < 0)
-	       refs->packed_ref_store->be->fsck(refs->packed_ref_store, o, wt);
+		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;
 }
 
 struct ref_storage_be refs_be_files = {

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"
@@ -25,8 +26,8 @@
 #include "../setup.h"
 #include "../strmap.h"
 #include "../trace2.h"
+#include "../worktree.h"
 #include "../write-or-die.h"
-#include "parse.h"
 #include "refs-internal.h"
 
 /*
@@ -172,6 +173,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 +223,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 +255,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);
+		ret = backend_for_worktree(out, store, wtname_buf.buf);
-		if (!be) {
-			strbuf_addf(&wt_dir, "%s/worktrees/%s/reftable",
-				    store->base.repo->commondir, 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 +265,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) {
+	if (reload && !ret)
-		int ret = reftable_stack_reload(be->stack);
+		ret = reftable_stack_reload((*out)->stack);
-		if (ret)
+	return ret;
-			return ret;
-	}
-	*out = be;
-
-	return 0;
 }
 
 static int should_write_log(struct reftable_ref_store *refs, const char *refname)
@@ -2746,24 +2763,92 @@ 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 reftable_ref_store *refs =
-	struct strmap_entry *entry;
+		reftable_be_downcast(ref_store, REF_STORE_READ, "fsck");
-	struct hashmap_iter iter;
+	struct reftable_ref_iterator *iter = NULL;
-	int ret = 0;
+	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;
 
-	refs = reftable_be_downcast(ref_store, REF_STORE_READ, "fsck");
+	if (is_main_worktree(wt)) {
-
+		backend = &refs->main_backend;
-	ret |= reftable_fsck_check(refs->main_backend.stack, reftable_fsck_error_handler,
+	} else {
-				   reftable_fsck_verbose_handler, o);
+		ret = backend_for_worktree(&backend, refs, wt->id);
-
+		if (ret < 0) {
-	strmap_for_each_entry(&refs->worktree_backends, &iter, entry) {
+			ret = error(_("reftable stack for worktree '%s' is broken"),
-		struct reftable_backend *b = (struct reftable_backend *)entry->value;
+				    wt->id);
-		ret |= reftable_fsck_check(b->stack, reftable_fsck_error_handler,
+			goto out;
-					   reftable_fsck_verbose_handler, o);
+		}
 	}
 
+	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);
+		}
+	}
+
+	ret = errors ? -1 : 0;
+
+out:
+	if (iter)
+		ref_iterator_free(&iter->base);
+	reftable_ref_record_release(&ref);
+	strbuf_release(&refname);
 	return ret;
 }
 

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/heads/../HEAD" >.git/HEAD &&
+		test_must_fail git refs verify 2>err &&
+		cat >expect <<-EOF &&
+		error: HEAD: badReferentName: points to invalid refname ${SQ}refs/heads/../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/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/heads/../HEAD${SQ}
+		EOF
+		test_cmp expect err
+	)
+'
+
 test_done

t/t0614-reftable-fsck.sh

@@ -55,4 +55,48 @@ 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_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

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' '
@@ -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)' '
@@ -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' '
@@ -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' '