On Wed, Sep 18, 2024 at 12:39:13PM -0700, Junio C Hamano wrote:
> shejialuo <shejialuo@xxxxxxxxx> 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@xxxxxxxxxxxxxxx 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
