On Tue, Jan 07, 2025 at 08:33:56AM -0800, Karthik Nayak wrote:
> shejialuo <shejialuo@xxxxxxxxx> writes:
[snip]
> > -static int packed_fsck(struct ref_store *ref_store UNUSED,
> > - struct fsck_options *o UNUSED,
> > +static int packed_fsck(struct ref_store *ref_store,
> > + struct fsck_options *o,
> > struct worktree *wt)
> > {
> > + struct packed_ref_store *refs = packed_downcast(ref_store,
> > + REF_STORE_READ, "fsck");
> > + struct stat st;
> > + int ret = 0;
> >
> > if (!is_main_worktree(wt))
> > - return 0;
> > + goto cleanup;
> >
> > - return 0;
> > + /*
> > + * If the packed-refs file doesn't exist, there's nothing to
> > + * check.
> > + */
> > + if (lstat(refs->path, &st) < 0)
> > + goto cleanup;
>
> Since `lstat` return '-1' for all errors, we should check that the
> `errno == ENOENT`.
>
I agree here, if the reason is not "errno == ENOENT", we should report
an error to the user.
[snip]
> > --- a/t/t0602-reffiles-fsck.sh
> > +++ b/t/t0602-reffiles-fsck.sh
> > @@ -626,4 +626,24 @@ test_expect_success 'ref content checks should work with worktrees' '
> > test_cmp expect err
> > '
> >
> > +test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '
> > + test_when_finished "rm -rf repo" &&
> > + git init repo &&
> > + cd repo &&
>
> This should be in a subshell, so that at the end we can actually remove
> the repo. This seems to be applicable to most of the other tests in this
> file too. Perhaps, we should clean it up as a precursor commit to this
> series?
I have searched the usage of "test_when_finished", and I don't know why
we need to use subshell. Could you please explain this further here.
>
> > + 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 &&
>
> This should be `ln -sf .git/packed-refs-back .git/packed-refs` no?
>
No. This should not be `ln -sf .git/packed-refs-back .git/packed-refs`.
This is because it is a relative symlink. And the file
".git/packed-refs-back" and ".git/packed-refs" are in the same
directory. So, from the perspective of ".git/packed-refs", it should be
the "packed-refs-back".
Thanks,
Jialuo
