From 20f3107931e703d3ed589a6543a60fc78b89c6a8 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:30 +0100 Subject: [PATCH 01/17] refs/files: simplify iterating through root refs 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 Signed-off-by: Junio C Hamano --- 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; From ec03c757070adf4b460eaa56da284f2370c69cd4 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:31 +0100 Subject: [PATCH 02/17] refs/files: move fsck functions into global scope 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 Signed-off-by: Junio C Hamano --- 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 @@ 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_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, From 9262da11b536e3067f4b23b1d1e19080f76af7f0 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:32 +0100 Subject: [PATCH 03/17] refs/files: remove `refs_check_dir` parameter 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 Signed-off-by: Junio C Hamano --- 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, From 328de3c71e8edc769e7675579038d84cf2fcfa85 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:33 +0100 Subject: [PATCH 04/17] refs/files: remove useless indirection 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 Signed-off-by: Junio C Hamano --- 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 @@ out: 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 = { From 5c8cf26919cf0ffee26ebf3463b4c30071f55020 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:34 +0100 Subject: [PATCH 05/17] refs/files: extract function to check single ref 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 Signed-off-by: Junio C Hamano --- 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 @@ out: 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 @@ 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; /* @@ -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); + 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); - - 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) From 04bbf2f0d83edb4b568a1830ddb1bf63d3224d2e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:35 +0100 Subject: [PATCH 06/17] refs/files: improve error handling when verifying symrefs 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 Signed-off-by: Junio C Hamano --- 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, From 4144f09d78cd9d927cf9556956268727226a83f5 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:36 +0100 Subject: [PATCH 07/17] refs/files: perform consistency checks for root refs 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 Signed-off-by: Junio C Hamano --- 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 @@ out: 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 From 3f330b445bf322b740681199032df8473b80da2c Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:37 +0100 Subject: [PATCH 08/17] fsck: drop unused fields from `struct fsck_ref_report` 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 Signed-off-by: Junio C Hamano --- 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 { From 0213630269f37e9c5c0ec8aecb27fc82f91da409 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:38 +0100 Subject: [PATCH 09/17] refs/files: extract generic symref target checks 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 Signed-off-by: Junio C Hamano --- refs.c | 21 +++++++++++++++++ refs.h | 10 ++++++++ refs/files-backend.c | 56 +++++++++++++++++--------------------------- 3 files changed, 53 insertions(+), 34 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) + + 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 (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 (!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); - } + ret |= refs_fsck_symref(ref_store, o, report, refname, referent->buf); - 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 - 1) { - ret |= fsck_report_ref(o, report, - FSCK_MSG_TRAILING_REF_CONTENT, - "has trailing whitespaces or newlines"); - } - -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; } From 0a83639888a96ccc63b9710d123ebd07e5ae0d00 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:39 +0100 Subject: [PATCH 10/17] refs/files: introduce function to perform normal ref checks 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 Signed-off-by: Junio C Hamano --- 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); From 3a423a3c0c363dc07f76aa179dc5fcc60f96bc6e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:40 +0100 Subject: [PATCH 11/17] refs/reftable: adapt includes to become consistent Adapt the includes to be sorted and to use include paths that are relative to the "refs/" directory. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- 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" /* From eb9c42c5a29e7a1a9b1bbc4923d7042965cd1cbc Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:41 +0100 Subject: [PATCH 12/17] refs/reftable: extract function to retrieve backend for worktree 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 Signed-off-by: Junio C Hamano --- 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) From d385bc1b7567bba3ba96d33420e7804dd03fbecf Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:42 +0100 Subject: [PATCH 13/17] refs/reftable: fix consistency checks with worktrees 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 Signed-off-by: Junio C Hamano --- 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; + struct reftable_ref_store *refs = + reftable_be_downcast(ref_store, REF_STORE_READ, "fsck"); + struct reftable_backend *backend; - refs = reftable_be_downcast(ref_store, REF_STORE_READ, "fsck"); - - ret |= reftable_fsck_check(refs->main_backend.stack, reftable_fsck_error_handler, - reftable_fsck_verbose_handler, o); - - strmap_for_each_entry(&refs->worktree_backends, &iter, entry) { - struct reftable_backend *b = (struct reftable_backend *)entry->value; - ret |= reftable_fsck_check(b->stack, reftable_fsck_error_handler, - reftable_fsck_verbose_handler, o); + 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 From 8a3de73c6441e4ded8cb00e3a619b8f9fd087879 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:43 +0100 Subject: [PATCH 14/17] refs/reftable: introduce generic checks for refs 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 Signed-off-by: Junio C Hamano --- 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; + } } - return reftable_fsck_check(backend->stack, reftable_fsck_error_handler, - 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; } 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 From aa3b256f559b506ff881e06309c48ed4b6c46af7 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:44 +0100 Subject: [PATCH 15/17] builtin/fsck: move generic object ID checks into `refs_fsck()` 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 Signed-off-by: Junio C Hamano --- 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); + error(_("%s points to something strange (%s)"), + head_ref_name, *head_points_at); + return; } - 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; + + 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' ' From b2f2a58a256f65716fccf7484662f1c56764a285 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:45 +0100 Subject: [PATCH 16/17] builtin/fsck: move generic HEAD check into `refs_fsck()` 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 Signed-off-by: Junio C Hamano --- 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' ' From df7333d8e60dd0ee8f26bdb390e0d52492926e0d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 9 Jan 2026 13:39:46 +0100 Subject: [PATCH 17/17] builtin/fsck: drop `fsck_head_link()` 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 Signed-off-by: Junio C Hamano --- 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;