| Message ID | Z3qN6C2IpQTdVn_S@ArchLinux (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add more ref consistency checks | expand |
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 > filetype of the "packed-refs". The user should always use "git > packed-refs" command to create the raw regular "packed-refs" file, so we > need to explicitly check this in "git refs verify". > > Use "lstat" to check the file mode. If we cannot check the file status, > this is OK because there is a chance that there is no "packed-refs" in > the repo. > > 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 | 33 +++++++++++++++++++++++++++++---- > t/t0602-reffiles-fsck.sh | 20 ++++++++++++++++++++ > 2 files changed, 49 insertions(+), 4 deletions(-) > > diff --git a/refs/packed-backend.c b/refs/packed-backend.c > index 3406f1e71d..d9eb2f8b71 100644 > --- a/refs/packed-backend.c > +++ b/refs/packed-backend.c > @@ -4,6 +4,7 @@ > #include "../config.h" > #include "../dir.h" > #include "../gettext.h" > +#include "../fsck.h" > #include "../hash.h" > #include "../hex.h" > #include "../refs.h" > @@ -1747,15 +1748,39 @@ 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"); > + 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`. > + if (o->verbose) > + fprintf_ln(stderr, "Checking packed-refs file %s", refs->path); > + > + if (!S_ISREG(st.st_mode)) { > + 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; > + } > + > +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 75f234a94a..307f94a3ca 100755 > --- 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? > + 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? > + test_must_fail git refs verify 2>err && > + cat >expect <<-EOF && > + error: packed-refs: badRefFiletype: not a regular file > + EOF > + rm .git/packed-refs && > + test_cmp expect err > +' > + > test_done > -- > 2.47.1
diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 3406f1e71d..d9eb2f8b71 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -4,6 +4,7 @@ #include "../config.h" #include "../dir.h" #include "../gettext.h" +#include "../fsck.h" #include "../hash.h" #include "../hex.h" #include "../refs.h" @@ -1747,15 +1748,39 @@ 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"); + 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; + + if (o->verbose) + fprintf_ln(stderr, "Checking packed-refs file %s", refs->path); + + if (!S_ISREG(st.st_mode)) { + 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; + } + +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 75f234a94a..307f94a3ca 100755 --- 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 && + 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 && + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && + error: packed-refs: badRefFiletype: not a regular file + EOF + rm .git/packed-refs && + test_cmp expect err +' + test_done
Although "git-fsck(1)" and "packed-backend.c" will check some consistency and correctness of "packed-refs" file, they never check the filetype of the "packed-refs". The user should always use "git packed-refs" command to create the raw regular "packed-refs" file, so we need to explicitly check this in "git refs verify". Use "lstat" to check the file mode. If we cannot check the file status, this is OK because there is a chance that there is no "packed-refs" in the repo. 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 | 33 +++++++++++++++++++++++++++++---- t/t0602-reffiles-fsck.sh | 20 ++++++++++++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-)