[v4,0/5] add ref content check for files backend

Commit Message

shejialuo Sept. 13, 2024, 5:14 p.m. UTC

Hi All:

This version handles some minor problems mainly focus at the improving
commit messages, comments and some minor problems.

1. Split [PATCH v3 2/4] into two commits [PATCH v4 2/5] and [PATCH v4
3/5]. [PATCH v4 2/5] integrates "git-fsck(1)"'s check and [PATCH v4 3/5]
tightens rules to check the refs with trailing garbage and refs without
newline.

2. Handle a lot of typo errors in original [PATCH v3 2/4]. And improve
the fsck-msgids documentation.

3. Improve [PATCH v4 4/5]'s commit message to first introduce the
tighten rules to be consistent with the [PATCH v4 3/5].

4. Remove "badSymrefTarget(ERROR)" fsck message. Add three new messages
to be more specific:

  1. badReferentFiletype(ERROR): The referent of a symref has a bad file
  type.

  2. badReferentName(ERROR): The referent name of a symref is invalid.

  3. escapeReferent(ERROR): The referent of a symref is outside the
  ref directory

5. Handle typos and some minor problems.

Because I add more commits, I provide the "--interdiff" here to make the
reviewer's life easier.

However, because I have not merged the latest ci fixup, so I cannot
verify some jobs in CIs. May need the help from Junio to verify.

Thanks,
Jialuo

shejialuo (5):
  ref: initialize "fsck_ref_report" with zero
  ref: port git-fsck(1) regular refs check for files backend
  ref: add more strict checks for regular refs
  ref: add symref content check for files backend
  ref: add symlink ref content check for files backend

 Documentation/fsck-msgids.txt |  25 +++
 fsck.h                        |   7 +
 refs.c                        |   2 +-
 refs/files-backend.c          | 202 +++++++++++++++++++-
 refs/refs-internal.h          |   2 +-
 t/t0602-reffiles-fsck.sh      | 334 ++++++++++++++++++++++++++++++++++
 6 files changed, 560 insertions(+), 12 deletions(-)

Interdiff against v3:

Comments

Junio C Hamano Sept. 18, 2024, 4:49 p.m. UTC | #1

shejialuo <[e-mail]> writes:

> Because I add more commits, I provide the "--interdiff" here to make the
> reviewer's life easier.

Yeah, for the changes from the previous iteration of this series,
range-diff comparison is pretty much useless.  Interdiff is indeed
more usable, but essentially this iteration deserves reviews with
fresh sets of eyes.

> However, because I have not merged the latest ci fixup, so I cannot
> verify some jobs in CIs. May need the help from Junio to verify.

A good way to do so is to fork a temporary branch at the tip of
these 5 commits, and then either merge or cherry-pick the CI fixup.
Such a temporary branch should be usable for CI testing, right?

Thanks.

PS.

I am not feeling well today; please expect delayed and/or sparse
responses.

Patch

diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt
index 9e8e1ac7f0..31626e765b 100644
--- a/Documentation/fsck-msgids.txt
+++ b/Documentation/fsck-msgids.txt
@@ -20,7 +20,7 @@ 
 	(ERROR) A commit object has a bad parent sha1.
 
 `badRefContent`::
-	(ERROR) A ref has a bad content.
+	(ERROR) A ref has bad content.
 
 `badRefFiletype`::
 	(ERROR) A ref has a bad file type.
@@ -28,9 +28,11 @@ 
 `badRefName`::
 	(ERROR) A ref has an invalid format.
 
-`badSymrefTarget`::
-	(ERROR) The symref target points outside the ref directory or
-	the name of the symref target is invalid.
+`badReferentFiletype`::
+	(ERROR) The referent of a symref has a bad file type.
+
+`badReferentName`::
+	(ERROR) The referent name of a symref is invalid.
 
 `badTagName`::
 	(INFO) A tag has an invalid format.
@@ -53,6 +55,9 @@ 
 `emptyName`::
 	(WARN) A path contains an empty name.
 
+`escapeReferent`::
+	(ERROR) The referent of a symref is outside the "ref" directory.
+
 `extraHeaderEntry`::
 	(IGNORE) Extra headers found after `tagger`.
 
@@ -178,8 +183,8 @@ 
 	(WARN) Tree contains entries pointing to a null sha1.
 
 `refMissingNewline`::
-	(INFO) A ref does not end with newline. This kind of ref may
-	be considered ERROR in the future.
+	(INFO) A ref does not end with newline. This will be
+	considered an error in the future.
 
 `symlinkRef`::
 	(INFO) A symref uses the symbolic link. This kind of symref may
@@ -187,8 +192,8 @@ 
 	symlink support.
 
 `trailingRefContent`::
-	(INFO) A ref has trailing contents. This kind of ref may be
-	considered ERROR in the future.
+	(INFO) A ref has trailing content. This will be
+	considered an error in the future.
 
 `treeNotSorted`::
 	(ERROR) A tree is not properly sorted.
diff --git a/fsck.h b/fsck.h
index 1c6f750812..b72ee632a4 100644
--- a/fsck.h
+++ b/fsck.h
@@ -34,12 +34,14 @@  enum fsck_msg_type {
 	FUNC(BAD_REF_CONTENT, ERROR) \
 	FUNC(BAD_REF_FILETYPE, ERROR) \
 	FUNC(BAD_REF_NAME, ERROR) \
-	FUNC(BAD_SYMREF_TARGET, ERROR) \
+	FUNC(BAD_REFERENT_FILETYPE, ERROR) \
+	FUNC(BAD_REFERENT_NAME, ERROR) \
 	FUNC(BAD_TIMEZONE, ERROR) \
 	FUNC(BAD_TREE, ERROR) \
 	FUNC(BAD_TREE_SHA1, ERROR) \
 	FUNC(BAD_TYPE, ERROR) \
 	FUNC(DUPLICATE_ENTRIES, ERROR) \
+	FUNC(ESCAPE_REFERENT, ERROR) \
 	FUNC(MISSING_AUTHOR, ERROR) \
 	FUNC(MISSING_COMMITTER, ERROR) \
 	FUNC(MISSING_EMAIL, ERROR) \
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2a1b952f0d..c511deb509 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3449,14 +3449,12 @@  static int files_fsck_symref_target(struct fsck_options *o,
 				    unsigned int symbolic_link)
 {
 	size_t len = referent->len - 1;
-	const char *p = NULL;
 	struct stat st;
 	int ret = 0;
 
-	if (!skip_prefix(referent->buf, "refs/", &p)) {
-
+	if (!starts_with(referent->buf, "refs/")) {
 		ret = fsck_report_ref(o, report,
-				      FSCK_MSG_BAD_SYMREF_TARGET,
+				      FSCK_MSG_ESCAPE_REFERENT,
 				      "points to ref outside the refs directory");
 		goto out;
 	}
@@ -3473,7 +3471,7 @@  static int files_fsck_symref_target(struct fsck_options *o,
 
 	if (check_refname_format(referent->buf, 0)) {
 		ret = fsck_report_ref(o, report,
-				      FSCK_MSG_BAD_SYMREF_TARGET,
+				      FSCK_MSG_BAD_REFERENT_NAME,
 				      "points to refname with invalid format");
 		goto out;
 	}
@@ -3485,22 +3483,24 @@  static int files_fsck_symref_target(struct fsck_options *o,
 	}
 
 	/*
-	 * 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.
+	 * Dangling symrefs are common and so we don't report them.
 	 */
-	if (lstat(referent_path->buf, &st))
+	if (lstat(referent_path->buf, &st)) {
+		if (errno != ENOENT) {
+			ret = error_errno(_("unable to stat '%s'"),
+					  referent_path->buf);
+		}
 		goto out;
+	}
 
 	/*
-	 * We cannot distinguish whether "refs/heads/a" is directory or nots by
+	 * We cannot distinguish whether "refs/heads/a" is a directory or not by
 	 * 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,
+				      FSCK_MSG_BAD_REFERENT_FILETYPE,
 				      "points to the directory");
 		goto out;
 	}
@@ -3520,7 +3520,6 @@  static int files_fsck_refs_content(struct ref_store *ref_store,
 	struct strbuf referent = STRBUF_INIT;
 	struct strbuf refname = STRBUF_INIT;
 	struct fsck_ref_report report = {0};
-	unsigned int symbolic_link = 0;
 	const char *trailing = NULL;
 	unsigned int type = 0;
 	int failure_errno = 0;
@@ -3533,7 +3532,6 @@  static int files_fsck_refs_content(struct ref_store *ref_store,
 	if (S_ISLNK(iter->st.st_mode)) {
 		const char* relative_referent_path;
 
-		symbolic_link = 1;
 		ret = fsck_report_ref(o, &report,
 				      FSCK_MSG_SYMLINK_REF,
 				      "use deprecated symbolic link for symref");
@@ -3549,21 +3547,20 @@  static int files_fsck_refs_content(struct ref_store *ref_store,
 				 abs_gitdir.buf,
 				 &relative_referent_path)) {
 			ret = fsck_report_ref(o, &report,
-					      FSCK_MSG_BAD_SYMREF_TARGET,
+					      FSCK_MSG_ESCAPE_REFERENT,
 					      "point to target outside gitdir");
 			goto cleanup;
 		}
 
 		strbuf_addstr(&referent, relative_referent_path);
 		ret = files_fsck_symref_target(o, &report,
-					       &referent, &referent_path,
-					       symbolic_link);
+					       &referent, &referent_path, 1);
 
 		goto cleanup;
 	}
 
 	if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) {
-		ret = error_errno(_("%s/%s: unable to read the ref"),
+		ret = error_errno(_("unable to read ref '%s/%s'"),
 				  refs_check_dir, iter->relative_path);
 		goto cleanup;
 	}
@@ -3578,14 +3575,14 @@  static int files_fsck_refs_content(struct ref_store *ref_store,
 	}
 
 	if (!(type & REF_ISSYMREF)) {
-		if (*trailing == '\0') {
+		if (!*trailing) {
 			ret = fsck_report_ref(o, &report,
 					      FSCK_MSG_REF_MISSING_NEWLINE,
 					      "missing newline");
 			goto cleanup;
 		}
 
-		if (*trailing != '\n' || (*(trailing + 1) != '\0')) {
+		if (*trailing != '\n' || *(trailing + 1)) {
 			ret = fsck_report_ref(o, &report,
 					      FSCK_MSG_TRAILING_REF_CONTENT,
 					      "trailing garbage in ref");
@@ -3602,7 +3599,7 @@  static int files_fsck_refs_content(struct ref_store *ref_store,
 		ret = files_fsck_symref_target(o, &report,
 					       &referent,
 					       &referent_path,
-					       symbolic_link);
+					       0);
 	}
 
 cleanup:
diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
index e735816d5b..7c3579705f 100755
--- a/t/t0602-reffiles-fsck.sh
+++ b/t/t0602-reffiles-fsck.sh
@@ -268,7 +268,7 @@  test_expect_success 'textual symref content should be checked (individual)' '
 	printf "ref: refs/heads/.branch\n" >$branch_dir_prefix/branch-bad-1 &&
 	test_must_fail git refs verify 2>err &&
 	cat >expect <<-EOF &&
-	error: refs/heads/branch-bad-1: badSymrefTarget: points to refname with invalid format
+	error: refs/heads/branch-bad-1: badReferentName: points to refname with invalid format
 	EOF
 	rm $branch_dir_prefix/branch-bad-1 &&
 	test_cmp expect err &&
@@ -276,7 +276,7 @@  test_expect_success 'textual symref content should be checked (individual)' '
 	printf "ref: reflogs/heads/main\n" >$branch_dir_prefix/branch-bad-2 &&
 	test_must_fail git refs verify 2>err &&
 	cat >expect <<-EOF &&
-	error: refs/heads/branch-bad-2: badSymrefTarget: points to ref outside the refs directory
+	error: refs/heads/branch-bad-2: escapeReferent: points to ref outside the refs directory
 	EOF
 	rm $branch_dir_prefix/branch-bad-2 &&
 	test_cmp expect err &&
@@ -284,7 +284,7 @@  test_expect_success 'textual symref content should be checked (individual)' '
 	printf "ref: refs/heads/a\n" >$branch_dir_prefix/branch-bad-3 &&
 	test_must_fail git refs verify 2>err &&
 	cat >expect <<-EOF &&
-	error: refs/heads/branch-bad-3: badSymrefTarget: points to the directory
+	error: refs/heads/branch-bad-3: badReferentFiletype: points to the directory
 	EOF
 	rm $branch_dir_prefix/branch-bad-3 &&
 	test_cmp expect err
@@ -311,9 +311,9 @@  test_expect_success 'textual symref content should be checked (aggregate)' '
 
 	test_must_fail git refs verify 2>err &&
 	cat >expect <<-EOF &&
-	error: refs/heads/branch-bad-1: badSymrefTarget: points to refname with invalid format
-	error: refs/heads/branch-bad-2: badSymrefTarget: points to ref outside the refs directory
-	error: refs/heads/branch-bad-3: badSymrefTarget: points to the directory
+	error: refs/heads/branch-bad-1: badReferentName: points to refname with invalid format
+	error: refs/heads/branch-bad-2: escapeReferent: points to ref outside the refs directory
+	error: refs/heads/branch-bad-3: badReferentFiletype: points to the directory
 	warning: refs/heads/a/b/branch-complicated: refMissingNewline: missing newline
 	warning: refs/heads/a/b/branch-complicated: trailingRefContent: trailing garbage in ref
 	warning: refs/heads/a/b/branch-trailing-1: refMissingNewline: missing newline
@@ -347,7 +347,7 @@  test_expect_success SYMLINKS 'symlink symref content should be checked (individu
 	test_must_fail git refs verify 2>err &&
 	cat >expect <<-EOF &&
 	warning: refs/heads/branch-symbolic-1: symlinkRef: use deprecated symbolic link for symref
-	error: refs/heads/branch-symbolic-1: badSymrefTarget: point to target outside gitdir
+	error: refs/heads/branch-symbolic-1: escapeReferent: point to target outside gitdir
 	EOF
 	rm $branch_dir_prefix/branch-symbolic-1 &&
 	test_cmp expect err &&
@@ -356,7 +356,7 @@  test_expect_success SYMLINKS 'symlink symref content should be checked (individu
 	test_must_fail git refs verify 2>err &&
 	cat >expect <<-EOF &&
 	warning: refs/heads/branch-symbolic-2: symlinkRef: use deprecated symbolic link for symref
-	error: refs/heads/branch-symbolic-2: badSymrefTarget: points to ref outside the refs directory
+	error: refs/heads/branch-symbolic-2: escapeReferent: points to ref outside the refs directory
 	EOF
 	rm $branch_dir_prefix/branch-symbolic-2 &&
 	test_cmp expect err &&
@@ -365,7 +365,7 @@  test_expect_success SYMLINKS 'symlink symref content should be checked (individu
 	test_must_fail git refs verify 2>err &&
 	cat >expect <<-EOF &&
 	warning: refs/heads/branch-symbolic-3: symlinkRef: use deprecated symbolic link for symref
-	error: refs/heads/branch-symbolic-3: badSymrefTarget: points to refname with invalid format
+	error: refs/heads/branch-symbolic-3: badReferentName: points to refname with invalid format
 	EOF
 	rm $branch_dir_prefix/branch-symbolic-3 &&
 	test_cmp expect err &&
@@ -374,7 +374,7 @@  test_expect_success SYMLINKS 'symlink symref content should be checked (individu
 	test_must_fail git refs verify 2>err &&
 	cat >expect <<-EOF &&
 	warning: refs/tags/tag-symbolic-1: symlinkRef: use deprecated symbolic link for symref
-	error: refs/tags/tag-symbolic-1: badSymrefTarget: points to refname with invalid format
+	error: refs/tags/tag-symbolic-1: badReferentName: points to refname with invalid format
 	EOF
 	rm $tag_dir_prefix/tag-symbolic-1 &&
 	test_cmp expect err &&
@@ -383,7 +383,7 @@  test_expect_success SYMLINKS 'symlink symref content should be checked (individu
 	test_must_fail git refs verify 2>err &&
 	cat >expect <<-EOF &&
 	warning: refs/tags/tag-symbolic-2: symlinkRef: use deprecated symbolic link for symref
-	error: refs/tags/tag-symbolic-2: badSymrefTarget: points to the directory
+	error: refs/tags/tag-symbolic-2: badReferentFiletype: points to the directory
 	EOF
 	rm $tag_dir_prefix/tag-symbolic-2 &&
 	test_cmp expect err
@@ -407,11 +407,11 @@  test_expect_success SYMLINKS 'symlink symref content should be checked (aggregat
 
 	test_must_fail git refs verify 2>err &&
 	cat >expect <<-EOF &&
-	error: refs/heads/branch-symbolic-1: badSymrefTarget: point to target outside gitdir
-	error: refs/heads/branch-symbolic-2: badSymrefTarget: points to ref outside the refs directory
-	error: refs/heads/branch-symbolic-3: badSymrefTarget: points to refname with invalid format
-	error: refs/tags/tag-symbolic-1: badSymrefTarget: points to refname with invalid format
-	error: refs/tags/tag-symbolic-2: badSymrefTarget: points to the directory
+	error: refs/heads/branch-symbolic-1: escapeReferent: point to target outside gitdir
+	error: refs/heads/branch-symbolic-2: escapeReferent: points to ref outside the refs directory
+	error: refs/heads/branch-symbolic-3: badReferentName: points to refname with invalid format
+	error: refs/tags/tag-symbolic-1: badReferentName: points to refname with invalid format
+	error: refs/tags/tag-symbolic-2: badReferentFiletype: points to the directory
 	warning: refs/heads/branch-symbolic-1: symlinkRef: use deprecated symbolic link for symref
 	warning: refs/heads/branch-symbolic-2: symlinkRef: use deprecated symbolic link for symref
 	warning: refs/heads/branch-symbolic-3: symlinkRef: use deprecated symbolic link for symref