[PATCH v2 0/4] add ref content check for files backend

[shejialuo <shejialuo@gmail.com>]

Hi All:

This new version handles the following reviews:

1. According to the advice from the Junio, we should just use "{0}" to
initialize the zero structure "fsck_ref_report". This version handles
this in [PATCH v2 1/4].
2. According to the advice from the Patrick, use "strrchr" instead of
looping to make the code more clean in [PATCH v2 3/4].
3. Use "goto" to remove ident.

However, the most important thing for this patch is which fsck message
type I choose. I have recorded the reason in the commit message. But I
wanna explain the motivation in cover letter for making the reviewers
easy to understand.

Actually, in the review process of the first version. Junio thought we
should use "FSCK_INFO" and Patrick thought we should use "FSCK_WARN".
And I raised a question here, what is the difference between the
"FSCK_INFO" and "FSCK_WARN" because in "fsck.c::fsck_vreport" function,
we will convert "FSCK_INFO" to "FSCK_WARN" like the following:

    static int fsck_vreport(...)
    {
        enum fsck_msg_type msg_type = fsck_msg_type(msg_id, options);

        if (msg_type == FSCK_FATAL)
            msg_type = FSCK_ERROR;
        if (msg_type == FSCK_INFO)
             msg_type = FSCK_WARN;
        ...
    }

And I have gone back to the history. Actually the first time the fsck
message type was set up at f27d05b170 (fsck: allow upgrading fsck
warnings to errors, 2015-06-22):

  https://lore.kernel.org/git/cover.1418055173.git.johannes.schindelin@gmx.de/

And I have understood why we need "FSCK_INFO". This is because when
setting the "strict" filed in "fsck_options", all the fsck warns will
become fsck errors. For example, this change verifies my thinking:
4dd3b045f5 (fsck: downgrade tree badFilemode to "info", 2022-08-10).

As you can see, this restriction makes the code safer. So, I agree with
Junio, at now, we should use "FSCK_INFO" for trailing garbage and ref
content ends without newline.

But we should report fsck errors for the following two situations for
"git-fsck(1)" will report fsck errors by implicitly checking the ref
database consistency.

1. "parse_loose_ref_contents" fail.
2. symref content is bad (cannot parse).

Thanks,
Jialuo

shejialuo (4):
  ref: initialize "fsck_ref_report" with zero
  ref: add regular ref content check for files backend
  ref: add symbolic ref content check for files backend
  ref: add symlink ref check for files backend

 Documentation/fsck-msgids.txt |  12 +++
 fsck.h                        |   4 +
 refs.c                        |   2 +-
 refs/files-backend.c          | 179 +++++++++++++++++++++++++++++++-
 refs/refs-internal.h          |   2 +-
 t/t0602-reffiles-fsck.sh      | 185 ++++++++++++++++++++++++++++++++++
 6 files changed, 379 insertions(+), 5 deletions(-)

Range-diff against v1:
1:  9ed3026ac5 ! 1:  0367904c81 fsck: introduce "FSCK_REF_REPORT_DEFAULT" macro
    @@ Metadata
     Author: shejialuo <shejialuo@gmail.com>
     
      ## Commit message ##
    -    fsck: introduce "FSCK_REF_REPORT_DEFAULT" macro
    +    ref: initialize "fsck_ref_report" with zero
     
         In "fsck.c::fsck_refs_error_function", we need to tell whether "oid" and
         "referent" is NULL. So, we need to always initialize these parameters to
         NULL instead of letting them point to anywhere when creating a new
         "fsck_ref_report" structure.
     
    -    In order to conveniently create a new "fsck_ref_report", add a new macro
    -    "FSCK_REF_REPORT_DEFAULT".
    +    The original code explicitly specifies the ".path" field to initialize
    +    the "fsck_ref_report" structure. However, it introduces confusion how we
    +    initialize the other fields. In order to avoid this, initialize the
    +    "fsck_ref_report" with zero to make clear that everything in
    +    "fsck_ref_report" is zero initialized.
     
         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Mentored-by: Karthik Nayak <karthik.188@gmail.com>
         Signed-off-by: shejialuo <shejialuo@gmail.com>
     
    - ## fsck.h ##
    -@@ fsck.h: struct fsck_ref_report {
    - 	const char *referent;
    - };
    - 
    -+#define FSCK_REF_REPORT_DEFAULT { \
    -+	.path = NULL, \
    -+	.oid = NULL, \
    -+	.referent = NULL, \
    -+}
    -+
    - struct fsck_options {
    - 	fsck_walk_func walk;
    - 	fsck_error error_func;
    -
      ## refs/files-backend.c ##
     @@ refs/files-backend.c: static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
      		goto cleanup;
      
      	if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {
     -		struct fsck_ref_report report = { .path = NULL };
    -+		struct fsck_ref_report report = FSCK_REF_REPORT_DEFAULT;
    ++		struct fsck_ref_report report = {0};
      
      		strbuf_addf(&sb, "%s/%s", refs_check_dir, iter->relative_path);
      		report.path = sb.buf;
2:  714284cf2b ! 2:  7b6f4145cd ref: add regular ref content check for files backend
    @@ Metadata
      ## Commit message ##
         ref: add regular ref content check for files backend
     
    -    We implicitly reply on "git-fsck(1)" to check the consistency of regular
    -    refs. However, when parsing the regular refs for files backend, we allow
    -    the ref content to end with no newline or contain some garbages. We
    -    should warn the user about above situations.
    +    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.
     
    -    In order to provide above functionality, enhance the "git-refs verify"
    -    command by adding consistency check for regular refs for files backend.
    +    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.
     
    -    Add the following three fsck messages to represent the above situations:
    +    And we cannot either report a warn fsck message to the user. This is
    +    because for "git-receive-pack(1)" and "git-fetch-pack(1)", they will
    +    parse the fsck message type and check the message type by
    +    "fsck.c::is_valid_msg_type". Only the fsck infos are not valid. If we
    +    make the fsck message type to be warn, the user could upgrade the fsck
    +    warnings to errors. And the user can also set the "strict" field in
    +    "fsck_options" to upgrade the fsck warnings to errors.
     
    -    1. "badRefContent(ERROR)": A ref has a bad content.
    -    2. "refMissingNewline(WARN)": A valid ref does not end with newline.
    -    3. "trailingRefContent(WARN)": A ref has trailing contents.
    +    We should not allow the user to upgrade the fsck warnings to errors. It
    +    might cause compatibility issue which will break the legacy repository.
    +    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.
    +
    +    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.
    +
    +    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.
     
         In order to tell whether the ref has trailing content, add a new
         parameter "trailing" to "parse_loose_ref_contents". Then introduce a new
    -    function "files_fsck_refs_content" to check the regular refs.
    +    function "files_fsck_refs_content" to check the regular refs to enhance
    +    the "git-refs verify".
     
         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Mentored-by: Karthik Nayak <karthik.188@gmail.com>
    @@ Documentation/fsck-msgids.txt
      	(WARN) Tree contains entries pointing to a null sha1.
      
     +`refMissingNewline`::
    -+	(WARN) A valid ref does not end with newline.
    ++	(INFO) A valid ref does not end with newline.
     +
     +`trailingRefContent`::
    -+	(WARN) A ref has trailing contents.
    ++	(INFO) A ref has trailing contents.
     +
      `treeNotSorted`::
      	(ERROR) A tree is not properly sorted.
    @@ fsck.h: enum fsck_msg_type {
      	FUNC(BAD_REF_NAME, ERROR) \
      	FUNC(BAD_TIMEZONE, ERROR) \
     @@ fsck.h: enum fsck_msg_type {
    - 	FUNC(HAS_DOTDOT, WARN) \
    - 	FUNC(HAS_DOTGIT, WARN) \
    - 	FUNC(NULL_SHA1, WARN) \
    -+	FUNC(REF_MISSING_NEWLINE, WARN) \
    -+	FUNC(TRAILING_REF_CONTENT, WARN) \
    - 	FUNC(ZERO_PADDED_FILEMODE, WARN) \
    - 	FUNC(NUL_IN_COMMIT, WARN) \
    - 	FUNC(LARGE_PATHNAME, WARN) \
    + 	FUNC(MAILMAP_SYMLINK, INFO) \
    + 	FUNC(BAD_TAG_NAME, INFO) \
    + 	FUNC(MISSING_TAGGER_ENTRY, INFO) \
    ++	FUNC(REF_MISSING_NEWLINE, INFO) \
    ++	FUNC(TRAILING_REF_CONTENT, INFO) \
    + 	/* ignored (elevated when requested) */ \
    + 	FUNC(EXTRA_HEADER_ENTRY, IGNORE)
    + 
     
      ## refs.c ##
     @@ refs.c: static int refs_read_special_head(struct ref_store *ref_store,
    @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
     +				   const char *refs_check_dir,
     +				   struct dir_iterator *iter)
     +{
    -+	struct fsck_ref_report report = FSCK_REF_REPORT_DEFAULT;
     +	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;
    @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
     +		}
     +
     +		if (parse_loose_ref_contents(ref_store->repo->hash_algo,
    -+					    ref_content.buf, &oid, &referent,
    -+					    &type, &trailing, &failure_errno)) {
    ++					     ref_content.buf, &oid, &referent,
    ++					     &type, &trailing, &failure_errno)) {
     +			ret = fsck_report_ref(o, &report,
     +					      FSCK_MSG_BAD_REF_CONTENT,
     +					      "invalid ref content");
    @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
     +				goto cleanup;
     +			}
     +		}
    ++		goto cleanup;
     +	}
     +
     +cleanup:
3:  032b0d6a64 ! 3:  20d8556902 ref: add symbolic ref content check for files backend
    @@ Commit message
         3. "ref: refs/heads/master\n\n"
     
         But we do not allow any non-null trailing garbage. The following are bad
    -    symbolic contents.
    +    symbolic contents which will be reported as fsck error by "git-fsck(1)".
     
         1. "ref: refs/heads/master garbage\n"
         2. "ref: refs/heads/master \n\n\n garbage  "
     
    -    In order to provide above checks, we will traverse the "pointee" to
    -    report the user whether this is null-garbage or no newline. And if
    -    symbolic refs contain non-null garbage, we will report
    -    "FSCK_MSG_BAD_REF_CONTENT" to the user.
    -
    -    Then, we will check the name of the "pointee" is correct by using
    -    "check_refname_format". And then if we can access the "pointee_path" in
    -    the file system, we should ensure that the file type is correct.
    +    In order to provide above checks, we will use "strrchr" to check whether
    +    we have newline in the ref content. Then we will check the name of the
    +    "pointee" is correct by using "check_refname_format". If the function
    +    fails, we need to trim the "pointee" to see whether the null-garbage
    +    causes the function fails. If so, we need to report that there is
    +    null-garbage in the symref content. Otherwise, we should report the user
    +    the "pointee" is bad.
     
         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Mentored-by: Karthik Nayak <karthik.188@gmail.com>
    @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
     +				    struct strbuf *pointee_name,
     +				    struct strbuf *pointee_path)
     +{
    -+	unsigned int newline_num = 0;
    -+	unsigned int space_num = 0;
    ++	const char *newline_pos = NULL;
     +	const char *p = NULL;
     +	struct stat st;
     +	int ret = 0;
    @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
     +		goto out;
     +	}
     +
    -+	while (*p != '\0') {
    -+		if ((space_num || newline_num) && !isspace(*p)) {
    -+			ret = fsck_report_ref(o, report,
    -+					      FSCK_MSG_BAD_REF_CONTENT,
    -+					      "contains non-null garbage");
    -+			goto out;
    -+		}
    -+
    -+		if (*p == '\n') {
    -+			newline_num++;
    -+		} else if (*p == ' ') {
    -+			space_num++;
    -+		}
    -+		p++;
    -+	}
    -+
    -+	if (space_num || newline_num > 1) {
    -+		ret = fsck_report_ref(o, report,
    -+				      FSCK_MSG_TRAILING_REF_CONTENT,
    -+				      "trailing null-garbage");
    -+	} else if (!newline_num) {
    ++	newline_pos = strrchr(p, '\n');
    ++	if (!newline_pos || *(newline_pos + 1)) {
     +		ret = fsck_report_ref(o, report,
     +				      FSCK_MSG_REF_MISSING_NEWLINE,
     +				      "missing newline");
     +	}
     +
    -+	strbuf_rtrim(pointee_name);
    -+
     +	if (check_refname_format(pointee_name->buf, 0)) {
    ++		/*
    ++		 * When containing null-garbage, "check_refname_format" will
    ++		 * fail, we should trim the "pointee" to check again.
    ++		 */
    ++		strbuf_rtrim(pointee_name);
    ++		if (!check_refname_format(pointee_name->buf, 0)) {
    ++			ret = fsck_report_ref(o, report,
    ++					      FSCK_MSG_TRAILING_REF_CONTENT,
    ++					      "trailing null-garbage");
    ++			goto out;
    ++		}
    ++
     +		ret = fsck_report_ref(o, report,
     +				      FSCK_MSG_BAD_SYMREF_POINTEE,
     +				      "points to refname with invalid format");
    @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
      				   const char *refs_check_dir,
      				   struct dir_iterator *iter)
      {
    - 	struct fsck_ref_report report = FSCK_REF_REPORT_DEFAULT;
     +	struct strbuf pointee_path = STRBUF_INIT;
      	struct strbuf ref_content = STRBUF_INIT;
      	struct strbuf referent = STRBUF_INIT;
    @@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_s
     +						       &referent,
     +						       &pointee_path);
      		}
    + 		goto cleanup;
      	}
    - 
     @@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_store,
      	strbuf_release(&refname);
      	strbuf_release(&ref_content);
    @@ t/t0602-reffiles-fsck.sh: test_expect_success 'regular ref content should be che
     +	printf "ref: refs/heads/branch     " > $branch_dir_prefix/a/b/branch-trailing &&
     +	git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    ++	warning: refs/heads/a/b/branch-trailing: refMissingNewline: missing newline
     +	warning: refs/heads/a/b/branch-trailing: trailingRefContent: trailing null-garbage
     +	EOF
     +	rm $branch_dir_prefix/a/b/branch-trailing &&
    @@ t/t0602-reffiles-fsck.sh: test_expect_success 'regular ref content should be che
     +	printf "ref: refs/heads/branch \n\n " > $branch_dir_prefix/a/b/branch-trailing &&
     +	git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    ++	warning: refs/heads/a/b/branch-trailing: refMissingNewline: missing newline
     +	warning: refs/heads/a/b/branch-trailing: trailingRefContent: trailing null-garbage
     +	EOF
     +	rm $branch_dir_prefix/a/b/branch-trailing &&
4:  147a873958 ! 4:  d9867c5f87 ref: add symlink ref consistency check for files backend
    @@ Metadata
     Author: shejialuo <shejialuo@gmail.com>
     
      ## Commit message ##
    -    ref: add symlink ref consistency check for files backend
    +    ref: add symlink ref check for files backend
     
         We have already introduced "files_fsck_symref_target". We should reuse
         this function to handle the symrefs which are legacy symbolic links. We
    @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
     +				    struct strbuf *pointee_path,
     +				    unsigned int symbolic_link)
      {
    - 	unsigned int newline_num = 0;
    - 	unsigned int space_num = 0;
    + 	const char *newline_pos = NULL;
    + 	const char *p = NULL;
     @@ refs/files-backend.c: static int files_fsck_symref_target(struct fsck_options *o,
      		goto out;
      	}
      
    --	while (*p != '\0') {
    --		if ((space_num || newline_num) && !isspace(*p)) {
    --			ret = fsck_report_ref(o, report,
    --					      FSCK_MSG_BAD_REF_CONTENT,
    --					      "contains non-null garbage");
    --			goto out;
    +-	newline_pos = strrchr(p, '\n');
    +-	if (!newline_pos || *(newline_pos + 1)) {
    +-		ret = fsck_report_ref(o, report,
    +-				      FSCK_MSG_REF_MISSING_NEWLINE,
    +-				      "missing newline");
     +	if (!symbolic_link) {
    -+		while (*p != '\0') {
    -+			if ((space_num || newline_num) && !isspace(*p)) {
    -+				ret = fsck_report_ref(o, report,
    -+						      FSCK_MSG_BAD_REF_CONTENT,
    -+						      "contains non-null garbage");
    -+				goto out;
    -+			}
    -+
    -+			if (*p == '\n') {
    -+				newline_num++;
    -+			} else if (*p == ' ') {
    -+				space_num++;
    -+			}
    -+			p++;
    - 		}
    - 
    --		if (*p == '\n') {
    --			newline_num++;
    --		} else if (*p == ' ') {
    --			space_num++;
    -+		if (space_num || newline_num > 1) {
    -+			ret = fsck_report_ref(o, report,
    -+					      FSCK_MSG_TRAILING_REF_CONTENT,
    -+					      "trailing null-garbage");
    -+		} else if (!newline_num) {
    ++		newline_pos = strrchr(p, '\n');
    ++		if (!newline_pos || *(newline_pos + 1)) {
     +			ret = fsck_report_ref(o, report,
     +					      FSCK_MSG_REF_MISSING_NEWLINE,
     +					      "missing newline");
    - 		}
    --		p++;
    --	}
    - 
    --	if (space_num || newline_num > 1) {
    --		ret = fsck_report_ref(o, report,
    --				      FSCK_MSG_TRAILING_REF_CONTENT,
    --				      "trailing null-garbage");
    --	} else if (!newline_num) {
    --		ret = fsck_report_ref(o, report,
    --				      FSCK_MSG_REF_MISSING_NEWLINE,
    --				      "missing newline");
    -+		strbuf_rtrim(pointee_name);
    ++		}
      	}
      
    --	strbuf_rtrim(pointee_name);
    --
      	if (check_refname_format(pointee_name->buf, 0)) {
    +-		/*
    +-		 * When containing null-garbage, "check_refname_format" will
    +-		 * fail, we should trim the "pointee" to check again.
    +-		 */
    +-		strbuf_rtrim(pointee_name);
    +-		if (!check_refname_format(pointee_name->buf, 0)) {
    +-			ret = fsck_report_ref(o, report,
    +-					      FSCK_MSG_TRAILING_REF_CONTENT,
    +-					      "trailing null-garbage");
    +-			goto out;
    ++		if (!symbolic_link) {
    ++			/*
    ++			* When containing null-garbage, "check_refname_format" will
    ++			* fail, we should trim the "pointee" to check again.
    ++			*/
    ++			strbuf_rtrim(pointee_name);
    ++			if (!check_refname_format(pointee_name->buf, 0)) {
    ++				ret = fsck_report_ref(o, report,
    ++						      FSCK_MSG_TRAILING_REF_CONTENT,
    ++						      "trailing null-garbage");
    ++				goto out;
    ++			}
    + 		}
    + 
      		ret = fsck_report_ref(o, report,
    - 				      FSCK_MSG_BAD_SYMREF_POINTEE,
     @@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_store,
    - 	struct fsck_ref_report report = FSCK_REF_REPORT_DEFAULT;
    + {
      	struct strbuf pointee_path = STRBUF_INIT;
      	struct strbuf ref_content = STRBUF_INIT;
     +	struct strbuf abs_gitdir = STRBUF_INIT;
      	struct strbuf referent = STRBUF_INIT;
      	struct strbuf refname = STRBUF_INIT;
    + 	struct fsck_ref_report report = {0};
    ++	const char *pointee_name = NULL;
     +	unsigned int symbolic_link = 0;
      	const char *trailing = NULL;
      	unsigned int type = 0;
    @@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_s
     -						       &pointee_path);
     +						       &pointee_path,
     +						       symbolic_link);
    -+		}
    -+	} else if (S_ISLNK(iter->st.st_mode)) {
    -+		const char *pointee_name = NULL;
    + 		}
    + 		goto cleanup;
    + 	}
    + 
    ++	symbolic_link = 1;
     +
    -+		symbolic_link = 1;
    ++	strbuf_add_real_path(&pointee_path, iter->path.buf);
    ++	strbuf_add_absolute_path(&abs_gitdir, ref_store->gitdir);
    ++	strbuf_normalize_path(&abs_gitdir);
    ++	if (!is_dir_sep(abs_gitdir.buf[abs_gitdir.len - 1]))
    ++		strbuf_addch(&abs_gitdir, '/');
     +
    -+		strbuf_add_real_path(&pointee_path, iter->path.buf);
    -+		strbuf_add_absolute_path(&abs_gitdir, ref_store->gitdir);
    -+		strbuf_normalize_path(&abs_gitdir);
    -+		if (!is_dir_sep(abs_gitdir.buf[abs_gitdir.len - 1]))
    -+			strbuf_addch(&abs_gitdir, '/');
    ++	if (!skip_prefix(pointee_path.buf, abs_gitdir.buf, &pointee_name)) {
    ++		ret = fsck_report_ref(o, &report,
    ++				      FSCK_MSG_BAD_SYMREF_POINTEE,
    ++				      "point to target outside gitdir");
    ++		goto cleanup;
    ++	}
     +
    -+		if (!skip_prefix(pointee_path.buf,
    -+				 abs_gitdir.buf, &pointee_name)) {
    -+			ret = fsck_report_ref(o, &report,
    -+					       FSCK_MSG_BAD_SYMREF_POINTEE,
    -+					       "point to target outside gitdir");
    -+			goto cleanup;
    - 		}
    ++	strbuf_addstr(&referent, pointee_name);
    ++	ret = files_fsck_symref_target(o, &report, refname.buf,
    ++				       &referent, &pointee_path,
    ++				       symbolic_link);
     +
    -+		strbuf_addstr(&referent, pointee_name);
    -+		ret = files_fsck_symref_target(o, &report, refname.buf,
    -+					       &referent, &pointee_path,
    -+					       symbolic_link);
    - 	}
    - 
      cleanup:
    -@@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_store,
    + 	strbuf_release(&refname);
      	strbuf_release(&ref_content);
      	strbuf_release(&referent);
      	strbuf_release(&pointee_path);
-- 
2.46.0