Re: [PATCH v3 3/4] ref: add symref content check for files backend

[shejialuo <shejialuo@gmail.com>]

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