Re: [PATCH v3 3/4] ref: add symref content check for files backend
From: karthik nayak <hidden>
Date: 2024-09-10 22:19:51
shejialuo [off-list ref] writes: [snip]
And we will remember the untrimmed length of the "referent" and call "strbuf_rtrim()" on "referent". Then, we will call "check_refname_format" to chceck whether the trimmed referent format is valid. If not, we will
s/chceck/check
report to the user that the symref points to referent which has invalid format. If it is valid, we will compare the untrimmed length and trimmed length, if they are not the same, we need to warn the user there is some trailing garbage in the symref content. At last, we need to check whether the referent is the directory. We
s/is the/is a/
cannot distinguish whether the "refs/heads/a" is a directory or not by
It would be a little clearer if we say We cannot distinguish whether a given reference like 'refs/heads/a' is a file or a directory.
using "check_refname_format". We have already checked bad file type when iterating the "refs/" directory but we ignore the directory. Thus, we need to explicitly add check here.
[snip]
quoted hunk ↗ jump to hunk
+/* + * Check the symref "referent" and "referent_path". For textual symref, + * "referent" would be the content after "refs:". + */ +static int files_fsck_symref_target(struct fsck_options *o, + struct fsck_ref_report *report, + struct strbuf *referent, + struct strbuf *referent_path) +{ + size_t len = referent->len - 1; + const char *p = NULL; + struct stat st; + int ret = 0; + + if (!skip_prefix(referent->buf, "refs/", &p)) { + + ret = fsck_report_ref(o, report, + FSCK_MSG_BAD_SYMREF_TARGET, + "points to ref outside the refs directory"); + goto out; + } + + 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?
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ if (lstat(referent_path->buf, &st)) + goto out; + + /* + * We cannot distinguish whether "refs/heads/a" is directory or nots by
s/is/is a/ s/nots/not/
quoted hunk ↗ jump to hunk
+ * using "check_refname_format(referent->buf, 0)". Instead, we need to + * check the file type of the target. + */ + if (S_ISDIR(st.st_mode)) { + ret = fsck_report_ref(o, report, + FSCK_MSG_BAD_SYMREF_TARGET, + "points to the directory"); + goto out; + } + +out: + return ret; +} +
[snip]
Attachments
- signature.asc [application/pgp-signature] 690 bytes