{"thread":{"id":"65519","subject":"[PATCH] refs/files: skip lock files during consistency checks","startedAt":"2026-04-20T14:03:27Z","lastAt":"2026-05-17T17:32:14Z","messageCount":10,"participants":["Karthik Nayak","Junio C Hamano","Christian Couder","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"541974","messageId":"20260420-refs-fsck-skip-lock-files-v1-1-c2595e206a76@gmail.com","threadId":"65519","inReplyTo":null,"subject":"[PATCH] refs/files: skip lock files during consistency checks","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-04-20T14:03:14Z","receivedAt":"2026-04-20T14:03:27Z","isPatch":true,"body":"Running consistency checks on the files reference backend involves\niterating over all the existing files in the 'refs/' directory and\nrunning all `files_fsck_ref()` on each of them.\n\nUnfortunately this also includes the '*.lock' files created by the files\nbackend. While the `files_fsck_refs_name()` check was skipping over such\nlock files, all other checks still continue to validate them.\n\nAvoid this situation by moving the check for lock files to a layer above\nand skipping all checks when encountering such a file.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n refs/files-backend.c     | 22 +++++++++++-----------\n t/t0602-reffiles-fsck.sh | 21 +++++++++++++++++++++\n 2 files changed, 32 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex b3b0c25f84..f1bdfbe88e 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3864,22 +3864,12 @@ static int files_fsck_refs_content(struct ref_store *ref_store,\n static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,\n \t\t\t\tstruct fsck_options *o,\n \t\t\t\tconst char *refname,\n-\t\t\t\tconst char *path,\n+\t\t\t\tconst char *path UNUSED,\n \t\t\t\tint mode UNUSED)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tconst char *filename;\n \tint ret = 0;\n \n-\tfilename = basename((char *) path);\n-\n-\t/*\n-\t * Ignore the files ending with \".lock\" as they may be lock files\n-\t * However, do not allow bare \".lock\" files.\n-\t */\n-\tif (filename[0] != '.' && ends_with(filename, \".lock\"))\n-\t\tgoto cleanup;\n-\n \tif (is_root_ref(refname))\n \t\tgoto cleanup;\n \n@@ -3939,6 +3929,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,\n \tstruct strbuf refname = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct dir_iterator *iter;\n+\tconst char *filename;\n \tint iter_status;\n \tint ret = 0;\n \n@@ -3962,6 +3953,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,\n \t\t\tstrbuf_addf(&refname, \"worktrees/%s/\", wt->id);\n \t\tstrbuf_addf(&refname, \"refs/%s\", iter->relative_path);\n \n+\t\tfilename = basename((char *) iter->path.buf);\n+\n+\t\t/*\n+\t\t * Ignore the files ending with \".lock\" as they may be lock files\n+\t\t * However, do not allow bare \".lock\" files.\n+\t\t */\n+\t\tif (filename[0] != '.' && ends_with(filename, \".lock\"))\n+\t\t\tcontinue;\n+\n \t\tif (files_fsck_ref(ref_store, o, refname.buf,\n \t\t\t\t   iter->path.buf, iter->st.st_mode) < 0)\n \t\t\tret = -1;\ndiff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh\nindex 3c1f553b81..fc67bee161 100755\n--- a/t/t0602-reffiles-fsck.sh\n+++ b/t/t0602-reffiles-fsck.sh\n@@ -87,6 +87,27 @@ test_expect_success 'ref name should be checked' '\n \t)\n '\n \n+test_expect_success 'lock files should be ignored' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit commit --allow-empty -m initial &&\n+\t\tgit checkout -b branch-1 &&\n+\n+\t\ttouch .git/refs/heads/branch-1.lock &&\n+\t\tgit refs verify 2>err &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\techo \"foobar\" >.git/refs/heads/branch-2 &&\n+\t\ttest_must_fail git refs verify 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: refs/heads/branch-2: badRefContent: foobar\n+\t\tEOF\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n test_expect_success 'ref name check should be adapted into fsck messages' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n\n\n\n"},{"id":"541989","messageId":"xmqqtst5pkg3.fsf@gitster.g","threadId":"65519","inReplyTo":"20260420-refs-fsck-skip-lock-files-v1-1-c2595e206a76@gmail.com","subject":"Re: [PATCH] refs/files: skip lock files during consistency checks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-20T18:15:24Z","receivedAt":"2026-04-20T18:15:27Z","isPatch":true,"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> Running consistency checks on the files reference backend involves\n> iterating over all the existing files in the 'refs/' directory and\n> running all `files_fsck_ref()` on each of them.\n>\n> Unfortunately this also includes the '*.lock' files created by the files\n> backend. While the `files_fsck_refs_name()` check was skipping over such\n> lock files, all other checks still continue to validate them.\n>\n> Avoid this situation by moving the check for lock files to a layer above\n> and skipping all checks when encountering such a file.\n\nSaying \"all other checks\" when there is only one other check is\nhighly confusing, even though it may not be telling any lies.\nfiles_fsck_ref() calls files_fsck_refs_name() and\nfiles_fsck_refs_content(), so this change moves the test from the\nformer to a higher level caller so that files_fsck_ref() itself\nwon't be called, the net result is that files_fsck_refs_content)( is\nnot called on these *.lock files.\n\nAnd this does not explain why only one of the callers of\nfiles_fsck_ref() is the best place to add this \"*.lock files are\nexempt\" knowledge to, compared to (presumably at the beginning of)\nfiles_fsck_ref() itself.  If we do not anticipate that we will ever\ngain new caller to the function and the only meaningful caller that\nneeds this protection is files_fsck_refs_dir(), then the choice may\nbe justifiable, but if we check at the beginning of the callee, we\ndo not have to rely on such an assuption, do we?\n\n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n>  refs/files-backend.c     | 22 +++++++++++-----------\n>  t/t0602-reffiles-fsck.sh | 21 +++++++++++++++++++++\n>  2 files changed, 32 insertions(+), 11 deletions(-)\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index b3b0c25f84..f1bdfbe88e 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -3864,22 +3864,12 @@ static int files_fsck_refs_content(struct ref_store *ref_store,\n>  static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,\n>  \t\t\t\tstruct fsck_options *o,\n>  \t\t\t\tconst char *refname,\n> -\t\t\t\tconst char *path,\n> +\t\t\t\tconst char *path UNUSED,\n>  \t\t\t\tint mode UNUSED)\n>  {\n>  \tstruct strbuf sb = STRBUF_INIT;\n> -\tconst char *filename;\n>  \tint ret = 0;\n>  \n> -\tfilename = basename((char *) path);\n> -\n> -\t/*\n> -\t * Ignore the files ending with \".lock\" as they may be lock files\n> -\t * However, do not allow bare \".lock\" files.\n> -\t */\n> -\tif (filename[0] != '.' && ends_with(filename, \".lock\"))\n> -\t\tgoto cleanup;\n> -\n>  \tif (is_root_ref(refname))\n>  \t\tgoto cleanup;\n>  \n> @@ -3939,6 +3929,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,\n>  \tstruct strbuf refname = STRBUF_INIT;\n>  \tstruct strbuf sb = STRBUF_INIT;\n>  \tstruct dir_iterator *iter;\n> +\tconst char *filename;\n>  \tint iter_status;\n>  \tint ret = 0;\n>  \n> @@ -3962,6 +3953,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,\n>  \t\t\tstrbuf_addf(&refname, \"worktrees/%s/\", wt->id);\n>  \t\tstrbuf_addf(&refname, \"refs/%s\", iter->relative_path);\n>  \n> +\t\tfilename = basename((char *) iter->path.buf);\n> +\n> +\t\t/*\n> +\t\t * Ignore the files ending with \".lock\" as they may be lock files\n> +\t\t * However, do not allow bare \".lock\" files.\n> +\t\t */\n> +\t\tif (filename[0] != '.' && ends_with(filename, \".lock\"))\n> +\t\t\tcontinue;\n> +\n>  \t\tif (files_fsck_ref(ref_store, o, refname.buf,\n>  \t\t\t\t   iter->path.buf, iter->st.st_mode) < 0)\n>  \t\t\tret = -1;\n> diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh\n> index 3c1f553b81..fc67bee161 100755\n> --- a/t/t0602-reffiles-fsck.sh\n> +++ b/t/t0602-reffiles-fsck.sh\n> @@ -87,6 +87,27 @@ test_expect_success 'ref name should be checked' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'lock files should be ignored' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tgit commit --allow-empty -m initial &&\n> +\t\tgit checkout -b branch-1 &&\n> +\n> +\t\ttouch .git/refs/heads/branch-1.lock &&\n> +\t\tgit refs verify 2>err &&\n> +\t\ttest_must_be_empty err &&\n> +\n> +\t\techo \"foobar\" >.git/refs/heads/branch-2 &&\n> +\t\ttest_must_fail git refs verify 2>err &&\n> +\t\tcat >expect <<-EOF &&\n> +\t\terror: refs/heads/branch-2: badRefContent: foobar\n> +\t\tEOF\n> +\t\ttest_cmp expect err\n> +\t)\n> +'\n> +\n>  test_expect_success 'ref name check should be adapted into fsck messages' '\n>  \ttest_when_finished \"rm -rf repo\" &&\n>  \tgit init repo &&\n"},{"id":"542031","messageId":"CAP8UFD2vO415UfEUw34_Whh3bTG0ECV99APH=uaDyiGLiNq1yw@mail.gmail.com","threadId":"65519","inReplyTo":"20260420-refs-fsck-skip-lock-files-v1-1-c2595e206a76@gmail.com","subject":"Re: [PATCH] refs/files: skip lock files during consistency checks","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-04-21T08:18:12Z","receivedAt":"2026-04-21T08:18:25Z","isPatch":true,"body":"On Mon, Apr 20, 2026 at 5:21 PM Karthik Nayak <karthik.188@gmail.com> wrote:\n\n> @@ -3962,6 +3953,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,\n>                         strbuf_addf(&refname, \"worktrees/%s/\", wt->id);\n>                 strbuf_addf(&refname, \"refs/%s\", iter->relative_path);\n>\n> +               filename = basename((char *) iter->path.buf);\n> +\n> +               /*\n> +                * Ignore the files ending with \".lock\" as they may be lock files\n> +                * However, do not allow bare \".lock\" files.\n> +                */\n> +               if (filename[0] != '.' && ends_with(filename, \".lock\"))\n> +                       continue;\n> +\n>                 if (files_fsck_ref(ref_store, o, refname.buf,\n>                                    iter->path.buf, iter->st.st_mode) < 0)\n>                         ret = -1;\n\nThis just moves code and associated comments, so the following are\nprobably pre-existing issues, but still it seems to me that:\n\n- \"do not allow\" is not quite what is actually done. There is no ret =\n-1 set for example, so if files_fsck_ref() succeeds with the \".lock\"\nfile it could be allowed, or I am missing something?\n\n- a filename like \".stuff.lock\" would be treated in the same way as\n\".lock\". I wonder if it's what we want.\n\nMaybe \".lock\" or \".stuff.lock\" would fail a check_refname_format()\nsomewhere, if they are not ignored, but it's still a bit confusing.\n\nIt seems to me that either:\n\n1) we want to ignore all files that end with \".lock\" as they might be\nused by some tool as lockfiles, and then:\n\n               if (ends_with(iter->path.buf, \".lock\"))\n                       continue;\n\nis enough, or\n\n2) we want to check that all files matching \"XXXX.lock\" correspond to\na valid XXXX ref, and then we should not completely ignore them, just\nignore their content but check the XXXX part.\n\nFor a bug fix, I think implementing 1) is enough. We could implement\n2) if we think it's worth it in a separate improvement (with perhaps\na new \"staleLockFile\" fsck message).\n\nThanks.\n"},{"id":"542042","messageId":"CAOLa=ZTUjqE4msQcTDcemczdOuD+dREGuxozttp1EaPgRnTNww@mail.gmail.com","threadId":"65519","inReplyTo":"xmqqtst5pkg3.fsf@gitster.g","subject":"Re: [PATCH] refs/files: skip lock files during consistency checks","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-04-21T12:45:25Z","receivedAt":"2026-04-21T12:45:27Z","isPatch":true,"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> Running consistency checks on the files reference backend involves\n>> iterating over all the existing files in the 'refs/' directory and\n>> running all `files_fsck_ref()` on each of them.\n>>\n>> Unfortunately this also includes the '*.lock' files created by the files\n>> backend. While the `files_fsck_refs_name()` check was skipping over such\n>> lock files, all other checks still continue to validate them.\n>>\n>> Avoid this situation by moving the check for lock files to a layer above\n>> and skipping all checks when encountering such a file.\n>\n> Saying \"all other checks\" when there is only one other check is\n> highly confusing, even though it may not be telling any lies.\n> files_fsck_ref() calls files_fsck_refs_name() and\n> files_fsck_refs_content(), so this change moves the test from the\n> former to a higher level caller so that files_fsck_ref() itself\n> won't be called, the net result is that files_fsck_refs_content)( is\n> not called on these *.lock files.\n\nYou're right. I think I wanted to say that moving it to the upper layer\nskips both files_fsck_refs_name() and files_fsck_refs_content() and any\nother new functions which would be added to fsck_refs_fn[]. I'll modify\nto explain this better with more details.\n\n>\n> And this does not explain why only one of the callers of\n> files_fsck_ref() is the best place to add this \"*.lock files are\n> exempt\" knowledge to, compared to (presumably at the beginning of)\n> files_fsck_ref() itself.  If we do not anticipate that we will ever\n> gain new caller to the function and the only meaningful caller that\n> needs this protection is files_fsck_refs_dir(), then the choice may\n> be justifiable, but if we check at the beginning of the callee, we\n> do not have to rely on such an assuption, do we?\n>\n\nThere are two callers to files_fsck_ref():\n\n1. files_fsck_root_ref(): where is a callback function passed to\nfor_each_root_ref(), so it only iterates over root refs.\n\n2. files_fsck_refs_dir(): which iterates overs entities within the\n'refs/' dir and then runs the checks provided in fsck_refs_fn[].\n\nWe don't have to worry about encountering lock files in #1 since we rely\non the refs API, but in #2 since we manually iterate over the directory,\nwe need to skip the '.lock' files.\n\nAny new checks which are added should be added via fsck_refs_fn[], so\nthis makes adding the check in files_fsck_refs_dir() the right place.\n\nWill add this additional information into the commit message.\n\n>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n>> ---\n>>  refs/files-backend.c     | 22 +++++++++++-----------\n>>  t/t0602-reffiles-fsck.sh | 21 +++++++++++++++++++++\n>>  2 files changed, 32 insertions(+), 11 deletions(-)\n>\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index b3b0c25f84..f1bdfbe88e 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -3864,22 +3864,12 @@ static int files_fsck_refs_content(struct ref_store *ref_store,\n>>  static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,\n>>  \t\t\t\tstruct fsck_options *o,\n>>  \t\t\t\tconst char *refname,\n>> -\t\t\t\tconst char *path,\n>> +\t\t\t\tconst char *path UNUSED,\n>>  \t\t\t\tint mode UNUSED)\n>>  {\n>>  \tstruct strbuf sb = STRBUF_INIT;\n>> -\tconst char *filename;\n>>  \tint ret = 0;\n>>\n>> -\tfilename = basename((char *) path);\n>> -\n>> -\t/*\n>> -\t * Ignore the files ending with \".lock\" as they may be lock files\n>> -\t * However, do not allow bare \".lock\" files.\n>> -\t */\n>> -\tif (filename[0] != '.' && ends_with(filename, \".lock\"))\n>> -\t\tgoto cleanup;\n>> -\n>>  \tif (is_root_ref(refname))\n>>  \t\tgoto cleanup;\n>>\n>> @@ -3939,6 +3929,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,\n>>  \tstruct strbuf refname = STRBUF_INIT;\n>>  \tstruct strbuf sb = STRBUF_INIT;\n>>  \tstruct dir_iterator *iter;\n>> +\tconst char *filename;\n>>  \tint iter_status;\n>>  \tint ret = 0;\n>>\n>> @@ -3962,6 +3953,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,\n>>  \t\t\tstrbuf_addf(&refname, \"worktrees/%s/\", wt->id);\n>>  \t\tstrbuf_addf(&refname, \"refs/%s\", iter->relative_path);\n>>\n>> +\t\tfilename = basename((char *) iter->path.buf);\n>> +\n>> +\t\t/*\n>> +\t\t * Ignore the files ending with \".lock\" as they may be lock files\n>> +\t\t * However, do not allow bare \".lock\" files.\n>> +\t\t */\n>> +\t\tif (filename[0] != '.' && ends_with(filename, \".lock\"))\n>> +\t\t\tcontinue;\n>> +\n>>  \t\tif (files_fsck_ref(ref_store, o, refname.buf,\n>>  \t\t\t\t   iter->path.buf, iter->st.st_mode) < 0)\n>>  \t\t\tret = -1;\n>> diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh\n>> index 3c1f553b81..fc67bee161 100755\n>> --- a/t/t0602-reffiles-fsck.sh\n>> +++ b/t/t0602-reffiles-fsck.sh\n>> @@ -87,6 +87,27 @@ test_expect_success 'ref name should be checked' '\n>>  \t)\n>>  '\n>>\n>> +test_expect_success 'lock files should be ignored' '\n>> +\ttest_when_finished \"rm -rf repo\" &&\n>> +\tgit init repo &&\n>> +\t(\n>> +\t\tcd repo &&\n>> +\t\tgit commit --allow-empty -m initial &&\n>> +\t\tgit checkout -b branch-1 &&\n>> +\n>> +\t\ttouch .git/refs/heads/branch-1.lock &&\n>> +\t\tgit refs verify 2>err &&\n>> +\t\ttest_must_be_empty err &&\n>> +\n>> +\t\techo \"foobar\" >.git/refs/heads/branch-2 &&\n>> +\t\ttest_must_fail git refs verify 2>err &&\n>> +\t\tcat >expect <<-EOF &&\n>> +\t\terror: refs/heads/branch-2: badRefContent: foobar\n>> +\t\tEOF\n>> +\t\ttest_cmp expect err\n>> +\t)\n>> +'\n>> +\n>>  test_expect_success 'ref name check should be adapted into fsck messages' '\n>>  \ttest_when_finished \"rm -rf repo\" &&\n>>  \tgit init repo &&\n"},{"id":"542043","messageId":"CAOLa=ZSj_fmDNo5bgtYeRs0piCq+QR4aydDtRsqK19nPnDFvbw@mail.gmail.com","threadId":"65519","inReplyTo":"CAP8UFD2vO415UfEUw34_Whh3bTG0ECV99APH=uaDyiGLiNq1yw@mail.gmail.com","subject":"Re: [PATCH] refs/files: skip lock files during consistency checks","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-04-21T13:22:46Z","receivedAt":"2026-04-21T13:22:49Z","isPatch":true,"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> On Mon, Apr 20, 2026 at 5:21 PM Karthik Nayak <karthik.188@gmail.com> wrote:\n>\n>> @@ -3962,6 +3953,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,\n>>                         strbuf_addf(&refname, \"worktrees/%s/\", wt->id);\n>>                 strbuf_addf(&refname, \"refs/%s\", iter->relative_path);\n>>\n>> +               filename = basename((char *) iter->path.buf);\n>> +\n>> +               /*\n>> +                * Ignore the files ending with \".lock\" as they may be lock files\n>> +                * However, do not allow bare \".lock\" files.\n>> +                */\n>> +               if (filename[0] != '.' && ends_with(filename, \".lock\"))\n>> +                       continue;\n>> +\n>>                 if (files_fsck_ref(ref_store, o, refname.buf,\n>>                                    iter->path.buf, iter->st.st_mode) < 0)\n>>                         ret = -1;\n>\n> This just moves code and associated comments, so the following are\n> probably pre-existing issues, but still it seems to me that:\n>\n> - \"do not allow\" is not quite what is actually done. There is no ret =\n> -1 set for example, so if files_fsck_ref() succeeds with the \".lock\"\n> file it could be allowed, or I am missing something?\n>\n\nThe intent was the same before too, we didn't want to ignore bare\n'.lock' files. Then, we raised an error and we'll do the same now. 'do\nnot allow' is a bit confusing though, will amend it.\n\n> - a filename like \".stuff.lock\" would be treated in the same way as\n> \".lock\". I wonder if it's what we want.\n>\n\nGood catch, we only want to ignore reference lock files, these are files\nwhich have a preceding text before the '.lock' text. We could simply\ncheck the strlen of the path instead.\n\n> Maybe \".lock\" or \".stuff.lock\" would fail a check_refname_format()\n> somewhere, if they are not ignored, but it's still a bit confusing.\n>\n> It seems to me that either:\n>\n> 1) we want to ignore all files that end with \".lock\" as they might be\n> used by some tool as lockfiles, and then:\n>\n>                if (ends_with(iter->path.buf, \".lock\"))\n>                        continue;\n>\n> is enough, or\n>\n> 2) we want to check that all files matching \"XXXX.lock\" correspond to\n> a valid XXXX ref, and then we should not completely ignore them, just\n> ignore their content but check the XXXX part.\n>\n\nThis would be the most idea solution, but practically it gets\ncomplicated. The lock files are created for all reference operations,\nfor new references the lock file would be created before the reference\nexits and we could encounter such a state.\n\nAlso there could be a lock file created for a reference update, while\nthe reference itself is packed.\n\nSo to keep it simple, we simply skip/ignore lock files.\n\n> For a bug fix, I think implementing 1) is enough. We could implement\n> 2) if we think it's worth it in a separate improvement (with perhaps\n> a new \"staleLockFile\" fsck message).\n>\n> Thanks.\n\nAgreed, will modify accordingly.\n"},{"id":"542047","messageId":"CAOLa=ZSZ=fFCjXt6bM3vojjtY+1imjbw5a7FASnGnDrPYoaBgA@mail.gmail.com","threadId":"65519","inReplyTo":"CAOLa=ZSj_fmDNo5bgtYeRs0piCq+QR4aydDtRsqK19nPnDFvbw@mail.gmail.com","subject":"Re: [PATCH] refs/files: skip lock files during consistency checks","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-04-21T16:04:03Z","receivedAt":"2026-04-21T16:04:05Z","isPatch":true,"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n>> On Mon, Apr 20, 2026 at 5:21 PM Karthik Nayak <karthik.188@gmail.com> wrote:\n>>\n>>> @@ -3962,6 +3953,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,\n>>>                         strbuf_addf(&refname, \"worktrees/%s/\", wt->id);\n>>>                 strbuf_addf(&refname, \"refs/%s\", iter->relative_path);\n>>>\n>>> +               filename = basename((char *) iter->path.buf);\n>>> +\n>>> +               /*\n>>> +                * Ignore the files ending with \".lock\" as they may be lock files\n>>> +                * However, do not allow bare \".lock\" files.\n>>> +                */\n>>> +               if (filename[0] != '.' && ends_with(filename, \".lock\"))\n>>> +                       continue;\n>>> +\n>>>                 if (files_fsck_ref(ref_store, o, refname.buf,\n>>>                                    iter->path.buf, iter->st.st_mode) < 0)\n>>>                         ret = -1;\n>>\n>> This just moves code and associated comments, so the following are\n>> probably pre-existing issues, but still it seems to me that:\n>>\n>> - \"do not allow\" is not quite what is actually done. There is no ret =\n>> -1 set for example, so if files_fsck_ref() succeeds with the \".lock\"\n>> file it could be allowed, or I am missing something?\n>>\n>\n> The intent was the same before too, we didn't want to ignore bare\n> '.lock' files. Then, we raised an error and we'll do the same now. 'do\n> not allow' is a bit confusing though, will amend it.\n>\n>> - a filename like \".stuff.lock\" would be treated in the same way as\n>> \".lock\". I wonder if it's what we want.\n>>\n>\n> Good catch, we only want to ignore reference lock files, these are files\n> which have a preceding text before the '.lock' text. We could simply\n> check the strlen of the path instead.\n>\n\nActually I'm wrong, the check is correct, since any refname starting\nwith a '.' is considered incorrect so we do want to report such refs.\n"},{"id":"542101","messageId":"20260422-refs-fsck-skip-lock-files-v2-1-9607571ae59a@gmail.com","threadId":"65519","inReplyTo":"20260420-refs-fsck-skip-lock-files-v1-1-c2595e206a76@gmail.com","subject":"[PATCH v2] refs/files: skip lock files during consistency checks","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-04-22T09:49:58Z","receivedAt":"2026-04-22T09:50:05Z","isPatch":true,"body":"Consistency checks in the files reference backend involve two steps:\n\n1. Iterate over all entries within the 'refs/' directory and call\n`files_fsck_ref()` on each.\n2. Iterate over all root refs via `for_each_root_ref()` and call\n`files_fsck_ref()` on each.\n\n`files_fsck_ref()` then runs all fsck checks defined in\n`fsck_refs_fn[]`. Step 2 goes through the refs API and only sees valid\nrefs, but step 1 iterates the directory directly and will also encounter\nintermediate '*.lock' files.\n\nCurrently, `files_fsck_refs_name()`, one of the functions in\n`fsck_refs_fn[]`, filters out lock files itself. The other function,\n`files_fsck_refs_content()`, has no such check and would parse the lock\nfile. Any new function added to `fsck_refs_fn[]` would have the same\nproblem.\n\nMove the filter up into `files_fsck_refs_dir()`, where the directory\niteration happens. Since step 2 cannot produce lock files, this is the\nonly site where the filter is needed, and individual checks no longer\nhave to re-implement it.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\nChanges in v2:\n- Modified the commit message to clarify the changes made and reasoning.\n- Modify the comment in the code to be more accurate.\n- Add another additional test for bare lock files.\n- Link to v1: https://patch.msgid.link/20260420-refs-fsck-skip-lock-files-v1-1-c2595e206a76@gmail.com\n---\n refs/files-backend.c     | 22 +++++++++++-----------\n t/t0602-reffiles-fsck.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex b3b0c25f84..1504a1e2f3 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3864,22 +3864,12 @@ static int files_fsck_refs_content(struct ref_store *ref_store,\n static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,\n \t\t\t\tstruct fsck_options *o,\n \t\t\t\tconst char *refname,\n-\t\t\t\tconst char *path,\n+\t\t\t\tconst char *path UNUSED,\n \t\t\t\tint mode UNUSED)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tconst char *filename;\n \tint ret = 0;\n \n-\tfilename = basename((char *) path);\n-\n-\t/*\n-\t * Ignore the files ending with \".lock\" as they may be lock files\n-\t * However, do not allow bare \".lock\" files.\n-\t */\n-\tif (filename[0] != '.' && ends_with(filename, \".lock\"))\n-\t\tgoto cleanup;\n-\n \tif (is_root_ref(refname))\n \t\tgoto cleanup;\n \n@@ -3939,6 +3929,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,\n \tstruct strbuf refname = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct dir_iterator *iter;\n+\tconst char *filename;\n \tint iter_status;\n \tint ret = 0;\n \n@@ -3962,6 +3953,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,\n \t\t\tstrbuf_addf(&refname, \"worktrees/%s/\", wt->id);\n \t\tstrbuf_addf(&refname, \"refs/%s\", iter->relative_path);\n \n+\t\tfilename = basename((char *) iter->path.buf);\n+\n+\t\t/*\n+\t\t * Ignore the files ending with \".lock\" as they may be lock files.\n+\t\t * However, do not skip invalid refnames with '.lock' suffix.\n+\t\t */\n+\t\tif (filename[0] != '.' && ends_with(filename, \".lock\"))\n+\t\t\tcontinue;\n+\n \t\tif (files_fsck_ref(ref_store, o, refname.buf,\n \t\t\t\t   iter->path.buf, iter->st.st_mode) < 0)\n \t\t\tret = -1;\ndiff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh\nindex 3c1f553b81..13259821a0 100755\n--- a/t/t0602-reffiles-fsck.sh\n+++ b/t/t0602-reffiles-fsck.sh\n@@ -87,6 +87,47 @@ test_expect_success 'ref name should be checked' '\n \t)\n '\n \n+test_expect_success 'lock files should be ignored' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit commit --allow-empty -m initial &&\n+\t\tgit checkout -b branch-1 &&\n+\n+\t\ttouch .git/refs/heads/branch-1.lock &&\n+\t\tgit refs verify 2>err &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\techo \"foobar\" >.git/refs/heads/branch-2 &&\n+\t\ttest_must_fail git refs verify 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: refs/heads/branch-2: badRefContent: foobar\n+\t\tEOF\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n+test_expect_success 'bare lock files should not be ignored' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit commit --allow-empty -m initial &&\n+\t\tgit checkout -b branch-1 &&\n+\n+\t\t# invalid refname should be reported\n+\t\tcp .git/refs/heads/branch-1 .git/refs/heads/.branch-1.lock &&\n+\t\t# invalid refname and content should be reported\n+\t\ttouch .git/refs/heads/.lock &&\n+\n+\t\ttest_must_fail git refs verify 2>err &&\n+\t\ttest_grep \"error: refs/heads/.branch-1.lock: badRefName: invalid refname format\" err &&\n+\t\ttest_grep \"error: refs/heads/.lock: badRefName: invalid refname format\" err &&\n+\t\ttest_grep \"error: refs/heads/.lock: badRefContent: \" err\n+\t)\n+'\n+\n test_expect_success 'ref name check should be adapted into fsck messages' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n\n\n\n"},{"id":"542106","messageId":"aeil2Q_Mh_fKCwGa@pks.im","threadId":"65519","inReplyTo":"20260422-refs-fsck-skip-lock-files-v2-1-9607571ae59a@gmail.com","subject":"Re: [PATCH v2] refs/files: skip lock files during consistency checks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-22T10:41:29Z","receivedAt":"2026-04-22T10:41:38Z","isPatch":true,"body":"On Wed, Apr 22, 2026 at 11:49:58AM +0200, Karthik Nayak wrote:\n> Consistency checks in the files reference backend involve two steps:\n> \n> 1. Iterate over all entries within the 'refs/' directory and call\n> `files_fsck_ref()` on each.\n> 2. Iterate over all root refs via `for_each_root_ref()` and call\n> `files_fsck_ref()` on each.\n> \n> `files_fsck_ref()` then runs all fsck checks defined in\n> `fsck_refs_fn[]`. Step 2 goes through the refs API and only sees valid\n> refs, but step 1 iterates the directory directly and will also encounter\n\nNit, obviously not worth a reroll: maybe do s/will/may/?\n\n> intermediate '*.lock' files.\n> \n> Currently, `files_fsck_refs_name()`, one of the functions in\n> `fsck_refs_fn[]`, filters out lock files itself. The other function,\n> `files_fsck_refs_content()`, has no such check and would parse the lock\n> file. Any new function added to `fsck_refs_fn[]` would have the same\n> problem.\n> \n> Move the filter up into `files_fsck_refs_dir()`, where the directory\n> iteration happens. Since step 2 cannot produce lock files, this is the\n> only site where the filter is needed, and individual checks no longer\n> have to re-implement it.\n\nMakes sense.\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index b3b0c25f84..1504a1e2f3 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -3962,6 +3953,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,\n>  \t\t\tstrbuf_addf(&refname, \"worktrees/%s/\", wt->id);\n>  \t\tstrbuf_addf(&refname, \"refs/%s\", iter->relative_path);\n>  \n> +\t\tfilename = basename((char *) iter->path.buf);\n\nNot a new issue, but this cast made me wonder. As it turns out,\nbasename(3p) is documented as \"may modify the string pointed to by\npath\". I assume that this can happen if the path itself ends with a\nslash for example, as in that case the basename should of course not\ninclude the slash itself. So maybe it modifies the caller-provided path\ndirectly in that case?\n\nIn any case, it shouldn't be much of an issue as we only use this on\ndiscovered path names, and those cannot contain contain a trailing\nslash.\n\nPatrick\n"},{"id":"542113","messageId":"CAOLa=ZT1zE+MLeaYE_5jWmNzSvtTTBw3ZAopai+2Ei27kmYm2g@mail.gmail.com","threadId":"65519","inReplyTo":"aeil2Q_Mh_fKCwGa@pks.im","subject":"Re: [PATCH v2] refs/files: skip lock files during consistency checks","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-04-22T12:04:54Z","receivedAt":"2026-04-22T12:04:57Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Wed, Apr 22, 2026 at 11:49:58AM +0200, Karthik Nayak wrote:\n>> Consistency checks in the files reference backend involve two steps:\n>>\n>> 1. Iterate over all entries within the 'refs/' directory and call\n>> `files_fsck_ref()` on each.\n>> 2. Iterate over all root refs via `for_each_root_ref()` and call\n>> `files_fsck_ref()` on each.\n>>\n>> `files_fsck_ref()` then runs all fsck checks defined in\n>> `fsck_refs_fn[]`. Step 2 goes through the refs API and only sees valid\n>> refs, but step 1 iterates the directory directly and will also encounter\n>\n> Nit, obviously not worth a reroll: maybe do s/will/may/?\n>\n\nI thought will would go with 'intermediate' better, but 'may' is the\nright choice I guess. Will add it in locally.\n\n>> intermediate '*.lock' files.\n>>\n>> Currently, `files_fsck_refs_name()`, one of the functions in\n>> `fsck_refs_fn[]`, filters out lock files itself. The other function,\n>> `files_fsck_refs_content()`, has no such check and would parse the lock\n>> file. Any new function added to `fsck_refs_fn[]` would have the same\n>> problem.\n>>\n>> Move the filter up into `files_fsck_refs_dir()`, where the directory\n>> iteration happens. Since step 2 cannot produce lock files, this is the\n>> only site where the filter is needed, and individual checks no longer\n>> have to re-implement it.\n>\n> Makes sense.\n>\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index b3b0c25f84..1504a1e2f3 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -3962,6 +3953,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,\n>>  \t\t\tstrbuf_addf(&refname, \"worktrees/%s/\", wt->id);\n>>  \t\tstrbuf_addf(&refname, \"refs/%s\", iter->relative_path);\n>>\n>> +\t\tfilename = basename((char *) iter->path.buf);\n>\n> Not a new issue, but this cast made me wonder. As it turns out,\n> basename(3p) is documented as \"may modify the string pointed to by\n> path\". I assume that this can happen if the path itself ends with a\n> slash for example, as in that case the basename should of course not\n> include the slash itself. So maybe it modifies the caller-provided path\n> directly in that case?\n>\n\nI guess it depends on the implementation, the glibc for example doesn't\nseem to [1].\n\n> In any case, it shouldn't be much of an issue as we only use this on\n> discovered path names, and those cannot contain contain a trailing\n> slash.\n>\n> Patrick\n\nYeah we should be fine here. Thanks for the review. I'll avoid\nre-rolling for now and see if there are other changes needed.\n\n[1]: https://sourceware.org/git/?p=glibc.git;a=blob;f=string/basename.c;h=1658ba98d3ff89e8257b36219599184866798d0d;hb=refs/heads/master\n"},{"id":"543484","messageId":"20260517-refs-fsck-skip-lock-files-v3-1-b24dfd673c7e@gmail.com","threadId":"65519","inReplyTo":"20260420-refs-fsck-skip-lock-files-v1-1-c2595e206a76@gmail.com","subject":"[PATCH v3] refs/files: skip lock files during consistency checks","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-05-17T17:32:05Z","receivedAt":"2026-05-17T17:32:14Z","isPatch":true,"body":"Consistency checks in the files reference backend involve two steps:\n\n1. Iterate over all entries within the 'refs/' directory and call\n`files_fsck_ref()` on each.\n2. Iterate over all root refs via `for_each_root_ref()` and call\n`files_fsck_ref()` on each.\n\n`files_fsck_ref()` then runs all fsck checks defined in\n`fsck_refs_fn[]`. Step 2 goes through the refs API and only sees valid\nrefs, but step 1 iterates the directory directly and may also encounter\nintermediate '*.lock' files.\n\nCurrently, `files_fsck_refs_name()`, one of the functions in\n`fsck_refs_fn[]`, filters out lock files itself. The other function,\n`files_fsck_refs_content()`, has no such check and would parse the lock\nfile. Any new function added to `fsck_refs_fn[]` would have the same\nproblem.\n\nMove the filter up into `files_fsck_refs_dir()`, where the directory\niteration happens. Since step 2 cannot produce lock files, this is the\nonly site where the filter is needed, and individual checks no longer\nhave to re-implement it.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\nChanges in v3:\n- Fix grammar in the commit message.\n- Link to v2: https://patch.msgid.link/20260422-refs-fsck-skip-lock-files-v2-1-9607571ae59a@gmail.com\n\nChanges in v2:\n- Modified the commit message to clarify the changes made and reasoning.\n- Modify the comment in the code to be more accurate.\n- Add another additional test for bare lock files.\n- Link to v1: https://patch.msgid.link/20260420-refs-fsck-skip-lock-files-v1-1-c2595e206a76@gmail.com\n---\n refs/files-backend.c     | 22 +++++++++++-----------\n t/t0602-reffiles-fsck.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex b3b0c25f84..1504a1e2f3 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3864,22 +3864,12 @@ static int files_fsck_refs_content(struct ref_store *ref_store,\n static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,\n \t\t\t\tstruct fsck_options *o,\n \t\t\t\tconst char *refname,\n-\t\t\t\tconst char *path,\n+\t\t\t\tconst char *path UNUSED,\n \t\t\t\tint mode UNUSED)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tconst char *filename;\n \tint ret = 0;\n \n-\tfilename = basename((char *) path);\n-\n-\t/*\n-\t * Ignore the files ending with \".lock\" as they may be lock files\n-\t * However, do not allow bare \".lock\" files.\n-\t */\n-\tif (filename[0] != '.' && ends_with(filename, \".lock\"))\n-\t\tgoto cleanup;\n-\n \tif (is_root_ref(refname))\n \t\tgoto cleanup;\n \n@@ -3939,6 +3929,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,\n \tstruct strbuf refname = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct dir_iterator *iter;\n+\tconst char *filename;\n \tint iter_status;\n \tint ret = 0;\n \n@@ -3962,6 +3953,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,\n \t\t\tstrbuf_addf(&refname, \"worktrees/%s/\", wt->id);\n \t\tstrbuf_addf(&refname, \"refs/%s\", iter->relative_path);\n \n+\t\tfilename = basename((char *) iter->path.buf);\n+\n+\t\t/*\n+\t\t * Ignore the files ending with \".lock\" as they may be lock files.\n+\t\t * However, do not skip invalid refnames with '.lock' suffix.\n+\t\t */\n+\t\tif (filename[0] != '.' && ends_with(filename, \".lock\"))\n+\t\t\tcontinue;\n+\n \t\tif (files_fsck_ref(ref_store, o, refname.buf,\n \t\t\t\t   iter->path.buf, iter->st.st_mode) < 0)\n \t\t\tret = -1;\ndiff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh\nindex 3c1f553b81..13259821a0 100755\n--- a/t/t0602-reffiles-fsck.sh\n+++ b/t/t0602-reffiles-fsck.sh\n@@ -87,6 +87,47 @@ test_expect_success 'ref name should be checked' '\n \t)\n '\n \n+test_expect_success 'lock files should be ignored' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit commit --allow-empty -m initial &&\n+\t\tgit checkout -b branch-1 &&\n+\n+\t\ttouch .git/refs/heads/branch-1.lock &&\n+\t\tgit refs verify 2>err &&\n+\t\ttest_must_be_empty err &&\n+\n+\t\techo \"foobar\" >.git/refs/heads/branch-2 &&\n+\t\ttest_must_fail git refs verify 2>err &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: refs/heads/branch-2: badRefContent: foobar\n+\t\tEOF\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n+test_expect_success 'bare lock files should not be ignored' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit commit --allow-empty -m initial &&\n+\t\tgit checkout -b branch-1 &&\n+\n+\t\t# invalid refname should be reported\n+\t\tcp .git/refs/heads/branch-1 .git/refs/heads/.branch-1.lock &&\n+\t\t# invalid refname and content should be reported\n+\t\ttouch .git/refs/heads/.lock &&\n+\n+\t\ttest_must_fail git refs verify 2>err &&\n+\t\ttest_grep \"error: refs/heads/.branch-1.lock: badRefName: invalid refname format\" err &&\n+\t\ttest_grep \"error: refs/heads/.lock: badRefName: invalid refname format\" err &&\n+\t\ttest_grep \"error: refs/heads/.lock: badRefContent: \" err\n+\t)\n+'\n+\n test_expect_success 'ref name check should be adapted into fsck messages' '\n \ttest_when_finished \"rm -rf repo\" &&\n \tgit init repo &&\n\n\n\n"}]}