| Message ID | Z78cAU69IUSDgpuD@ArchLinux (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add more ref consistency checks | expand |
shejialuo <shejialuo@gmail.com> writes: > +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; > + int fd; > > if (!is_main_worktree(wt)) > return 0; I do not think it is worth a reroll only to improve this one, but for future reference, initializing "fd = -1" and jumping to cleanup here instead of "return 0" would future-proof the code better. This is especially so, given that in a few patches later, we would add a strbuf that is initialized before this "we do not do anything outside the primary worktree" short-cut, and many "goto cleanup"s we see in this patch below would jump to cleanup to strbuf_release() on that initialized but unused strbuf. Jumping there with negative fd to cleanup that already avoids close(fd) for negative fd would be like jumping there with initialized but unused strbuf. Having a single exit point ("cleanup:" label) would help future evolution of the code, by making it easier to add more resource-acquriing code to this function in the future. > - 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 but a symlink"); > + goto cleanup; > + } > + > + ret = error_errno(_("unable to open '%s'"), refs->path); > + goto cleanup; > + } else if (fstat(fd, &st) < 0) { > + ret = error_errno(_("unable to stat '%s'"), refs->path); > + goto cleanup; > + } else 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: > + if (fd >= 0) > + close(fd); > + return ret; > }
On Wed, Feb 26, 2025 at 10:36:29AM -0800, Junio C Hamano wrote: > shejialuo <shejialuo@gmail.com> writes: > > > +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; > > + int fd; > > > > if (!is_main_worktree(wt)) > > return 0; > > I do not think it is worth a reroll only to improve this one, but > for future reference, initializing "fd = -1" and jumping to cleanup > here instead of "return 0" would future-proof the code better. This > is especially so, given that in a few patches later, we would add a > strbuf that is initialized before this "we do not do anything > outside the primary worktree" short-cut, and many "goto cleanup"s we > see in this patch below would jump to cleanup to strbuf_release() on > that initialized but unused strbuf. Jumping there with negative fd > to cleanup that already avoids close(fd) for negative fd would be > like jumping there with initialized but unused strbuf. Having a > single exit point ("cleanup:" label) would help future evolution of > the code, by making it easier to add more resource-acquriing code to > this function in the future. > You are right. Actually, I just want to avoid assigning the `fd` to -1. However, I didn't realize that I would initialize the strbuf later. After waking up, I have suddenly realized this problem. If other reviewers don't have any comments for this new version, I will send out a reroll. We have already iterated many times, if we could make it better, why not? Thanks, Jialuo
diff --git a/refs/packed-backend.c b/refs/packed-backend.c index a7b6f74b6e..f69a0598c7 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,58 @@ 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; + int fd; if (!is_main_worktree(wt)) return 0; - 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 but a symlink"); + goto cleanup; + } + + ret = error_errno(_("unable to open '%s'"), refs->path); + goto cleanup; + } else if (fstat(fd, &st) < 0) { + ret = error_errno(_("unable to stat '%s'"), refs->path); + goto cleanup; + } else 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: + if (fd >= 0) + close(fd); + 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..68b7d4999e 100755 --- a/t/t0602-reffiles-fsck.sh +++ b/t/t0602-reffiles-fsck.sh @@ -617,4 +617,34 @@ 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-back .git/packed-refs && + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && + error: packed-refs: badRefFiletype: not a regular file but a symlink + EOF + rm .git/packed-refs && + test_cmp expect err && + + mkdir .git/packed-refs && + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && + error: packed-refs: badRefFiletype: not a regular file + EOF + rm -r .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". Let's verify that the "packed-refs" has the expected filetype, confirming it is created by "git pack-refs" command. 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. And then we use "fstat" to check whether the "packed-refs" file is a regular file. 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 | 50 +++++++++++++++++++++++++++++++++++++--- t/t0602-reffiles-fsck.sh | 30 ++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 3 deletions(-)