Re: [PATCH v4 3/8] packed-backend: check whether the "packed-refs" is regular file

[shejialuo <shejialuo@gmail.com>]

On Fri, Feb 14, 2025 at 01:50:26AM -0800, Karthik Nayak wrote:
> shejialuo <shejialuo@gmail.com> writes:
> 
> > Although "git-fsck(1)" and "packed-backend.c" will check some
> > consistency and correctness of "packed-refs" file, they never check the
> 
> Because you say 'some' here, it made me more curious. Could you state
> exactly what checks are being done here?
> 

Well, I don't think we need to elaborate on this at now for the
following two reasons:

1. We will explain this in the later patches.
2. Here I just want to emphasis that it does not check the filetype.

> > filetype of the "packed-refs". The user should always use "git
> > pack-refs" command to create the raw regular "packed-refs" file, so we
> > need to explicitly check this in "git refs verify".
> >
> 
> Not sure I understand how the start of this last sentence correlates to
> the end of it. Is the intention to say that we want to explicitly check
> the filetype to ensure that the 'packed-refs' file was only created via
> 'git pack-refs'? If so, perhaps:
> 
>     Verify that the 'packed-refs' file has the expected filetype,
>     confirming it was created by 'git pack-refs'.
> 

Thanks for the suggestion, I will improve this in the next version.

> > We could use "open_nofollow" wrapper to open the raw "packed-refs" file.
> > If the returned "fd" value is less than 0, we could check whether the
> > "errno" is "ELOOP" to report an error to the user.
> >
> > Reuse "FSCK_MSG_BAD_REF_FILETYPE" fsck message id to report the error to
> > the user if "packed-refs" is not a regular file.
> >
> > Mentored-by: Patrick Steinhardt <ps@pks.im>
> > Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> > Signed-off-by: shejialuo <shejialuo@gmail.com>
> > ---
> >  refs/packed-backend.c    | 39 +++++++++++++++++++++++++++++++++++----
> >  t/t0602-reffiles-fsck.sh | 22 ++++++++++++++++++++++
> >  2 files changed, 57 insertions(+), 4 deletions(-)
> >
> > diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> > index a7b6f74b6e..6401cecd5f 100644
> > --- a/refs/packed-backend.c
> > +++ b/refs/packed-backend.c
> > @@ -4,6 +4,7 @@
> >  #include "../git-compat-util.h"
> >  #include "../config.h"
> >  #include "../dir.h"
> > +#include "../fsck.h"
> >  #include "../gettext.h"
> >  #include "../hash.h"
> >  #include "../hex.h"
> > @@ -1748,15 +1749,45 @@ static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_s
> >  	return empty_ref_iterator_begin();
> >  }
> >
> > -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");
> > +	int ret = 0;
> > +	int fd;
> >
> >  	if (!is_main_worktree(wt))
> > -		return 0;
> > +		goto cleanup;
> >
> > -	return 0;
> > +	if (o->verbose)
> > +		fprintf_ln(stderr, "Checking packed-refs file %s", refs->path);
> > +
> > +	fd = open_nofollow(refs->path, O_RDONLY);
> > +	if (fd < 0) {
> > +		/*
> > +		 * If the packed-refs file doesn't exist, there's nothing
> > +		 * to check.
> > +		 */
> > +		if (errno == ENOENT)
> > +			goto cleanup;
> > +
> > +		if (errno == ELOOP) {
> > +			struct fsck_ref_report report = { 0 };
> > +			report.path = "packed-refs";
> > +			ret = fsck_report_ref(o, &report,
> > +					      FSCK_MSG_BAD_REF_FILETYPE,
> > +					      "not a regular file");
> > +			goto cleanup;
> > +		}
> > +
> > +		ret = error_errno(_("unable to open %s"), refs->path);
> > +		goto cleanup;
> 
> The paragraph in the commit message:
> 
>     Reuse "FSCK_MSG_BAD_REF_FILETYPE" fsck message id to report the error to
>     the user if "packed-refs" is not a regular file.
> 
> Gave me the indication that any error would be reported via
> 'fsck_report_ref()', but it seems like we are only reporting for
> symbolic links. Why is that being singled out?
> 

IIRC, when Patrick told me in first version that if I first stat the
file type and then use the `strbuf_read_file` to read the content, there
is a corner case that the file could be converted into symlink between
the `stat` and read.

So, I use `open_nofollow` to avoid this situation. (Actually, this could
not be avoided because in Windows, we would first stat the file and
then open the file due to that there is no "O_NOFOLLOW" flag for Windows).

I will find a solution to do this in the next version.

> > +	}
> > +
> > +cleanup:
> > +	return ret;
> >  }
> >
> >  struct ref_storage_be refs_be_packed = {
> > 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 &&
> 
> This still doesn't make sense to me. 'packed-refs-bak' doesn't exist, is
> the intention to symlink '.git/packed-refs' -> something which doesn't
> exist?
> 
> In that case why even make the effort to build a packed-refs file, could
> we simply do 'ln -sf packed-refs-bak .git/packed-refs' in an empty repo?
> 

You are correct. My intention is not this. If the "packed-refs" is a
symlink and points to file which we can successfully parse. Current Git
won't complain. So my motivation here is to imitate this situation.

> If not, then 'packed-refs-bak' is definitely a typo and needs to be made
> 'packed-refs-back' which would go in hand with how we setup the test...
> 

Thanks for noticing this problem. I definitely made a mistake to type the
"packed-refs-back" to "packed-refs-bak".

Jialuo