[v7,0/9] add more ref consistency checks

Message

shejialuo Feb. 26, 2025, 1:48 p.m. UTC

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 <[e-mail]>
         Mentored-by: Karthik Nayak <[e-mail]>
 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