[PATCH] refs/files: skip lock files during consistency checks

[PATCH] refs/files: skip lock files during consistency checks

[Karthik Nayak]

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(-)

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

[Junio C Hamano]

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.

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?

> 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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 5512 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> 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.

>
> 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.

>> 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 &&

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH] refs/files: skip lock files during consistency checks

[Christian Couder]

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?

- 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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3069 bytes --]

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.

> 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.

> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH] refs/files: skip lock files during consistency checks

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2042 bytes --]

Karthik Nayak <karthik.188@gmail.com> writes:

> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v2] refs/files: skip lock files during consistency checks

[Karthik Nayak]

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(-)

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

[Patrick Steinhardt]

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/?

> 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?

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2676 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> 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.

>> 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].

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]