Subject: Re: [PATCH v1 3/4] ref: add symbolic ref content check for files backend

From: shejialuo <[email-masked]>
Date: Fri, 23 Aug 2024 19:37:05 +0800

On Fri, Aug 23, 2024 at 07:36:10AM +0200, Patrick Steinhardt wrote:
> On Thu, Aug 22, 2024 at 08:42:02PM +0800, shejialuo wrote:
> > On Thu, Aug 22, 2024 at 10:53:57AM +0200, Patrick Steinhardt wrote:
> > 
> > 
> > > > +		if ((space_num || newline_num) && !isspace(*p)) {
> > > > +			ret = fsck_report_ref(o, report,
> > > > +					      FSCK_MSG_BAD_REF_CONTENT,
> > > > +					      "contains non-null garbage");
> > > > +			goto out;
> > > > +		}
> > > > +
> > > > +		if (*p == '\n') {
> > > > +			newline_num++;
> > > > +		} else if (*p == ' ') {
> > > > +			space_num++;
> > > > +		}
> > > > +		p++;
> > > > +	}
> > > 
> > > Can't we replace this with a single `strchr('\n')` call to check for the
> > > newline and then verify that the next character is a `\0`? The check for
> > > spaces would then be handled by `check_refname_format()`.
> > > 
> > 
> > We cannot. Think about this situation.
> > 
> >   "ref: refs/heads/master  \n   "
> > 
> > We find that the next character of '\n' is not '\0'. Then we leave it to
> > "check_refname_format". But "check_refname_format" will report an error
> > here, but this is an allowed symref.
> 
> Wouldn't it be correct to warn about this? To me the above very much
> looks like garbage after the refname, same like we'd also warn about
> such garbage for direct refs.
> 

Yes, we should warn about this. But only null-garbage is allowed for
symref. The following situation is bad:

  "ref: refs/heads/master  \n   garbage\n"

We should report error here, from my perspective, it's a FATAL ERROR.
However, let's decide how to do this when we know what fsck error level
we should set.

> Patrick