On Tue, Oct 08, 2024 at 10:44:53AM -0700, Junio C Hamano wrote:
> shejialuo <shejialuo@xxxxxxxxx> writes:
>
> > On Tue, Oct 08, 2024 at 12:43:20AM -0700, Karthik Nayak wrote:
> >> shejialuo <shejialuo@xxxxxxxxx> writes:
> >>
> >> [snip]
> >>
> >> > + 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");
> >> > + goto cleanup;
> >> > + }
> >> > +
> >>
> >> Shouldn't we use `die_errno` here instead? I mean, this is not really a
> >> bad ref content issue. If we don't want to die here, it would still
> >> probably be nice to get the actual issue using `strerror` instead and
> >> use that instead of the generic message we have here.
> >>
> >
> > Well, I think I need to dive into the "open" system call here. Actually,
> > we have two opinions now. Junio thought that we should use
> > "fsck_report_ref" to report. Karthik, Patrick and I thought that we
> > should report using "*errno" because this is a general error.
>
> What do you mean by "a general error"? It is true that we failed to
> read a ref file, so even if it is an I/O error, I'd think it is OK
> to report it as an error while reading one particular ref.
>
> Giving more information is a separate issue. If fsck_report_ref()
> can be extended to take something like
>
> "cannot read ref file '%s': (%s)", iter->path.buf, strerror(errno)
>
> that would give the user necessary information.
Yeah, this is also in line with what I proposed elsewhere, where we have
been discussing the 1:1 mapping between error codes and error messages.
If the error messages were dynamic they'd be a whole lot useful overall
and could provide more context.
> And I agree with half-of what Karthik said, i.e., we do not want to
> die here if this is meant to run as a part of "git fsck".
>
> I may have said this before, but quite frankly, the API into the
> fsck_report_ref() function is misdesigned. If the single constant
> string "cannot read ref file" cnanot give more information than
> FSCK_MSG_BAD_REF_CONTENT, the parameter has no value.
True in the current form, yeah. If `fsck_report_ref()` learned to take a
vararg argument and treat its first argument as a string format it would
be justified though, as the message is now dynamic and can contain more
context around the specific failure that cannot be provided statically
via the 1:1 mapping between error code and message.
> The fsck.c:report() function, which is the main function to report
> fsck's findings before fsck_report_ref() was introduced, did not
> have such a problem, as it allowed "const char *fmt, ..." at the
> end. Is it too late to fix the fsck_report_ref()?
I don't think so, I think we should be able to refactor the code rather
easily to do so.
Patrick
