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

[Patrick Steinhardt <ps@pks.im>]

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