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

[shejialuo <shejialuo@gmail.com>]

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
> > @@ -179,6 +179,14 @@
> >  `unknownType`::
> >  	(ERROR) Found an unknown object type.
> >  
> > +`unofficialFormattedRef`::
> > +	(INFO) The content of a loose ref file is not in the official
> > +	format such as not having a LF at the end or having trailing
> > +	garbage. As valid implementations of Git never created such a
> > +	loose ref file, it may become an error in the future. Report
> > +	to the git@vger.kernel.org mailing list if you see this error,
> > +	as we need to know what tools created such a file.
> > +
> 
> I find "unofficial" to be a tad weird. Do we rather want to say
> something like "badRefTrailingGarbage"?
> 

Well, I will answer this question just in below question together.

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

> Patrick

Thanks,
Jialuo