patchrefs/files: skip lock files during consistency checks
10 messages between Apr 20, 2026 and May 17, 2026, from Karthik Nayak, Junio C Hamano, Christian Couder, Patrick Steinhardt.
Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.
Running consistency checks on the files reference backend involves iterating over all the existing files in the 'refs/' directory and running all `files_fsck_ref()` on each of them.
Unfortunately this also includes the '*.lock' files created by the files backend. While the `files_fsck_refs_name()` check was skipping over such lock files, all other checks still continue to validate them.
Avoid this situation by moving the check for lock files to a layer above and skipping all checks when encountering such a file.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
refs/files-backend.c | 22 +++++++++++-----------
t/t0602-reffiles-fsck.sh | 21 +++++++++++++++++++++
2 files changed, 32 insertions(+), 11 deletions(-)
Show changes to 2 files +32 −11
refs/files-backend.c, t/t0602-reffiles-fsck.sh
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b3b0c25f84..f1bdfbe88e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3864,22 +3864,12 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
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 allow bare ".lock" files.
+ */
+ 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 3c1f553b81..fc67bee161 100755
--- a/t/t0602-reffiles-fsck.sh
+++ b/t/t0602-reffiles-fsck.sh
@@ -87,6 +87,27 @@ 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 'ref name check should be adapted into fsck messages' '
test_when_finished "rm -rf repo" &&
git init repo &&
Re: [PATCH] refs/files: skip lock files during consistency checks
Karthik Nayak <karthik.188@gmail.com> writes:
Show 10 quoted lines
> Running consistency checks on the files reference backend involves
> iterating over all the existing files in the 'refs/' directory and
> running all `files_fsck_ref()` on each of them.
>
> Unfortunately this also includes the '*.lock' files created by the files
> backend. While the `files_fsck_refs_name()` check was skipping over such
> lock files, all other checks still continue to validate them.
>
> Avoid this situation by moving the check for lock files to a layer above
> and skipping all checks when encountering such a file.
Saying "all other checks" when there is only one other check is highly confusing, even though it may not be telling any lies. files_fsck_ref() calls files_fsck_refs_name() and files_fsck_refs_content(), so this change moves the test from the former to a higher level caller so that files_fsck_ref() itself won't be called, the net result is that files_fsck_refs_content)( is not called on these *.lock files.
And this does not explain why only one of the callers of files_fsck_ref() is the best place to add this "*.lock files are exempt" knowledge to, compared to (presumably at the beginning of) files_fsck_ref() itself. If we do not anticipate that we will ever gain new caller to the function and the only meaningful caller that needs this protection is files_fsck_refs_dir(), then the choice may be justifiable, but if we check at the beginning of the callee, we do not have to rely on such an assuption, do we?
Show 5 quoted lines
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> refs/files-backend.c | 22 +++++++++++-----------
> t/t0602-reffiles-fsck.sh | 21 +++++++++++++++++++++
> 2 files changed, 32 insertions(+), 11 deletions(-)
Show 84 quoted lines
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index b3b0c25f84..f1bdfbe88e 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3864,22 +3864,12 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
> 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 allow bare ".lock" files.
> + */
> + 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 3c1f553b81..fc67bee161 100755
> --- a/t/t0602-reffiles-fsck.sh
> +++ b/t/t0602-reffiles-fsck.sh
> @@ -87,6 +87,27 @@ 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 'ref name check should be adapted into fsck messages' '
> test_when_finished "rm -rf repo" &&
> git init repo &&Re: [PATCH] refs/files: skip lock files during consistency checks
On Mon, Apr 20, 2026 at 5:21 PM Karthik Nayak <karthik.188@gmail.com> wrote:
Show 16 quoted lines
> @@ -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 allow bare ".lock" files.
> + */
> + 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;
This just moves code and associated comments, so the following are probably pre-existing issues, but still it seems to me that:
- "do not allow" is not quite what is actually done. There is no ret =
-1 set for example, so if files_fsck_ref() succeeds with the ".lock"
file it could be allowed, or I am missing something?
- a filename like ".stuff.lock" would be treated in the same way as
".lock". I wonder if it's what we want.
Maybe ".lock" or ".stuff.lock" would fail a check_refname_format() somewhere, if they are not ignored, but it's still a bit confusing.
It seems to me that either:
1) we want to ignore all files that end with ".lock" as they might be
used by some tool as lockfiles, and then:
if (ends_with(iter->path.buf, ".lock"))
continue;is enough, or
2) we want to check that all files matching "XXXX.lock" correspond to
a valid XXXX ref, and then we should not completely ignore them, just
ignore their content but check the XXXX part.
For a bug fix, I think implementing 1) is enough. We could implement
2) if we think it's worth it in a separate improvement (with perhaps
a new "staleLockFile" fsck message).
Thanks.
Re: [PATCH] refs/files: skip lock files during consistency checks
Junio C Hamano <gitster@pobox.com> writes:
Show 20 quoted lines
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Running consistency checks on the files reference backend involves
>> iterating over all the existing files in the 'refs/' directory and
>> running all `files_fsck_ref()` on each of them.
>>
>> Unfortunately this also includes the '*.lock' files created by the files
>> backend. While the `files_fsck_refs_name()` check was skipping over such
>> lock files, all other checks still continue to validate them.
>>
>> Avoid this situation by moving the check for lock files to a layer above
>> and skipping all checks when encountering such a file.
>
> Saying "all other checks" when there is only one other check is
> highly confusing, even though it may not be telling any lies.
> files_fsck_ref() calls files_fsck_refs_name() and
> files_fsck_refs_content(), so this change moves the test from the
> former to a higher level caller so that files_fsck_ref() itself
> won't be called, the net result is that files_fsck_refs_content)( is
> not called on these *.lock files.
You're right. I think I wanted to say that moving it to the upper layer skips both files_fsck_refs_name() and files_fsck_refs_content() and any other new functions which would be added to fsck_refs_fn[]. I'll modify to explain this better with more details.
Show 10 quoted lines
>
> And this does not explain why only one of the callers of
> files_fsck_ref() is the best place to add this "*.lock files are
> exempt" knowledge to, compared to (presumably at the beginning of)
> files_fsck_ref() itself. If we do not anticipate that we will ever
> gain new caller to the function and the only meaningful caller that
> needs this protection is files_fsck_refs_dir(), then the choice may
> be justifiable, but if we check at the beginning of the callee, we
> do not have to rely on such an assuption, do we?
>
There are two callers to files_fsck_ref():
1. files_fsck_root_ref(): where is a callback function passed to
for_each_root_ref(), so it only iterates over root refs.
2. files_fsck_refs_dir(): which iterates overs entities within the
'refs/' dir and then runs the checks provided in fsck_refs_fn[].
We don't have to worry about encountering lock files in #1 since we rely on the refs API, but in #2 since we manually iterate over the directory, we need to skip the '.lock' files.
Any new checks which are added should be added via fsck_refs_fn[], so this makes adding the check in files_fsck_refs_dir() the right place.
Will add this additional information into the commit message.
Show 90 quoted lines
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> refs/files-backend.c | 22 +++++++++++-----------
>> t/t0602-reffiles-fsck.sh | 21 +++++++++++++++++++++
>> 2 files changed, 32 insertions(+), 11 deletions(-)
>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index b3b0c25f84..f1bdfbe88e 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -3864,22 +3864,12 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
>> 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 allow bare ".lock" files.
>> + */
>> + 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 3c1f553b81..fc67bee161 100755
>> --- a/t/t0602-reffiles-fsck.sh
>> +++ b/t/t0602-reffiles-fsck.sh
>> @@ -87,6 +87,27 @@ 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 'ref name check should be adapted into fsck messages' '
>> test_when_finished "rm -rf repo" &&
>> git init repo &&Re: [PATCH] refs/files: skip lock files during consistency checks
Christian Couder <christian.couder@gmail.com> writes:
Show 26 quoted lines
> On Mon, Apr 20, 2026 at 5:21 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>
>> @@ -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 allow bare ".lock" files.
>> + */
>> + 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;
>
> This just moves code and associated comments, so the following are
> probably pre-existing issues, but still it seems to me that:
>
> - "do not allow" is not quite what is actually done. There is no ret =
> -1 set for example, so if files_fsck_ref() succeeds with the ".lock"
> file it could be allowed, or I am missing something?
>
The intent was the same before too, we didn't want to ignore bare '.lock' files. Then, we raised an error and we'll do the same now. 'do not allow' is a bit confusing though, will amend it.
> - a filename like ".stuff.lock" would be treated in the same way as
> ".lock". I wonder if it's what we want.
>
Good catch, we only want to ignore reference lock files, these are files which have a preceding text before the '.lock' text. We could simply check the strlen of the path instead.
Show 17 quoted lines
> Maybe ".lock" or ".stuff.lock" would fail a check_refname_format()
> somewhere, if they are not ignored, but it's still a bit confusing.
>
> It seems to me that either:
>
> 1) we want to ignore all files that end with ".lock" as they might be
> used by some tool as lockfiles, and then:
>
> if (ends_with(iter->path.buf, ".lock"))
> continue;
>
> is enough, or
>
> 2) we want to check that all files matching "XXXX.lock" correspond to
> a valid XXXX ref, and then we should not completely ignore them, just
> ignore their content but check the XXXX part.
>
This would be the most idea solution, but practically it gets complicated. The lock files are created for all reference operations, for new references the lock file would be created before the reference exits and we could encounter such a state.
Also there could be a lock file created for a reference update, while the reference itself is packed.
So to keep it simple, we simply skip/ignore lock files.
Show 5 quoted lines
> For a bug fix, I think implementing 1) is enough. We could implement
> 2) if we think it's worth it in a separate improvement (with perhaps
> a new "staleLockFile" fsck message).
>
> Thanks.
Agreed, will modify accordingly.
Re: [PATCH] refs/files: skip lock files during consistency checks
Karthik Nayak <karthik.188@gmail.com> writes:
Show 41 quoted lines
> Christian Couder <christian.couder@gmail.com> writes:
>
>> On Mon, Apr 20, 2026 at 5:21 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>>> @@ -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 allow bare ".lock" files.
>>> + */
>>> + 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;
>>
>> This just moves code and associated comments, so the following are
>> probably pre-existing issues, but still it seems to me that:
>>
>> - "do not allow" is not quite what is actually done. There is no ret =
>> -1 set for example, so if files_fsck_ref() succeeds with the ".lock"
>> file it could be allowed, or I am missing something?
>>
>
> The intent was the same before too, we didn't want to ignore bare
> '.lock' files. Then, we raised an error and we'll do the same now. 'do
> not allow' is a bit confusing though, will amend it.
>
>> - a filename like ".stuff.lock" would be treated in the same way as
>> ".lock". I wonder if it's what we want.
>>
>
> Good catch, we only want to ignore reference lock files, these are files
> which have a preceding text before the '.lock' text. We could simply
> check the strlen of the path instead.
>
Actually I'm wrong, the check is correct, since any refname starting with a '.' is considered incorrect so we do want to report such refs.
[PATCH v2] 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 will 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>
---
Changes in v2:
- Modified the commit message to clarify the changes made and reasoning.
- Modify the comment in the code to be more accurate.
- Add another additional test for bare lock files.
- Link to v1: https://patch.msgid.link/20260420-refs-fsck-skip-lock-files-v1-1-c2595e206a76@gmail.com
---
refs/files-backend.c | 22 +++++++++++-----------
t/t0602-reffiles-fsck.sh | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 52 insertions(+), 11 deletions(-)
Show changes to 2 files +52 −11
refs/files-backend.c, t/t0602-reffiles-fsck.sh
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b3b0c25f84..1504a1e2f3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3864,22 +3864,12 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
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 3c1f553b81..13259821a0 100755
--- 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 &&
Re: [PATCH v2] refs/files: skip lock files during consistency checks
On Wed, Apr 22, 2026 at 11:49:58AM +0200, Karthik Nayak wrote:
Show 10 quoted lines
> 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 will also encounter
Nit, obviously not worth a reroll: maybe do s/will/may/?
Show 12 quoted lines
> 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.
Show 9 quoted lines
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index b3b0c25f84..1504a1e2f3 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -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);
Not a new issue, but this cast made me wonder. As it turns out, basename(3p) is documented as "may modify the string pointed to by path". I assume that this can happen if the path itself ends with a slash for example, as in that case the basename should of course not include the slash itself. So maybe it modifies the caller-provided path directly in that case?
In any case, it shouldn't be much of an issue as we only use this on discovered path names, and those cannot contain contain a trailing slash.
Patrick
Re: [PATCH v2] refs/files: skip lock files during consistency checks
Patrick Steinhardt <ps@pks.im> writes:
Show 14 quoted lines
> On Wed, Apr 22, 2026 at 11:49:58AM +0200, Karthik Nayak wrote:
>> 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 will also encounter
>
> Nit, obviously not worth a reroll: maybe do s/will/may/?
>
I thought will would go with 'intermediate' better, but 'may' is the right choice I guess. Will add it in locally.
Show 32 quoted lines
>> 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.
>
> Makes sense.
>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index b3b0c25f84..1504a1e2f3 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -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);
>
> Not a new issue, but this cast made me wonder. As it turns out,
> basename(3p) is documented as "may modify the string pointed to by
> path". I assume that this can happen if the path itself ends with a
> slash for example, as in that case the basename should of course not
> include the slash itself. So maybe it modifies the caller-provided path
> directly in that case?
>
I guess it depends on the implementation, the glibc for example doesn't seem to [1].
Show 5 quoted lines
> In any case, it shouldn't be much of an issue as we only use this on
> discovered path names, and those cannot contain contain a trailing
> slash.
>
> Patrick
Yeah we should be fine here. Thanks for the review. I'll avoid re-rolling for now and see if there are other changes needed.
[1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=string/basename.c;h=1658ba98d3ff89e8257b36219599184866798d0d;hb=refs/heads/master
[PATCH v3] 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>
---
Changes in v3:
- Fix grammar in the commit message.
- Link to v2: https://patch.msgid.link/20260422-refs-fsck-skip-lock-files-v2-1-9607571ae59a@gmail.com
Changes in v2:
- Modified the commit message to clarify the changes made and reasoning.
- Modify the comment in the code to be more accurate.
- Add another additional test for bare lock files.
- Link to v1: https://patch.msgid.link/20260420-refs-fsck-skip-lock-files-v1-1-c2595e206a76@gmail.com
---
refs/files-backend.c | 22 +++++++++++-----------
t/t0602-reffiles-fsck.sh | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 52 insertions(+), 11 deletions(-)
Show changes to 2 files +52 −11
refs/files-backend.c, t/t0602-reffiles-fsck.sh
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b3b0c25f84..1504a1e2f3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3864,22 +3864,12 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
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 3c1f553b81..13259821a0 100755
--- 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 &&