shejialuo <shejialuo@xxxxxxxxx> 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]
> +/*
> + * 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?
> + 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.
> + 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/
> + * 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]
Attachment:
signature.asc
Description: PGP signature
