refs/files: skip lock files during consistency checks

Consistency checks in the files reference backend involve two steps:

1. Iterate over all entries within the 'refs/' directory and call
`files_fsck_ref()` on each.
2. Iterate over all root refs via `for_each_root_ref()` and call
`files_fsck_ref()` on each.

`files_fsck_ref()` then runs all fsck checks defined in
`fsck_refs_fn[]`. Step 2 goes through the refs API and only sees valid
refs, but step 1 iterates the directory directly and may also encounter
intermediate '*.lock' files.

Currently, `files_fsck_refs_name()`, one of the functions in
`fsck_refs_fn[]`, filters out lock files itself. The other function,
`files_fsck_refs_content()`, has no such check and would parse the lock
file. Any new function added to `fsck_refs_fn[]` would have the same
problem.

Move the filter up into `files_fsck_refs_dir()`, where the directory
iteration happens. Since step 2 cannot produce lock files, this is the
only site where the filter is needed, and individual checks no longer
have to re-implement it.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/refs/files-backend.c b/refs/files-backend.c
index b3b0c25f84e5033f078546a0840079b4bbaa6e90..1504a1e2f36c56e4e2ed6ad33034389c236ab914 100644 (file)
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3864,22 +3864,12 @@ cleanup:
 static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
                                struct fsck_options *o,
                                const char *refname,
-                               const char *path,
+                               const char *path UNUSED,
                                int mode UNUSED)
 {
        struct strbuf sb = STRBUF_INIT;
-       const char *filename;
        int ret = 0;
 
-       filename = basename((char *) path);
-
-       /*
-        * Ignore the files ending with ".lock" as they may be lock files
-        * However, do not allow bare ".lock" files.
-        */
-       if (filename[0] != '.' && ends_with(filename, ".lock"))
-               goto cleanup;
-
        if (is_root_ref(refname))
                goto cleanup;
 
@@ -3939,6 +3929,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
        struct strbuf refname = STRBUF_INIT;
        struct strbuf sb = STRBUF_INIT;
        struct dir_iterator *iter;
+       const char *filename;
        int iter_status;
        int ret = 0;
 
@@ -3962,6 +3953,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
                        strbuf_addf(&refname, "worktrees/%s/", wt->id);
                strbuf_addf(&refname, "refs/%s", iter->relative_path);
 
+               filename = basename((char *) iter->path.buf);
+
+               /*
+                * Ignore the files ending with ".lock" as they may be lock files.
+                * However, do not skip invalid refnames with '.lock' suffix.
+                */
+               if (filename[0] != '.' && ends_with(filename, ".lock"))
+                       continue;
+
                if (files_fsck_ref(ref_store, o, refname.buf,
                                   iter->path.buf, iter->st.st_mode) < 0)
                        ret = -1;

diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
index 3c1f553b8120ec6d8de641696e85c3cb79191f6a..13259821a02ef106dcf559048b1c3f9d8c96b1ec 100755 (executable)
--- a/t/t0602-reffiles-fsck.sh
+++ b/t/t0602-reffiles-fsck.sh
@@ -87,6 +87,47 @@ test_expect_success 'ref name should be checked' '
        )
 '
 
+test_expect_success 'lock files should be ignored' '
+       test_when_finished "rm -rf repo" &&
+       git init repo &&
+       (
+               cd repo &&
+               git commit --allow-empty -m initial &&
+               git checkout -b branch-1 &&
+
+               touch .git/refs/heads/branch-1.lock &&
+               git refs verify 2>err &&
+               test_must_be_empty err &&
+
+               echo "foobar" >.git/refs/heads/branch-2 &&
+               test_must_fail git refs verify 2>err &&
+               cat >expect <<-EOF &&
+               error: refs/heads/branch-2: badRefContent: foobar
+               EOF
+               test_cmp expect err
+       )
+'
+
+test_expect_success 'bare lock files should not be ignored' '
+       test_when_finished "rm -rf repo" &&
+       git init repo &&
+       (
+               cd repo &&
+               git commit --allow-empty -m initial &&
+               git checkout -b branch-1 &&
+
+               # invalid refname should be reported
+               cp .git/refs/heads/branch-1 .git/refs/heads/.branch-1.lock &&
+               # invalid refname and content should be reported
+               touch .git/refs/heads/.lock &&
+
+               test_must_fail git refs verify 2>err &&
+               test_grep "error: refs/heads/.branch-1.lock: badRefName: invalid refname format" err &&
+               test_grep "error: refs/heads/.lock: badRefName: invalid refname format" err &&
+               test_grep "error: refs/heads/.lock: badRefContent: " err
+       )
+'
+
 test_expect_success 'ref name check should be adapted into fsck messages' '
        test_when_finished "rm -rf repo" &&
        git init repo &&