From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 07 Oct 2024 09:25:17 GMT
Subject: Re: [PATCH v5 4/9] ref: add more strict checks for regular refs
Message-ID: <ZwOo9dQSr8Xu-PBb@pks.im>
In-Reply-To: <ZwOfYGi21oa302sS@ArchLinux>

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.

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.

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.

Patrick