[PATCH v7 0/9] add more ref consistency checks
- From
shejialuo <shejialuo@gmail.com>
- Date
- Feb 26, 2025, 13:48 UTC
- Message-ID
- <Z78bmBSrDR20GY6g@ArchLinux>
- In-Reply-To
- <Z73DTwr9RicKMINe@ArchLinux>
Hi All:
This changes enhances the following things:
1. [PATCH v7 3/9]: use "open_nofollow" with "fstat" to check whether the file is regular. And update the test to improve coverage. 2. [PACTH v7 4/9]: improve the commit message suggested by Patrick.
Thanks, Jialuo
---
This series mainly does the following things:
1. Fix subshell issues
2. Add ref checks for packed-backend.
1. Check whether the filetype of "packed-refs" is correct.
2. Check whether the syntax of "packed-refs" is correct by using the
rules from "packed-backend.c::create_snapshot" and
"packed-backend.c::next_record".
3. Check whether the pointed object exists and whether the
"packed-refs" file is sorted.
3. Call "git refs verify" for "git-fsck(1)".shejialuo (9): t0602: use subshell to ensure working directory unchanged builtin/refs: get worktrees without reading head information packed-backend: check whether the "packed-refs" is regular file packed-backend: check if header starts with "# pack-refs with: " packed-backend: add "packed-refs" header consistency check packed-backend: check whether the refname contains NUL characters packed-backend: add "packed-refs" entry consistency check packed-backend: check whether the "packed-refs" is sorted builtin/fsck: add `git refs verify` child process
Documentation/fsck-msgids.adoc | 14 + Documentation/git-fsck.adoc | 7 +- builtin/fsck.c | 33 +- builtin/refs.c | 2 +- fsck.h | 4 + refs/packed-backend.c | 361 +++++++++- t/t0602-reffiles-fsck.sh | 1209 +++++++++++++++++++------------- worktree.c | 5 + worktree.h | 8 + 9 files changed, 1161 insertions(+), 482 deletions(-)
Range-diff against v6:
1: b3952d80a2 = 1: b3952d80a2 t0602: use subshell to ensure working directory unchanged
2: fa5ce20bb7 = 2: fa5ce20bb7 builtin/refs: get worktrees without reading head information
3: 787645a700 ! 3: 861583f417 packed-backend: check whether the "packed-refs" is regular file
@@ Commit message
the expected filetype, confirming it is created by "git pack-refs"
command.
- Use "lstat" to check the file mode. If we cannot check the file status
- due to there is no such file this is OK because there is a possibility
- that there is no "packed-refs" in the repo.
+ 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.
@@ refs/packed-backend.c: static struct ref_iterator *packed_reflog_iterator_begin(
+ REF_STORE_READ, "fsck");
+ struct stat st;
+ int ret = 0;
++ int fd;
if (!is_main_worktree(wt))
-- return 0;
-+ goto cleanup;
+ return 0;
- return 0;
+ if (o->verbose)
+ fprintf_ln(stderr, "Checking packed-refs file %s", refs->path);
+
-+ if (lstat(refs->path, &st) < 0) {
++ 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;
-+ }
-+
-+ if (!S_ISREG(st.st_mode)) {
++ } else if (!S_ISREG(st.st_mode)) {
+ struct fsck_ref_report report = { 0 };
+ report.path = "packed-refs";
+ ret = fsck_report_ref(o, &report,
@@ refs/packed-backend.c: static struct ref_iterator *packed_reflog_iterator_begin(
+ }
+
+cleanup:
++ if (fd >= 0)
++ close(fd);
+ return ret;
}
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref content checks should work wi
+ 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
++ 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
+ )
+'
4: f097e0f093 ! 4: 5f54cb05c3 packed-backend: check if header starts with "# pack-refs with: "
@@ Metadata
## Commit message ##
packed-backend: check if header starts with "# pack-refs with: "
- We always write a space after "# pack-refs with:". However, when
- creating the packed-ref snapshot, we only check whether the header
- starts with "# pack-refs with:". However, we need to make sure that we
- would not break compatibility by tightening the rule. The following is
- how some third-party libraries handle the header of "packed-ref" file.
+ We always write a space after "# pack-refs with:" but we don't align
+ with this rule in the "create_snapshot" method where we would check
+ whether header starts with "# pack-refs with:". It might seem that we
+ should undoubtedly tighten this rule, however, we don't have any
+ technical documentation about this and there is a possibility that we
+ would break the compatibility for other third-party libraries.
+
+ By investigating influential third-party libraries, we could conclude
+ how these libraries handle the header of "packed-refs" file:
1. libgit2 is fine and always writes the space. It also expects the
whitespace to exist.
@@ Commit message
3. gitoxide expects the space t exist and writes it.
4. go-git doesn't create the header by default.
- So, we are safe to tighten the rule by checking whether the header
- starts with "# pack-refs with: ".
+ As many third-party libraries expect a single space after "# pack-refs
+ with:", if we forget to write the space after the colon,
+ "create_snapshot" won't catch this. And we would break other
+ re-implementations. So, we'd better tighten the rule by checking whether
+ the header starts with "# pack-refs with: ".
Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
5: a589a38b68 ! 5: 7d7dc899ad packed-backend: add "packed-refs" header consistency check
@@ refs/packed-backend.c: static struct ref_iterator *packed_reflog_iterator_begin(
REF_STORE_READ, "fsck");
+ struct strbuf packed_ref_content = STRBUF_INIT;
struct stat st;
-+ int fd;
int ret = 0;
-
- if (!is_main_worktree(wt))
+ int fd;
@@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,
goto cleanup;
}
-+ /*
-+ * There is a chance that "packed-refs" file is removed or converted to
-+ * a symlink after filetype check and before open. So we need to avoid
-+ * this race condition by opening the file.
-+ */
-+ fd = open_nofollow(refs->path, O_RDONLY);
-+ if (fd < 0) {
-+ 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;
-+ }
-+ }
-+
+ if (strbuf_read(&packed_ref_content, fd, 0) < 0) {
-+ ret = error_errno(_("unable to read %s"), refs->path);
++ ret = error_errno(_("unable to read '%s'"), refs->path);
+ goto cleanup;
+ }
+
@@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,
+ packed_ref_content.buf + packed_ref_content.len);
+
cleanup:
+ if (fd >= 0)
+ close(fd);
+ strbuf_release(&packed_ref_content);
return ret;
}
6: 7255c2b597 = 6: 571479d3e7 packed-backend: check whether the refname contains NUL characters
7: 7794a2ebfd = 7: e498a57286 packed-backend: add "packed-refs" entry consistency check
8: 2a9138b14d ! 8: 3638cb118d packed-backend: check whether the "packed-refs" is sorted
@@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,
struct strbuf packed_ref_content = STRBUF_INIT;
+ unsigned int sorted = 0;
struct stat st;
-- int fd;
int ret = 0;
-+ int fd;
-
- if (!is_main_worktree(wt))
- goto cleanup;
+ int fd;
@@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,
goto cleanup;
}
@@ refs/packed-backend.c: static int packed_fsck(struct ref_store *ref_store,
+ packed_ref_content.buf + packed_ref_content.len);
cleanup:
- strbuf_release(&packed_ref_content);
+ if (fd >= 0)
## t/t0602-reffiles-fsck.sh ##
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'packed-refs content should be checked' '
9: ccde32491f = 9: 5d87e76d28 builtin/fsck: add `git refs verify` child process-- 2.48.1