From: shejialuo Date: Sat, 10 Jan 2026 12:59:10 GMT Subject: Re: [PATCH 09/17] refs/files: extract generic symref target checks Message-ID: In-Reply-To: <20260109-pks-refs-verify-fixes-v1-9-3587dba18294@pks.im> 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 > --- > 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