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

> 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.
>