Re: [PATCH v2 2/4] ref: add regular ref content check for files backend

[Junio C Hamano <gitster@pobox.com>]

shejialuo <shejialuo@gmail.com> 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
> "files-backend.c::parse_loose_ref_contents", we allow the ref content to
> be end with no newline or contain some garbages.

"to be end with" -> "to end with".
"or contain" -> "or to contain" (optional, I think).

Or "... the ref content without terminating newline, or with extra
bytes after the terminating newline."

> It may seem that we should report an error or warn fsck message to the
> user about above situations. However, there may be some third-party
> tools customizing the content of refs. We should not report an error
> fsck message.

    Even though we never created such loose refs ourselves, we have
    accepted such loose refs forever, so it is entirely possible
    that third-party tools may rely on such loose refs being valid.
    Let's notice such a "curiously formatted" loose ref files and
    tell the users our findings, so that we can assess the possible
    extent of damage if/when we retroactively tightened the parsing
    rules in the future.

> We should not allow the user to upgrade the fsck warnings to errors. It
> might cause compatibility issue which will break the legacy repository.

I am not sure this is a right thing to say.  If the user wants to
ensure that the tool they use in their repository, which may include
some third-party reimplementation of Git, would never create such a
(semi-)malformed loose ref files, it is within their right, and it
is the most reasonable way, to promote these "curiously formatted
loose ref" fsck warnings to errors.

Is your "We should not allow" above backed by code that prevents
them from promoting the warnings to errors, or is it merely a
declaration of your intention?

> So we add the following two fsck infos to represent the situation where
> the ref content ends without newline or has garbages:
>
> 1. "refMissingNewline(INFO)": A valid ref does not end with newline.
> 2. "trailingRefContent(INFO)": A ref has trailing contents.

OK.

> In "fsck.c::fsck_vreport", we will convert "FSCK_INFO" to "FSCK_WARN",
> and we can still warn the user about these situations when using
> "git-refs verify" without introducing compatibility issue.

OK.

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

Good.

> @@ -170,6 +173,12 @@
>  `nullSha1`::
>  	(WARN) Tree contains entries pointing to a null sha1.
>  
> +`refMissingNewline`::
> +	(INFO) A valid ref does not end with newline.
> +
> +`trailingRefContent`::
> +	(INFO) A ref has trailing contents.
> +
>  `treeNotSorted`::
>  	(ERROR) A tree is not properly sorted.

There is no mention of "you shouldn't promote these to error" here,
which is good.  But wouldn't we want to tell users to report such
curiously formatted loose refs, after figuring out who created them,
to help us to eventually make the check stricter in the future?

Git 3.0 boundary might be a good time to tighten interoperability
rules such that we won't accept anything we wouldn't have written
ourselves (not limited to loose ref format, but this applies to
anything on-disk or on-wire), but we'd need enough preparation if we
want to be able to do so in the future.

Thanks.