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

> 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?
> 

> > 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'.
> 

> > 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?
> 

> > +	}
> > +
> > +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?
>