On Mon, Sep 09, 2024 at 05:04:11PM +0200, Patrick Steinhardt wrote:
> > In order to provide above checks, we will first check whether the symref
> > content misses the newline by peeking the last byte of the "referent" to
> > see whether it is '\n'.
>
> I'd still argue that we should do the same retroactive tightening as we
> introduce for normal references, also with an INFO level at first.
> Otherwise we're being inconsistent across the ref types.
>
Actually, for above situations, we will use the same fsck error message
ids introduce in [PATCH v3 2/4]. And I think we must refer to this in
this commit message.
But it makes me wonder should we use a new commit to introduce these
two fsck message ids?
> > diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt
> > index 06d045ac48..beb6c4e49e 100644
> > --- a/Documentation/fsck-msgids.txt
> > +++ b/Documentation/fsck-msgids.txt
> > @@ -28,6 +28,10 @@
> > `badRefName`::
> > (ERROR) A ref has an invalid format.
> >
> > +`badSymrefTarget`::
> > + (ERROR) The symref target points outside the ref directory or
> > + the name of the symref target is invalid.
>
> These are two separate error cases, and we even have different code
> paths raising them. Shouldn't we thus also have two different diagnostic
> codes for this?
>
I agree. I will improve in the next version.
> > + if (!skip_prefix(referent->buf, "refs/", &p)) {
> > +
>
> There's a superfluous newline here.
>
> Also, you never use the value of `p`, so you can instead use
> `starts_with()`.
>
Thanks, actually I have searched the code with "is_prefix". Well, I
didn't think about "starts_<>".
> > + /*
> > + * Missing target should not be treated as any error worthy event and
> > + * not even warn. It is a common case that a symbolic ref points to a
> > + * ref that does not exist yet. If the target ref does not exist, just
> > + * skip the check for the file type.
> > + */
> > + if (lstat(referent_path->buf, &st))
> > + goto out;
>
> We may also want to verify that `errno == ENOENT` here.
>
I agree, if "errno != ENOENT", we should report to the user about this
ref-unrelated failure.
Thanks,
Jialuo
