On Thu, Jan 30, 2025 at 10:23:15AM -0800, Junio C Hamano wrote:

shejialuo [off-list ref] writes:

quoted

It might seems that the method one is much easier than method two.
However, method one has a significant drawback. When we have checked the
file mode using "lstat", we will need to read the file content, there is
a possibility that when finishing reading the file content to the
memory, the file could be changed into a symlink and we cannot notice.

To me, the above sounds like saying:

    The user can run 'git refs verify' and it may declare that refs
    are all good, and then somebody else can come in and turn the
    packed-refs file into a bad one, but the user will not notice
    the mischeif until the check is run the next time.

Yes, it is.

It is just the time that somebody else comes in becomes a bit
earlier than the time the 'git refs verify' command finishes, and
there is no fundamental difference.

quoted

With method two, we could get the "fd" firstly. Even if the file is
changed into a symlink, we could still operate the "fd" in the memory
which is consistent across the checking which avoids race condition.

The end result is the same with the lstat(2) approach, isn't it,
though?.  'git refs verify' may say "I opened the file without
following symlink and checked the contents, which turned out to be
perfectly fine".  But because that somebody else came in just after
the command did nofollow-open and swapped the packed-refs file, the
repository has a packed-refs file that is not a regular file after
the command returns success.  So I am not sure if I am following
your argument to favor the latter over the former.  What am I
missing?

Let me give you some background. In the version 1, I used the following
way:

lstat(...)
if (!IS_REG(...))
    report_error(...);
strbuf_read(...)

Patrick has told me that there is a possibility that between the `IS_REG`
and `strbuf_read`, the "packed-refs" could be converted into a symlink.
So, my idea is that we could use `open_nofollow`, when we have got the
file descriptor, no matter what happens to `packed-refs` file (deleted or
changed into a symlink), we could operate the file descriptor and read
its content.

However, on a platform with O_NOFOLLOW, this situation will also happen.
So, I think we may just use "open_nofollow" now and don't talk about the
method one at all to avoid confusing readers.

As long as both approaches are equally portable, I do not think it
matters which one we pick from correctness point of view, and we can
pick the one that is easier to use to implement the feature.

On a platform without O_NOFOLLOW, open_nofollow() falls back to the
lstat and open, so your "open_nofollow() is better than lstat() and
open()" argument does not portably work, though.

Yes, actually in my first implementation, I didn't notice this. But the
CI told me that and I finally chose "open_nofollow".

quoted

Reuse "FSCK_MSG_BAD_REF_FILETYPE" fsck message id to report the error to
the user if "packed-refs" is not a regular file.

Good.  Say "regular file" on the commit title, too, and it would be
perfect.

Let me improve this in the next version.

quoted

diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
index cf7a202d0d..42c8d4ca1e 100755
--- a/t/t0602-reffiles-fsck.sh
+++ b/t/t0602-reffiles-fsck.sh

@@ -617,4 +617,26 @@ test_expect_success 'ref content checks should work with worktrees' '
 	)
 '
 
+test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit default &&
+		git branch branch-1 &&
+		git branch branch-2 &&
+		git branch branch-3 &&
+		git pack-refs --all &&
+
+		mv .git/packed-refs .git/packed-refs-back &&
+		ln -sf packed-refs-bak .git/packed-refs &&
+		test_must_fail git refs verify 2>err &&
+		cat >expect <<-EOF &&
+		error: packed-refs: badRefFiletype: not a regular file
+		EOF
+		rm .git/packed-refs &&
+		test_cmp expect err
+	)
+'
+
 test_done

OK.  I notice that the previous step did not have any new test
associated with it.  Perhaps we can corrupt "HEAD" *and* replace
packed-refs file with a symbolic link (or do some other damage
to the refs) and make sure both breakages are reported?

As I have said in the previous comment, we cannot detect the error if
"HEAD" itself is corrupted. However, we will check the referent in the
later. So, we don't need to do this.

It does not have to be done in this step, and certainly not as a
part of this single test this step adds, but we'd want it tested
somewhere.

If we need to check the referent of the "HEAD" in the "packed-refs". We
could do this in the later test. I could cover this in [PATCH 6/8].

Thanks,
Jialuo