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
