Re: [PATCH] fsck: ignore missing "refs" directory for linked worktrees

[shejialuo <shejialuo@gmail.com>]

On Sat, May 31, 2025 at 02:17:34PM +0200, Kristoffer Haugsbakk wrote:
> On Sat, May 31, 2025, at 05:39, shejialuo wrote:
> > It is reported that "git refs verify" would fail when encountering
> > worktrees created on Git v2.43.0 or older versions. These versions
> 
> Nit: maybe
> 
>     "git refs verify" doesn't work if there are worktrees created on Git
>     v2.43.0 ...
> 
> The part about it specifically not working if there are no worktree refs
> might be obvious when you take in all of the context here (no refs/
> directory).  I don’t know.
> 

Right, I will improve this.

> > don't automatically create the "refs" directory, causing the error:
> >
> >     error: cannot open directory .git/worktrees/<worktree name>/refs:
> >     No such file or directory
> >
> > Since 8f4c00de95 (builtin/worktree: create refdb via ref backend,
> > 2024-01-08), we automatically create the "refs" directory for new
> > worktrees. However, the fsck code incorrectly assumes all linked
> > worktrees have this directory, thus introducing compatibility issue.
> 
> Thanks for finding that commit.
> 
> At this point in the message it seems like the fsck code never worked
> with these old linked worktrees.  But `git refs verify` used to work
> with them until 7c78d819e6a (ref: support multiple worktrees check for
> refs, 2024-11-20) which was part of v2.48.0.  So I think it’s worth
> mentioning that commit as well.
> 

Good suggestion.

> You wrote on the first email which I’ll just respond to here since
> it’s relevant:
> 
> https://lore.kernel.org/git/a2a50127-6ab9-4d8a-abcc-b1a741df293e@app.fastmail.com/T/#mada29f8b0d02091d21412d8bd57cc666bc657c04
> 
> > > And hope that this fix would land in this release.
> 
> Like I said in the first email the only minor regression in this release
> cycle is that git-fsck(1) reports these errors on stderr because the
> default `--reference`.  This was how I spotted the issue on rc0.  But I
> neglected to mention that the commit that introduced `--references`
> (default) for git-fsck(1) is v2.48.0-rc1-49-gc1cf918d3ad (builtin/fsck:
> add `git refs verify` child process, 2025-02-28).[1]
> 

Because we would call "git refs verify" subprocess in "git-fsck(1)" in
this release cycle, I just want to fix this problem before the release.
Thus, it won't affect the users.

> So based on the last what’s cooking email[2] it depends on if the stderr
> output regresssion from git-fsck(1) in this release cycle is severe
> enough to need be fixed in this release.  Because the `git refs-verify`
> problem has been there since v2.48.0.
> 
> † 1: Part of the reason for neglecting that was that building that
>     commit fails for me.  So the bisection skipped it and couldn’t find the
>     commit.  Is that just me?  The merge commit does build de35b7b3ff (Merge
>     branch 'sj/ref-consistency-checks-more', 2025-03-26).  I changed
>     `start_progress(r, _("Checking ref database"), 1);` to
>     `progress = start_progress(_("Checking ref database"), 1);`.  I
>     don’t know if that is wrong but it made the bisection script run
>     without having to error out with `125`.
> 
> [2]: https://lore.kernel.org/git/xmqqiklhd3tc.fsf@gitster.g/T/#u
> 
> >
> > Check for ENOENT errno before reporting directory access errors for
> > linked worktrees to maintain backward compatibility.
> >
> > Reported-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> > Signed-off-by: shejialuo <shejialuo@gmail.com>
> > ---
> >  refs/files-backend.c     |  3 +++
> >  t/t0602-reffiles-fsck.sh | 15 +++++++++++++++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > index 4d1f65a57a..bf6f89b1d1 100644
> > --- a/refs/files-backend.c
> > +++ b/refs/files-backend.c
> > @@ -3762,6 +3762,9 @@ static int files_fsck_refs_dir(struct ref_store
> > *ref_store,
> >
> >  	iter = dir_iterator_begin(sb.buf, 0);
> >  	if (!iter) {
> > +		if (errno == ENOENT && !is_main_worktree(wt))
> > +			goto out;
> > +
> >  		ret = error_errno(_("cannot open directory %s"), sb.buf);
> >  		goto out;
> >  	}
> > diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
> > index f671ac4d3a..615b7c0683 100755
> > --- a/t/t0602-reffiles-fsck.sh
> > +++ b/t/t0602-reffiles-fsck.sh
> > @@ -110,6 +110,21 @@ test_expect_success 'ref name check should be
> > adapted into fsck messages' '
> >  	)
> >  '
> >
> > +test_expect_success 'no refs directory of worktree should not cause problems' '
> > +	test_when_finished "rm -rf repo" &&
> > +	git init repo &&
> > +	(
> > +		cd repo &&
> > +		test_commit initial &&
> > +
> 
> Nit: blank line?
> 

I would improve this. Normally in this test file, I would add a blank
line to indicate the basic setup ends. So, I should do the following

	cd repo &&
	test_commit initial &&
	git worktree add --detach ./worktree &&

	rm -rf ...

I would improve this in the next version.

> > +		git worktree add --detach ./worktree &&
> > +		# Simulate old directory layout
> > +		rm -rf ./git/worktrees/worktree/refs &&
> 
> Eric[3] had a `git rev-parse --git-dir` suggestion instead of using `.git`.
> 
> [3]: https://lore.kernel.org/git/a2a50127-6ab9-4d8a-abcc-b1a741df293e@app.fastmail.com/T/#mb42bdb046c391f2583c2200668945408a2d0396f
> 

Good suggestion, let me improve this.

> > +		git refs verify 2>err &&
> > +		test_must_be_empty err
> > +	)
> > +'
> > +
> >  test_expect_success 'ref name check should work for multiple worktrees' '
> >  	test_when_finished "rm -rf repo" &&
> >  	git init repo &&
> > --
> > 2.49.0

Thanks,
Jialuo