From: shejialuo <shejialuo@gmail.com>
Date: Thu, 12 Sep 2024 04:00:08 GMT
Subject: Re: [PATCH v3 3/4] ref: add symref content check for files backend
Message-ID: <ZuJnSHXJw6AVzbxL@ArchLinux>
In-Reply-To: <CAOLa=ZS2TsRAeAHJ6B9h82-H2tSG-vZMRBSpspQ3hOW5GBdciw@mail.gmail.com>

On Tue, Sep 10, 2024 at 03:19:49PM -0700, karthik nayak wrote:

[snip]

> > +	if (referent->buf[referent->len - 1] != '\n') {
> > +		ret = fsck_report_ref(o, report,
> > +				      FSCK_MSG_REF_MISSING_NEWLINE,
> > +				      "missing newline");
> > +		len++;
> > +	}
> > +
> > +	strbuf_rtrim(referent);
> > +	if (check_refname_format(referent->buf, 0)) {
> > +		ret = fsck_report_ref(o, report,
> > +				      FSCK_MSG_BAD_SYMREF_TARGET,
> > +				      "points to refname with invalid format");
> > +		goto out;
> > +	}
> > +
> > +	if (len != referent->len) {
> 
> Would this work with a symref containing:
> 
>     ref: refs/heads/feature\ngarbage\n
> 
> Since we check last character and rtrim, wouldn't this bypass our
> checks? Isn't it better to find the first `\n` and check if the index <
> referent->len?
> 

We will check the above example by "check_refname_format". It will
report the following message:

  error: ... : badSymrefTarget: points to refname with invalid format

From the context, I guess you suggest that we should report there is a
trailing garbage in the ref. However, for the above situation, we should
report an error which is align with the behavior of the "git-fsck(1)".

So there is no need to check whether there is a trailing garbage when we
encounter an error.

And we cannot use this way, for example:

  ref: refs/heads/feature   \n

If we find the first '\n' index. In this example, index will be equal to
"referent->len". And we totally ignore this case.

> > +		ret = fsck_report_ref(o, report,
> > +				      FSCK_MSG_TRAILING_REF_CONTENT,
> > +				      "trailing garbage in ref");
> > +	}
> > +
> > +	/*
> > +	 * 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.
> > +	 */
> 
> I think the common terminology for this is 'dangling symref'. Perhaps we
> could shorten this to simply say:
> 
>     Dangling symrefs are common and so we don't report them.
> 

Thanks, I will improve this in the next version.