Re: [PATCH v5 4/9] ref: add more strict checks for regular refs

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