On Fri, Feb 14, 2025 at 01:50:26AM -0800, Karthik Nayak wrote:
> shejialuo <shejialuo@xxxxxxxxx> 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@xxxxxx>
> > Mentored-by: Karthik Nayak <karthik.188@xxxxxxxxx>
> > Signed-off-by: shejialuo <shejialuo@xxxxxxxxx>
> > ---
> > 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
