Re: [PATCH v5 4/9] ref: add more strict checks for regular refs
- From
shejialuo <shejialuo@gmail.com>
- Date
- Oct 7, 2024, 12:19 UTC
- Message-ID
- <ZwPRvENH0TA5YX6J@ArchLinux>
- In-Reply-To
- <ZwOo9dQSr8Xu-PBb@pks.im>
On Mon, Oct 07, 2024 at 11:25:17AM +0200, Patrick Steinhardt wrote:
[snip]
Show 34 quoted lines
> > > > 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.
Show 11 quoted lines
> 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