[PATCH 0/1] ref: add object check for regular ref

[PATCH 0/1] ref: add object check for regular ref

[shejialuo]

Hi All:

When I implement the code for packed ref content checks, I somehow
notice that I ignore checks for the object. In the first glance, I think
I could make this patch in the first of my packed ref content check
series. However, this is not a good idea which may cause the reviewers
more overhead.

And this patch aims at checking whether the object exists and whether
the type of the object is correct.

Thanks,
Jialuo

shejialuo (1):
  ref: add object check for regular ref

 refs/files-backend.c     | 50 ++++++++++++++++++++++++++++--------
 t/t0602-reffiles-fsck.sh | 55 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+), 10 deletions(-)

-- 
2.47.1

[PATCH 1/1] ref: add object check for regular ref

[shejialuo]

Although we use "parse_loose_ref_content" to check whether the object id
is correct, we never parse it into the "struct object" structure thus we
ignore checking whether there is a real object existing in the repo and
whether the object type is correct.

Use "parse_object" to parse the oid for the regular ref content. If the
object does not exist, report the error to the user by reusing the fsck
message "BAD_REF_CONTENT".

Then, we need to check the type of the object. Unlike "git-fsck(1)"
which only report "not a commit" error when the ref is a branch. We will
check whether tags point to either annotated tag object or commit
object. For other refs, we check that they could only point to commit
object.

Last, update the test to exercise the code.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 refs/files-backend.c     | 50 ++++++++++++++++++++++++++++--------
 t/t0602-reffiles-fsck.sh | 55 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+), 10 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5cfb8b7ca8..816aced923 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -21,6 +21,7 @@
 #include "../lockfile.h"
 #include "../object.h"
 #include "../object-file.h"
+#include "../packfile.h"
 #include "../path.h"
 #include "../dir.h"
 #include "../chdir-notify.h"
@@ -3703,18 +3704,47 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 	}
 
 	if (!(type & REF_ISSYMREF)) {
-		if (!*trailing) {
-			ret = fsck_report_ref(o, &report,
-					      FSCK_MSG_REF_MISSING_NEWLINE,
-					      "misses LF at the end");
-			goto cleanup;
+		struct object *obj = parse_object(the_repository, &oid);
+
+		if (!obj) {
+			if (!is_promisor_object(the_repository, &oid)) {
+				ret |= fsck_report_ref(o, &report,
+						       FSCK_MSG_BAD_REF_CONTENT,
+						       "points to non-existing object %s",
+						       oid_to_hex(&oid));
+			}
+		} else if (obj->type != OBJ_COMMIT) {
+			const char *err_fmt = NULL;
+
+			/*
+			 * Only tag can point to annotated tag object. All other refs
+			 * should point to commit object.
+			 */
+			if (starts_with(target_name, "refs/tags/")) {
+				if (obj->type != OBJ_TAG)
+					err_fmt = "points to neither commit nor tag object %s";
+			} else {
+				err_fmt = "points to non-commit object %s";
+			}
+
+			if (err_fmt) {
+				ret |= fsck_report_ref(o, &report,
+						       FSCK_MSG_BAD_REF_CONTENT,
+						       err_fmt, oid_to_hex(&oid));
+			}
 		}
-		if (*trailing != '\n' || *(trailing + 1)) {
-			ret = fsck_report_ref(o, &report,
-					      FSCK_MSG_TRAILING_REF_CONTENT,
-					      "has trailing garbage: '%s'", trailing);
-			goto cleanup;
+
+		if (!*trailing) {
+			ret |= fsck_report_ref(o, &report,
+					       FSCK_MSG_REF_MISSING_NEWLINE,
+					       "misses LF at the end");
+		} else if (*trailing != '\n' || *(trailing + 1)) {
+			ret |= fsck_report_ref(o, &report,
+					       FSCK_MSG_TRAILING_REF_CONTENT,
+					       "has trailing garbage: '%s'", trailing);
 		}
+
+		goto cleanup;
 	} else {
 		ret = files_fsck_symref_target(o, &report, &referent, 0);
 		goto cleanup;
diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
index d4a08b823b..ff4ccd250a 100755
--- a/t/t0602-reffiles-fsck.sh
+++ b/t/t0602-reffiles-fsck.sh
@@ -161,8 +161,10 @@ test_expect_success 'regular ref content should be checked (individual)' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
 	branch_dir_prefix=.git/refs/heads &&
+	tag_dir_prefix=.git/refs/tags &&
 	cd repo &&
 	test_commit default &&
+	git tag -a annotated-tag -m "annotated tag" &&
 	mkdir -p "$branch_dir_prefix/a/b" &&
 
 	git refs verify 2>err &&
@@ -198,6 +200,47 @@ test_expect_success 'regular ref content should be checked (individual)' '
 	rm $branch_dir_prefix/branch-no-newline &&
 	test_cmp expect err &&
 
+	for non_existing_oid in "$(test_oid 001)" "$(test_oid 002)"
+	do
+		printf "%s\n" $non_existing_oid >$branch_dir_prefix/invalid-commit &&
+		test_must_fail git refs verify 2>err &&
+		cat >expect <<-EOF &&
+		error: refs/heads/invalid-commit: badRefContent: points to non-existing object $non_existing_oid
+		EOF
+		rm $branch_dir_prefix/invalid-commit &&
+		test_cmp expect err || return 1
+	done &&
+
+	for tree_oid in "$(git rev-parse main^{tree})"
+	do
+		printf "%s\n" $tree_oid >$branch_dir_prefix/branch-tree &&
+		test_must_fail git refs verify 2>err &&
+		cat >expect <<-EOF &&
+		error: refs/heads/branch-tree: badRefContent: points to non-commit object $tree_oid
+		EOF
+		rm $branch_dir_prefix/branch-tree &&
+		test_cmp expect err &&
+
+		printf "%s\n" $tree_oid >$tag_dir_prefix/tag-tree &&
+		test_must_fail git refs verify 2>err &&
+		cat >expect <<-EOF &&
+		error: refs/tags/tag-tree: badRefContent: points to neither commit nor tag object $tree_oid
+		EOF
+		rm $tag_dir_prefix/tag-tree &&
+		test_cmp expect err || return 1
+	done &&
+
+	for tag_object_hash in "$(git rev-parse annotated-tag)"
+	do
+		printf "%s\n" $tag_object_hash >$branch_dir_prefix/branch-annotated-tag &&
+		test_must_fail git refs verify 2>err &&
+		cat >expect <<-EOF &&
+		error: refs/heads/branch-annotated-tag: badRefContent: points to non-commit object $tag_object_hash
+		EOF
+		rm $branch_dir_prefix/branch-annotated-tag &&
+		test_cmp expect err || return 1
+	done &&
+
 	for trailing_content in " garbage" "    more garbage"
 	do
 		printf "%s" "$(git rev-parse main)$trailing_content" >$branch_dir_prefix/branch-garbage &&
@@ -239,22 +282,34 @@ test_expect_success 'regular ref content should be checked (aggregate)' '
 	tag_dir_prefix=.git/refs/tags &&
 	cd repo &&
 	test_commit default &&
+	git tag -a annotated-tag -m "annotated tag" &&
 	mkdir -p "$branch_dir_prefix/a/b" &&
 
 	bad_content_1=$(git rev-parse main)x &&
 	bad_content_2=xfsazqfxcadas &&
 	bad_content_3=Xfsazqfxcadas &&
+	non_existing_oid=$(test_oid 001) &&
+	tree_oid=$(git rev-parse main^{tree}) &&
+	tag_oid=$(git rev-parse annotated-tag) &&
 	printf "%s" $bad_content_1 >$tag_dir_prefix/tag-bad-1 &&
 	printf "%s" $bad_content_2 >$tag_dir_prefix/tag-bad-2 &&
 	printf "%s" $bad_content_3 >$branch_dir_prefix/a/b/branch-bad &&
 	printf "%s" "$(git rev-parse main)" >$branch_dir_prefix/branch-no-newline &&
 	printf "%s garbage" "$(git rev-parse main)" >$branch_dir_prefix/branch-garbage &&
+	printf "%s\n" $non_existing_oid >$branch_dir_prefix/branch-non-existing-oid &&
+	printf "%s\n" $tree_oid >$branch_dir_prefix/branch-tree &&
+	printf "%s\n" $tag_oid >$branch_dir_prefix/branch-tag &&
+	printf "%s\n" $tree_oid >$tag_dir_prefix/tag-tree &&
 
 	test_must_fail git refs verify 2>err &&
 	cat >expect <<-EOF &&
 	error: refs/heads/a/b/branch-bad: badRefContent: $bad_content_3
+	error: refs/heads/branch-non-existing-oid: badRefContent: points to non-existing object $non_existing_oid
+	error: refs/heads/branch-tag: badRefContent: points to non-commit object $tag_oid
+	error: refs/heads/branch-tree: badRefContent: points to non-commit object $tree_oid
 	error: refs/tags/tag-bad-1: badRefContent: $bad_content_1
 	error: refs/tags/tag-bad-2: badRefContent: $bad_content_2
+	error: refs/tags/tag-tree: badRefContent: points to neither commit nor tag object $tree_oid
 	warning: refs/heads/branch-garbage: trailingRefContent: has trailing garbage: '\'' garbage'\''
 	warning: refs/heads/branch-no-newline: refMissingNewline: misses LF at the end
 	EOF
-- 
2.47.1

Re: [PATCH 0/1] ref: add object check for regular ref

[shejialuo]

On Fri, Dec 27, 2024 at 09:40:47PM +0800, shejialuo wrote:
> Hi All:
> 
> When I implement the code for packed ref content checks, I somehow
> notice that I ignore checks for the object. In the first glance, I think
> I could make this patch in the first of my packed ref content check
> series. However, this is not a good idea which may cause the reviewers
> more overhead.
> 
> And this patch aims at checking whether the object exists and whether
> the type of the object is correct.
> 
> Thanks,
> Jialuo
> 
> shejialuo (1):
>   ref: add object check for regular ref
> 
>  refs/files-backend.c     | 50 ++++++++++++++++++++++++++++--------
>  t/t0602-reffiles-fsck.sh | 55 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 95 insertions(+), 10 deletions(-)
> 
> -- 
> 2.47.1
> 

Forget to tell Junio, this patch is based on the latest master branch.
I don't use the "sj/refs-content-check" due to there are some conflicts
when using "is_promisor_object" in the latest master branch.

Re: [PATCH 0/1] ref: add object check for regular ref

[shejialuo]

On Fri, Dec 27, 2024 at 09:40:47PM +0800, shejialuo wrote:
> Hi All:
> 
> When I implement the code for packed ref content checks, I somehow
> notice that I ignore checks for the object. In the first glance, I think
> I could make this patch in the first of my packed ref content check
> series. However, this is not a good idea which may cause the reviewers
> more overhead.
> 
> And this patch aims at checking whether the object exists and whether
> the type of the object is correct.
> 
> Thanks,
> Jialuo
> 
> shejialuo (1):
>   ref: add object check for regular ref
> 
>  refs/files-backend.c     | 50 ++++++++++++++++++++++++++++--------
>  t/t0602-reffiles-fsck.sh | 55 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 95 insertions(+), 10 deletions(-)
> 
> -- 
> 2.47.1
> 

Please ignore this patch, I will send it together within the packed-ref
consistency implementation.

Thanks,