Re: [PATCH v4 3/5] ref: add more strict checks for regular refs

[shejialuo <shejialuo@gmail.com>]

On Wed, Sep 18, 2024 at 12:39:13PM -0700, Junio C Hamano wrote:
> shejialuo <shejialuo@gmail.com> writes:
> 
> > +`refMissingNewline`::
> > +	(INFO) A ref does not end with newline. This will be
> > +	considered an error in the future.
> 
> It is ONLY files backend's loose-ref representation to store the
> object name that is the value of the ref as hexadecimal text
> terminated with a newline.  With packed backend, even if the file
> ends with an incomplete line, it would be confusing to say that such
> lack of terminating LF is associated with a particular ref.  With
> reftable backend, the object name may not even be hexadecimal but
> binary without any terminating LF.
> 
> At least you should say "A loose ref file that does not end with...",
> because a ref NEVER ends or contains newline, and what you are
> expecting to be terminated with LF is not even a ref, but the value
> of it.
> 

Thanks, I will improve this in the next version.

> Also, isn't it too strong to say "will be" without giving any
> further information, like:
> 
>     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.
> 
> or something?
> 

This is nice. I know the intention here.

> > +		if (!*trailing) {
> > +			ret = fsck_report_ref(o, &report,
> > +					      FSCK_MSG_REF_MISSING_NEWLINE,
> > +					      "missing newline");
> > +			goto cleanup;
> > +		}
> > +
> > +		if (*trailing != '\n' || *(trailing + 1)) {
> > +			ret = fsck_report_ref(o, &report,
> > +					      FSCK_MSG_TRAILING_REF_CONTENT,
> > +					      "trailing garbage in ref");
> > +			goto cleanup;
> > +		}
> 
> 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.
> 
> In fact, MSG_REF_MISSING_NEWLINE at least says that the complaint is
> about refs, but "missing newline" does not even say from what the
> newline is missing.  For TRAILING_REF_CONTENT, people may expect to
> see what garbage follows the expected contents, but that information
> (i.e. contents of *trailing) is lost here.

I agree with you here, I use way too general words to describe what
happens. I will improve this. Actually, I feel hard to find words for
"MSG_REF_MISSING_NEWLINE". I think we should say:

	LF should be at the end of the file.

Thanks,
Jialuo