{"thread":{"id":"62699","subject":"[PATCH 0/1] ref: add object check for regular ref","startedAt":"2024-12-27T13:39:59Z","lastAt":"2025-01-05T06:30:41Z","messageCount":4,"participants":["shejialuo"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"509609","messageId":"Z26uXy31q83dyxvD@ArchLinux","threadId":"62699","inReplyTo":null,"subject":"[PATCH 0/1] ref: add object check for regular ref","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-12-27T13:40:47Z","receivedAt":"2024-12-27T13:39:59Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"Hi All:\n\nWhen I implement the code for packed ref content checks, I somehow\nnotice that I ignore checks for the object. In the first glance, I think\nI could make this patch in the first of my packed ref content check\nseries. However, this is not a good idea which may cause the reviewers\nmore overhead.\n\nAnd this patch aims at checking whether the object exists and whether\nthe type of the object is correct.\n\nThanks,\nJialuo\n\nshejialuo (1):\n  ref: add object check for regular ref\n\n refs/files-backend.c     | 50 ++++++++++++++++++++++++++++--------\n t/t0602-reffiles-fsck.sh | 55 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 95 insertions(+), 10 deletions(-)\n\n-- \n2.47.1\n\n"},{"id":"509610","messageId":"Z26uzrjxXeXgrx2J@ArchLinux","threadId":"62699","inReplyTo":"Z26uXy31q83dyxvD@ArchLinux","subject":"[PATCH 1/1] ref: add object check for regular ref","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-12-27T13:42:38Z","receivedAt":"2024-12-27T13:41:48Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"Although we use \"parse_loose_ref_content\" to check whether the object id\nis correct, we never parse it into the \"struct object\" structure thus we\nignore checking whether there is a real object existing in the repo and\nwhether the object type is correct.\n\nUse \"parse_object\" to parse the oid for the regular ref content. If the\nobject does not exist, report the error to the user by reusing the fsck\nmessage \"BAD_REF_CONTENT\".\n\nThen, we need to check the type of the object. Unlike \"git-fsck(1)\"\nwhich only report \"not a commit\" error when the ref is a branch. We will\ncheck whether tags point to either annotated tag object or commit\nobject. For other refs, we check that they could only point to commit\nobject.\n\nLast, update the test to exercise the code.\n\nMentored-by: Patrick Steinhardt <ps@pks.im>\nMentored-by: Karthik Nayak <karthik.188@gmail.com>\nSigned-off-by: shejialuo <shejialuo@gmail.com>\n---\n refs/files-backend.c     | 50 ++++++++++++++++++++++++++++--------\n t/t0602-reffiles-fsck.sh | 55 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 95 insertions(+), 10 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 5cfb8b7ca8..816aced923 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -21,6 +21,7 @@\n #include \"../lockfile.h\"\n #include \"../object.h\"\n #include \"../object-file.h\"\n+#include \"../packfile.h\"\n #include \"../path.h\"\n #include \"../dir.h\"\n #include \"../chdir-notify.h\"\n@@ -3703,18 +3704,47 @@ static int files_fsck_refs_content(struct ref_store *ref_store,\n \t}\n \n \tif (!(type & REF_ISSYMREF)) {\n-\t\tif (!*trailing) {\n-\t\t\tret = fsck_report_ref(o, &report,\n-\t\t\t\t\t      FSCK_MSG_REF_MISSING_NEWLINE,\n-\t\t\t\t\t      \"misses LF at the end\");\n-\t\t\tgoto cleanup;\n+\t\tstruct object *obj = parse_object(the_repository, &oid);\n+\n+\t\tif (!obj) {\n+\t\t\tif (!is_promisor_object(the_repository, &oid)) {\n+\t\t\t\tret |= fsck_report_ref(o, &report,\n+\t\t\t\t\t\t       FSCK_MSG_BAD_REF_CONTENT,\n+\t\t\t\t\t\t       \"points to non-existing object %s\",\n+\t\t\t\t\t\t       oid_to_hex(&oid));\n+\t\t\t}\n+\t\t} else if (obj->type != OBJ_COMMIT) {\n+\t\t\tconst char *err_fmt = NULL;\n+\n+\t\t\t/*\n+\t\t\t * Only tag can point to annotated tag object. All other refs\n+\t\t\t * should point to commit object.\n+\t\t\t */\n+\t\t\tif (starts_with(target_name, \"refs/tags/\")) {\n+\t\t\t\tif (obj->type != OBJ_TAG)\n+\t\t\t\t\terr_fmt = \"points to neither commit nor tag object %s\";\n+\t\t\t} else {\n+\t\t\t\terr_fmt = \"points to non-commit object %s\";\n+\t\t\t}\n+\n+\t\t\tif (err_fmt) {\n+\t\t\t\tret |= fsck_report_ref(o, &report,\n+\t\t\t\t\t\t       FSCK_MSG_BAD_REF_CONTENT,\n+\t\t\t\t\t\t       err_fmt, oid_to_hex(&oid));\n+\t\t\t}\n \t\t}\n-\t\tif (*trailing != '\\n' || *(trailing + 1)) {\n-\t\t\tret = fsck_report_ref(o, &report,\n-\t\t\t\t\t      FSCK_MSG_TRAILING_REF_CONTENT,\n-\t\t\t\t\t      \"has trailing garbage: '%s'\", trailing);\n-\t\t\tgoto cleanup;\n+\n+\t\tif (!*trailing) {\n+\t\t\tret |= fsck_report_ref(o, &report,\n+\t\t\t\t\t       FSCK_MSG_REF_MISSING_NEWLINE,\n+\t\t\t\t\t       \"misses LF at the end\");\n+\t\t} else if (*trailing != '\\n' || *(trailing + 1)) {\n+\t\t\tret |= fsck_report_ref(o, &report,\n+\t\t\t\t\t       FSCK_MSG_TRAILING_REF_CONTENT,\n+\t\t\t\t\t       \"has trailing garbage: '%s'\", trailing);\n \t\t}\n+\n+\t\tgoto cleanup;\n \t} else {\n \t\tret = files_fsck_symref_target(o, &report, &referent, 0);\n \t\tgoto cleanup;\ndiff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh\nindex d4a08b823b..ff4ccd250a 100755\n--- a/t/t0602-reffiles-fsck.sh\n+++ b/t/t0602-reffiles-fsck.sh\n@@ -161,8 +161,10 @@ test_expect_success 'regular ref content should be checked (individual)' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n \tbranch_dir_prefix=.git/refs/heads &&\n+\ttag_dir_prefix=.git/refs/tags &&\n \tcd repo &&\n \ttest_commit default &&\n+\tgit tag -a annotated-tag -m \"annotated tag\" &&\n \tmkdir -p \"$branch_dir_prefix/a/b\" &&\n \n \tgit refs verify 2>err &&\n@@ -198,6 +200,47 @@ test_expect_success 'regular ref content should be checked (individual)' '\n \trm $branch_dir_prefix/branch-no-newline &&\n \ttest_cmp expect err &&\n \n+\tfor non_existing_oid in \"$(test_oid 001)\" \"$(test_oid 002)\"\n+\tdo\n+\t\tprintf \"%s\\n\" $non_existing_oid >$branch_dir_prefix/invalid-commit &&\n+\t\ttest_must_fail git refs verify 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: refs/heads/invalid-commit: badRefContent: points to non-existing object $non_existing_oid\n+\t\tEOF\n+\t\trm $branch_dir_prefix/invalid-commit &&\n+\t\ttest_cmp expect err || return 1\n+\tdone &&\n+\n+\tfor tree_oid in \"$(git rev-parse main^{tree})\"\n+\tdo\n+\t\tprintf \"%s\\n\" $tree_oid >$branch_dir_prefix/branch-tree &&\n+\t\ttest_must_fail git refs verify 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: refs/heads/branch-tree: badRefContent: points to non-commit object $tree_oid\n+\t\tEOF\n+\t\trm $branch_dir_prefix/branch-tree &&\n+\t\ttest_cmp expect err &&\n+\n+\t\tprintf \"%s\\n\" $tree_oid >$tag_dir_prefix/tag-tree &&\n+\t\ttest_must_fail git refs verify 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: refs/tags/tag-tree: badRefContent: points to neither commit nor tag object $tree_oid\n+\t\tEOF\n+\t\trm $tag_dir_prefix/tag-tree &&\n+\t\ttest_cmp expect err || return 1\n+\tdone &&\n+\n+\tfor tag_object_hash in \"$(git rev-parse annotated-tag)\"\n+\tdo\n+\t\tprintf \"%s\\n\" $tag_object_hash >$branch_dir_prefix/branch-annotated-tag &&\n+\t\ttest_must_fail git refs verify 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: refs/heads/branch-annotated-tag: badRefContent: points to non-commit object $tag_object_hash\n+\t\tEOF\n+\t\trm $branch_dir_prefix/branch-annotated-tag &&\n+\t\ttest_cmp expect err || return 1\n+\tdone &&\n+\n \tfor trailing_content in \" garbage\" \"    more garbage\"\n \tdo\n \t\tprintf \"%s\" \"$(git rev-parse main)$trailing_content\" >$branch_dir_prefix/branch-garbage &&\n@@ -239,22 +282,34 @@ test_expect_success 'regular ref content should be checked (aggregate)' '\n \ttag_dir_prefix=.git/refs/tags &&\n \tcd repo &&\n \ttest_commit default &&\n+\tgit tag -a annotated-tag -m \"annotated tag\" &&\n \tmkdir -p \"$branch_dir_prefix/a/b\" &&\n \n \tbad_content_1=$(git rev-parse main)x &&\n \tbad_content_2=xfsazqfxcadas &&\n \tbad_content_3=Xfsazqfxcadas &&\n+\tnon_existing_oid=$(test_oid 001) &&\n+\ttree_oid=$(git rev-parse main^{tree}) &&\n+\ttag_oid=$(git rev-parse annotated-tag) &&\n \tprintf \"%s\" $bad_content_1 >$tag_dir_prefix/tag-bad-1 &&\n \tprintf \"%s\" $bad_content_2 >$tag_dir_prefix/tag-bad-2 &&\n \tprintf \"%s\" $bad_content_3 >$branch_dir_prefix/a/b/branch-bad &&\n \tprintf \"%s\" \"$(git rev-parse main)\" >$branch_dir_prefix/branch-no-newline &&\n \tprintf \"%s garbage\" \"$(git rev-parse main)\" >$branch_dir_prefix/branch-garbage &&\n+\tprintf \"%s\\n\" $non_existing_oid >$branch_dir_prefix/branch-non-existing-oid &&\n+\tprintf \"%s\\n\" $tree_oid >$branch_dir_prefix/branch-tree &&\n+\tprintf \"%s\\n\" $tag_oid >$branch_dir_prefix/branch-tag &&\n+\tprintf \"%s\\n\" $tree_oid >$tag_dir_prefix/tag-tree &&\n \n \ttest_must_fail git refs verify 2>err &&\n \tcat >expect <<-EOF &&\n \terror: refs/heads/a/b/branch-bad: badRefContent: $bad_content_3\n+\terror: refs/heads/branch-non-existing-oid: badRefContent: points to non-existing object $non_existing_oid\n+\terror: refs/heads/branch-tag: badRefContent: points to non-commit object $tag_oid\n+\terror: refs/heads/branch-tree: badRefContent: points to non-commit object $tree_oid\n \terror: refs/tags/tag-bad-1: badRefContent: $bad_content_1\n \terror: refs/tags/tag-bad-2: badRefContent: $bad_content_2\n+\terror: refs/tags/tag-tree: badRefContent: points to neither commit nor tag object $tree_oid\n \twarning: refs/heads/branch-garbage: trailingRefContent: has trailing garbage: '\\'' garbage'\\''\n \twarning: refs/heads/branch-no-newline: refMissingNewline: misses LF at the end\n \tEOF\n-- \n2.47.1\n\n"},{"id":"509612","messageId":"Z26ylU3umgLiwuZT@ArchLinux","threadId":"62699","inReplyTo":"Z26uXy31q83dyxvD@ArchLinux","subject":"Re: [PATCH 0/1] ref: add object check for regular ref","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-12-27T13:58:45Z","receivedAt":"2024-12-27T13:57:56Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Fri, Dec 27, 2024 at 09:40:47PM +0800, shejialuo wrote:\n> Hi All:\n> \n> When I implement the code for packed ref content checks, I somehow\n> notice that I ignore checks for the object. In the first glance, I think\n> I could make this patch in the first of my packed ref content check\n> series. However, this is not a good idea which may cause the reviewers\n> more overhead.\n> \n> And this patch aims at checking whether the object exists and whether\n> the type of the object is correct.\n> \n> Thanks,\n> Jialuo\n> \n> shejialuo (1):\n>   ref: add object check for regular ref\n> \n>  refs/files-backend.c     | 50 ++++++++++++++++++++++++++++--------\n>  t/t0602-reffiles-fsck.sh | 55 ++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 95 insertions(+), 10 deletions(-)\n> \n> -- \n> 2.47.1\n> \n\nForget to tell Junio, this patch is based on the latest master branch.\nI don't use the \"sj/refs-content-check\" due to there are some conflicts\nwhen using \"is_promisor_object\" in the latest master branch.\n"},{"id":"509921","messageId":"Z3onCyH1ZUoY3Ukg@ArchLinux","threadId":"62699","inReplyTo":"Z26uXy31q83dyxvD@ArchLinux","subject":"Re: [PATCH 0/1] ref: add object check for regular ref","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-01-05T06:30:35Z","receivedAt":"2025-01-05T06:30:41Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Fri, Dec 27, 2024 at 09:40:47PM +0800, shejialuo wrote:\n> Hi All:\n> \n> When I implement the code for packed ref content checks, I somehow\n> notice that I ignore checks for the object. In the first glance, I think\n> I could make this patch in the first of my packed ref content check\n> series. However, this is not a good idea which may cause the reviewers\n> more overhead.\n> \n> And this patch aims at checking whether the object exists and whether\n> the type of the object is correct.\n> \n> Thanks,\n> Jialuo\n> \n> shejialuo (1):\n>   ref: add object check for regular ref\n> \n>  refs/files-backend.c     | 50 ++++++++++++++++++++++++++++--------\n>  t/t0602-reffiles-fsck.sh | 55 ++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 95 insertions(+), 10 deletions(-)\n> \n> -- \n> 2.47.1\n> \n\nPlease ignore this patch, I will send it together within the packed-ref\nconsistency implementation.\n\nThanks,\n"}]}