Re: [PATCH v7 5/9] ref: port git-fsck(1) regular refs check for files backend

[Patrick Steinhardt <ps@pks.im>]

On Sun, Nov 10, 2024 at 08:09:51PM +0800, shejialuo wrote:
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 8bfdce64bc..2d126ecbbe 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3505,6 +3505,48 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
>  				  const char *refname,
>  				  struct dir_iterator *iter);
>  
> +static int files_fsck_refs_content(struct ref_store *ref_store,
> +				   struct fsck_options *o,
> +				   const char *target_name,
> +				   struct dir_iterator *iter)
> +{
> +	struct strbuf ref_content = STRBUF_INIT;
> +	struct strbuf referent = STRBUF_INIT;
> +	struct fsck_ref_report report = { 0 };
> +	unsigned int type = 0;
> +	int failure_errno = 0;
> +	struct object_id oid;
> +	int ret = 0;
> +
> +	report.path = target_name;
> +
> +	if (S_ISLNK(iter->st.st_mode))
> +		goto cleanup;
> +
> +	if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) {
> +		ret = fsck_report_ref(o, &report,
> +				      FSCK_MSG_BAD_REF_CONTENT,
> +				      "cannot read ref file '%s': %s",
> +				      iter->path.buf, strerror(errno));
> +		goto cleanup;
> +	}

I didn't catch this in previous rounds, but it's a little dubious
whether we should report this as an actual fsck error. I can expect
multiple situations:

  - The file has weird permissions and thus cannot be read, failing with
    EPERM, which doesn't match well with BAD_REF_CONTENT.

  - The file does not exist anymore because we were racing with a
    concurrent writer, failing with ENOENT. This is benign and expected
    to happen in busy repos, so generating an error here feels wrong.

  - The file cannot be read at all due to an I/O error. This may be
    reported with BAD_REF_CONTENT, but conflating this with the case
    where we have actually bad content may not be the best idea.

So maybe we should ignore ENOENT, report bad permissions and otherwise
return an actual error to the caller?

Patrick