Re: [PATCH] fsck: ignore missing "refs" directory for linked worktrees
- From
shejialuo <shejialuo@gmail.com>
- Date
- Jun 2, 2025, 11:30 UTC
- Message-ID
- <aD2LaKR-nJVoGMWu@ArchLinux>
- In-Reply-To
- <3f731776-9a9e-4c8f-8de9-99d470503345@app.fastmail.com>
On Sat, May 31, 2025 at 02:17:34PM +0200, Kristoffer Haugsbakk wrote:
Show 13 quoted lines
> 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.
Show 18 quoted lines
> > 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.
Show 14 quoted lines
> 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.
Show 61 quoted lines
> 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.
Show 8 quoted lines
> > + 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.
Show 10 quoted lines
> > + 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