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

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

[snip]

quoted

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

quoted

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