On Mon, Oct 07, 2024 at 04:44:16PM +0800, shejialuo wrote:
> On Mon, Oct 07, 2024 at 08:58:37AM +0200, Patrick Steinhardt wrote:
> > On Sun, Sep 29, 2024 at 03:16:00PM +0800, shejialuo wrote:
> > > diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt
> > > index 22c385ea22..e310b5bce9 100644
> > > --- a/Documentation/fsck-msgids.txt
> > > +++ b/Documentation/fsck-msgids.txt
> > > @@ -3541,6 +3546,21 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
> > > goto cleanup;
> > > }
> > >
> > > + if (!(type & REF_ISSYMREF)) {
> > > + if (!*trailing) {
> > > + ret = fsck_report_ref(o, &report,
> > > + FSCK_MSG_UNOFFICIAL_FORMATTED_REF,
> > > + "misses LF at the end");
> > > + goto cleanup;
> > > + }
> > > + if (*trailing != '\n' || *(trailing + 1)) {
> > > + ret = fsck_report_ref(o, &report,
> > > + FSCK_MSG_UNOFFICIAL_FORMATTED_REF,
> > > + "has trailing garbage: '%s'", trailing);
> > > + goto cleanup;
> > > + }
> > > + }
> > > +
> >
> > I think we should discern these two error cases and provide different
> > message IDs.
> >
>
> Actually, in the previous versions, I have mapped one message id to one
> error case. But, in the v4, Junio asked a question
>
> Not limited to this patch, but isn't fsck_report_ref() misdesigned,
> or is it just they are used poorly in these patches? In these two
> callsites, the message string parameter does not give any more
> information than what the FSCK_MSG_* enum gives.
>
> That is what I meant by "misdesigned"---if one message enum always
> corresponds to one human-readable message, there is not much point
> in forcing callers to supply both, is there?
>
> In my opinion, we should have only one case here for trailing garbage
> and not end with a newline. When writing the code, I chose the name
> "unofficialFormattedRef" for the following reason:
>
> 1. If we use two message ids here, for every message id, we need write
> to info the user "please report this to git mailing list".
>
> 2. If we decide to make this as an error. We could just classify them
> into "badRefContent" message category.
>
> 3. The semantic is correct here, they are truly curious formatted
> refs, and eventually we will give the info to the user what is
> curious.
>
> So, I think we should not always map one message to one error case.
