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

[shejialuo <shejialuo@gmail.com>]

On Mon, Oct 07, 2024 at 11:25:17AM +0200, Patrick Steinhardt wrote:

[snip]

> > 
> > 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.
> 
> From my point of view the error codes should be the single source of
> truth, as this is what a user can use to disable specific checks. So if
> one code maps to multiple messages they have the problem that they can
> only disable all of those messages.
> 

Thanks for your remind here. I totally forgot this. I have changed my
mind now, we should use one to one mapping here. As you said, if we do
not, we will give the user the bad experience.

> I don't disagree with what Junio is saying. It is somewhat duplicate
> that the user has to pass both a code and a message in the current
> form-- it should be sufficient for them to pass the code, and the
> message can then e.g. be extracted from a central array that maps codes
> to messages.
> 
> But you can also make the reverse argument: messages can be dynamic, so
> that the caller may include additional details around why specfically
> the check failed. The code and message would still be 1:1, but we may
> include additional details like that to guide the user.
> 

Yes, I will refactor the "fsck_report" to allow the user pass the "NULL"
message if the fsck message id is clear enough to indicate the error
case.

So, more things to do here.

> Patrick

Thanks,
Jialuo