On Wed, Nov 13, 2024 at 08:36:12AM +0100, Patrick Steinhardt wrote:
> 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?
>
So, I think we should just use "error_errno" method to report the actual
error to the caller. And we also need to add some comments.
Thanks for this wonderful suggestion.
> Patrick
