git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs
@ 2025-05-30 19:00 kristofferhaugsbakk
  2025-05-30 22:23 ` Eric Sunshine
  2025-05-31  3:39 ` [PATCH] fsck: ignore missing "refs" directory for linked worktrees shejialuo
  0 siblings, 2 replies; 21+ messages in thread
From: kristofferhaugsbakk @ 2025-05-30 19:00 UTC (permalink / raw)
  To: git; +Cc: Kristoffer Haugsbakk, Patrick Steinhardt, Karthik Nayak,
	shejialuo

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

(regular git-bugreport(1) follows after this, then a demo patch)

git-refs-verify(1) checks worktree refs since v2.47.0-111-g7c78d819e6a
(ref: support multiple worktrees check for refs, 2024-11-20).  This
causes the command to always exit with code `255` and stderr output
lines for each worktree created on v2.43.0 or older that does not have
worktree refs:

    error: cannot open directory .git/worktrees/<worktree name>/refs: No such file or directory

This is apparently caused by worktrees created on Git v2.43.0 or older.
Apparently these worktrees don’t have this directory unless there exist
worktree refs:

    .git/worktrees/<worktree name>/refs

Again: any such worktrees work fine if you for example have bisect refs.
But the command will always fail if you have one or more v2.43.0 or
older worktrees with no worktree refs.

git-fsck(1) also now prints the same warnings because of the default
`--reference`.  But the operation of the command seems unaffected.

So to reproduce (also see patch at the end)

1. Make a worktree on v2.43.0 or just make a worktree and delete the
   `refs/` directory for the worktree
2. Run `git refs verify`
   • On your regular git(1): not on v2.43.0
3. Expected: succeeds without output
4. Actual: exit code `255`, `cannot open directory` on stderr

Or reproduce with this script (replace with clone with worktree if
you prefer):

    git config set --global safe.directory /tmp &&
    cd /tmp &&
    dir=$(mktemp -d)
    cd $dir
    git clone https://github.com/git/git git-older &&
    cd git-older &&
    git checkout v2.43.0 &&
    make &&
    # use Git v2.43.0
    ./git worktree add --detach worktree1234 &&
    # will fail
    git refs verify
    # Cleanup
    git config unset --global safe.directory

§ Testing on `seen` and `next`

• seen: bfa90786bc5 (Merge branch 'jk/diff-no-index-with-pathspec' into
  seen, 2025-05-29)
• next: d4ff7b7c865 (Sync with 'master', 2025-05-29)

§ Regular report

> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)

Using a repository with worktrees that were apparently made on v2.43.0
or older, based on testing.  Some of them have no worktree refs which is
what triggers this behavior.

> What did you expect to happen? (Expected behavior)

`git refs verify` with exit code `0` and no output.

> What happened instead? (Actual behavior)

The same command exits with exit code `255` and output like

    error: cannot open directory .git/worktrees/<worktree name>/refs: No such file or directory

What's different between what you expected and what actually happened?

See above.

> Anything else you want to add:
>
> Please review the rest of the bug report below.
> You can delete any lines you don't wish to share.

[System Info]
git version:
git version 2.50.0.rc0
cpu: x86_64
built from commit: b32feae0f1b21faaf8e191e8d3314a32470a536b
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
libcurl: 7.81.0
OpenSSL: OpenSSL 3.0.2 15 Mar 2022
zlib: 1.2.11
SHA-1: SHA1_DC
SHA-256: SHA256_BLK
uname: Linux 6.8.0-59-generic #61~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Tue Apr 15 17:03:15 UTC 2 x86_64
compiler info: gnuc: 11.4
libc info: glibc: 2.35
$SHELL (typically, interactive shell): /bin/bash

[Enabled Hooks]
post-rewrite
sendemail-validate

-- 8< --
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Subject: [PATCH] t0602: demo v2.43.0 worktree problem

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 t/t0602-reffiles-fsck.sh | 43 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
index f671ac4d3ab..90b68f6561e 100755
--- a/t/t0602-reffiles-fsck.sh
+++ b/t/t0602-reffiles-fsck.sh
@@ -886,4 +886,47 @@ test_expect_success '--[no-]references option should apply to fsck' '
 	)
 '
 
+# These worktrees will not have a refs/ directory unless there
+# actually exist worktree refs
+test_expect_failure 'works with worktrees from v2.43.0 or older without worktree refs' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit initial &&
+		git checkout -b default-branch &&
+		git worktree add --detach ./worktree &&
+		# Simulate old directory layout
+		rmdir .git/worktrees/worktree/refs &&
+		git refs verify 2>err &&
+		test_must_be_empty err
+	)
+'
+
+test_expect_success 'works with worktrees from v2.43.0 or older with worktree refs' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit initial &&
+		test_commit second &&
+		git checkout -b default-branch &&
+		git worktree add --detach ./worktree &&
+		(
+			cd worktree &&
+			git bisect start &&
+			git bisect bad HEAD &&
+			git bisect good initial &&
+			# Simulate old directory layout: delete if empty
+			# But there should exist a refs/bisect/ directory now
+			if [ ! -e ../.git/worktrees/worktree/refs/bisect ]
+			then
+				rmdir ../.git/worktrees/worktree/refs
+			fi &&
+			git refs verify 2>err &&
+			test_must_be_empty err
+		)
+	)
+'
+
 test_done
-- 
Don’t cry because the bug is fixed. Smile because it happened.

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs
  2025-05-30 19:00 [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs kristofferhaugsbakk
@ 2025-05-30 22:23 ` Eric Sunshine
  2025-05-31  1:03   ` shejialuo
  2025-05-31  9:52   ` Kristoffer Haugsbakk
  2025-05-31  3:39 ` [PATCH] fsck: ignore missing "refs" directory for linked worktrees shejialuo
  1 sibling, 2 replies; 21+ messages in thread
From: Eric Sunshine @ 2025-05-30 22:23 UTC (permalink / raw)
  To: kristofferhaugsbakk
  Cc: git, Kristoffer Haugsbakk, Patrick Steinhardt, Karthik Nayak,
	shejialuo

On Fri, May 30, 2025 at 3:00 PM <kristofferhaugsbakk@fastmail.com> wrote:
> git-refs-verify(1) checks worktree refs since v2.47.0-111-g7c78d819e6a
> (ref: support multiple worktrees check for refs, 2024-11-20).  This
> causes the command to always exit with code `255` and stderr output
> lines for each worktree created on v2.43.0 or older that does not have
> worktree refs:
>
>     error: cannot open directory .git/worktrees/<worktree name>/refs: No such file or directory

Interesting. I didn't follow the topic which introduced 7c78d819e6
(ref: support multiple worktrees check for refs, 2024-11-20), but I
can confirm that this is a problem.

> This is apparently caused by worktrees created on Git v2.43.0 or older.
> Apparently these worktrees don’t have this directory unless there exist
> worktree refs:
>
>     .git/worktrees/<worktree name>/refs

Indeed, the "refs" subdirectory was not present by default in older
Git versions. Were you able to track down which commit is responsible
for that directory getting created automatically when the worktree
gets created?

> -- 8< --
> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
> Subject: [PATCH] t0602: demo v2.43.0 worktree problem
>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>

Even though this is a bug report and the patch you included doesn't
provide a fix, you did craft a couple tests, presumably with the
intention that they should be used by whomever fixes the problem. As
such, I'll give them a bit of a critique...

>  t/t0602-reffiles-fsck.sh | 43 ++++++++++++++++++++++++++++++++++++++++
> @@ -886,4 +886,47 @@ test_expect_success '--[no-]references option should apply to fsck' '
> +# These worktrees will not have a refs/ directory unless there
> +# actually exist worktree refs
> +test_expect_failure 'works with worktrees from v2.43.0 or older without worktree refs' '
> +       test_when_finished "rm -rf repo" &&
> +       git init repo &&
> +       (
> +               cd repo &&
> +               test_commit initial &&
> +               git checkout -b default-branch &&

This `git checkout -b` seems unnecessary. The expected test failure
occurs without this step. As such, it's probably just noise which will
confuse readers rather than help them. I suggest omitting it.

> +               git worktree add --detach ./worktree &&
> +               # Simulate old directory layout
> +               rmdir .git/worktrees/worktree/refs &&
> +               git refs verify 2>err &&
> +               test_must_be_empty err
> +       )
> +'
> +
> +test_expect_success 'works with worktrees from v2.43.0 or older with worktree refs' '
> +       test_when_finished "rm -rf repo" &&
> +       git init repo &&
> +       (
> +               cd repo &&
> +               test_commit initial &&
> +               test_commit second &&
> +               git checkout -b default-branch &&

Unnecessary branch creation?

> +               git worktree add --detach ./worktree &&
> +               (
> +                       cd worktree &&
> +                       git bisect start &&
> +                       git bisect bad HEAD &&
> +                       git bisect good initial &&
> +                       # Simulate old directory layout: delete if empty
> +                       # But there should exist a refs/bisect/ directory now
> +                       if [ ! -e ../.git/worktrees/worktree/refs/bisect ]
> +                       then
> +                               rmdir ../.git/worktrees/worktree/refs
> +                       fi &&

A few comments...

First, I'm having trouble understanding what the intention is here;
the comment does not illuminate. Even with v2.43.0,
.git/worktrees/worktree/refs/bisect exists after "git bisect bad
HEAD", so it seems that the `if` condition can never fail, and the
`rmdir` is dead code.

Second, this project uses `test` rather than `[` in shell scripts.

Finally, I see that other parts of the script are already (perhaps)
too intimate with the structure of the .git/ directory, and you may
have simply been following suit, but these days we often want to
abstract away such familiarity. Hence, rather than hardcoding the path
"../.git/worktrees/<worktree>/refs", you could do this:

    refs="$(git rev-parse --git-dir)/refs" &&
    if test ! -e "$refs/bisect"
    then
        rmdir "$refs"
    fi &&

> +                       git refs verify 2>err &&
> +                       test_must_be_empty err
> +               )
> +       )
> +'

Overall, although the first new test makes sense, it is not at all
clear to me what the second test is checking or what its purpose is.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs
  2025-05-30 22:23 ` Eric Sunshine
@ 2025-05-31  1:03   ` shejialuo
  2025-05-31  9:52   ` Kristoffer Haugsbakk
  1 sibling, 0 replies; 21+ messages in thread
From: shejialuo @ 2025-05-31  1:03 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: kristofferhaugsbakk, git, Kristoffer Haugsbakk,
	Patrick Steinhardt, Karthik Nayak

On Fri, May 30, 2025 at 06:23:25PM -0400, Eric Sunshine wrote:
> On Fri, May 30, 2025 at 3:00 PM <kristofferhaugsbakk@fastmail.com> wrote:
> > git-refs-verify(1) checks worktree refs since v2.47.0-111-g7c78d819e6a
> > (ref: support multiple worktrees check for refs, 2024-11-20).  This
> > causes the command to always exit with code `255` and stderr output
> > lines for each worktree created on v2.43.0 or older that does not have
> > worktree refs:
> >
> >     error: cannot open directory .git/worktrees/<worktree name>/refs: No such file or directory
> 
> Interesting. I didn't follow the topic which introduced 7c78d819e6
> (ref: support multiple worktrees check for refs, 2024-11-20), but I
> can confirm that this is a problem.
> 

Yes, I didn't realize about this. Thank Kristoffer for reporting this
issue and you for confirming this. I would write a patch today to fix
this issue. And hope that this fix would land in this release.

> > This is apparently caused by worktrees created on Git v2.43.0 or older.
> > Apparently these worktrees don’t have this directory unless there exist
> > worktree refs:
> >
> >     .git/worktrees/<worktree name>/refs
> 
> Indeed, the "refs" subdirectory was not present by default in older
> Git versions. Were you able to track down which commit is responsible
> for that directory getting created automatically when the worktree
> gets created?
> 

I find out that; in 8f4c00de95 (builtin/worktree: create refdb via ref
backend, 2024-01-08)

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH] fsck: ignore missing "refs" directory for linked worktrees
  2025-05-30 19:00 [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs kristofferhaugsbakk
  2025-05-30 22:23 ` Eric Sunshine
@ 2025-05-31  3:39 ` shejialuo
  2025-05-31 12:17   ` Kristoffer Haugsbakk
                     ` (3 more replies)
  1 sibling, 4 replies; 21+ messages in thread
From: shejialuo @ 2025-05-31  3:39 UTC (permalink / raw)
  To: git; +Cc: Kristoffer Haugsbakk, Patrick Steinhardt, Karthik Nayak,
	Eric Sunshine

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:

    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.

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 &&
+
+		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 &&
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs
  2025-05-30 22:23 ` Eric Sunshine
  2025-05-31  1:03   ` shejialuo
@ 2025-05-31  9:52   ` Kristoffer Haugsbakk
  1 sibling, 0 replies; 21+ messages in thread
From: Kristoffer Haugsbakk @ 2025-05-31  9:52 UTC (permalink / raw)
  To: Eric Sunshine, Kristoffer Haugsbakk
  Cc: git, Patrick Steinhardt, Karthik Nayak, shejialuo

On Sat, May 31, 2025, at 00:23, Eric Sunshine wrote:
>> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>> Subject: [PATCH] t0602: demo v2.43.0 worktree problem
>>
>> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> Even though this is a bug report and the patch you included doesn't
> provide a fix, you did craft a couple tests, presumably with the
> intention that they should be used by whomever fixes the problem. As
> such, I'll give them a bit of a critique...

Yes if s/should/could.[1]  These are reproduction scripts as patches.  So
they can be applied and show the current state (first test is
expect-failed, the second is expect-success) of the code.

My previous reproduction script with the git-clone(1) is inconvenient
but either cloning or using a worktree is necessary in order to truly
reproduce the problem (as opposed to simulating it).

A `-subject-prefix='PATCH THROWAWAY'` would have been in order.

On the other hand I did write the first test (the second is ugly) as if
I was doing a quote-unquote real patch. In that light learning more
about the proper style is useful for me. So thanks for the review!

> Overall, although the first new test makes sense, it is not at all
> clear to me what the second test is checking or what its purpose is.

The idea behind the second test was to show a case where it does work
with old worktrees.  But simulating the old worktree didn’t make sense
since it looks just like a new worktree when there *are* indeed worktree
refs.  So it just ended up being confusing.

† 1: As in troubleshooting and fixing the problem, not the final test in
    the submitted patch.  The test is unlikely to be good enough for
    that.  But the patch is signed off on the small chance that it can
    be used because why not.

^ 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  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  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-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-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] 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 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

* 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

end of thread, other threads:[~2025-06-02 19:49 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-30 19:00 [BUG] refs: verify does not work if there are v2.43.0 or older worktrees w/o wt. refs kristofferhaugsbakk
2025-05-30 22:23 ` Eric Sunshine
2025-05-31  1:03   ` shejialuo
2025-05-31  9:52   ` Kristoffer Haugsbakk
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
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
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
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
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       ` [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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).