shejialuo <shejialuo@xxxxxxxxx> writes:
> 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. Just like "git-fsck(1)",
> we only report "not a commit" error when the ref is a branch. Last,
> update the test to exercise the code.
I found this a bit confusing at first, the code does clear up the
confusion. Perhaps we can say something like:
Branches that do not point to a commit type are explicitly called out,
similar to 'git-fsck(1)'.
>
> Mentored-by: Patrick Steinhardt <ps@xxxxxx>
> Mentored-by: Karthik Nayak <karthik.188@xxxxxxxxx>
> Signed-off-by: shejialuo <shejialuo@xxxxxxxxx>
> ---
> refs/files-backend.c | 50 ++++++++++++++++++++++++++++++++--------
> t/t0602-reffiles-fsck.sh | 30 ++++++++++++++++++++++++
> 2 files changed, 70 insertions(+), 10 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 64f51f0da9..0a4912c009 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -20,6 +20,7 @@
> #include "../lockfile.h"
> #include "../object.h"
> #include "../object-file.h"
> +#include "../packfile.h"
> #include "../path.h"
> #include "../dir.h"
> #include "../chdir-notify.h"
> @@ -3589,6 +3590,34 @@ static int files_fsck_symref_target(struct fsck_options *o,
> return ret;
> }
>
> +static int files_fsck_refs_oid(struct fsck_options *o,
> + struct ref_store *ref_store,
> + struct fsck_ref_report report,
> + const char *target_name,
> + struct object_id *oid)
> +{
> + struct object *obj;
> + int ret = 0;
> +
> + if (is_promisor_object(ref_store->repo, oid))
> + return 0;
> +
> + obj = parse_object(ref_store->repo, oid);
> + if (!obj) {
> + ret |= fsck_report_ref(o, &report,
> + FSCK_MSG_BAD_REF_CONTENT,
> + "points to non-existing object %s",
> + oid_to_hex(oid));
Nit: The two conditionals here are mutually exclusive. So we don't have
to do `ret |=`, no? We don't even need `ret` here, we could simply do a
`return fsck_report_ref(...)`.
> + } else if (obj->type != OBJ_COMMIT && is_branch(target_name)) {
> + ret |= fsck_report_ref(o, &report,
> + FSCK_MSG_BAD_REF_CONTENT,
> + "points to non-commit object %s",
> + oid_to_hex(oid));
> + }
Since this is a single lined if/else, we can skip the braces here.
> + return ret;
> +}
> +
> static int files_fsck_refs_content(struct ref_store *ref_store,
> struct fsck_options *o,
> const char *target_name,
> @@ -3654,18 +3683,19 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
> }
>
> if (!(type & REF_ISSYMREF)) {
> + ret |= files_fsck_refs_oid(o, ref_store, report, target_name, &oid);
> +
> if (!*trailing) {
> - ret = fsck_report_ref(o, &report,
> - FSCK_MSG_REF_MISSING_NEWLINE,
> - "misses LF at the end");
> - goto cleanup;
> - }
> - if (*trailing != '\n' || *(trailing + 1)) {
> - ret = fsck_report_ref(o, &report,
> - FSCK_MSG_TRAILING_REF_CONTENT,
> - "has trailing garbage: '%s'", trailing);
> - goto cleanup;
> + 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..75f234a94a 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 branch branch-1 &&
> mkdir -p "$branch_dir_prefix/a/b" &&
>
> git refs verify 2>err &&
> @@ -198,6 +200,28 @@ 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})" "$(git rev-parse branch-1^{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
Reading this error here, I think it would be nicer to say
'badRefContent: branch points to ....' so we know that the specified ref
is a branch.
> + EOF
> + rm $branch_dir_prefix/branch-tree &&
> + 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 &&
> @@ -244,15 +268,21 @@ test_expect_success 'regular ref content should be checked (aggregate)' '
> 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}) &&
> 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 &&
>
> 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-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
> warning: refs/heads/branch-garbage: trailingRefContent: has trailing garbage: '\'' garbage'\''
> --
> 2.47.1
Attachment:
signature.asc
Description: PGP signature
