* Re: [PATCH] fsck: ignore missing "refs" directory for linked worktrees
2025-05-31 3:39 ` [PATCH] fsck: ignore missing "refs" directory for linked worktrees shejialuo
@ 2025-05-31 12:17 ` Kristoffer Haugsbakk
2025-06-02 1:33 ` Junio C Hamano
2025-06-02 11:30 ` shejialuo
2025-06-02 9:53 ` Phillip Wood
` (2 subsequent siblings)
3 siblings, 2 replies; 21+ messages in thread
From: Kristoffer Haugsbakk @ 2025-05-31 12:17 UTC (permalink / raw)
To: shejialuo, git; +Cc: Patrick Steinhardt, Karthik Nayak, Eric Sunshine
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.
> 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.
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]
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?
> + 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
> + 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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] fsck: ignore missing "refs" directory for linked worktrees
2025-05-31 12:17 ` Kristoffer Haugsbakk
@ 2025-06-02 1:33 ` Junio C Hamano
2025-06-02 11:30 ` shejialuo
1 sibling, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2025-06-02 1:33 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: shejialuo, git, Patrick Steinhardt, Karthik Nayak, Eric Sunshine
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> 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 ...
Yeah, "It is reported that" was somewhat odd introduction.
>> don't automatically create the "refs" directory, causing the error:
>>
>> error: cannot open directory .git/worktrees/<worktree name>/refs:
>> No such file or directory
The original of this part already reads quite well, I think.
>> 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.
Yup. And that one is v2.44.0-rc0~58^2, and that is where "v2.43" in
the above description comes from.
> 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.
> 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]
Thanks for a careful analysis. The "fix" is rather obvious, so
let's see if we can come up with the final wording of the commit log
message and merge it down in time ;-).
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] fsck: ignore missing "refs" directory for linked worktrees
2025-05-31 12:17 ` Kristoffer Haugsbakk
2025-06-02 1:33 ` Junio C Hamano
@ 2025-06-02 11:30 ` shejialuo
1 sibling, 0 replies; 21+ messages in thread
From: shejialuo @ 2025-06-02 11:30 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: git, Patrick Steinhardt, Karthik Nayak, Eric Sunshine
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] fsck: ignore missing "refs" directory for linked worktrees
2025-05-31 3:39 ` [PATCH] fsck: ignore missing "refs" directory for linked worktrees shejialuo
2025-05-31 12:17 ` Kristoffer Haugsbakk
@ 2025-06-02 9:53 ` Phillip Wood
2025-06-02 10:24 ` Patrick Steinhardt
2025-06-02 12:16 ` shejialuo
2025-06-02 12:41 ` shejialuo
2025-06-02 13:26 ` [PATCH v2 0/1] [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs shejialuo
3 siblings, 2 replies; 21+ messages in thread
From: Phillip Wood @ 2025-06-02 9:53 UTC (permalink / raw)
To: shejialuo, git
Cc: Kristoffer Haugsbakk, Patrick Steinhardt, Karthik Nayak,
Eric Sunshine, Junio C Hamano
Hi Shejialuo
On 31/05/2025 04:39, shejialuo wrote:
> 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;
> }
I think it would be clearer to write this as
if (is_main_worktree(wt) || errno != ENOENT)
ret = error_errno(_("cannot open directory %s"), sb.buf);
goto out;
so that the condition that triggers the error message is explicit rather
than having to mentally invert the condition to figure out when we
return an error
Best Wishes
Phillip
> 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 &&
> +
> + git worktree add --detach ./worktree &&
> + # Simulate old directory layout
> + rm -rf ./git/worktrees/worktree/refs &&
> + 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 &&
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] fsck: ignore missing "refs" directory for linked worktrees
2025-06-02 9:53 ` Phillip Wood
@ 2025-06-02 10:24 ` Patrick Steinhardt
2025-06-02 13:50 ` phillip.wood123
2025-06-02 19:49 ` Junio C Hamano
2025-06-02 12:16 ` shejialuo
1 sibling, 2 replies; 21+ messages in thread
From: Patrick Steinhardt @ 2025-06-02 10:24 UTC (permalink / raw)
To: phillip.wood
Cc: shejialuo, git, Kristoffer Haugsbakk, Karthik Nayak,
Eric Sunshine, Junio C Hamano
On Mon, Jun 02, 2025 at 10:53:50AM +0100, Phillip Wood wrote:
> Hi Shejialuo
>
> On 31/05/2025 04:39, shejialuo wrote:
> > 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;
> > }
>
> I think it would be clearer to write this as
>
> if (is_main_worktree(wt) || errno != ENOENT)
> ret = error_errno(_("cannot open directory %s"), sb.buf);
> goto out;
>
> so that the condition that triggers the error message is explicit rather
> than having to mentally invert the condition to figure out when we return an
> error
The downside though is that this mandates that `is_main_worktree()` must
never set `errno` itself. So while it may be clearer, the original
version feels safer to me.
Patrick
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] fsck: ignore missing "refs" directory for linked worktrees
2025-06-02 10:24 ` Patrick Steinhardt
@ 2025-06-02 13:50 ` phillip.wood123
2025-06-02 19:49 ` Junio C Hamano
1 sibling, 0 replies; 21+ messages in thread
From: phillip.wood123 @ 2025-06-02 13:50 UTC (permalink / raw)
To: Patrick Steinhardt, phillip.wood
Cc: shejialuo, git, Kristoffer Haugsbakk, Karthik Nayak,
Eric Sunshine, Junio C Hamano
On 02/06/2025 11:24, Patrick Steinhardt wrote:
> On Mon, Jun 02, 2025 at 10:53:50AM +0100, Phillip Wood wrote:
>> Hi Shejialuo
>>
>> On 31/05/2025 04:39, shejialuo wrote:
>>> 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;
>>> }
>>
>> I think it would be clearer to write this as
>>
>> if (is_main_worktree(wt) || errno != ENOENT)
>> ret = error_errno(_("cannot open directory %s"), sb.buf);
>> goto out;
>>
>> so that the condition that triggers the error message is explicit rather
>> than having to mentally invert the condition to figure out when we return an
>> error
>
> The downside though is that this mandates that `is_main_worktree()` must
> never set `errno` itself. So while it may be clearer, the original
> version feels safer to me.
Oh good point. We could save errno and then check the saved version but
it maybe it is not worth changing it.
Best Wishes
Phillip>
> Patrick
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH] fsck: ignore missing "refs" directory for linked worktrees
2025-06-02 10:24 ` Patrick Steinhardt
2025-06-02 13:50 ` phillip.wood123
@ 2025-06-02 19:49 ` Junio C Hamano
1 sibling, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2025-06-02 19:49 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: phillip.wood, shejialuo, git, Kristoffer Haugsbakk, Karthik Nayak,
Eric Sunshine
Patrick Steinhardt <ps@pks.im> writes:
> On Mon, Jun 02, 2025 at 10:53:50AM +0100, Phillip Wood wrote:
>> Hi Shejialuo
>>
>> On 31/05/2025 04:39, shejialuo wrote:
>> > 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;
>> > }
>>
>> I think it would be clearer to write this as
>>
>> if (is_main_worktree(wt) || errno != ENOENT)
>> ret = error_errno(_("cannot open directory %s"), sb.buf);
>> goto out;
>>
>> so that the condition that triggers the error message is explicit rather
>> than having to mentally invert the condition to figure out when we return an
>> error
>
> The downside though is that this mandates that `is_main_worktree()` must
> never set `errno` itself. So while it may be clearer, the original
> version feels safer to me.
FWIW, I found that the logic flow of the original more natural than
the proposed rewrite. "dir_iterator_begin() appears to have failed
by not returning a usable iterator, so we may need to complain, but
as a special case, we can tolerate missing refs/ hierarchy if we are
not in the primary working tree." was how I read these three
additional lines.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] fsck: ignore missing "refs" directory for linked worktrees
2025-06-02 9:53 ` Phillip Wood
2025-06-02 10:24 ` Patrick Steinhardt
@ 2025-06-02 12:16 ` shejialuo
1 sibling, 0 replies; 21+ messages in thread
From: shejialuo @ 2025-06-02 12:16 UTC (permalink / raw)
To: phillip.wood
Cc: git, Kristoffer Haugsbakk, Patrick Steinhardt, Karthik Nayak,
Eric Sunshine, Junio C Hamano
On Mon, Jun 02, 2025 at 10:53:50AM +0100, Phillip Wood wrote:
> Hi Shejialuo
>
> On 31/05/2025 04:39, shejialuo wrote:
> > 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;
> > }
>
> I think it would be clearer to write this as
>
> if (is_main_worktree(wt) || errno != ENOENT)
> ret = error_errno(_("cannot open directory %s"), sb.buf);
> goto out;
>
> so that the condition that triggers the error message is explicit rather
> than having to mentally invert the condition to figure out when we return an
> error
>
I agree with you that by using this way, when reading above code, we
could know explicitly in which situation, we would report the error.
Patrick has given his safety concern with reordering the condition
check. If `is_main_worktree(wt)` were to modify error (although there is
a minor possibility that it would), it could interfere with next errno
check.
Besides this, I somehow prefer the short-circuit way. Although in the
current code, we only have small code paths after the short-circuit way,
this pattern follows a common defensive programming practice where we
handle special cases early and exit quickly. This approach reduces nesting
and makes the main logic flow cleaner by filtering out edge cases upfront.
So, let's keep this. Really thanks for your suggestion.
> Best Wishes
>
> Phillip
>
Jialuo
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] fsck: ignore missing "refs" directory for linked worktrees
2025-05-31 3:39 ` [PATCH] fsck: ignore missing "refs" directory for linked worktrees shejialuo
2025-05-31 12:17 ` Kristoffer Haugsbakk
2025-06-02 9:53 ` Phillip Wood
@ 2025-06-02 12:41 ` shejialuo
2025-06-02 13:26 ` [PATCH v2 0/1] [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs shejialuo
3 siblings, 0 replies; 21+ messages in thread
From: shejialuo @ 2025-06-02 12:41 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Patrick Steinhardt, Karthik Nayak,
Eric Sunshine
On Sat, May 31, 2025 at 11:39:18AM +0800, shejialuo wrote:
> +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 &&
> +
> + git worktree add --detach ./worktree &&
> + # Simulate old directory layout
> + rm -rf ./git/worktrees/worktree/refs &&
FYI. I made a mistake here, it should be ".git/worktrees" but not "./git".
> + 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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 0/1] [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs
2025-05-31 3:39 ` [PATCH] fsck: ignore missing "refs" directory for linked worktrees shejialuo
` (2 preceding siblings ...)
2025-06-02 12:41 ` shejialuo
@ 2025-06-02 13:26 ` shejialuo
2025-06-02 13:29 ` [PATCH v2 1/1] fsck: ignore missing "refs" directory for linked worktrees shejialuo
2025-06-02 14:40 ` [PATCH v3 0/1] [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs shejialuo
3 siblings, 2 replies; 21+ messages in thread
From: shejialuo @ 2025-06-02 13:26 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Karthik Nayak, Eric Sunshine,
Kristoffer Haugsbakk, Junio C Hamano
Hi All:
This version updates the following things:
1. Update the commit message to incorporate the commit message that
introduces the BUG to better explain.
2. Update the shell script to avoid hardcode gitdir path.
3. Change "rm -rf" to be "rmdir", which would be safer because we should
delete an empty directory, which has a better semantics.
Thanks,
Jialuo
shejialuo (1):
fsck: ignore missing "refs" directory for linked worktrees
refs/files-backend.c | 3 +++
t/t0602-reffiles-fsck.sh | 17 +++++++++++++++++
2 files changed, 20 insertions(+)
Range-diff against v1:
1: bfde11adb6 ! 1: d949a8a646 fsck: ignore missing "refs" directory for linked worktrees
@@ Metadata
## Commit message ##
fsck: ignore missing "refs" directory for linked worktrees
- It is reported that "git refs verify" would fail when encountering
- worktrees created on Git v2.43.0 or older versions. These versions
- don't automatically create the "refs" directory, causing the error:
+ "git refs verify" doesn't work if there are worktrees created on Git
+ v2.43.0 or older versions. These versions 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.
+ worktrees. And in 7c78d819e6 (ref: support multiple worktrees check for
+ refs, 2024-11-20), we assume that all linked worktrees have this
+ directory and would wrongly report an error to the user, thus
+ introducing compatibility issue.
Check for ENOENT errno before reporting directory access errors for
linked worktrees to maintain backward compatibility.
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref name check should be adapted
+ (
+ cd repo &&
+ test_commit initial &&
-+
+ git worktree add --detach ./worktree &&
++
++ cd worktree &&
++ worktree_refdir="$(git rev-parse --git-dir)/refs" &&
+ # Simulate old directory layout
-+ rm -rf ./git/worktrees/worktree/refs &&
++ rmdir "$worktree_refdir" &&
+ git refs verify 2>err &&
+ test_must_be_empty err
+ )
--
2.49.0
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH v2 1/1] fsck: ignore missing "refs" directory for linked worktrees
2025-06-02 13:26 ` [PATCH v2 0/1] [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs shejialuo
@ 2025-06-02 13:29 ` shejialuo
2025-06-02 13:59 ` Kristoffer Haugsbakk
2025-06-02 14:40 ` [PATCH v3 0/1] [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs shejialuo
1 sibling, 1 reply; 21+ messages in thread
From: shejialuo @ 2025-06-02 13:29 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Karthik Nayak, Eric Sunshine,
Kristoffer Haugsbakk, Junio C Hamano, Phillip Wood
"git refs verify" doesn't work if there are worktrees created on Git
v2.43.0 or older versions. These versions 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. And in 7c78d819e6 (ref: support multiple worktrees check for
refs, 2024-11-20), we assume that all linked worktrees have this
directory and would wrongly report an error to the user, thus
introducing compatibility issue.
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 | 17 +++++++++++++++++
2 files changed, 20 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..9ff91d1a2b 100755
--- a/t/t0602-reffiles-fsck.sh
+++ b/t/t0602-reffiles-fsck.sh
@@ -110,6 +110,23 @@ 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 &&
+ git worktree add --detach ./worktree &&
+
+ cd worktree &&
+ worktree_refdir="$(git rev-parse --git-dir)/refs" &&
+ # Simulate old directory layout
+ rmdir "$worktree_refdir" &&
+ 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 1/1] fsck: ignore missing "refs" directory for linked worktrees
2025-06-02 13:29 ` [PATCH v2 1/1] fsck: ignore missing "refs" directory for linked worktrees shejialuo
@ 2025-06-02 13:59 ` Kristoffer Haugsbakk
2025-06-02 14:11 ` shejialuo
0 siblings, 1 reply; 21+ messages in thread
From: Kristoffer Haugsbakk @ 2025-06-02 13:59 UTC (permalink / raw)
To: shejialuo, git
Cc: Patrick Steinhardt, Karthik Nayak, Eric Sunshine, Junio C Hamano,
Phillip Wood
On Mon, Jun 2, 2025, at 15:29, shejialuo wrote:
> "git refs verify" doesn't work if there are worktrees created on Git
> v2.43.0 or older versions. These versions don't automatically create the
> "refs" directory, causing the error:
>
> error: cannot open directory .git/worktrees/<worktree name>/refs:
> No such file or directory
Good.
> Since 8f4c00de95 (builtin/worktree: create refdb via ref backend,
> 2024-01-08), we automatically create the "refs" directory for new
> worktrees. And in 7c78d819e6 (ref: support multiple worktrees check for
> refs, 2024-11-20), we assume that all linked worktrees have this
> directory and would wrongly report an error to the user, thus
> introducing compatibility issue.
Okay, you don’t mention c1cf918d3ad (builtin/fsck: add `git refs verify`
child process, 2025-02-28) in the commit message because of your reply:
> > 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.
https://lore.kernel.org/git/a2a50127-6ab9-4d8a-abcc-b1a741df293e@app.fastmail.com/T/#m01231abb77735b0f480743e0d2adecc172e1f170
Which I just mention here for completeness and my own memory. ;)
All good.
> 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>
> [snip]
> +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 &&
> + git worktree add --detach ./worktree &&
> +
> + cd worktree &&
Now in this version you change-directory into `worktree`. You would
need a new subshell for that (test style). But I don’t see the need to
chdir in the first place?
> + worktree_refdir="$(git rev-parse --git-dir)/refs" &&
> + # Simulate old directory layout
> + rmdir "$worktree_refdir" &&
> + 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
The test passes for me. Also when applying only the test (and not the
fix) the test fails as expected. Good.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/1] fsck: ignore missing "refs" directory for linked worktrees
2025-06-02 13:59 ` Kristoffer Haugsbakk
@ 2025-06-02 14:11 ` shejialuo
0 siblings, 0 replies; 21+ messages in thread
From: shejialuo @ 2025-06-02 14:11 UTC (permalink / raw)
To: Kristoffer Haugsbakk
Cc: git, Patrick Steinhardt, Karthik Nayak, Eric Sunshine,
Junio C Hamano, Phillip Wood
On Mon, Jun 02, 2025 at 03:59:01PM +0200, Kristoffer Haugsbakk wrote:
> > +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 &&
> > + git worktree add --detach ./worktree &&
> > +
> > + cd worktree &&
>
> Now in this version you change-directory into `worktree`. You would
> need a new subshell for that (test style). But I don’t see the need to
> chdir in the first place?
>
We should change directory into the `worktree`. This is because now in
the test, we use "git rev-parse --git-dir" to get the git directory. We
need to find `gitdir` of the linked worktree but not the `gitdir` of the
main worktree. If we do not cd into the directory, we would get the
`gitdir` of the main worktree.
And I agree with you that we need to spawn a subshell. Let me update the
code. Thanks for reminding me, I forgot about that.
> > + worktree_refdir="$(git rev-parse --git-dir)/refs" &&
> > + # Simulate old directory layout
> > + rmdir "$worktree_refdir" &&
> > + 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
>
> The test passes for me. Also when applying only the test (and not the
> fix) the test fails as expected. Good.
Thanks for the feedback.
Jialuo
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 0/1] [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs
2025-06-02 13:26 ` [PATCH v2 0/1] [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs shejialuo
2025-06-02 13:29 ` [PATCH v2 1/1] fsck: ignore missing "refs" directory for linked worktrees shejialuo
@ 2025-06-02 14:40 ` shejialuo
2025-06-02 14:41 ` [PATCH v3 1/1] fsck: ignore missing "refs" directory for linked worktrees shejialuo
2025-06-02 15:01 ` [PATCH v3 0/1] [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs Kristoffer Haugsbakk
1 sibling, 2 replies; 21+ messages in thread
From: shejialuo @ 2025-06-02 14:40 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Karthik Nayak, Eric Sunshine,
Kristoffer Haugsbakk, Junio C Hamano, Phillip Wood
Hi All:
Changes in v2:
1. Update the commit message to incorporate the commit message that
introduces the BUG to better explain.
2. Update the shell script to avoid hardcode gitdir path.
3. Change "rm -rf" to be "rmdir", which would be safer because we should
delete an empty directory, which has a better semantics.
---
Changes in v3:
1. Use subshell for test style.
Thanks,
Jialuo
shejialuo (1):
fsck: ignore missing "refs" directory for linked worktrees
refs/files-backend.c | 3 +++
t/t0602-reffiles-fsck.sh | 19 +++++++++++++++++++
2 files changed, 22 insertions(+)
Range-diff against v2:
1: d949a8a646 ! 1: e4c32971da fsck: ignore missing "refs" directory for linked worktrees
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref name check should be adapted
+ test_commit initial &&
+ git worktree add --detach ./worktree &&
+
-+ cd worktree &&
-+ worktree_refdir="$(git rev-parse --git-dir)/refs" &&
-+ # Simulate old directory layout
-+ rmdir "$worktree_refdir" &&
-+ git refs verify 2>err &&
-+ test_must_be_empty err
++ (
++ cd worktree &&
++ worktree_refdir="$(git rev-parse --git-dir)/refs" &&
++ # Simulate old directory layout
++ rmdir "$worktree_refdir" &&
++ git refs verify 2>err &&
++ test_must_be_empty err
++ )
+ )
+'
+
--
2.49.0
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH v3 1/1] fsck: ignore missing "refs" directory for linked worktrees
2025-06-02 14:40 ` [PATCH v3 0/1] [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs shejialuo
@ 2025-06-02 14:41 ` shejialuo
2025-06-02 15:01 ` [PATCH v3 0/1] [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs Kristoffer Haugsbakk
1 sibling, 0 replies; 21+ messages in thread
From: shejialuo @ 2025-06-02 14:41 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Karthik Nayak, Eric Sunshine,
Kristoffer Haugsbakk, Junio C Hamano, Phillip Wood
"git refs verify" doesn't work if there are worktrees created on Git
v2.43.0 or older versions. These versions 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. And in 7c78d819e6 (ref: support multiple worktrees check for
refs, 2024-11-20), we assume that all linked worktrees have this
directory and would wrongly report an error to the user, thus
introducing compatibility issue.
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 | 19 +++++++++++++++++++
2 files changed, 22 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..0ef483659d 100755
--- a/t/t0602-reffiles-fsck.sh
+++ b/t/t0602-reffiles-fsck.sh
@@ -110,6 +110,25 @@ 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 &&
+ git worktree add --detach ./worktree &&
+
+ (
+ cd worktree &&
+ worktree_refdir="$(git rev-parse --git-dir)/refs" &&
+ # Simulate old directory layout
+ rmdir "$worktree_refdir" &&
+ 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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 0/1] [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs
2025-06-02 14:40 ` [PATCH v3 0/1] [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs shejialuo
2025-06-02 14:41 ` [PATCH v3 1/1] fsck: ignore missing "refs" directory for linked worktrees shejialuo
@ 2025-06-02 15:01 ` Kristoffer Haugsbakk
1 sibling, 0 replies; 21+ messages in thread
From: Kristoffer Haugsbakk @ 2025-06-02 15:01 UTC (permalink / raw)
To: shejialuo, git
Cc: Patrick Steinhardt, Karthik Nayak, Eric Sunshine, Junio C Hamano,
Phillip Wood
On Mon, Jun 2, 2025, at 16:40, shejialuo wrote:
> [snip]
>
> Range-diff against v2:
> 1: d949a8a646 ! 1: e4c32971da fsck: ignore missing "refs" directory
> for linked worktrees
> @@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref name check
> should be adapted
> + test_commit initial &&
> + git worktree add --detach ./worktree &&
> +
> -+ cd worktree &&
> -+ worktree_refdir="$(git rev-parse --git-dir)/refs" &&
> -+ # Simulate old directory layout
> -+ rmdir "$worktree_refdir" &&
> -+ git refs verify 2>err &&
> -+ test_must_be_empty err
> ++ (
> ++ cd worktree &&
> ++ worktree_refdir="$(git rev-parse --git-dir)/refs" &&
> ++ # Simulate old directory layout
> ++ rmdir "$worktree_refdir" &&
> ++ git refs verify 2>err &&
> ++ test_must_be_empty err
> ++ )
> + )
> +'
> +
> --
> 2.49.0
This version works as well. Thanks!
$ git range-diff --no-color --no-notes --inter-hunk-context=1 --ignore-all-space <range>
1: ddbf0f8cf4c ! 1: ce802ef6698 fsck: ignore missing "refs" directory for linked worktrees
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref name check should be adapted
+ test_commit initial &&
+ git worktree add --detach ./worktree &&
+
++ (
+ cd worktree &&
+ worktree_refdir="$(git rev-parse --git-dir)/refs" &&
+ # Simulate old directory layout
+ rmdir "$worktree_refdir" &&
+ git refs verify 2>err &&
+ test_must_be_empty err
+ )
++ )
+'
+
test_expect_success 'ref name check should work for multiple worktrees' '
^ permalink raw reply [flat|nested] 21+ messages in thread