Re: [PATCH v2 2/4] ref: add regular ref content check for files backend
From: Patrick Steinhardt <hidden>
Date: 2024-08-28 12:50:06
On Wed, Aug 28, 2024 at 12:07:58AM +0800, shejialuo wrote:
quoted hunk ↗ jump to hunk
@@ -170,6 +173,12 @@ `nullSha1`:: (WARN) Tree contains entries pointing to a null sha1. +`refMissingNewline`:: + (INFO) A valid ref does not end with newline.
This reads a bit funny to me. If the ref is valid, why do we complain? Maybe this would read better if you said "An otherwise valid ref does not end with a newline".
quoted hunk ↗ jump to hunk
@@ -3430,6 +3434,65 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store, const char *refs_check_dir, struct dir_iterator *iter); +static int files_fsck_refs_content(struct ref_store *ref_store, + struct fsck_options *o, + const char *refs_check_dir, + struct dir_iterator *iter) +{ + 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; + struct object_id oid; + int ret = 0; + + strbuf_addf(&refname, "%s/%s", refs_check_dir, iter->relative_path); + report.path = refname.buf; + + if (S_ISREG(iter->st.st_mode)) {
This is still indenting the whole body. You mentioned that you don't want to use `goto`, but in our codebase it's actually quite idiomatic. And you already use it anyway.
quoted hunk ↗ jump to hunk
diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh index 71a4d1a5ae..7c1910d784 100755 --- a/t/t0602-reffiles-fsck.sh +++ b/t/t0602-reffiles-fsck.sh@@ -89,4 +89,91 @@ test_expect_success 'ref name check should be adapted into fsck messages' ' test_must_be_empty err ' +test_expect_success 'regular ref content should be checked' ' + test_when_finished "rm -rf repo" && + git init repo && + branch_dir_prefix=.git/refs/heads && + tag_dir_prefix=.git/refs/tags && + cd repo && + git commit --allow-empty -m initial && + git checkout -b branch-1 && + git tag tag-1 && + git commit --allow-empty -m second && + git checkout -b branch-2 && + git tag tag-2 && + git checkout -b a/b/tag-2 &&
Wouldn't it be sufficient to only create a single commit, e.g. via `test_commit`? From all I can see all you need is some object ID, so creating the tags and second commit doesn't seem to be necessary.
+ printf "%s" "$(git rev-parse branch-1)" > $branch_dir_prefix/branch-1-no-newline &&
We don't typically have spaces after the redirect. So you should remove them here and in all the subsequent instances.
quoted hunk ↗ jump to hunk
+ git refs verify 2>err && + cat >expect <<-EOF && + warning: refs/heads/branch-1-no-newline: refMissingNewline: missing newline + EOF + rm $branch_dir_prefix/branch-1-no-newline && + test_cmp expect err &&
I was wondering whether each of these cases should be a separate test, but that may be a bit wasteful. Alternatively, can we maybe set up a single repository with all the garbage that we want to verify and then double check that executing `git refs verify` surfaces them all in a single invocation? Patrick