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

> 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?
>