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

From: karthik nayak <karthik.188@gmail.com>
Date: Sep 10, 2024, 22:19 UTC
Message-ID: <CAOLa=ZS2TsRAeAHJ6B9h82-H2tSG-vZMRBSpspQ3hOW5GBdciw@mail.gmail.com>
In-Reply-To: <Ztb_JuMjaoAbIZXq@ArchLinux>

shejialuo <shejialuo@gmail.com> 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

Show 6 quoted lines

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

Show 38 quoted lines

> +/*
> + * 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?

Show 11 quoted lines

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

Show 5 quoted lines

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

Show 14 quoted lines

> +	 * 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]