shejialuo <shejialuo@xxxxxxxxx> writes:
> We implicitly rely on "git-fsck(1)" to check the consistency of regular
> refs. However, when parsing the regular refs for files backend by using
Nit: s/for/in the/
> "files-backend.c::parse_loose_ref_contents", we allow the ref content to
> end with no newline or to contain some garbages.
The 'no newline' reads a bit odd, perhaps, "we allow the ref's content
to end with garbage or without a newline."
> Even though we never create such loose refs ourselves, we have accepted
> such loose refs. So, it is entirely possible that some third-party tools
> may rely on such loose refs being valid. We should not report an error
> fsck message at current. But let's notice such a "curiously formatted"
s/such a/such/ since the next line uses 'refs' in plural form.
> loose refs being valid and tell the user our findings, so we can access
s/access/assess
> the possible extent of damage when we tighten the parsing rules in the
> future.
>
We could also rewrite the last sentence to make it a little more clearer
as "We should notify the users about such 'curiously formatted' loose
refs so that adequate care is taken before we decide to tighter the rules
in the future."
> And it's not suitable to either report a warn fsck message to the user.
> This is because if the caller set the "strict" field in "fsck_options",
> fsck warns will be automatically upgraded to errors. We should not allow
> user to specify the "--strict" flag to upgrade the fsck warnings to
> errors at current. It might cause compatibility issue which may break
> the legacy repository. So we add the following two fsck infos to
I think Patrick touched base here and I agree with his comments.
> represent the situation where the ref content ends without newline or has
> garbages:
>
> 1. "refMissingNewline(INFO)": A ref does not end with newline. This kind
> of ref may be considered ERROR in the future.
> 2. "trailingRefContent(INFO)": A ref has trailing contents. This kind of
s/contents/content
> ref may be considered ERROR in the future.
>
> It may seem that we could not give the user any warnings by creating
s/could/would
> fsck infos. However, in "fsck.c::fsck_vreport", we will convert
I think we can also rephrase this first sentence a little better,
perhaps:
It might appear that we can't provide the user with any warnings by
using FSCK_INFO.
> "FSCK_INFO" to "FSCK_WARN" and we can still warn the user about these
> situations when using "git-refs verify" without introducing
> compatibility issue.
s/issue/issues
> In current "git-fsck(1)", it will report an error when the ref content
> is bad, so we should following this to report an error to the user when
> "parse_loose_ref_contents" fails. And we add a new fsck error message
> called "badRefContent(ERROR)" to represent that a ref has a bad content.
I would rephrase this a bit, as:
The "git-fsck(1)" command reports an error when the ref content is
invalid. Following this, add a similar check to "git refs verify". A
a new fsck error message called "badRefContent(ERROR)" to represent
that a ref has a invalid content.
[snip]
> +static int files_fsck_refs_content(struct ref_store *ref_store,
> + struct fsck_options *o,
> + const char *refs_check_dir,
> + struct dir_iterator *iter)
> +{
> + struct strbuf ref_content = STRBUF_INIT;
> + struct strbuf referent = STRBUF_INIT;
> + struct strbuf refname = STRBUF_INIT;
> + struct fsck_ref_report report = {0};
> + const char *trailing = NULL;
> + unsigned int type = 0;
> + int failure_errno = 0;
> + struct object_id oid;
> + int ret = 0;
> +
> + strbuf_addf(&refname, "%s/%s", refs_check_dir, iter->relative_path);
> + report.path = refname.buf;
> +
> + if (S_ISLNK(iter->st.st_mode))
> + goto cleanup;
Since we iterate over all refs, we don't need to check the target for a
symbolic link. So we skip all symbolic links. Makes sense. Would be nice
to have a comment here.
[snip]
Attachment:
signature.asc
Description: PGP signature
