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

[shejialuo <shejialuo@gmail.com>]

On Tue, Oct 08, 2024 at 10:44:53AM -0700, Junio C Hamano wrote:
> shejialuo <shejialuo@gmail.com> writes:
> 
> > 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.

Make sense.

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

At current, the `fsck_report_ref` can do this. I think I used
`fsck_report_ref` function badly in this case.

> 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".

Yes, we should not die the program. Instead, we need to continuously
check other refs.

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

I agree that if the FSCK message id could explain the error well, there
is no need for us to provide extra message. But, I want to say the
`fsck_report_ref` is not misdesigned here. It is just the same as the
"fsck.c::report" function which has "const char *fmt, ..." at the end
like the following shows:

    int fsck_report_ref(struct fsck_options *options,
                        struct fsck_ref_report *report,
                        enum fsck_msg_id msg_id,
                        const char *fmt, ...)

And I do think "fsck.c::report" function also has the above problems.
Let me give you some examples here in "fsck.c":

    report(options, tree_oid, OBJ_TREE,
           FSCK_MSG_BAD_FILEMODE,
           "contains bad file modes");

    report(options, tree_oid, OBJ_TREE,
           FSCK_MSG_DUPLICATE_ENTRIES,
           "contains duplicate file entries");

    ...

So, I want to say there is no difference between "fsck_ref_report" and
"fsck.c::report". When I refactored the code in GSoC journey, the main
problem is that we should reuse the original "fsck.c::report" code
instead of writing redundant codes.

The final result is I extract a new function "fsck_vreport" here (I
leverage the original "fsck.c::report" function) which will be called by
"fsck_ref_report" and "fsck.c::report".

    static int fsck_vreport(struct fsck_options *options,
                            void *fsck_report,
                            enum fsck_msg_id msg_id,
                            const char *fmt, va_list ap)

From my perspective, if we decide to refactor, we should allow the user
call the followings:

    fsck_ref_report(..., FSCK_MSG_BAD_REF_CONTENT, NULL);
    report(..., FSCK_MSG_DUPLICATE_ENTRIES, NULL);

So, we should check whether `fmt` is NULL in the `fsck_vreport`
function to make sure that if FSCK message is good enough to explain
what happens, we should not pass any message.

> Thanks.

Thanks,
Jialuo