Re: [PATCH 03/10] packed-backend: check whether the "packed-refs" is regular

[shejialuo <shejialuo@gmail.com>]

On Tue, Jan 07, 2025 at 08:33:56AM -0800, Karthik Nayak wrote:
> shejialuo <shejialuo@gmail.com> 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