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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 8, 2024, 17:44 UTC
Message-ID: <xmqq5xq232wa.fsf@gitster.g>
In-Reply-To: <ZwUkZuCtYu7niuFM@ArchLinux>

shejialuo <shejialuo@gmail.com> writes:

Show 23 quoted lines

> On Tue, Oct 08, 2024 at 12:43:20AM -0700, Karthik Nayak wrote:
>> shejialuo <shejialuo@gmail.com> 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.

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.

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

Thanks.