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

[shejialuo <shejialuo@gmail.com>]

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

s/my/by

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

Make sense, we should fail early in this case.

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


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

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

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

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

Thanks,
Jialuo