Implement ref content consistency check

Hi All:

We have already set up the infrastructure of the ref consistency. However, we have only add ref name check when establishing the infrastructure in below:

https://lore.kernel.org/git/[email protected]/

Actually, we already have a patch here which has already implemented the ref content consistency check. But during the review process, we have encountered some problems. The intention of this RFC is to make sure what content we should check and also to what extend.

I conclude the following info:

1. For the regular ref which has a trailing garbage, we should warn the user. This is the most simplest situation, we could reply on "parse_loose_ref_content" to do this.
2. For the symref, we could also rely on "parse_loose_ref_content" to get the "pointee", and check the location of the "pointee", check the name of the "pointee" and the file type of the "pointee".
3. FOr the symbolic ref, we could follow the idea of 2.

But Patrick gives a question here:

For symref, I guess we have no spec here. From my experiments, a symref could have a newline or no newline, even multiple newlines. And also symref could have multiple spaces. But the following is a bad symref

ref: refs/heads/main garbage

I think we should fully discuss what we should check here. Thus I will implement the code.

Thanks,
Jialuo

Just to understand clearly, when you're talking about 'symbolic ref' you are referring to symbolic links?

I ask because, as per our documentation in
'Documentation/git-symbolic-ref.txt':

In the past, .git/HEAD was a symbolic link pointing at refs/heads/master. When we wanted to switch to another branch, we did ln -sf refs/heads/newbranch .git/HEAD, and when we wanted to find out which branch we are on, we did readlink .git/HEAD. But symbolic links are not entirely portable, so they are now deprecated and symbolic refs (as described above) are used by default.

Agreed, in refs/files-backend.c:create_symref_lock, we write symrefs as "ref: %s\n" so it makes sense to validate that there is nothing extra.

I am sorry about this. It's symbolic links here.

Yes, we should do this. I will implement the code and the send the patches to the mailing list.

Thanks

Wait, is it really symbolic link? I don't think so, you actually were talking about symbolic refs correctly. The fact that symbolic refs have been implemented as a symbolic link in the past (and still can be used for that purpose) is rather an implementation detail. But the overall context, and what we actually want to check on disk, is a symbolic ref in its modern incarnation.

And checking the format of both normal and symbolic refs does make sense in my opinion.

Agreed. We have to exclude pseudorefs (FETCH_HEAD, MERGE_HEAD, see gitglossary(7)) from these checks, as those _are_ allowed to contain extra data. But no other reference should carry more data than that. Namely, a regular ref should always be "hex * hash_len + \n", while a symbolic ref should always be "ref: $valid_refname\n".

A ref that does not conform to this is not a properly formatted reference and thus worth being warned about.

Patrick

In #2, yes. I think #3 is about what to do with a random symbolic link inside or near .git/refs/ hierarchy, which may or may not meant as a symref. I agree that we should assume that the user meant them to be used as symrefs, check its validity the same way as a textual symrefs, and complain if they look bogus.

Yup.

Hi All:

This patch aims at adding ref content check for files backend. By the RFC we have discussed, I add three types of checks.

1. Check regular ref content. I enhance the "parse_loose_ref_contents" to validate the content of the regular ref and warn the user about the trailing garbage.
2. Check symbolic ref content. Check the trailing garbage and content.
3. Check symlink ref by reusing the function introduced by #2.

The CI is passed:

https://github.com/shejialuo/git/pull/14

Thanks,
Jialuo

shejialuo (4):
fsck: introduce "FSCK_REF_REPORT_DEFAULT" macro
ref: add regular ref content check for files backend
ref: add symbolic ref content check for files backend
ref: add symlink ref consistency check for files backend

 Documentation/fsck-msgids.txt |  12 +++
 fsck.h                        |  10 ++
 refs.c                        |   2 +-
 refs/files-backend.c          | 188 +++++++++++++++++++++++++++++++++-
 refs/refs-internal.h          |   2 +-
 t/t0602-reffiles-fsck.sh      | 183 +++++++++++++++++++++++++++++++++
 6 files changed, 392 insertions(+), 5 deletions(-)

2.46.0

In "fsck.c::fsck_refs_error_function", we need to tell whether "oid" and "referent" is NULL. So, we need to always initialize these parameters to NULL instead of letting them point to anywhere when creating a new "fsck_ref_report" structure.

In order to conveniently create a new "fsck_ref_report", add a new macro "FSCK_REF_REPORT_DEFAULT".

---
 fsck.h               | 6 ++++++
 refs/files-backend.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

index 500b4c04d2..8894394d16 100644
@@ -152,6 +152,12 @@ struct fsck_ref_report {
 	const char *referent;
 };
 
+#define FSCK_REF_REPORT_DEFAULT { \
+	.path = NULL, \
+	.oid = NULL, \
+	.referent = NULL, \
+}
+
 struct fsck_options {
 	fsck_walk_func walk;
 	fsck_error error_func;

index 8d6ec9458d..725a4f52e3 100644
@@ -3446,7 +3446,7 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
 		goto cleanup;
 
 	if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {
-		struct fsck_ref_report report = { .path = NULL };
+		struct fsck_ref_report report = FSCK_REF_REPORT_DEFAULT;
 
 		strbuf_addf(&sb, "%s/%s", refs_check_dir, iter->relative_path);
 		report.path = sb.buf;

2.46.0

The above is correct, but ...

		struct fsck_ref_report report = { 0 }

Yes, I should use this way. Thanks.

We implicitly reply on "git-fsck(1)" to check the consistency of regular refs. However, when parsing the regular refs for files backend, we allow the ref content to end with no newline or contain some garbages. We should warn the user about above situations.

In order to provide above functionality, enhance the "git-refs verify" command by adding consistency check for regular refs for files backend.

Add the following three fsck messages to represent the above situations:

1. "badRefContent(ERROR)": A ref has a bad content.
2. "refMissingNewline(WARN)": A valid ref does not end with newline.
3. "trailingRefContent(WARN)": A ref has trailing contents.

In order to tell whether the ref has trailing content, add a new parameter "trailing" to "parse_loose_ref_contents". Then introduce a new function "files_fsck_refs_content" to check the regular refs.

---
 Documentation/fsck-msgids.txt |  9 ++++
 fsck.h                        |  3 ++
 refs.c                        |  2 +-
 refs/files-backend.c          | 67 ++++++++++++++++++++++++++-
 refs/refs-internal.h          |  2 +-
 t/t0602-reffiles-fsck.sh      | 87 +++++++++++++++++++++++++++++++++++
 6 files changed, 166 insertions(+), 4 deletions(-)

index 68a2801f15..1688c2f1fe 100644
@@ -19,6 +19,9 @@
 `badParentSha1`::
 	(ERROR) A commit object has a bad parent sha1.
 
+`badRefContent`::
+	(ERROR) A ref has a bad content.
+
 `badRefFiletype`::
 	(ERROR) A ref has a bad file type.
 

@@ -170,6 +173,12 @@
 `nullSha1`::
 	(WARN) Tree contains entries pointing to a null sha1.
 
+`refMissingNewline`::
+	(WARN) A valid ref does not end with newline.
+
+`trailingRefContent`::
+	(WARN) A ref has trailing contents.
+
 `treeNotSorted`::
 	(ERROR) A tree is not properly sorted.
 

index 8894394d16..975d9b9da9 100644
@@ -31,6 +31,7 @@ enum fsck_msg_type {
 	FUNC(BAD_NAME, ERROR) \
 	FUNC(BAD_OBJECT_SHA1, ERROR) \
 	FUNC(BAD_PARENT_SHA1, ERROR) \
+	FUNC(BAD_REF_CONTENT, ERROR) \
 	FUNC(BAD_REF_FILETYPE, ERROR) \
 	FUNC(BAD_REF_NAME, ERROR) \
 	FUNC(BAD_TIMEZONE, ERROR) \

@@ -73,6 +74,8 @@ enum fsck_msg_type {
 	FUNC(HAS_DOTDOT, WARN) \
 	FUNC(HAS_DOTGIT, WARN) \
 	FUNC(NULL_SHA1, WARN) \
+	FUNC(REF_MISSING_NEWLINE, WARN) \
+	FUNC(TRAILING_REF_CONTENT, WARN) \
 	FUNC(ZERO_PADDED_FILEMODE, WARN) \
 	FUNC(NUL_IN_COMMIT, WARN) \
 	FUNC(LARGE_PATHNAME, WARN) \

index 74de3d3009..5e74881945 100644
@@ -1758,7 +1758,7 @@ static int refs_read_special_head(struct ref_store *ref_store,
 	}
 
 	result = parse_loose_ref_contents(ref_store->repo->hash_algo, content.buf,
-					  oid, referent, type, failure_errno);
+					  oid, referent, type, NULL, failure_errno);
 
 done:
 	strbuf_release(&full_path);

index 2313c830d8..73b05f971b 100644
@@ -715,7 +715,7 @@ struct ref_store {
 int parse_loose_ref_contents(const struct git_hash_algo *algop,
 			     const char *buf, struct object_id *oid,
 			     struct strbuf *referent, unsigned int *type,
-			     int *failure_errno);
+			     const char **trailing, int *failure_errno);
 
 /*
  * Fill in the generic part of refs and add it to our collection of

2.46.0

"reply" -> "rely", I think.

Hmph, should we?

If the content is short (e.g., in SHA-1 repository it only has 39 hexdigit) even if that may be sufficient to uniquely name the object, we should warn about it, of course. A file that has 64-hexdigit with a terminating LF at the end may be a valid file to be in $GIT_DIR/refs/ hierarchy in a SHA-256 repository, but such a file in a SHA-1 repository should also be subject to a warning, as it could be a sign that somebody screwed up object format conversion.

But a file that has only 40-hexdigit without a terminating LF at the end? Or a file that has 40-hexdigit followed by a CRLF instead of LF? Or a file that has the identical content as a valid ref on its first line, but has extra stuff on its second and subsequent lines?

What does the name-to-object-name-mapping layer (aka "get_oid" API) do when they see such a file in the $GIT_DIR/refs/ hierarchy? If they are treated as valid ref in the "normal" code path, it needs a strong justification to tighten the rules retroactively, much stronger than "Our current code, and any of our older versions, would have written such a file as a loose ref with our code."

"What are we protecting us from with this tightening?" is the question we should be asking ourselves, when evaluating each of these new rules that fsck used not to care about.

I will fix in the next version.

I am very sorry about this. Actually, I should not use "should". I don't give compelling reasons here why we need to introduce such checks. I just told the reviewer "we should warn". I will try to avoid above mistakes where I didn't give enough motivation.

Let me first talk about what will happen when we use the following command:

  $ git checkout bad-branch

I use "gdb" to find the following call sequence:

"cmd_checkout" -> "checkout_main" -> "parse_branchname_arg" -> ... -> "get_oid_basic" -> "repo_dwim_ref" -> ... -> "parse_loose_ref_contents" -> "parse_oid_hex_algop" -> "get_oid_hex_algop"

I dive into the "object-name.c::get_oid_basic" function. If we pass the actually "oid", it will call the "get_oid_hex_algop" directly. Otherwise, it will execute the following code:

  if (!len && reflog_len)
      refs_found = ...;
  else if (reflog_len)
      refs_found = ...
  else
      refs_found = repo_dwim_ref(r, str, len, oid, &real_ref, !fatal);

if (!refs_found)

      return -1;

As we can see, when there is no corresponding refs found by calling "repo_dwim_ref" function, "get_oid_basic" function will return -1. And here we could have one important conclusion:

The "get_oid_basic" function relies on "repo_dwim_ref" function to parse the ref and get the pointee "oid". So, it uses the interfaces provided by ref backend.

Next, we look at what will "parse_loose_ref_contents" do for regular refs.

  int parse_loose_ref_contents(...)
  {
      ...
      if (parse_oid_hex_algop(buf, oid, *p, algop) ||
         (*p != '\0' && !isspace(*p))) {
            *type |= REF_ISBROKEN;
            *failure_errno = EINVAL;
            return -1;
      }
      return 0;
  }

Let's continue to see what "parse_oid_hex_algop" will do:

  int parse_oid_hex_algop(...)
  {
      int ret = get_oid_hex_algop(hex, oid, algop);
      if (!ret) {
          *end = hex + algop->hexsz;
      }
      return ret;
  }

If the result of "get_oid_hex_algop" is successful. We will set the "end" pointer here. The "get_oid_hex_algop" will eventually call the "get_hash_hex_algop" function

  static int get_hash_hex_algop(...)
  {
      int i;
      for (i = 0; i < algop->rawsz; i++) {
          int val = hex2chr(hex);
          if (val < 0)
              return -1;
          *hash+= = val;
          hex += 2;
      }
      return 0;
  }

This function will convert the hex to char by the raw size of the algorithm. And by the following code, we could conclude the following things:

1. "41053a9084501db79c72b14e8a5a0b67de3f91ae" is correct, because it will be parsed successfully by "get_hash_hex_algop" and "*p == '\0'".
2. "41053a9084501db79c72b14e8a5a0b67de3f91aef" is not correct, it will be parsed successfully by "get_hash_hex_algop" but "*p != '\0'" and "isspace(*p)" is false. So the check in "parse_loose_ref_contents" cannot be passed.
3. "1053a9084501db79c72b14e8a5a0b67de3f91a" is not correct, it cannot be parsed successfully by "get_hash_hex_algop".
4. "41053a9084501db79c72b14e8a5a0b67de3f91ae garbage" is correct, because it will be parsed successfully by "get_hash_hex_algop" and "isspace(*p)" is true.

By the above discussion, I could answer you comments one by one.

When the content is short, although it may be sufficient to identify the object, we should still report an error here. This is because we care about the ref. As we can see from above discussion, the "object-name.c" totally relies on the interfaces provided by the ref backend. And "get_hash_hex_algop" is unhappy about this situation. And eventually the "object-name.c::get_oid_basic" will be unhappy, return -1.

I agree with this idea. But in this implementation, we want to reuse the "parse_loose_ref_contents" to check the consistency of the regular refs. If we are in a SHA-1 repository, "parse_loose_ref_contents" will be unhappy about this. However, I don't think we need to provide user that "the content is 64-hexdigit ...". We just report "bad ref content" to the user. This will also indicate the user something is wrong, you need to check the ref database.

This is the core problem why we want to introduce more strict check. Because in the current "parse_loose_ref_contents" function, as long as the next byte of the end of the hex is '\0', spaces, LF, CRLF. We could know that the content of the ref is OK.

But in my view, we should warn the user about this situation. This is because in the original code, we do not check the ref strictly for files backend. And I think at current, the normal user should not interact with the git database. If there are some garbages we found in the ref database, I guess this could be a sign for the user: "Watch out! there may be something wrong".

That's a hard question, really. I find it hard to know what should we do? The motivation is hard to describe. But I think this reply could make thing more clear here.

Thanks,
Jialuo

I'd say filesystem corruption, buggy implementations and compatibility with other implementations of Git. The format for refs does not allow for any other information than either an object ID for plain refs, and the referee for symbolic refs. The fact that we do accept that is a mere implementation detail because we reuse the same function to parse refs that we also use for pseudorefs. And these _can_ have additional data.

So any reference that contains additional data is not a proper ref and thus should be warned about from my point of view. No Git tooling should write them, so if something does it's a red flag to me.

Patrick

If you find such a file in $GIT_DIR/refs/ hierarchy, because our consumer side has been looser than necessary forever, and we never have written such a file ourselves, it is a sign that a third-party tool wrote it, and that the third-party tool used our reader implementation as the specification. That is why I am hesitant to retroactively tighten the rules like this patch does.

Thanks.

I forgot to add my recommended course of action, without which a review is worth much less X-<.

I am OK if we tightened the rules retroactively, as long as it starts as a probing check (i.e. "info: we found an unusual thing in the wild. Please report this to us so that we can ask you for more details like how such a ref that would violate a rule that was retroactively tightened got there", not "error: malformed ref").

Thanks.

Okay, that makes sense. The fsck infrastructure does have info message types, so this should certainly be doable. I'd argue that we might want to make this an FSCK_WARN, but I'm also fine with iteratively bumping up the severity from INFO to WARN to ERROR when we don't observe any complaints about this tightening.

Patrick

From the perspective of the implementation, there is no difference between the info and warn. But I have a doubt here. Do we really distinguish the info and warn in code?

Let's see the "fsck_vreport" (although this is a new function, but I never change the implementation) function:

  static int fsck_vreport(...)
  {
      enum fsck_msg_type msg_type = fsck_msg_type(msg_id, options);

      if (msg_type == FSCK_FATAL)
          msg_type = FSCK_ERROR;
      if (msg_type == FSCK_INFO)
          msg_type = FSCK_WARN;

      ...

}

We eventually convert the "FSCK_INFO" to "FSCK_WARN". Confusing.

Yes, actually I have thought about this way. But I don't want to use "goto". However, ident is noisy too. I will fix in the next version.

We have already introduced the checks for regular refs. There is no need to check the consistency of the target which the symbolic ref points to. Instead, we just check the content of the symbolic ref itself.

In order to check the content of the symbolic ref, create a function "files_fsck_symref_target". It will first check whether the "pointee" is under the "refs/" directory and then we will check the "pointee" itself.

There is no specification about the content of the symbolic ref. Although we do write "ref: %s\n" to create a symbolic ref by using "git-symbolic-ref(1)" command. However, this is not mandatory. We still accept symbolic refs with null trailing garbage. Put it more specific, the following are correct:

1. "ref: refs/heads/master "
2. "ref: refs/heads/master \n \n"
3. "ref: refs/heads/master\n\n"

But we do not allow any non-null trailing garbage. The following are bad symbolic contents.

1. "ref: refs/heads/master garbage\n"
2. "ref: refs/heads/master \n\n\n garbage "

In order to provide above checks, we will traverse the "pointee" to report the user whether this is null-garbage or no newline. And if symbolic refs contain non-null garbage, we will report "FSCK_MSG_BAD_REF_CONTENT" to the user.

Then, we will check the name of the "pointee" is correct by using "check_refname_format". And then if we can access the "pointee_path" in the file system, we should ensure that the file type is correct.

---
 Documentation/fsck-msgids.txt |  3 ++
 fsck.h                        |  1 +
 refs/files-backend.c          | 87 +++++++++++++++++++++++++++++++++++
 t/t0602-reffiles-fsck.sh      | 52 +++++++++++++++++++++
 4 files changed, 143 insertions(+)

index 1688c2f1fe..73587661dc 100644
@@ -28,6 +28,9 @@
 `badRefName`::
 	(ERROR) A ref has an invalid format.
 
+`badSymrefPointee`::
+	(ERROR) The pointee of a symref is bad.
+
 `badTagName`::
 	(INFO) A tag has an invalid format.
 

index 975d9b9da9..985b674dd9 100644
@@ -34,6 +34,7 @@ enum fsck_msg_type {
 	FUNC(BAD_REF_CONTENT, ERROR) \
 	FUNC(BAD_REF_FILETYPE, ERROR) \
 	FUNC(BAD_REF_NAME, ERROR) \
+	FUNC(BAD_SYMREF_POINTEE, ERROR) \
 	FUNC(BAD_TIMEZONE, ERROR) \
 	FUNC(BAD_TREE, ERROR) \
 	FUNC(BAD_TREE_SHA1, ERROR) \

2.46.0

We cannot. Think about this situation.

"ref: refs/heads/master \n "

We find that the next character of '\n' is not '\0'. Then we leave it to "check_refname_format". But "check_refname_format" will report an error here, but this is an allowed symref.

But I think using strchr is a nice way. I will try to find an elegant way here to handle this logic here.

When scanning the "refs" directory, we will check the file in the ref database, but we ignore the directory. So we are checking to know whether it does not point to a directory. If the ref points to a bad file type for example "ref/heads/bad-file"

If it is a block type file. We will first report that "refs/heads/bad-file" is a bad file and then report ref points to bad file "refs/heads/bad-file".

Actually, I think this is a little redundant here, but we can be tolerant about this because we need to guard against directory. We need to consider this situation.

So we could let this be.

Wouldn't it be correct to warn about this? To me the above very much looks like garbage after the refname, same like we'd also warn about such garbage for direct refs.

Patrick

Yes, we should warn about this. But only null-garbage is allowed for symref. The following situation is bad:

"ref: refs/heads/master \n garbage\n"

We should report error here, from my perspective, it's a FATAL ERROR. However, let's decide how to do this when we know what fsck error level we should set.

We have already introduced "files_fsck_symref_target". We should reuse this function to handle the symrefs which are legacy symbolic links. We should not check the trailing garbage for symbolic links. Add a new parameter "symbolic_link" to disable some checks which should only be used for symbolic ref.

We firstly use the "strbuf_add_real_path" to resolve the symlinks and get the absolute path "pointee_path" which the symlink ref points to. Then we can get the absolute path "abs_gitdir" of the "gitdir". By combining "pointee_path" and "abs_gitdir", we can extract the "referent". Thus, we can reuse "files_fsck_symref_target" function to seamlessly check the symlink refs.

---
 refs/files-backend.c     | 82 ++++++++++++++++++++++++++++------------
 t/t0602-reffiles-fsck.sh | 44 +++++++++++++++++++++
 2 files changed, 101 insertions(+), 25 deletions(-)

index bfb8d338d2..398afedaf0 100644
@@ -1,4 +1,5 @@
 #include "../git-compat-util.h"
+#include "../abspath.h"
 #include "../copy.h"
 #include "../environment.h"
 #include "../gettext.h"

@@ -3437,13 +3438,15 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
 /*
  * Check the symref "pointee_name" and "pointee_path". The caller should
  * make sure that "pointee_path" is absolute. For symbolic ref, "pointee_name"
- * would be the content after "refs:".
+ * would be the content after "refs:". For symblic link, "pointee_name" would
+ * be the relative path agaignst "gitdir".
  */
 static int files_fsck_symref_target(struct fsck_options *o,
 				    struct fsck_ref_report *report,
 				    const char *refname,
 				    struct strbuf *pointee_name,
-				    struct strbuf *pointee_path)
+				    struct strbuf *pointee_path,
+				    unsigned int symbolic_link)
 {
 	unsigned int newline_num = 0;
 	unsigned int space_num = 0;

@@ -3459,34 +3462,36 @@ static int files_fsck_symref_target(struct fsck_options *o,
 		goto out;
 	}
 
-	while (*p != '\0') {
-		if ((space_num || newline_num) && !isspace(*p)) {
-			ret = fsck_report_ref(o, report,
-					      FSCK_MSG_BAD_REF_CONTENT,
-					      "contains non-null garbage");
-			goto out;
+	if (!symbolic_link) {
+		while (*p != '\0') {
+			if ((space_num || newline_num) && !isspace(*p)) {
+				ret = fsck_report_ref(o, report,
+						      FSCK_MSG_BAD_REF_CONTENT,
+						      "contains non-null garbage");
+				goto out;
+			}
+
+			if (*p == '\n') {
+				newline_num++;
+			} else if (*p == ' ') {
+				space_num++;
+			}
+			p++;
 		}
 
-		if (*p == '\n') {
-			newline_num++;
-		} else if (*p == ' ') {
-			space_num++;
+		if (space_num || newline_num > 1) {
+			ret = fsck_report_ref(o, report,
+					      FSCK_MSG_TRAILING_REF_CONTENT,
+					      "trailing null-garbage");
+		} else if (!newline_num) {
+			ret = fsck_report_ref(o, report,
+					      FSCK_MSG_REF_MISSING_NEWLINE,
+					      "missing newline");
 		}
-		p++;
-	}
 
-	if (space_num || newline_num > 1) {
-		ret = fsck_report_ref(o, report,
-				      FSCK_MSG_TRAILING_REF_CONTENT,
-				      "trailing null-garbage");
-	} else if (!newline_num) {
-		ret = fsck_report_ref(o, report,
-				      FSCK_MSG_REF_MISSING_NEWLINE,
-				      "missing newline");
+		strbuf_rtrim(pointee_name);
 	}
 
-	strbuf_rtrim(pointee_name);
-
 	if (check_refname_format(pointee_name->buf, 0)) {
 		ret = fsck_report_ref(o, report,
 				      FSCK_MSG_BAD_SYMREF_POINTEE,

@@ -3521,8 +3526,10 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 	struct fsck_ref_report report = FSCK_REF_REPORT_DEFAULT;
 	struct strbuf pointee_path = STRBUF_INIT;
 	struct strbuf ref_content = STRBUF_INIT;
+	struct strbuf abs_gitdir = STRBUF_INIT;
 	struct strbuf referent = STRBUF_INIT;
 	struct strbuf refname = STRBUF_INIT;
+	unsigned int symbolic_link = 0;
 	const char *trailing = NULL;
 	unsigned int type = 0;
 	int failure_errno = 0;

@@ -3567,8 +3574,32 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 				    ref_store->gitdir, referent.buf);
 			ret = files_fsck_symref_target(o, &report, refname.buf,
 						       &referent,
-						       &pointee_path);
+						       &pointee_path,
+						       symbolic_link);
+		}
+	} else if (S_ISLNK(iter->st.st_mode)) {
+		const char *pointee_name = NULL;
+
+		symbolic_link = 1;
+
+		strbuf_add_real_path(&pointee_path, iter->path.buf);
+		strbuf_add_absolute_path(&abs_gitdir, ref_store->gitdir);
+		strbuf_normalize_path(&abs_gitdir);
+		if (!is_dir_sep(abs_gitdir.buf[abs_gitdir.len - 1]))
+			strbuf_addch(&abs_gitdir, '/');
+
+		if (!skip_prefix(pointee_path.buf,
+				 abs_gitdir.buf, &pointee_name)) {
+			ret = fsck_report_ref(o, &report,
+					       FSCK_MSG_BAD_SYMREF_POINTEE,
+					       "point to target outside gitdir");
+			goto cleanup;
 		}
+
+		strbuf_addstr(&referent, pointee_name);
+		ret = files_fsck_symref_target(o, &report, refname.buf,
+					       &referent, &pointee_path,
+					       symbolic_link);
 	}
 
 cleanup:

@@ -3576,6 +3607,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 	strbuf_release(&ref_content);
 	strbuf_release(&referent);
 	strbuf_release(&pointee_path);
+	strbuf_release(&abs_gitdir);
 	return ret;
 }
 

2.46.0

Hi All:

This new version handles the following reviews:

1. According to the advice from the Junio, we should just use "{0}" to initialize the zero structure "fsck_ref_report". This version handles this in [PATCH v2 1/4].
2. According to the advice from the Patrick, use "strrchr" instead of looping to make the code more clean in [PATCH v2 3/4].
3. Use "goto" to remove ident.

However, the most important thing for this patch is which fsck message type I choose. I have recorded the reason in the commit message. But I wanna explain the motivation in cover letter for making the reviewers easy to understand.

Actually, in the review process of the first version. Junio thought we should use "FSCK_INFO" and Patrick thought we should use "FSCK_WARN". And I raised a question here, what is the difference between the "FSCK_INFO" and "FSCK_WARN" because in "fsck.c::fsck_vreport" function, we will convert "FSCK_INFO" to "FSCK_WARN" like the following:

    static int fsck_vreport(...)
    {
        enum fsck_msg_type msg_type = fsck_msg_type(msg_id, options);

        if (msg_type == FSCK_FATAL)
            msg_type = FSCK_ERROR;
        if (msg_type == FSCK_INFO)
             msg_type = FSCK_WARN;
        ...
    }

And I have gone back to the history. Actually the first time the fsck message type was set up at f27d05b170 (fsck: allow upgrading fsck warnings to errors, 2015-06-22):

https://lore.kernel.org/git/[email protected]/

And I have understood why we need "FSCK_INFO". This is because when setting the "strict" filed in "fsck_options", all the fsck warns will become fsck errors. For example, this change verifies my thinking:
4dd3b045f5 (fsck: downgrade tree badFilemode to "info", 2022-08-10).

As you can see, this restriction makes the code safer. So, I agree with Junio, at now, we should use "FSCK_INFO" for trailing garbage and ref content ends without newline.

But we should report fsck errors for the following two situations for "git-fsck(1)" will report fsck errors by implicitly checking the ref database consistency.

1. "parse_loose_ref_contents" fail.
2. symref content is bad (cannot parse).

Thanks,
Jialuo

shejialuo (4):
ref: initialize "fsck_ref_report" with zero
ref: add regular ref content check for files backend
ref: add symbolic ref content check for files backend
ref: add symlink ref check for files backend

 Documentation/fsck-msgids.txt |  12 +++
 fsck.h                        |   4 +
 refs.c                        |   2 +-
 refs/files-backend.c          | 179 +++++++++++++++++++++++++++++++-
 refs/refs-internal.h          |   2 +-
 t/t0602-reffiles-fsck.sh      | 185 ++++++++++++++++++++++++++++++++++
 6 files changed, 379 insertions(+), 5 deletions(-)

Range-diff against v1:
1: 9ed3026ac5 ! 1: 0367904c81 fsck: introduce "FSCK_REF_REPORT_DEFAULT" macro
@@ Metadata
Author: shejialuo <[email protected]>

## Commit message ##
- fsck: introduce "FSCK_REF_REPORT_DEFAULT" macro
+ ref: initialize "fsck_ref_report" with zero

In "fsck.c::fsck_refs_error_function", we need to tell whether "oid" and "referent" is NULL. So, we need to always initialize these parameters to NULL instead of letting them point to anywhere when creating a new "fsck_ref_report" structure.

- In order to conveniently create a new "fsck_ref_report", add a new macro
- "FSCK_REF_REPORT_DEFAULT".
+ The original code explicitly specifies the ".path" field to initialize
+ the "fsck_ref_report" structure. However, it introduces confusion how we
+ initialize the other fields. In order to avoid this, initialize the
+ "fsck_ref_report" with zero to make clear that everything in
+ "fsck_ref_report" is zero initialized.

- ## fsck.h ##
-@@ fsck.h: struct fsck_ref_report {
- const char *referent;
- };
-
-+#define FSCK_REF_REPORT_DEFAULT { \
-+ .path = NULL, \
-+ .oid = NULL, \
-+ .referent = NULL, \
-+}
-+
- struct fsck_options {
- fsck_walk_func walk;
- fsck_error error_func;
-
## refs/files-backend.c ##
@@ refs/files-backend.c: static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
goto cleanup;

if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {
- struct fsck_ref_report report = { .path = NULL };
-+ struct fsck_ref_report report = FSCK_REF_REPORT_DEFAULT;
++ struct fsck_ref_report report = {0};

strbuf_addf(&sb, "%s/%s", refs_check_dir, iter->relative_path); report.path = sb.buf;
2: 714284cf2b ! 2: 7b6f4145cd ref: add regular ref content check for files backend
@@ Metadata
## Commit message ##
ref: add regular ref content check for files backend

- We implicitly reply on "git-fsck(1)" to check the consistency of regular
- refs. However, when parsing the regular refs for files backend, we allow
- the ref content to end with no newline or contain some garbages. We
- should warn the user about above situations.
+ We implicitly rely on "git-fsck(1)" to check the consistency of regular
+ refs. However, when parsing the regular refs for files backend by using
+ "files-backend.c::parse_loose_ref_contents", we allow the ref content to
+ be end with no newline or contain some garbages.

- In order to provide above functionality, enhance the "git-refs verify"
- command by adding consistency check for regular refs for files backend.
+ It may seem that we should report an error or warn fsck message to the
+ user about above situations. However, there may be some third-party
+ tools customizing the content of refs. We should not report an error
+ fsck message.

- Add the following three fsck messages to represent the above situations:
+ And we cannot either report a warn fsck message to the user. This is
+ because for "git-receive-pack(1)" and "git-fetch-pack(1)", they will
+ parse the fsck message type and check the message type by
+ "fsck.c::is_valid_msg_type". Only the fsck infos are not valid. If we
+ make the fsck message type to be warn, the user could upgrade the fsck
+ warnings to errors. And the user can also set the "strict" field in
+ "fsck_options" to upgrade the fsck warnings to errors.

- 1. "badRefContent(ERROR)": A ref has a bad content.
- 2. "refMissingNewline(WARN)": A valid ref does not end with newline.
- 3. "trailingRefContent(WARN)": A ref has trailing contents.
+ We should not allow the user to upgrade the fsck warnings to errors. It
+ might cause compatibility issue which will break the legacy repository.
+ So we add the following two fsck infos to represent the situation where
+ the ref content ends without newline or has garbages:
+
+ 1. "refMissingNewline(INFO)": A valid ref does not end with newline.
+ 2. "trailingRefContent(INFO)": A ref has trailing contents.
+
+ In "fsck.c::fsck_vreport", we will convert "FSCK_INFO" to "FSCK_WARN",
+ and we can still warn the user about these situations when using
+ "git-refs verify" without introducing compatibility issue.
+
+ In current "git-fsck(1)", it will report an error when the ref content
+ is bad, so we should following this to report an error to the user when
+ "parse_loose_ref_contents" fails. And we add a new fsck error message
+ called "badRefContent(ERROR)" to represent that a ref has a bad content.

In order to tell whether the ref has trailing content, add a new parameter "trailing" to "parse_loose_ref_contents". Then introduce a new
- function "files_fsck_refs_content" to check the regular refs.
+ function "files_fsck_refs_content" to check the regular refs to enhance
+ the "git-refs verify".

@@ Documentation/fsck-msgids.txt
(WARN) Tree contains entries pointing to a null sha1.

+refMissingNewline::
-+ (WARN) A valid ref does not end with newline.
++ (INFO) A valid ref does not end with newline.
+
+trailingRefContent::
-+ (WARN) A ref has trailing contents.
++ (INFO) A ref has trailing contents.
+
treeNotSorted::
(ERROR) A tree is not properly sorted.
@@ fsck.h: enum fsck_msg_type {
FUNC(BAD_REF_NAME, ERROR) \
FUNC(BAD_TIMEZONE, ERROR) \
@@ fsck.h: enum fsck_msg_type {
- FUNC(HAS_DOTDOT, WARN) \
- FUNC(HAS_DOTGIT, WARN) \
- FUNC(NULL_SHA1, WARN) \
-+ FUNC(REF_MISSING_NEWLINE, WARN) \
-+ FUNC(TRAILING_REF_CONTENT, WARN) \
- FUNC(ZERO_PADDED_FILEMODE, WARN) \
- FUNC(NUL_IN_COMMIT, WARN) \
- FUNC(LARGE_PATHNAME, WARN) \
+ FUNC(MAILMAP_SYMLINK, INFO) \
+ FUNC(BAD_TAG_NAME, INFO) \
+ FUNC(MISSING_TAGGER_ENTRY, INFO) \
++ FUNC(REF_MISSING_NEWLINE, INFO) \
++ FUNC(TRAILING_REF_CONTENT, INFO) \
+ /* ignored (elevated when requested) */ \
+ FUNC(EXTRA_HEADER_ENTRY, IGNORE)
+

## refs.c ##
@@ refs.c: static int refs_read_special_head(struct ref_store *ref_store,
@@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
+ const char *refs_check_dir,
+ struct dir_iterator *iter)
+{
-+ struct fsck_ref_report report = FSCK_REF_REPORT_DEFAULT;
+ struct strbuf ref_content = STRBUF_INIT;
+ struct strbuf referent = STRBUF_INIT;
+ struct strbuf refname = STRBUF_INIT;
++ struct fsck_ref_report report = {0};
+ const char *trailing = NULL;
+ unsigned int type = 0;
+ int failure_errno = 0;
@@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
+ }
+
+ if (parse_loose_ref_contents(ref_store->repo->hash_algo,
-+ ref_content.buf, &oid, &referent,
-+ &type, &trailing, &failure_errno)) {
++ ref_content.buf, &oid, &referent,
++ &type, &trailing, &failure_errno)) {
+ ret = fsck_report_ref(o, &report,
+ FSCK_MSG_BAD_REF_CONTENT,
+ "invalid ref content");
@@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
+ goto cleanup;
+ }
+ }
++ goto cleanup;
+ }
+
+cleanup:
3: 032b0d6a64 ! 3: 20d8556902 ref: add symbolic ref content check for files backend

    @@ Commit message
         3. "ref: refs/heads/master\n\n"
     
         But we do not allow any non-null trailing garbage. The following are bad
    -    symbolic contents.
    +    symbolic contents which will be reported as fsck error by "git-fsck(1)".
     
         1. "ref: refs/heads/master garbage\n"
         2. "ref: refs/heads/master \n\n\n garbage  "
     
    -    In order to provide above checks, we will traverse the "pointee" to
    -    report the user whether this is null-garbage or no newline. And if
    -    symbolic refs contain non-null garbage, we will report
    -    "FSCK_MSG_BAD_REF_CONTENT" to the user.
    -
    -    Then, we will check the name of the "pointee" is correct by using
    -    "check_refname_format". And then if we can access the "pointee_path" in
    -    the file system, we should ensure that the file type is correct.
    +    In order to provide above checks, we will use "strrchr" to check whether
    +    we have newline in the ref content. Then we will check the name of the
    +    "pointee" is correct by using "check_refname_format". If the function
    +    fails, we need to trim the "pointee" to see whether the null-garbage
    +    causes the function fails. If so, we need to report that there is
    +    null-garbage in the symref content. Otherwise, we should report the user
    +    the "pointee" is bad.
     
         Mentored-by: Patrick Steinhardt <[email protected]>
         Mentored-by: Karthik Nayak <[email protected]>
    @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
     +				    struct strbuf *pointee_name,
     +				    struct strbuf *pointee_path)
     +{
    -+	unsigned int newline_num = 0;
    -+	unsigned int space_num = 0;
    ++	const char *newline_pos = NULL;
     +	const char *p = NULL;
     +	struct stat st;
     +	int ret = 0;
    @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
     +		goto out;
     +	}
     +
    -+	while (*p != '\0') {
    -+		if ((space_num || newline_num) && !isspace(*p)) {
    -+			ret = fsck_report_ref(o, report,
    -+					      FSCK_MSG_BAD_REF_CONTENT,
    -+					      "contains non-null garbage");
    -+			goto out;
    -+		}
    -+
    -+		if (*p == '\n') {
    -+			newline_num++;
    -+		} else if (*p == ' ') {
    -+			space_num++;
    -+		}
    -+		p++;
    -+	}
    -+
    -+	if (space_num || newline_num > 1) {
    -+		ret = fsck_report_ref(o, report,
    -+				      FSCK_MSG_TRAILING_REF_CONTENT,
    -+				      "trailing null-garbage");
    -+	} else if (!newline_num) {
    ++	newline_pos = strrchr(p, '\n');
    ++	if (!newline_pos || *(newline_pos + 1)) {
     +		ret = fsck_report_ref(o, report,
     +				      FSCK_MSG_REF_MISSING_NEWLINE,
     +				      "missing newline");
     +	}
     +
    -+	strbuf_rtrim(pointee_name);
    -+
     +	if (check_refname_format(pointee_name->buf, 0)) {
    ++		/*
    ++		 * When containing null-garbage, "check_refname_format" will
    ++		 * fail, we should trim the "pointee" to check again.
    ++		 */
    ++		strbuf_rtrim(pointee_name);
    ++		if (!check_refname_format(pointee_name->buf, 0)) {
    ++			ret = fsck_report_ref(o, report,
    ++					      FSCK_MSG_TRAILING_REF_CONTENT,
    ++					      "trailing null-garbage");
    ++			goto out;
    ++		}
    ++
     +		ret = fsck_report_ref(o, report,
     +				      FSCK_MSG_BAD_SYMREF_POINTEE,
     +				      "points to refname with invalid format");
    @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
      				   const char *refs_check_dir,
      				   struct dir_iterator *iter)
      {
    - 	struct fsck_ref_report report = FSCK_REF_REPORT_DEFAULT;
     +	struct strbuf pointee_path = STRBUF_INIT;
      	struct strbuf ref_content = STRBUF_INIT;
      	struct strbuf referent = STRBUF_INIT;
    @@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_s
     +						       &referent,
     +						       &pointee_path);
      		}
    + 		goto cleanup;
      	}
    - 
     @@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_store,
      	strbuf_release(&refname);
      	strbuf_release(&ref_content);
    @@ t/t0602-reffiles-fsck.sh: test_expect_success 'regular ref content should be che
     +	printf "ref: refs/heads/branch     " > $branch_dir_prefix/a/b/branch-trailing &&
     +	git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    ++	warning: refs/heads/a/b/branch-trailing: refMissingNewline: missing newline
     +	warning: refs/heads/a/b/branch-trailing: trailingRefContent: trailing null-garbage
     +	EOF
     +	rm $branch_dir_prefix/a/b/branch-trailing &&
    @@ t/t0602-reffiles-fsck.sh: test_expect_success 'regular ref content should be che
     +	printf "ref: refs/heads/branch \n\n " > $branch_dir_prefix/a/b/branch-trailing &&
     +	git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    ++	warning: refs/heads/a/b/branch-trailing: refMissingNewline: missing newline
     +	warning: refs/heads/a/b/branch-trailing: trailingRefContent: trailing null-garbage
     +	EOF
     +	rm $branch_dir_prefix/a/b/branch-trailing &&
4:  147a873958 ! 4:  d9867c5f87 ref: add symlink ref consistency check for files backend
    @@ Metadata
     Author: shejialuo <[email protected]>
     
      ## Commit message ##
    -    ref: add symlink ref consistency check for files backend
    +    ref: add symlink ref check for files backend
     
         We have already introduced "files_fsck_symref_target". We should reuse
         this function to handle the symrefs which are legacy symbolic links. We
    @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
     +				    struct strbuf *pointee_path,
     +				    unsigned int symbolic_link)
      {
    - 	unsigned int newline_num = 0;
    - 	unsigned int space_num = 0;
    + 	const char *newline_pos = NULL;
    + 	const char *p = NULL;
     @@ refs/files-backend.c: static int files_fsck_symref_target(struct fsck_options *o,
      		goto out;
      	}
      
    --	while (*p != '\0') {
    --		if ((space_num || newline_num) && !isspace(*p)) {
    --			ret = fsck_report_ref(o, report,
    --					      FSCK_MSG_BAD_REF_CONTENT,
    --					      "contains non-null garbage");
    --			goto out;
    +-	newline_pos = strrchr(p, '\n');
    +-	if (!newline_pos || *(newline_pos + 1)) {
    +-		ret = fsck_report_ref(o, report,
    +-				      FSCK_MSG_REF_MISSING_NEWLINE,
    +-				      "missing newline");
     +	if (!symbolic_link) {
    -+		while (*p != '\0') {
    -+			if ((space_num || newline_num) && !isspace(*p)) {
    -+				ret = fsck_report_ref(o, report,
    -+						      FSCK_MSG_BAD_REF_CONTENT,
    -+						      "contains non-null garbage");
    -+				goto out;
    -+			}
    -+
    -+			if (*p == '\n') {
    -+				newline_num++;
    -+			} else if (*p == ' ') {
    -+				space_num++;
    -+			}
    -+			p++;
    - 		}
    - 
    --		if (*p == '\n') {
    --			newline_num++;
    --		} else if (*p == ' ') {
    --			space_num++;
    -+		if (space_num || newline_num > 1) {
    -+			ret = fsck_report_ref(o, report,
    -+					      FSCK_MSG_TRAILING_REF_CONTENT,
    -+					      "trailing null-garbage");
    -+		} else if (!newline_num) {
    ++		newline_pos = strrchr(p, '\n');
    ++		if (!newline_pos || *(newline_pos + 1)) {
     +			ret = fsck_report_ref(o, report,
     +					      FSCK_MSG_REF_MISSING_NEWLINE,
     +					      "missing newline");
    - 		}
    --		p++;
    --	}
    - 
    --	if (space_num || newline_num > 1) {
    --		ret = fsck_report_ref(o, report,
    --				      FSCK_MSG_TRAILING_REF_CONTENT,
    --				      "trailing null-garbage");
    --	} else if (!newline_num) {
    --		ret = fsck_report_ref(o, report,
    --				      FSCK_MSG_REF_MISSING_NEWLINE,
    --				      "missing newline");
    -+		strbuf_rtrim(pointee_name);
    ++		}
      	}
      
    --	strbuf_rtrim(pointee_name);
    --
      	if (check_refname_format(pointee_name->buf, 0)) {
    +-		/*
    +-		 * When containing null-garbage, "check_refname_format" will
    +-		 * fail, we should trim the "pointee" to check again.
    +-		 */
    +-		strbuf_rtrim(pointee_name);
    +-		if (!check_refname_format(pointee_name->buf, 0)) {
    +-			ret = fsck_report_ref(o, report,
    +-					      FSCK_MSG_TRAILING_REF_CONTENT,
    +-					      "trailing null-garbage");
    +-			goto out;
    ++		if (!symbolic_link) {
    ++			/*
    ++			* When containing null-garbage, "check_refname_format" will
    ++			* fail, we should trim the "pointee" to check again.
    ++			*/
    ++			strbuf_rtrim(pointee_name);
    ++			if (!check_refname_format(pointee_name->buf, 0)) {
    ++				ret = fsck_report_ref(o, report,
    ++						      FSCK_MSG_TRAILING_REF_CONTENT,
    ++						      "trailing null-garbage");
    ++				goto out;
    ++			}
    + 		}
    + 
      		ret = fsck_report_ref(o, report,
    - 				      FSCK_MSG_BAD_SYMREF_POINTEE,
     @@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_store,
    - 	struct fsck_ref_report report = FSCK_REF_REPORT_DEFAULT;
    + {
      	struct strbuf pointee_path = STRBUF_INIT;
      	struct strbuf ref_content = STRBUF_INIT;
     +	struct strbuf abs_gitdir = STRBUF_INIT;
      	struct strbuf referent = STRBUF_INIT;
      	struct strbuf refname = STRBUF_INIT;
    + 	struct fsck_ref_report report = {0};
    ++	const char *pointee_name = NULL;
     +	unsigned int symbolic_link = 0;
      	const char *trailing = NULL;
      	unsigned int type = 0;
    @@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_s
     -						       &pointee_path);
     +						       &pointee_path,
     +						       symbolic_link);
    -+		}
    -+	} else if (S_ISLNK(iter->st.st_mode)) {
    -+		const char *pointee_name = NULL;
    + 		}
    + 		goto cleanup;
    + 	}
    + 
    ++	symbolic_link = 1;
     +
    -+		symbolic_link = 1;
    ++	strbuf_add_real_path(&pointee_path, iter->path.buf);
    ++	strbuf_add_absolute_path(&abs_gitdir, ref_store->gitdir);
    ++	strbuf_normalize_path(&abs_gitdir);
    ++	if (!is_dir_sep(abs_gitdir.buf[abs_gitdir.len - 1]))
    ++		strbuf_addch(&abs_gitdir, '/');
     +
    -+		strbuf_add_real_path(&pointee_path, iter->path.buf);
    -+		strbuf_add_absolute_path(&abs_gitdir, ref_store->gitdir);
    -+		strbuf_normalize_path(&abs_gitdir);
    -+		if (!is_dir_sep(abs_gitdir.buf[abs_gitdir.len - 1]))
    -+			strbuf_addch(&abs_gitdir, '/');
    ++	if (!skip_prefix(pointee_path.buf, abs_gitdir.buf, &pointee_name)) {
    ++		ret = fsck_report_ref(o, &report,
    ++				      FSCK_MSG_BAD_SYMREF_POINTEE,
    ++				      "point to target outside gitdir");
    ++		goto cleanup;
    ++	}
     +
    -+		if (!skip_prefix(pointee_path.buf,
    -+				 abs_gitdir.buf, &pointee_name)) {
    -+			ret = fsck_report_ref(o, &report,
    -+					       FSCK_MSG_BAD_SYMREF_POINTEE,
    -+					       "point to target outside gitdir");
    -+			goto cleanup;
    - 		}
    ++	strbuf_addstr(&referent, pointee_name);
    ++	ret = files_fsck_symref_target(o, &report, refname.buf,
    ++				       &referent, &pointee_path,
    ++				       symbolic_link);
     +
    -+		strbuf_addstr(&referent, pointee_name);
    -+		ret = files_fsck_symref_target(o, &report, refname.buf,
    -+					       &referent, &pointee_path,
    -+					       symbolic_link);
    - 	}
    - 
      cleanup:
    -@@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_store,
    + 	strbuf_release(&refname);
      	strbuf_release(&ref_content);
      	strbuf_release(&referent);
      	strbuf_release(&pointee_path);

2.46.0

In "fsck.c::fsck_refs_error_function", we need to tell whether "oid" and "referent" is NULL. So, we need to always initialize these parameters to NULL instead of letting them point to anywhere when creating a new "fsck_ref_report" structure.

The original code explicitly specifies the ".path" field to initialize the "fsck_ref_report" structure. However, it introduces confusion how we initialize the other fields. In order to avoid this, initialize the "fsck_ref_report" with zero to make clear that everything in "fsck_ref_report" is zero initialized.

---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8d6ec9458d..d6fc3bd67c 100644
@@ -3446,7 +3446,7 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
 		goto cleanup;
 
 	if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {
-		struct fsck_ref_report report = { .path = NULL };
+		struct fsck_ref_report report = {0};
 
 		strbuf_addf(&sb, "%s/%s", refs_check_dir, iter->relative_path);
 		report.path = sb.buf;

2.46.0

The above description is a bit too strong than what this patch is actually fixing. If you explicitly initialize any member of an aggregate type, other members not mentioned will be implicitly 0-initialized, so the original does not give any confusion to readers who know what they are reading.

What the patch improves is that the common idiom used in this code base (and possibly elsewhere) is to use "{ 0 }", instead of explicitly saying "this particular member is 0-initialized".

The original code explicitly initializes the "path" member in the "struct fsck_ref_report" to NULL (which implicitly 0-initializes other members in the struct). It is more customary to use "{ 0 }" to express that we are 0-initializing everything.

The patch is correct, but spelling it like "{ 0 }" with a space on both sides is more common [*], and because this patch is all about making it more idiomatic, let's write it that way.

Thanks.

[Footnote]

* "git grep -e '{0};' -e '{ 0 };' '*.[ch]'" tells us so.

We implicitly rely on "git-fsck(1)" to check the consistency of regular refs. However, when parsing the regular refs for files backend by using "files-backend.c::parse_loose_ref_contents", we allow the ref content to be end with no newline or contain some garbages.

It may seem that we should report an error or warn fsck message to the user about above situations. However, there may be some third-party tools customizing the content of refs. We should not report an error fsck message.

And we cannot either report a warn fsck message to the user. This is because if the caller set the "strict" field in "fsck_options" to to upgrade the fsck warnings to errors.

We should not allow the user to upgrade the fsck warnings to errors. It might cause compatibility issue which will break the legacy repository. So we add the following two fsck infos to represent the situation where the ref content ends without newline or has garbages:

1. "refMissingNewline(INFO)": A valid ref does not end with newline.
2. "trailingRefContent(INFO)": A ref has trailing contents.

In "fsck.c::fsck_vreport", we will convert "FSCK_INFO" to "FSCK_WARN", and we can still warn the user about these situations when using "git-refs verify" without introducing compatibility issue.

In current "git-fsck(1)", it will report an error when the ref content is bad, so we should following this to report an error to the user when "parse_loose_ref_contents" fails. And we add a new fsck error message called "badRefContent(ERROR)" to represent that a ref has a bad content.

In order to tell whether the ref has trailing content, add a new parameter "trailing" to "parse_loose_ref_contents". Then introduce a new function "files_fsck_refs_content" to check the regular refs to enhance the "git-refs verify".

---
 Documentation/fsck-msgids.txt |  9 ++++
 fsck.h                        |  3 ++
 refs.c                        |  2 +-
 refs/files-backend.c          | 68 ++++++++++++++++++++++++++-
 refs/refs-internal.h          |  2 +-
 t/t0602-reffiles-fsck.sh      | 87 +++++++++++++++++++++++++++++++++++
 6 files changed, 167 insertions(+), 4 deletions(-)

index 68a2801f15..fc074fc571 100644
@@ -19,6 +19,9 @@
 `badParentSha1`::
 	(ERROR) A commit object has a bad parent sha1.
 
+`badRefContent`::
+	(ERROR) A ref has a bad content.
+
 `badRefFiletype`::
 	(ERROR) A ref has a bad file type.
 

@@ -170,6 +173,12 @@
 `nullSha1`::
 	(WARN) Tree contains entries pointing to a null sha1.
 
+`refMissingNewline`::
+	(INFO) A valid ref does not end with newline.
+
+`trailingRefContent`::
+	(INFO) A ref has trailing contents.
+
 `treeNotSorted`::
 	(ERROR) A tree is not properly sorted.
 

index 500b4c04d2..b85072df57 100644
@@ -31,6 +31,7 @@ enum fsck_msg_type {
 	FUNC(BAD_NAME, ERROR) \
 	FUNC(BAD_OBJECT_SHA1, ERROR) \
 	FUNC(BAD_PARENT_SHA1, ERROR) \
+	FUNC(BAD_REF_CONTENT, ERROR) \
 	FUNC(BAD_REF_FILETYPE, ERROR) \
 	FUNC(BAD_REF_NAME, ERROR) \
 	FUNC(BAD_TIMEZONE, ERROR) \

@@ -84,6 +85,8 @@ enum fsck_msg_type {
 	FUNC(MAILMAP_SYMLINK, INFO) \
 	FUNC(BAD_TAG_NAME, INFO) \
 	FUNC(MISSING_TAGGER_ENTRY, INFO) \
+	FUNC(REF_MISSING_NEWLINE, INFO) \
+	FUNC(TRAILING_REF_CONTENT, INFO) \
 	/* ignored (elevated when requested) */ \
 	FUNC(EXTRA_HEADER_ENTRY, IGNORE)
 

index 74de3d3009..5e74881945 100644
@@ -1758,7 +1758,7 @@ static int refs_read_special_head(struct ref_store *ref_store,
 	}
 
 	result = parse_loose_ref_contents(ref_store->repo->hash_algo, content.buf,
-					  oid, referent, type, failure_errno);
+					  oid, referent, type, NULL, failure_errno);
 
 done:
 	strbuf_release(&full_path);

index 2313c830d8..73b05f971b 100644
@@ -715,7 +715,7 @@ struct ref_store {
 int parse_loose_ref_contents(const struct git_hash_algo *algop,
 			     const char *buf, struct object_id *oid,
 			     struct strbuf *referent, unsigned int *type,
-			     int *failure_errno);
+			     const char **trailing, int *failure_errno);
 
 /*
  * Fill in the generic part of refs and add it to our collection of

2.46.0

Sorry for this paragraph, because I have changed commit message for this patch, the range-diff part would be outdated. But the code is still the same. So I hope this will not cause much trouble. And this paragraph should be the following:

And we cannot either report a warn fsck message to the user. This is because if the caller set the "strict" field in "fsck_options", fsck warnings will be converted to errors.

I will fix this in the next version until I receive enough feedback.

Thanks.

"to be end with" -> "to end with".
"or contain" -> "or to contain" (optional, I think).

Or "... the ref content without terminating newline, or with extra bytes after the terminating newline."

Even though we never created such loose refs ourselves, we have accepted such loose refs forever, so it is entirely possible that third-party tools may rely on such loose refs being valid. Let's notice such a "curiously formatted" loose ref files and tell the users our findings, so that we can assess the possible extent of damage if/when we retroactively tightened the parsing rules in the future.

I am not sure this is a right thing to say. If the user wants to ensure that the tool they use in their repository, which may include some third-party reimplementation of Git, would never create such a (semi-)malformed loose ref files, it is within their right, and it is the most reasonable way, to promote these "curiously formatted loose ref" fsck warnings to errors.

Is your "We should not allow" above backed by code that prevents them from promoting the warnings to errors, or is it merely a declaration of your intention?

OK.

OK.

Good.

I quite like this idea. Jialuo, would you maybe want to include another patch on top that adds a paragraph to Documentation/BreakingChanges.txt? It should note that this is not yet settled and depends on whether or not we see complaints with your new checks.

I guess another prereq for the change is to integrate git refs verify with git-fsck(1), because otherwise people likely wouldn't see the new messages in the first place.

Patrick

I wouldn't say that I wrote it as a devil's advocate comment, but I was hoping that somebody quote Postel in response, as the above advocates a directly opposite position, which I wouldn't usually take.

Absolutely.

For context, this is the quote you probably refer to: "be conservative in what you do, be liberal in what you accept from others".

In any case, I still think it is sensible to at least warn about refs like this. It is unexpected to me and may indicate real issues in the understanding of others that end up writing to the refdb. If there are implementations of Git out there that intentionally use our lax parsing to e.g. stuff additional metadata into refs, then we need to tell them that this is not okay.

This may have been fine in the past where there was only a single ref backend, but now with multiple ref backends the picture has changed in my opinion.

Patrick

Thanks, I will fix this in the next version.

I think I could organize the above to the commit message to better show the motivation why we should not report an error fsck message.

I have introduced some misunderstanding here. In the previous paragraph, I have mentioned that if the caller set the "strict" field in "fsck_options", the fsck warns would be automatically converted to fsck errors which may cause some trouble.

So I think here we should move this paragraph just after the previous paragraph to indicate why we do want to make a info fsck message here. Actually, the user could still explicitly use the following command

git -c fsck.refMissingNewline=error refs verify

to upgrade the fsck info to fsck error. But if the user use "--strict" like the following:

  git refs verify --strict

The fsck warns would be automatically converted to fsck errors. But actually at current, we do not want to the user implicitly upgrade fsck warns to fsck errors by using "--strict" flag. That's why we need to introduce the "FSCK_INO" here.

Actually, I was inspired by the Jeff King's commit:

4dd3b045f5 (fsck: downgrade tree badFilemode to "info", 2022-08-10)

In this commit, Jeff downgrades badFilemode to "info" to avoid above situation. I will improve the commit message to make things clearer.

However, from my perspective, the semantic of "FSCK_INFO" is a little unsuitable here. The comment says:

/* infos (reported as warnings, but ignored by default) */

The "ignored by default" here is very confusing. Actually, we make the "info" lower than the "warn" to avoid automatically converting the "warn" to "error" by setting "strict" field in "fsck_options".

But "ignored by default" will make the user think "oh, it's info, but we report it as warnings". We cannot know the real intention of the "FSCK_INFO" unless we have above context.

But I guess this is too far from the intention of this patch. We may improve this later.

From the review from the Patrick, I will add another patch in the "Documentation/BreakingChanges.txt" later.

Thanks,
Jialuo

As that documentation is not end-user facing, it is orthogonal and unrelated.

What I meant was that we need to tell the user that the refs they have (and the third-party tools they used to create them) may be declared invalid in a future version of Git and they would want to report it, in order to influence our possible future direction. And we need to do so in an end-user facing documentation (i.e. the part of the patch quoted above) and/or in the info messages themselves.

I think we should just drop the "valid" here. Because for symref, it may miss newline and is NOT valid.

I will improve this in the next version.

I think so, indenting is noisy. Will use "goto" to avoid indenting.

I agree with this. I will clean the code for the next version.

I will clean the code style here.

Actually, I have also thought about separating the tests which may clear and I dropped this idea due to the reason the same as yours. I DO agree that we should set up a single repository with all the garbage that we want to verify. This is necessary.

Thanks,
Jialuo

I think you understood after reading the series through and responded to my "curiously formatted" comment. I understand that these marked as INFO are not about "to complain" but are for us to ask the user to report so that we can learn of any third-party tools that may get in our way to later tighten the parsing rules retroactively.

So I do agree that the text above is less than optimal. It is "this is valid, but something we wouldn't have written. Who creates such a ref?"

We have already introduced the checks for regular refs. There is no need to check the consistency of the target which the symbolic ref points to. Instead, we just check the content of the symbolic ref itself.

In order to check the content of the symbolic ref, create a function "files_fsck_symref_target". It will first check whether the "pointee" is under the "refs/" directory and then we will check the "pointee" itself.

There is no specification about the content of the symbolic ref. Although we do write "ref: %s\n" to create a symbolic ref by using "git-symbolic-ref(1)" command. However, this is not mandatory. We still accept symbolic refs with null trailing garbage. Put it more specific, the following are correct:

1. "ref: refs/heads/master "
2. "ref: refs/heads/master \n \n"
3. "ref: refs/heads/master\n\n"

But we do not allow any non-null trailing garbage. The following are bad symbolic contents which will be reported as fsck error by "git-fsck(1)".

1. "ref: refs/heads/master garbage\n"
2. "ref: refs/heads/master \n\n\n garbage "

In order to provide above checks, we will use "strrchr" to check whether we have newline in the ref content. Then we will check the name of the "pointee" is correct by using "check_refname_format". If the function fails, we need to trim the "pointee" to see whether the null-garbage causes the function fails. If so, we need to report that there is null-garbage in the symref content. Otherwise, we should report the user the "pointee" is bad.

---
 Documentation/fsck-msgids.txt |  3 ++
 fsck.h                        |  1 +
 refs/files-backend.c          | 77 +++++++++++++++++++++++++++++++++++
 t/t0602-reffiles-fsck.sh      | 54 ++++++++++++++++++++++++
 4 files changed, 135 insertions(+)

index fc074fc571..85fd058c81 100644
@@ -28,6 +28,9 @@
 `badRefName`::
 	(ERROR) A ref has an invalid format.
 
+`badSymrefPointee`::
+	(ERROR) The pointee of a symref is bad.
+
 `badTagName`::
 	(INFO) A tag has an invalid format.
 

index b85072df57..cbe837f84c 100644
@@ -34,6 +34,7 @@ enum fsck_msg_type {
 	FUNC(BAD_REF_CONTENT, ERROR) \
 	FUNC(BAD_REF_FILETYPE, ERROR) \
 	FUNC(BAD_REF_NAME, ERROR) \
+	FUNC(BAD_SYMREF_POINTEE, ERROR) \
 	FUNC(BAD_TIMEZONE, ERROR) \
 	FUNC(BAD_TREE, ERROR) \
 	FUNC(BAD_TREE_SHA1, ERROR) \

2.46.0

Just in case you need it in the future, if you ever need to refer to a symbolic ref in a way that it is clear which of the two kinds you are talking about, you can say "textual symref" (a regular file whose contents is "ref: " followed by the target), to contrast them with "symbolic link used as symref".

In the proposed log message of this commit, all references to "symbolic ref" talk about textual ones, so I do not see any need to be extra explicit by saying "textual symref".

Hmph, as the pointee must be within the usual places that you would find refs (either in refs/ directory or pseudo ref files immediately below $GIT_DIR), wouldn't we check the pointee when fsck (or "git refs verify") run and check everything? The pointee will have its turn to be checked, and I am not sure why you need to check the pointee when you find a symbolic ref is pointing at it, which will lead for it to be checked twice (or more).

I however did not find an additional code to "check the pointee itself" in the patch, so perhaps it is OK---the only thing that needs fixing may be the above paragraph if that is the case.

Your use of word "null" is probably too confusing to contributors to this project. None of the above has NUL bytes in them. I think you want to say something like this:

A regular file is accepted as a textual symbolic ref if it begins with "ref:", followed by zero or more whitespaces, followed by the full refname (e.g. "refs/heads/master", "refs/tags/v1.0"), followed only by whitespace characters. We always write a single SP after "ref:" and a single LF after the full refname, but third-party reimplementations of Git may have taken advantage of the looser syntax that is allowed as above.

strrchr() to look for only LF is overly strict. You need to match what refs/files-backend.c:read_ref_internal() does to the contents read from such a loose ref file, i.e. strbuf_rtrim(). Any isspace() bytes are trimmed at the end, including SP, HT, CR and LF.

Yes, "we will check the 'pointee'" itself makes the reader confused. I will fix the above paragraph. Actually we do not check the "pointee", but check the symref content. Will fix this in the next version.

Thanks for your suggestion. I will improve this in the next version.

I will look into how "strbuf_rtrim" does to see whether we can reuse some functions to avoid repetition.

When I design this, I actually consider "ref: refs/heads/master\n " is still missing the newline. And then we also report that it has garbage. I think "ref: refs/heads/master\n \n" is not missing the newline. But, I don't think this is good.

I will find a good way to handle this.

Yes, these situations will be taken as a valid symbolic ref but actually there are something wrong. So this is what we need to care about.

I totally ignored this situation, and in current patch, we cannot check this. I know why Patrick lets me use "strchr" but not "strrchr". I think we should find the last '\n'. But instead we need to find the first '\n'. However, in this example, we will still fail by using "strchr". This part should be totally re-designed.

Yes, this is a good idea.

Yes, actually, I have thought I could compare the length change after executing the "strbuf_rtrim". I don't want to create two new variables, so I call "check_refname_format" twice.

Will fix this in the next version.

When setting up the infrastructure, actually we DO report filesystem entity that is not a regular file or symbolic link like the following:

    if (S_ISDIR(iter->st.st_mode)) {
        continue;
    } else if (S_ISREG(iter->st.st_mode) ||
               S_ISLNK(iter->st.st_mode)) {
        ...;
    } else {
      // report file system error
    }

We do not check the directory, because the directory will be always valid in the filesystem. we could not say that

"refs/heads/a/" is a bad ref.

So, this check mainly need to check whether the symref points to a directory. Actually, Patrick has also gave the review about this question in the previous version:

However, we should remove this line, because "check_refname_format" will take care for us.

git check-ref-format 'refs/heads/'

It will generate an error. So, we could entirely remove this line and let "check_refname_format" do this. And we could also remove the

    if (lstat(pointee_path->buf, &st) < 0)
        goto out;

The code will be much more clean.

Thanks,
Jialuo

Now that we're talking about tightening the rules for direct refs, I wonder whether we'd also want to apply the same rules to symrefs. Namely, when there is trailing whitespace we should generate an INFO-level message about that, too. This is mostly for the sake of consistency.

[snip]

Yes, actually this patch does this. I think I need to mention we reuse the "FSCK_INFO" message id defined in the [PATCH v2 2/4].

I agree, bad is too general here, we need to make it concrete.

Thanks, I will fix this in the next version.

Yes, I totally made a mistake here. I will try to think about a new design. I have already replied to Junio like the following:

Well, I guess the implementation about this is totally wrong, which will make the reviewers hard to understand. I will drop this way to explicitly check the garbage.

Thanks,
Jialuo

Yesterday while I was studying for reviewing this series, I saw some existing code that call them "referent". There may also be "target".

None of the above. It should strbuf_rtrim() and if we removed anything but just a single terminating LF, we are looking at something we wouldn't ahve written. The next check_refname_format() call would then find "trailing garbage".

- "refs/heads/master \n " gets rtrimmed to "refs/heads/master", which is "valid but curious".

- "refs/heads/main trash\n " becomes "refs/heads/main trash", which is outright bad.

Ah, true, I totally forgot about "referent". I guess we use both, but it would of course be great if we only had a single term to refer them. Referent seems to be used more widely, at least in the refs subsystem.

Fair.

Okay. This _may_ be something to generate an info message for, mostly in the same spirit as we want to do it for direct refs.

Yeah, this one should be an error indeed.

Patrick

We have already introduced "files_fsck_symref_target". We should reuse this function to handle the symrefs which are legacy symbolic links. We should not check the trailing garbage for symbolic links. Add a new parameter "symbolic_link" to disable some checks which should only be used for symbolic ref.

We firstly use the "strbuf_add_real_path" to resolve the symlinks and get the absolute path "pointee_path" which the symlink ref points to. Then we can get the absolute path "abs_gitdir" of the "gitdir". By combining "pointee_path" and "abs_gitdir", we can extract the "referent". Thus, we can reuse "files_fsck_symref_target" function to seamlessly check the symlink refs.

---
 refs/files-backend.c     | 68 +++++++++++++++++++++++++++++-----------
 t/t0602-reffiles-fsck.sh | 44 ++++++++++++++++++++++++++
 2 files changed, 94 insertions(+), 18 deletions(-)

index 382c73fcf7..8641e3ba65 100644
@@ -1,4 +1,5 @@
 #include "../git-compat-util.h"
+#include "../abspath.h"
 #include "../copy.h"
 #include "../environment.h"
 #include "../gettext.h"

@@ -3437,13 +3438,15 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
 /*
  * Check the symref "pointee_name" and "pointee_path". The caller should
  * make sure that "pointee_path" is absolute. For symbolic ref, "pointee_name"
- * would be the content after "refs:".
+ * would be the content after "refs:". For symblic link, "pointee_name" would
+ * be the relative path agaignst "gitdir".
  */
 static int files_fsck_symref_target(struct fsck_options *o,
 				    struct fsck_ref_report *report,
 				    const char *refname,
 				    struct strbuf *pointee_name,
-				    struct strbuf *pointee_path)
+				    struct strbuf *pointee_path,
+				    unsigned int symbolic_link)
 {
 	const char *newline_pos = NULL;
 	const char *p = NULL;

@@ -3458,24 +3461,28 @@ static int files_fsck_symref_target(struct fsck_options *o,
 		goto out;
 	}
 
-	newline_pos = strrchr(p, '\n');
-	if (!newline_pos || *(newline_pos + 1)) {
-		ret = fsck_report_ref(o, report,
-				      FSCK_MSG_REF_MISSING_NEWLINE,
-				      "missing newline");
+	if (!symbolic_link) {
+		newline_pos = strrchr(p, '\n');
+		if (!newline_pos || *(newline_pos + 1)) {
+			ret = fsck_report_ref(o, report,
+					      FSCK_MSG_REF_MISSING_NEWLINE,
+					      "missing newline");
+		}
 	}
 
 	if (check_refname_format(pointee_name->buf, 0)) {
-		/*
-		 * When containing null-garbage, "check_refname_format" will
-		 * fail, we should trim the "pointee" to check again.
-		 */
-		strbuf_rtrim(pointee_name);
-		if (!check_refname_format(pointee_name->buf, 0)) {
-			ret = fsck_report_ref(o, report,
-					      FSCK_MSG_TRAILING_REF_CONTENT,
-					      "trailing null-garbage");
-			goto out;
+		if (!symbolic_link) {
+			/*
+			* When containing null-garbage, "check_refname_format" will
+			* fail, we should trim the "pointee" to check again.
+			*/
+			strbuf_rtrim(pointee_name);
+			if (!check_refname_format(pointee_name->buf, 0)) {
+				ret = fsck_report_ref(o, report,
+						      FSCK_MSG_TRAILING_REF_CONTENT,
+						      "trailing null-garbage");
+				goto out;
+			}
 		}
 
 		ret = fsck_report_ref(o, report,

@@ -3510,9 +3517,12 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 {
 	struct strbuf pointee_path = STRBUF_INIT;
 	struct strbuf ref_content = STRBUF_INIT;
+	struct strbuf abs_gitdir = STRBUF_INIT;
 	struct strbuf referent = STRBUF_INIT;
 	struct strbuf refname = STRBUF_INIT;
 	struct fsck_ref_report report = {0};
+	const char *pointee_name = NULL;
+	unsigned int symbolic_link = 0;
 	const char *trailing = NULL;
 	unsigned int type = 0;
 	int failure_errno = 0;

@@ -3557,16 +3567,38 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 				    ref_store->gitdir, referent.buf);
 			ret = files_fsck_symref_target(o, &report, refname.buf,
 						       &referent,
-						       &pointee_path);
+						       &pointee_path,
+						       symbolic_link);
 		}
 		goto cleanup;
 	}
 
+	symbolic_link = 1;
+
+	strbuf_add_real_path(&pointee_path, iter->path.buf);
+	strbuf_add_absolute_path(&abs_gitdir, ref_store->gitdir);
+	strbuf_normalize_path(&abs_gitdir);
+	if (!is_dir_sep(abs_gitdir.buf[abs_gitdir.len - 1]))
+		strbuf_addch(&abs_gitdir, '/');
+
+	if (!skip_prefix(pointee_path.buf, abs_gitdir.buf, &pointee_name)) {
+		ret = fsck_report_ref(o, &report,
+				      FSCK_MSG_BAD_SYMREF_POINTEE,
+				      "point to target outside gitdir");
+		goto cleanup;
+	}
+
+	strbuf_addstr(&referent, pointee_name);
+	ret = files_fsck_symref_target(o, &report, refname.buf,
+				       &referent, &pointee_path,
+				       symbolic_link);
+
 cleanup:
 	strbuf_release(&refname);
 	strbuf_release(&ref_content);
 	strbuf_release(&referent);
 	strbuf_release(&pointee_path);
+	strbuf_release(&abs_gitdir);
 	return ret;
 }
 

2.46.0

With -Wunused-parameter, the compiler notices that many parameters are unused. They are truly unused, and the signatures for the functions involved are not constrained externally, so we can simply drop the parameters from the definition of these functions and their callers.

Please squash these in when the topic gets rerolled. Thanks.

 refs/files-backend.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

index 8641e3ba65..69dd283c9d 100644
@@ -1966,9 +1966,8 @@ static int create_ref_symlink(struct ref_lock *lock, const char *target)
 	return ret;
 }
 
-static int create_symref_lock(struct files_ref_store *refs,
-			      struct ref_lock *lock, const char *refname,
-			      const char *target, struct strbuf *err)
+static int create_symref_lock(struct ref_lock *lock, const char *target,
+			      struct strbuf *err)
 {
 	if (!fdopen_lock_file(&lock->lk, "w")) {
 		strbuf_addf(err, "unable to fdopen %s: %s",

@@ -2584,8 +2583,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
 	}
 
 	if (update->new_target && !(update->flags & REF_LOG_ONLY)) {
-		if (create_symref_lock(refs, lock, update->refname,
-				       update->new_target, err)) {
+		if (create_symref_lock(lock, update->new_target, err)) {
 			ret = TRANSACTION_GENERIC_ERROR;
 			goto out;
 		}

@@ -3443,7 +3441,6 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
  */
 static int files_fsck_symref_target(struct fsck_options *o,
 				    struct fsck_ref_report *report,
-				    const char *refname,
 				    struct strbuf *pointee_name,
 				    struct strbuf *pointee_path,
 				    unsigned int symbolic_link)

@@ -3565,7 +3562,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 		} else {
 			strbuf_addf(&pointee_path, "%s/%s",
 				    ref_store->gitdir, referent.buf);
-			ret = files_fsck_symref_target(o, &report, refname.buf,
+			ret = files_fsck_symref_target(o, &report,
 						       &referent,
 						       &pointee_path,
 						       symbolic_link);

@@ -3589,7 +3586,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
 	}
 
 	strbuf_addstr(&referent, pointee_name);
-	ret = files_fsck_symref_target(o, &report, refname.buf,
+	ret = files_fsck_symref_target(o, &report,
 				       &referent, &pointee_path,
 				       symbolic_link);
 

2.46.0-563-gaeb9b172ce

Here is another one.

By the way, Peff, do we have MAYBE_UNUSED that can be used in a case like this one? Platforms without symbolic links supported may well define NO_SYMLINK_HEAD, which makes the incoming parameters unused.

static int create_ref_symlink(struct ref_lock *lock, const char *target)

{
	int ret = -1;
#ifndef NO_SYMLINK_HEAD
	char *ref_path = get_locked_file_path(&lock->lk);
	unlink(ref_path);
	ret = symlink(target, ref_path);
	free(ref_path);

	if (ret)
		fprintf(stderr, "no symlink - falling back to symbolic ref\n");
#endif
	return ret;
}

We can of course do the attached, which I'll let shejialuo to squash into an appropriate patch in the series.

Thanks.

 refs/files-backend.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index 69dd283c9d..110af32788 100644
@@ -1951,10 +1951,13 @@ static int commit_ref_update(struct files_ref_store *refs,
 	return 0;
 }
 
+#ifdef NO_SYMLINK_HEAD
+#define create_ref_symlink(lock, referent) (-1)
+#else
 static int create_ref_symlink(struct ref_lock *lock, const char *target)
 {
 	int ret = -1;
-#ifndef NO_SYMLINK_HEAD
+
 	char *ref_path = get_locked_file_path(&lock->lk);
 	unlink(ref_path);
 	ret = symlink(target, ref_path);

@@ -1962,9 +1965,9 @@ static int create_ref_symlink(struct ref_lock *lock, const char *target)
 
 	if (ret)
 		fprintf(stderr, "no symlink - falling back to symbolic ref\n");
-#endif
 	return ret;
 }
+#endif
 
 static int create_symref_lock(struct ref_lock *lock, const char *target,
 			      struct strbuf *err)

Yes, it would be fine to use MAYBE_UNUSED in a case like this.

The other option, and what I did for a conditional compilation in imap-send.c, is to just mention the variable like:

  /* mark as used to appease -Wunused-parameter with NO_SYMLINK_HEAD */
  (void)lock;
  (void)target;

In retrospect I think MAYBE_UNUSED is probably a little less magical, and I perhaps should have used it there.

In this particular case, though, where there's no actual code in one half of the #ifdef, I think just defining two separate functions is cleaner. I.e., what you did with a macro below, though I'd probably have just used a real function with UNUSED markers.

As an aside, I wonder if we should consider deprecating and eventually dropping support for core.prefersymlinkrefs. I can't think of a reason anybody would want to use it, and of course it makes no sense as we move on to alternate backends like reftables. I sent patches ages ago:

https://lore.kernel.org/git/[email protected]/

but I think it may have just gotten lost in the shuffle, and I've somehow been meaning to re-submit them for 9 years. :-/

-Peff

Yup. Perhaps add an entry or two to BreakingChanges document?

 Documentation/BreakingChanges.txt | 6 ++++++
 1 file changed, 6 insertions(+)

index 0532bfcf7f..2a85740f3c 100644
@@ -115,6 +115,12 @@ info/grafts as outdated, 2014-03-05) and will be removed.
 +
 Cf. <[email protected]>.
 
+* Support for core.prefersymlinkrefs will be dropped.  Support for
+  existing repositories that use symbolic links to represent a
+  symbolic ref may or may not be dropped.
++
+Cf. <[email protected]>
+
 == Superseded features that will not be deprecated
 
 Some features have gained newer replacements that aim to improve the design in

Yup, that is why the above suggestion is on _this_ thread that is about the "check for curiously formatted symrefs, in the hope that we can retroactively tighten our checks later" topic.

I think the first step of the proposal (and what I had written in the patches that I linked) was just that we would stop _writing_ symlinks. And there we'd only need to warn people who have that config option set.

Whether to drop the reading side is less clear to me. I think in the long run it is good as a cleanup (and one less source of weird behavior that malicious local repos can trigger). But that decision can be made separately. I think it would be OK to just issue a deprecation warning whenever we actually follow a symlink (because I think we do so manually, since we need to know the target name as part of the resolution process).

-Peff

They are to be squashed into your patch, "suggested edit" for your changes, not "to be rebased". In other words, we do not want to see a patch (from your v2 as-is) to create problems and then another patch (taken from one of these links) applied on top to remedy them. We instead want to see a patch (start from your v2 but with the changes from these links) that does not introduce problems in the first place.

It turns out that I was, without realizing it myself, making an oblique reference to your patch 7/6 ;-)

Perhaps something along this line?

---- >8 ----
Subject: CodingGuidelines: also mention MAYBE_UNUSED

A function that uses a parameter in one build may lose all uses of the parameter in another build, depending on the configuration. A workaround for such a case, MAYBE_UNUSED, should also be mentioned when we recommend the use of UNUSED to our developers.

---
 Documentation/CodingGuidelines |  5 +++--
 git-compat-util.h              | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

index d0fc7cfe60..3263245b03 100644
@@ -262,8 +262,9 @@ For C programs:
    like "error: unused parameter 'foo' [-Werror=unused-parameter]",
    which indicates that a function ignores its argument. If the unused
    parameter can't be removed (e.g., because the function is used as a
-   callback and has to match a certain interface), you can annotate the
-   individual parameters with the UNUSED keyword, like "int foo UNUSED".
+   callback and has to match a certain interface), you can annotate
+   the individual parameters with the UNUSED (or MAYBE_UNUSED)
+   keyword, like "int foo UNUSED".
 
  - We try to support a wide range of C compilers to compile Git with,
    including old ones.  As of Git v2.35.0 Git requires C99 (we check

index 71b4d23f03..23307ce780 100644
@@ -195,6 +195,17 @@ struct strbuf;
 #define _NETBSD_SOURCE 1
 #define _SGI_SOURCE 1
 
+/*
+ * UNUSED marks a function parameter that is always unused.
+ *
+ * A callback interface may dictate that a function accepts a
+ * parameter at that position, but the implementation of the function
+ * may not need to use the parameter.  In such a case, mark the parameter
+ * with UNUSED.
+ *
+ * When a parameter may be used or unused, depending on conditional
+ * compilation, consider using MAYBE_UNUSED instead.
+ */
 #if GIT_GNUC_PREREQ(4, 5)
 #define UNUSED __attribute__((unused)) \
 	__attribute__((deprecated ("parameter declared as UNUSED")))

@@ -649,6 +660,16 @@ static inline int git_has_dir_sep(const char *path)
 #define RESULT_MUST_BE_USED
 #endif
 
+/*
+ * MAYBE_UNUSED marks a function parameter that may be unused, but
+ * whose use is not an error.
+ *
+ * Depending on a configuration, all uses of a function parameter may
+ * become #ifdef'ed away.  Marking such a parameter with UNUSED would
+ * give a warning in a compilation where the parameter is indeed used,
+ * and not marking such a parameter would give a warning in a
+ * compilation where the parameter is unused.
+ */
 #define MAYBE_UNUSED __attribute__((__unused__))
 
 #include "compat/bswap.h"

Yeah, this looks good. A few small comments below (but I'm not sure anything needs to be changed).

This is all good as pertains to function parameters. But the original reason we added MAYBE_UNUSED was actually for static functions that were auto-generated by the commit-slab macros. Saying "...marks a function parameter" implies to me that it's the only use. I don't know if we want to be more expansive here or not. Adding auto-generated macro functions should be quite a rarity, I'd think.

-Peff

True. You can annotate types, variables, and functions with the attributes as well. How about saying something like this

MAYBE_UNUSED marks a function parameter that may be unused but whose use is not an error. It also can be applied to functions, types and variables.

and then keep the explanation of why you may want to use the maybe- variant as-is, using a function parameter as an example? Or I could rewrite "parameter" and "function parameter" in it with "thing" (with double quotes around), like:

Depending on a configuration, all uses of a "thing" may become #ifdef'ed away....

Unlike the use of deprecated attribute, our definition of MAYBE_UNUSED is not guarded with anything. Shouldn't we at least do

    #if defined(__GNUC__)
    #define MAYBE_UNUSED __attribute__((__unused__))
    #else
    #define MAYBE_UNUSED /* noop */
    #endif

or something, by the way?

Thanks.

A function that uses a parameter in one build may lose all uses of the parameter in another build, depending on the configuration. A workaround for such a case, MAYBE_UNUSED, should also be mentioned when we recommend the use of UNUSED to our developers.

Keep the addition to the guideline short and document the criteria to choose between UNUSED and MAYBE_UNUSED near their definition.

---
 Documentation/CodingGuidelines |  5 +++--
 git-compat-util.h              | 24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

index d0fc7cfe60..3263245b03 100644
@@ -262,8 +262,9 @@ For C programs:
    like "error: unused parameter 'foo' [-Werror=unused-parameter]",
    which indicates that a function ignores its argument. If the unused
    parameter can't be removed (e.g., because the function is used as a
-   callback and has to match a certain interface), you can annotate the
-   individual parameters with the UNUSED keyword, like "int foo UNUSED".
+   callback and has to match a certain interface), you can annotate
+   the individual parameters with the UNUSED (or MAYBE_UNUSED)
+   keyword, like "int foo UNUSED".
 
  - We try to support a wide range of C compilers to compile Git with,
    including old ones.  As of Git v2.35.0 Git requires C99 (we check

index 71b4d23f03..e4a306dd56 100644
@@ -195,6 +195,19 @@ struct strbuf;
 #define _NETBSD_SOURCE 1
 #define _SGI_SOURCE 1
 
+/*
+ * UNUSED marks a function parameter that is always unused.  It also
+ * can be used to annotate a function, a variable, or a type that is
+ * always unused.
+ *
+ * A callback interface may dictate that a function accepts a
+ * parameter at that position, but the implementation of the function
+ * may not need to use the parameter.  In such a case, mark the parameter
+ * with UNUSED.
+ *
+ * When a parameter may be used or unused, depending on conditional
+ * compilation, consider using MAYBE_UNUSED instead.
+ */
 #if GIT_GNUC_PREREQ(4, 5)
 #define UNUSED __attribute__((unused)) \
 	__attribute__((deprecated ("parameter declared as UNUSED")))

@@ -649,6 +662,17 @@ static inline int git_has_dir_sep(const char *path)
 #define RESULT_MUST_BE_USED
 #endif
 
+/*
+ * MAYBE_UNUSED marks a function parameter that may be unused, but
+ * whose use is not an error.  It also can be used to annotate a
+ * function, a variable, or a type that may be unused.
+ *
+ * Depending on a configuration, all uses of such a thing may become
+ * #ifdef'ed away.  Marking it with UNUSED would give a warning in a
+ * compilation where it is indeed used, and not marking it at all
+ * would give a warning in a compilation where it is unused.  In such
+ * a case, MAYBE_UNUSED is the appropriate annotation to use.
+ */
 #define MAYBE_UNUSED __attribute__((__unused__))
 
 #include "compat/bswap.h"

Interdiff against v1:
diff --git a/git-compat-util.h b/git-compat-util.h index 23307ce780..e4a306dd56 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -196,7 +196,9 @@ struct strbuf;
#define _SGI_SOURCE 1

/*
- * UNUSED marks a function parameter that is always unused.
+ * UNUSED marks a function parameter that is always unused. It also
+ * can be used to annotate a function, a variable, or a type that is
+ * always unused.
*
* A callback interface may dictate that a function accepts a
* parameter at that position, but the implementation of the function
@@ -662,13 +664,14 @@ static inline int git_has_dir_sep(const char *path)

/*
* MAYBE_UNUSED marks a function parameter that may be unused, but
- * whose use is not an error.
+ * whose use is not an error. It also can be used to annotate a
+ * function, a variable, or a type that may be unused.

    *
  - * Depending on a configuration, all uses of a function parameter may
  - * become #ifdef'ed away.  Marking such a parameter with UNUSED would
  - * give a warning in a compilation where the parameter is indeed used,
  - * and not marking such a parameter would give a warning in a
  - * compilation where the parameter is unused.
  + * Depending on a configuration, all uses of such a thing may become
  + * #ifdef'ed away.  Marking it with UNUSED would give a warning in a
  + * compilation where it is indeed used, and not marking it at all
  + * would give a warning in a compilation where it is unused.  In such
  + * a case, MAYBE_UNUSED is the appropriate annotation to use.
    */

#define MAYBE_UNUSED __attribute__((__unused__))

2.46.0-563-gaeb9b172ce

Just like we only define UNUSED macro when __GNUC__ is defined, and fall back to an empty definition otherwise, we should do the same for MAYBE_UNUSED.

---
* Before I forget that we have discussed this, just as a documentation (read: this is not a patch to be applied).

I think this only matters when a compiler satisfies all three traits:

- does not define __GNUC__
- does have its own __attribute__() macro
- barfs on __attribute__((__unused__))

Otherwise we will define __attribute__(x) away to empty to cause no harm.

Since we have survived without complaints without such a guard for quite some time, it may be a sign that no compiler that knows __attribute__() that people ever tried to compile Git with barfs with __attribute__((__unused__)). I dunno.

 git-compat-util.h | 4 ++++
 1 file changed, 4 insertions(+)

index e4a306dd56..74ed581b5d 100644
@@ -673,7 +673,11 @@ static inline int git_has_dir_sep(const char *path)
  * would give a warning in a compilation where it is unused.  In such
  * a case, MAYBE_UNUSED is the appropriate annotation to use.
  */
+#ifdef __GNUC__
 #define MAYBE_UNUSED __attribute__((__unused__))
+#else
+#define MAYBE_UNUSED
+#endif
 
 #include "compat/bswap.h"
 

2.46.0-563-gaeb9b172ce

Yeah, I was surprised that this didn't have a guard and was not currently barfing on other compilers. And the answer is that we already turn __attribute__ into a noop on non-GNUC platforms.

Which made me wonder if UNUSED really needs its guards. It does, because it is defined early in the file, before the __attribute__ handling. I don't think we want to move it down, since it needs to be available for use by inline'd compat wrappers. But arguably we should move the attribute macro earlier in the file?

I don't know that it is really worth spending too much time futzing with, though.

Plus these non-GNUC platforms either

(1) do not have their own __attribute__, which lets us turn __attribute__() into noop, or

(2) have their own __attribute__, but they happen to support

     __attribute__((__unused__)).

If somebody has __attribute__() and does not support (__unused__) in it, use of MAYBE_UNUSED would be broken (maybe their __attribute__() supports other things but not unused).

And moving __attribute__ definition earlier in the file would not help such a platform with broken __attribute__((__unused__))

I am inclined to think it is not. So let's scrap the patch. The list archive will hopefully remember when it becomes necessary ;-)

Thanks, I think this is good. There's more nuanced discussion about when the "MAYBE" variant could be used for non-parameters, but I don't know that it's worth trying to enumerate every place we've found it useful.

-Peff

Hi All:

This new version does the following things:

1. [PATCH v3 1/4]

+ the motivation of the previous commit message is too strong, this version improves this.

2. [PATCH v3 2/4]

+ Enhance the commit message to make things clearer.
+ Enhance the descriptions of the fsck message ids. Tell the user we may consider converting info to error later to let the user know this and report some feedback to us.
+ Use "goto" to avoid unnecessary indentation.
+ Use "test_commit" to create a single commit in the test file to avoid unnecessary setups.
+ Enhance the test cases by adding normal situation case test and add a new aggregation test to double verify the functionality.
+ Clean the "> $file" to ">$file" to make the code style correct.

3. [PATCH v3 3/4]

+ Enhance the commit message by better describing the motivation.
+ Change the fsck message name "badSymrefPointee" to "badSymrefTarget" to be align with the codebase. And talking more about what is the "bad" in the documentation.
+ Use idea from Junio to check the textual symref content.
+ Still keep the following code:

if (lstat(referent->buf, &st))
goto out;

if (S_ISDIR(st.st_mode)) {
ret = report(...);
goto out;
}

This is because that we cannot know whether "refs/heads/a" is a regular ref or a directory by using "check_refname_format". So we have to add this check. It may seem we have done this when iterating the "refs" directory. However, we do report error for other NON-symlink and NON-regular file type but for directory, we omit. We cannot say oh this is not right. So we need to explicitly check here.

+ Like [PATCH v3 2/4], enhance the test code.

4. [PATCH v3 4/4]

+ Enhance the commit message
+ Introduce a new fsck info "symlinkRef" to warn the user that we will see this warning as an error when we drop the symlink ref support.
+ Squash the following two patches into this patch:
https://lore.kernel.org/git/[email protected]/
https://lore.kernel.org/git/[email protected]/

Thanks,
Jialuo

shejialuo (4):
ref: initialize "fsck_ref_report" with zero
ref: add regular ref content check for files backend
ref: add symref content check for files backend
ref: add symlink ref content check for files backend

 Documentation/fsck-msgids.txt |  20 ++
 fsck.h                        |   5 +
 refs.c                        |   2 +-
 refs/files-backend.c          | 205 ++++++++++++++++++++-
 refs/refs-internal.h          |   2 +-
 t/t0602-reffiles-fsck.sh      | 334 ++++++++++++++++++++++++++++++++++
 6 files changed, 556 insertions(+), 12 deletions(-)

Range-diff against v2:
1: c49a216b70 ! 1: 9fdab751c1 ref: initialize "fsck_ref_report" with zero
@@ Commit message
NULL instead of letting them point to anywhere when creating a new "fsck_ref_report" structure.

- The original code explicitly specifies the ".path" field to initialize
- the "fsck_ref_report" structure. However, it introduces confusion how we
- initialize the other fields. In order to avoid this, initialize the
- "fsck_ref_report" with zero to make clear that everything in
- "fsck_ref_report" is zero initialized.
+ The original code explicitly initializes the "path" member in the
+ "struct fsck_ref_report" to NULL (which implicitly 0-initializes other
+ members in the struct). It is more customary to use " {0} " to express
+ that we are 0-initializing everything. In order to be align with the the
+ codebase, initialize "fsck_ref_report" with zero.

@@ refs/files-backend.c: static int files_fsck_refs_name(struct ref_store *ref_stor

if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {
- struct fsck_ref_report report = { .path = NULL };
-+ struct fsck_ref_report report = {0};
++ struct fsck_ref_report report = { 0 };

strbuf_addf(&sb, "%s/%s", refs_check_dir, iter->relative_path); report.path = sb.buf;
2: 99e37b0304 ! 2: 4640b6e345 ref: add regular ref content check for files backend

    @@ Commit message
         We implicitly rely on "git-fsck(1)" to check the consistency of regular
         refs. However, when parsing the regular refs for files backend by using
         "files-backend.c::parse_loose_ref_contents", we allow the ref content to
    -    be end with no newline or contain some garbages.
    +    end with no newline or to contain some garbages.
     
    -    It may seem that we should report an error or warn fsck message to the
    -    user about above situations. However, there may be some third-party
    -    tools customizing the content of refs. We should not report an error
    -    fsck message.
    +    Even though we never create such loose refs ourselves, we have accepted
    +    such loose refs. So, it is entirely possible that some third-party tools
    +    may rely on such loose refs being valid. We should not report an error
    +    fsck message at current. But let's notice such a "curiously formatted"
    +    loose refs being valid and tell the user our findings, so we can access
    +    the possible extent of damage when we tighten the parsing rules in the
    +    future.
     
    -    And we cannot either report a warn fsck message to the user. This is
    -    because if the caller set the "strict" field in "fsck_options" to
    -    to upgrade the fsck warnings to errors.
    +    And it's not suitable to either report a warn fsck message to the user.
    +    This is because if the caller set the "strict" field in "fsck_options",
    +    fsck warns will be automatically upgraded to errors. We should not allow
    +    user to specify the "--strict" flag to upgrade the fsck warnings to
    +    errors at current. It might cause compatibility issue which may break
    +    the legacy repository. So we add the following two fsck infos to
    +    represent the situation where the ref content ends without newline or has
    +    garbages:
     
    -    We should not allow the user to upgrade the fsck warnings to errors. It
    -    might cause compatibility issue which will break the legacy repository.
    -    So we add the following two fsck infos to represent the situation where
    -    the ref content ends without newline or has garbages:
    +    1. "refMissingNewline(INFO)": A ref does not end with newline. This kind
    +       of ref may be considered ERROR in the future.
    +    2. "trailingRefContent(INFO)": A ref has trailing contents. This kind of
    +       ref may be considered ERROR in the future.
     
    -    1. "refMissingNewline(INFO)": A valid ref does not end with newline.
    -    2. "trailingRefContent(INFO)": A ref has trailing contents.
    -
    -    In "fsck.c::fsck_vreport", we will convert "FSCK_INFO" to "FSCK_WARN",
    -    and we can still warn the user about these situations when using
    -    "git-refs verify" without introducing compatibility issue.
    +    It may seem that we could not give the user any warnings by creating
    +    fsck infos. However, in "fsck.c::fsck_vreport", we will convert
    +    "FSCK_INFO" to "FSCK_WARN" and we can still warn the user about these
    +    situations when using "git-refs verify" without introducing
    +    compatibility issue.
     
         In current "git-fsck(1)", it will report an error when the ref content
         is bad, so we should following this to report an error to the user when
    @@ Documentation/fsck-msgids.txt
      	(WARN) Tree contains entries pointing to a null sha1.
      
     +`refMissingNewline`::
    -+	(INFO) A valid ref does not end with newline.
    ++	(INFO) A ref does not end with newline. This kind of ref may
    ++	be considered ERROR in the future.
     +
     +`trailingRefContent`::
    -+	(INFO) A ref has trailing contents.
    ++	(INFO) A ref has trailing contents. This kind of ref may be
    ++	considered ERROR in the future.
     +
      `treeNotSorted`::
      	(ERROR) A tree is not properly sorted.
    @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
     +	strbuf_addf(&refname, "%s/%s", refs_check_dir, iter->relative_path);
     +	report.path = refname.buf;
     +
    -+	if (S_ISREG(iter->st.st_mode)) {
    -+		if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) {
    -+			ret = error_errno(_("%s/%s: unable to read the ref"),
    -+					  refs_check_dir, iter->relative_path);
    -+			goto cleanup;
    -+		}
    ++	if (S_ISLNK(iter->st.st_mode))
    ++		goto cleanup;
    ++
    ++	if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) {
    ++		ret = error_errno(_("%s/%s: unable to read the ref"),
    ++				  refs_check_dir, iter->relative_path);
    ++		goto cleanup;
    ++	}
     +
    -+		if (parse_loose_ref_contents(ref_store->repo->hash_algo,
    -+					     ref_content.buf, &oid, &referent,
    -+					     &type, &trailing, &failure_errno)) {
    ++	if (parse_loose_ref_contents(ref_store->repo->hash_algo,
    ++				     ref_content.buf, &oid, &referent,
    ++				     &type, &trailing, &failure_errno)) {
    ++		ret = fsck_report_ref(o, &report,
    ++				      FSCK_MSG_BAD_REF_CONTENT,
    ++				      "invalid ref content");
    ++		goto cleanup;
    ++	}
    ++
    ++	if (!(type & REF_ISSYMREF)) {
    ++		if (*trailing == '\0') {
     +			ret = fsck_report_ref(o, &report,
    -+					      FSCK_MSG_BAD_REF_CONTENT,
    -+					      "invalid ref content");
    ++					      FSCK_MSG_REF_MISSING_NEWLINE,
    ++					      "missing newline");
     +			goto cleanup;
     +		}
     +
    -+		if (!(type & REF_ISSYMREF)) {
    -+			if (*trailing == '\0') {
    -+				ret = fsck_report_ref(o, &report,
    -+						      FSCK_MSG_REF_MISSING_NEWLINE,
    -+						      "missing newline");
    -+				goto cleanup;
    -+			}
    -+
    -+			if (*trailing != '\n' || (*(trailing + 1) != '\0')) {
    -+				ret = fsck_report_ref(o, &report,
    -+						      FSCK_MSG_TRAILING_REF_CONTENT,
    -+						      "trailing garbage in ref");
    -+				goto cleanup;
    -+			}
    ++		if (*trailing != '\n' || (*(trailing + 1) != '\0')) {
    ++			ret = fsck_report_ref(o, &report,
    ++					      FSCK_MSG_TRAILING_REF_CONTENT,
    ++					      "trailing garbage in ref");
    ++			goto cleanup;
     +		}
    -+		goto cleanup;
     +	}
     +
     +cleanup:
    @@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref name check should be adapted
      	test_must_be_empty err
      '
      
    -+test_expect_success 'regular ref content should be checked' '
    ++test_expect_success 'regular ref content should be checked (individual)' '
     +	test_when_finished "rm -rf repo" &&
     +	git init repo &&
     +	branch_dir_prefix=.git/refs/heads &&
     +	tag_dir_prefix=.git/refs/tags &&
     +	cd repo &&
    -+	git commit --allow-empty -m initial &&
    -+	git checkout -b branch-1 &&
    -+	git tag tag-1 &&
    -+	git commit --allow-empty -m second &&
    -+	git checkout -b branch-2 &&
    -+	git tag tag-2 &&
    -+	git checkout -b a/b/tag-2 &&
    ++	test_commit default &&
    ++	mkdir -p "$branch_dir_prefix/a/b" &&
     +
    -+	printf "%s" "$(git rev-parse branch-1)" > $branch_dir_prefix/branch-1-no-newline &&
    ++	git refs verify 2>err &&
    ++	test_must_be_empty err &&
    ++
    ++	printf "%s" "$(git rev-parse main)" >$branch_dir_prefix/branch-no-newline &&
     +	git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	warning: refs/heads/branch-1-no-newline: refMissingNewline: missing newline
    ++	warning: refs/heads/branch-no-newline: refMissingNewline: missing newline
     +	EOF
    -+	rm $branch_dir_prefix/branch-1-no-newline &&
    ++	rm $branch_dir_prefix/branch-no-newline &&
     +	test_cmp expect err &&
     +
    -+	printf "%s garbage" "$(git rev-parse branch-1)" > $branch_dir_prefix/branch-1-garbage &&
    ++	printf "%s garbage" "$(git rev-parse main)" >$branch_dir_prefix/branch-garbage &&
     +	git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	warning: refs/heads/branch-1-garbage: trailingRefContent: trailing garbage in ref
    ++	warning: refs/heads/branch-garbage: trailingRefContent: trailing garbage in ref
     +	EOF
    -+	rm $branch_dir_prefix/branch-1-garbage &&
    ++	rm $branch_dir_prefix/branch-garbage &&
     +	test_cmp expect err &&
     +
    -+	printf "%s\n\n\n" "$(git rev-parse tag-1)" > $tag_dir_prefix/tag-1-garbage &&
    ++	printf "%s\n\n\n" "$(git rev-parse main)" >$tag_dir_prefix/tag-garbage-1 &&
     +	git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	warning: refs/tags/tag-1-garbage: trailingRefContent: trailing garbage in ref
    ++	warning: refs/tags/tag-garbage-1: trailingRefContent: trailing garbage in ref
     +	EOF
    -+	rm $tag_dir_prefix/tag-1-garbage &&
    ++	rm $tag_dir_prefix/tag-garbage-1 &&
     +	test_cmp expect err &&
     +
    -+	printf "%s\n\n\n  garbage" "$(git rev-parse tag-1)" > $tag_dir_prefix/tag-1-garbage &&
    ++	printf "%s\n\n\n  garbage" "$(git rev-parse main)" >$tag_dir_prefix/tag-garbage-2 &&
     +	git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	warning: refs/tags/tag-1-garbage: trailingRefContent: trailing garbage in ref
    ++	warning: refs/tags/tag-garbage-2: trailingRefContent: trailing garbage in ref
     +	EOF
    -+	rm $tag_dir_prefix/tag-1-garbage &&
    ++	rm $tag_dir_prefix/tag-garbage-2 &&
     +	test_cmp expect err &&
     +
    -+	printf "%s    garbage\n\na" "$(git rev-parse tag-2)" > $tag_dir_prefix/tag-2-garbage &&
    ++	printf "%s    garbage\n\na" "$(git rev-parse main)" >$tag_dir_prefix/tag-garbage-3 &&
     +	git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	warning: refs/tags/tag-2-garbage: trailingRefContent: trailing garbage in ref
    ++	warning: refs/tags/tag-garbage-3: trailingRefContent: trailing garbage in ref
     +	EOF
    -+	rm $tag_dir_prefix/tag-2-garbage &&
    ++	rm $tag_dir_prefix/tag-garbage-3 &&
     +	test_cmp expect err &&
     +
    -+	printf "%s garbage" "$(git rev-parse tag-1)" > $tag_dir_prefix/tag-1-garbage &&
    ++	printf "%s garbage" "$(git rev-parse main)" >$tag_dir_prefix/tag-garbage-4 &&
     +	test_must_fail git -c fsck.trailingRefContent=error refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	error: refs/tags/tag-1-garbage: trailingRefContent: trailing garbage in ref
    ++	error: refs/tags/tag-garbage-4: trailingRefContent: trailing garbage in ref
     +	EOF
    -+	rm $tag_dir_prefix/tag-1-garbage &&
    ++	rm $tag_dir_prefix/tag-garbage-4 &&
     +	test_cmp expect err &&
     +
    -+	printf "%sx" "$(git rev-parse tag-1)" > $tag_dir_prefix/tag-1-bad &&
    ++	printf "%sx" "$(git rev-parse main)" >$tag_dir_prefix/tag-bad-1 &&
     +	test_must_fail git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	error: refs/tags/tag-1-bad: badRefContent: invalid ref content
    ++	error: refs/tags/tag-bad-1: badRefContent: invalid ref content
     +	EOF
    -+	rm $tag_dir_prefix/tag-1-bad &&
    ++	rm $tag_dir_prefix/tag-bad-1 &&
     +	test_cmp expect err &&
     +
    -+	printf "xfsazqfxcadas" > $tag_dir_prefix/tag-2-bad &&
    ++	printf "xfsazqfxcadas" >$tag_dir_prefix/tag-bad-2 &&
     +	test_must_fail git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	error: refs/tags/tag-2-bad: badRefContent: invalid ref content
    ++	error: refs/tags/tag-bad-2: badRefContent: invalid ref content
     +	EOF
    -+	rm $tag_dir_prefix/tag-2-bad &&
    ++	rm $tag_dir_prefix/tag-bad-2 &&
     +	test_cmp expect err &&
     +
    -+	printf "xfsazqfxcadas" > $branch_dir_prefix/a/b/branch-2-bad &&
    ++	printf "xfsazqfxcadas" >$branch_dir_prefix/a/b/branch-bad &&
     +	test_must_fail git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	error: refs/heads/a/b/branch-2-bad: badRefContent: invalid ref content
    ++	error: refs/heads/a/b/branch-bad: badRefContent: invalid ref content
     +	EOF
    -+	rm $branch_dir_prefix/a/b/branch-2-bad &&
    ++	rm $branch_dir_prefix/a/b/branch-bad &&
     +	test_cmp expect err
     +'
    ++
    ++test_expect_success 'regular ref content should be checked (aggregate)' '
    ++	test_when_finished "rm -rf repo" &&
    ++	git init repo &&
    ++	branch_dir_prefix=.git/refs/heads &&
    ++	tag_dir_prefix=.git/refs/tags &&
    ++	cd repo &&
    ++	test_commit default &&
    ++	mkdir -p "$branch_dir_prefix/a/b" &&
    ++
    ++	printf "%s" "$(git rev-parse main)" >$branch_dir_prefix/branch-no-newline &&
    ++	printf "%s garbage" "$(git rev-parse main)" >$branch_dir_prefix/branch-garbage &&
    ++	printf "%s\n\n\n" "$(git rev-parse main)" >$tag_dir_prefix/tag-garbage-1 &&
    ++	printf "%s\n\n\n  garbage" "$(git rev-parse main)" >$tag_dir_prefix/tag-garbage-2 &&
    ++	printf "%s    garbage\n\na" "$(git rev-parse main)" >$tag_dir_prefix/tag-garbage-3 &&
    ++	printf "%s garbage" "$(git rev-parse main)" >$tag_dir_prefix/tag-garbage-4 &&
    ++	printf "%sx" "$(git rev-parse main)" >$tag_dir_prefix/tag-bad-1 &&
    ++	printf "xfsazqfxcadas" >$tag_dir_prefix/tag-bad-2 &&
    ++	printf "xfsazqfxcadas" >$branch_dir_prefix/a/b/branch-bad &&
    ++
    ++	test_must_fail git refs verify 2>err &&
    ++	cat >expect <<-EOF &&
    ++	error: refs/heads/a/b/branch-bad: badRefContent: invalid ref content
    ++	error: refs/tags/tag-bad-1: badRefContent: invalid ref content
    ++	error: refs/tags/tag-bad-2: badRefContent: invalid ref content
    ++	warning: refs/heads/branch-garbage: trailingRefContent: trailing garbage in ref
    ++	warning: refs/heads/branch-no-newline: refMissingNewline: missing newline
    ++	warning: refs/tags/tag-garbage-1: trailingRefContent: trailing garbage in ref
    ++	warning: refs/tags/tag-garbage-2: trailingRefContent: trailing garbage in ref
    ++	warning: refs/tags/tag-garbage-3: trailingRefContent: trailing garbage in ref
    ++	warning: refs/tags/tag-garbage-4: trailingRefContent: trailing garbage in ref
    ++	EOF
    ++	sort err >sorted_err &&
    ++	test_cmp expect sorted_err
    ++'
     +
      test_done
3:  76dcf6bf58 ! 3:  0691e2960d ref: add symbolic ref content check for files backend
    @@ Metadata
     Author: shejialuo <[email protected]>
     
      ## Commit message ##
    -    ref: add symbolic ref content check for files backend
    +    ref: add symref content check for files backend
     
         We have already introduced the checks for regular refs. There is no need
    -    to check the consistency of the target which the symbolic ref points to.
    -    Instead, we just check the content of the symbolic ref itself.
    +    to check the consistency of the target which the symref points to.
    +    Instead, we just need to check the content of teh symref itself.
     
    -    In order to check the content of the symbolic ref, create a function
    -    "files_fsck_symref_target". It will first check whether the "pointee" is
    -    under the "refs/" directory and then we will check the "pointee" itself.
    +    In order to check the content of the symref, create a function
    +    "files_fsck_symref_target". It will first check whether the "referent"
    +    is under the "refs/" directory and then we will check the symref
    +    contents.
     
    -    There is no specification about the content of the symbolic ref.
    -    Although we do write "ref: %s\n" to create a symbolic ref by using
    -    "git-symbolic-ref(1)" command. However, this is not mandatory. We still
    -    accept symbolic refs with null trailing garbage. Put it more specific,
    -    the following are correct:
    +    A regular file is accepted as a textual symref if it begins with
    +    "ref:", followed by zero or more whitespaces, followed by the full
    +    refname, followed only by whitespace characters. We always write
    +    a single SP after "ref:" and a single LF after the refname, but
    +    third-party reimplementations of Git may have taken advantage of the
    +    looser syntax. Put it more specific, we accept the following contents
    +    of the symref:
     
         1. "ref: refs/heads/master   "
         2. "ref: refs/heads/master   \n  \n"
         3. "ref: refs/heads/master\n\n"
     
    -    But we do not allow any non-null trailing garbage. The following are bad
    -    symbolic contents which will be reported as fsck error by "git-fsck(1)".
    +    But we do not allow any other trailing garbage. The followings are bad
    +    symref contents which will be reported as fsck error by "git-fsck(1)".
     
         1. "ref: refs/heads/master garbage\n"
         2. "ref: refs/heads/master \n\n\n garbage  "
     
    -    In order to provide above checks, we will use "strrchr" to check whether
    -    we have newline in the ref content. Then we will check the name of the
    -    "pointee" is correct by using "check_refname_format". If the function
    -    fails, we need to trim the "pointee" to see whether the null-garbage
    -    causes the function fails. If so, we need to report that there is
    -    null-garbage in the symref content. Otherwise, we should report the user
    -    the "pointee" is bad.
    +    In order to provide above checks, we will first check whether the symref
    +    content misses the newline by peeking the last byte of the "referent" to
    +    see whether it is '\n'.
    +
    +    And we will remember the untrimmed length of the "referent" and call
    +    "strbuf_rtrim()" on "referent". Then, we will call "check_refname_format"
    +    to chceck whether the trimmed referent format is valid. If not, we will
    +    report to the user that the symref points to referent which has invalid
    +    format. If it is valid, we will compare the untrimmed length and trimmed
    +    length, if they are not the same, we need to warn the user there is some
    +    trailing garbage in the symref content.
    +
    +    At last, we need to check whether the referent is the directory. We
    +    cannot distinguish whether the "refs/heads/a" is a directory or not by
    +    using "check_refname_format". We have already checked bad file type when
    +    iterating the "refs/" directory but we ignore the directory. Thus, we
    +    need to explicitly add check here.
     
         Mentored-by: Patrick Steinhardt <[email protected]>
         Mentored-by: Karthik Nayak <[email protected]>
    @@ Documentation/fsck-msgids.txt
      `badRefName`::
      	(ERROR) A ref has an invalid format.
      
    -+`badSymrefPointee`::
    -+	(ERROR) The pointee of a symref is bad.
    ++`badSymrefTarget`::
    ++	(ERROR) The symref target points outside the ref directory or
    ++	the name of the symref target is invalid.
     +
      `badTagName`::
      	(INFO) A tag has an invalid format.
    @@ fsck.h: enum fsck_msg_type {
      	FUNC(BAD_REF_CONTENT, ERROR) \
      	FUNC(BAD_REF_FILETYPE, ERROR) \
      	FUNC(BAD_REF_NAME, ERROR) \
    -+	FUNC(BAD_SYMREF_POINTEE, ERROR) \
    ++	FUNC(BAD_SYMREF_TARGET, ERROR) \
      	FUNC(BAD_TIMEZONE, ERROR) \
      	FUNC(BAD_TREE, ERROR) \
      	FUNC(BAD_TREE_SHA1, ERROR) \
    @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
      				  struct dir_iterator *iter);
      
     +/*
    -+ * Check the symref "pointee_name" and "pointee_path". The caller should
    -+ * make sure that "pointee_path" is absolute. For symbolic ref, "pointee_name"
    -+ * would be the content after "refs:".
    ++ * Check the symref "referent" and "referent_path". For textual symref,
    ++ * "referent" would be the content after "refs:".
     + */
     +static int files_fsck_symref_target(struct fsck_options *o,
     +				    struct fsck_ref_report *report,
    -+				    const char *refname,
    -+				    struct strbuf *pointee_name,
    -+				    struct strbuf *pointee_path)
    ++				    struct strbuf *referent,
    ++				    struct strbuf *referent_path)
     +{
    -+	const char *newline_pos = NULL;
    ++	size_t len = referent->len - 1;
     +	const char *p = NULL;
     +	struct stat st;
     +	int ret = 0;
     +
    -+	if (!skip_prefix(pointee_name->buf, "refs/", &p)) {
    ++	if (!skip_prefix(referent->buf, "refs/", &p)) {
     +
     +		ret = fsck_report_ref(o, report,
    -+				      FSCK_MSG_BAD_SYMREF_POINTEE,
    ++				      FSCK_MSG_BAD_SYMREF_TARGET,
     +				      "points to ref outside the refs directory");
     +		goto out;
     +	}
     +
    -+	newline_pos = strrchr(p, '\n');
    -+	if (!newline_pos || *(newline_pos + 1)) {
    ++	if (referent->buf[referent->len - 1] != '\n') {
     +		ret = fsck_report_ref(o, report,
     +				      FSCK_MSG_REF_MISSING_NEWLINE,
     +				      "missing newline");
    ++		len++;
     +	}
     +
    -+	if (check_refname_format(pointee_name->buf, 0)) {
    -+		/*
    -+		 * When containing null-garbage, "check_refname_format" will
    -+		 * fail, we should trim the "pointee" to check again.
    -+		 */
    -+		strbuf_rtrim(pointee_name);
    -+		if (!check_refname_format(pointee_name->buf, 0)) {
    -+			ret = fsck_report_ref(o, report,
    -+					      FSCK_MSG_TRAILING_REF_CONTENT,
    -+					      "trailing null-garbage");
    -+			goto out;
    -+		}
    -+
    ++	strbuf_rtrim(referent);
    ++	if (check_refname_format(referent->buf, 0)) {
     +		ret = fsck_report_ref(o, report,
    -+				      FSCK_MSG_BAD_SYMREF_POINTEE,
    ++				      FSCK_MSG_BAD_SYMREF_TARGET,
     +				      "points to refname with invalid format");
    ++		goto out;
    ++	}
    ++
    ++	if (len != referent->len) {
    ++		ret = fsck_report_ref(o, report,
    ++				      FSCK_MSG_TRAILING_REF_CONTENT,
    ++				      "trailing garbage in ref");
     +	}
     +
     +	/*
    @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
     +	 * ref that does not exist yet. If the target ref does not exist, just
     +	 * skip the check for the file type.
     +	 */
    -+	if (lstat(pointee_path->buf, &st) < 0)
    ++	if (lstat(referent_path->buf, &st))
     +		goto out;
     +
    -+	if (!S_ISREG(st.st_mode) && !S_ISLNK(st.st_mode)) {
    ++	/*
    ++	 * We cannot distinguish whether "refs/heads/a" is directory or nots by
    ++	 * using "check_refname_format(referent->buf, 0)". Instead, we need to
    ++	 * check the file type of the target.
    ++	 */
    ++	if (S_ISDIR(st.st_mode)) {
     +		ret = fsck_report_ref(o, report,
    -+				      FSCK_MSG_BAD_SYMREF_POINTEE,
    -+				      "points to an invalid file type");
    ++				      FSCK_MSG_BAD_SYMREF_TARGET,
    ++				      "points to the directory");
     +		goto out;
     +	}
     +
    @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
      				   const char *refs_check_dir,
      				   struct dir_iterator *iter)
      {
    -+	struct strbuf pointee_path = STRBUF_INIT;
    ++	struct strbuf referent_path = STRBUF_INIT;
      	struct strbuf ref_content = STRBUF_INIT;
      	struct strbuf referent = STRBUF_INIT;
      	struct strbuf refname = STRBUF_INIT;
     @@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_store,
    - 						      "trailing garbage in ref");
    - 				goto cleanup;
    - 			}
    -+		} else {
    -+			strbuf_addf(&pointee_path, "%s/%s",
    -+				    ref_store->gitdir, referent.buf);
    -+			ret = files_fsck_symref_target(o, &report, refname.buf,
    -+						       &referent,
    -+						       &pointee_path);
    + 					      "trailing garbage in ref");
    + 			goto cleanup;
      		}
    - 		goto cleanup;
    ++	} else {
    ++		strbuf_addf(&referent_path, "%s/%s",
    ++			    ref_store->gitdir, referent.buf);
    ++		/*
    ++		 * the referent may contain the spaces and the newline, need to
    ++		 * trim for path.
    ++		 */
    ++		strbuf_rtrim(&referent_path);
    ++		ret = files_fsck_symref_target(o, &report,
    ++					       &referent,
    ++					       &referent_path);
      	}
    -@@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_store,
    + 
    + cleanup:
      	strbuf_release(&refname);
      	strbuf_release(&ref_content);
      	strbuf_release(&referent);
    -+	strbuf_release(&pointee_path);
    ++	strbuf_release(&referent_path);
      	return ret;
      }
      
     
      ## t/t0602-reffiles-fsck.sh ##
    -@@ t/t0602-reffiles-fsck.sh: test_expect_success 'regular ref content should be checked' '
    - 	test_cmp expect err
    +@@ t/t0602-reffiles-fsck.sh: test_expect_success 'regular ref content should be checked (aggregate)' '
    + 	test_cmp expect sorted_err
      '
      
    -+test_expect_success 'symbolic ref content should be checked' '
    ++test_expect_success 'textual symref content should be checked (individual)' '
     +	test_when_finished "rm -rf repo" &&
     +	git init repo &&
     +	branch_dir_prefix=.git/refs/heads &&
     +	tag_dir_prefix=.git/refs/tags &&
     +	cd repo &&
    -+	git commit --allow-empty -m initial &&
    -+	git checkout -b branch-1 &&
    -+	git tag tag-1 &&
    -+	git checkout -b a/b/branch-2 &&
    ++	test_commit default &&
    ++	mkdir -p "$branch_dir_prefix/a/b" &&
    ++
    ++	printf "ref: refs/heads/branch\n" >$branch_dir_prefix/branch-good &&
    ++	git refs verify 2>err &&
    ++	rm $branch_dir_prefix/branch-good &&
    ++	test_must_be_empty err &&
    ++
    ++	printf "ref: refs/heads/branch" >$branch_dir_prefix/branch-no-newline-1 &&
    ++	git refs verify 2>err &&
    ++	cat >expect <<-EOF &&
    ++	warning: refs/heads/branch-no-newline-1: refMissingNewline: missing newline
    ++	EOF
    ++	rm $branch_dir_prefix/branch-no-newline-1 &&
    ++	test_cmp expect err &&
     +
    -+	printf "ref: refs/heads/branch" > $branch_dir_prefix/branch-1-no-newline &&
    ++	printf "ref: refs/heads/branch     " >$branch_dir_prefix/a/b/branch-trailing-1 &&
     +	git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	warning: refs/heads/branch-1-no-newline: refMissingNewline: missing newline
    ++	warning: refs/heads/a/b/branch-trailing-1: refMissingNewline: missing newline
    ++	warning: refs/heads/a/b/branch-trailing-1: trailingRefContent: trailing garbage in ref
     +	EOF
    -+	rm $branch_dir_prefix/branch-1-no-newline &&
    ++	rm $branch_dir_prefix/a/b/branch-trailing-1 &&
     +	test_cmp expect err &&
     +
    -+	printf "ref: refs/heads/branch     " > $branch_dir_prefix/a/b/branch-trailing &&
    ++	printf "ref: refs/heads/branch\n\n" >$branch_dir_prefix/a/b/branch-trailing-2 &&
     +	git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	warning: refs/heads/a/b/branch-trailing: refMissingNewline: missing newline
    -+	warning: refs/heads/a/b/branch-trailing: trailingRefContent: trailing null-garbage
    ++	warning: refs/heads/a/b/branch-trailing-2: trailingRefContent: trailing garbage in ref
     +	EOF
    -+	rm $branch_dir_prefix/a/b/branch-trailing &&
    ++	rm $branch_dir_prefix/a/b/branch-trailing-2 &&
     +	test_cmp expect err &&
     +
    -+	printf "ref: refs/heads/branch\n\n" > $branch_dir_prefix/a/b/branch-trailing &&
    ++	printf "ref: refs/heads/branch \n" >$branch_dir_prefix/a/b/branch-trailing-3 &&
     +	git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	warning: refs/heads/a/b/branch-trailing: trailingRefContent: trailing null-garbage
    ++	warning: refs/heads/a/b/branch-trailing-3: trailingRefContent: trailing garbage in ref
     +	EOF
    -+	rm $branch_dir_prefix/a/b/branch-trailing &&
    ++	rm $branch_dir_prefix/a/b/branch-trailing-3 &&
     +	test_cmp expect err &&
     +
    -+	printf "ref: refs/heads/branch \n\n " > $branch_dir_prefix/a/b/branch-trailing &&
    ++	printf "ref: refs/heads/branch \n  " >$branch_dir_prefix/a/b/branch-complicated &&
     +	git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	warning: refs/heads/a/b/branch-trailing: refMissingNewline: missing newline
    -+	warning: refs/heads/a/b/branch-trailing: trailingRefContent: trailing null-garbage
    ++	warning: refs/heads/a/b/branch-complicated: refMissingNewline: missing newline
    ++	warning: refs/heads/a/b/branch-complicated: trailingRefContent: trailing garbage in ref
     +	EOF
    -+	rm $branch_dir_prefix/a/b/branch-trailing &&
    ++	rm $branch_dir_prefix/a/b/branch-complicated &&
     +	test_cmp expect err &&
     +
    -+	printf "ref: refs/heads/.branch\n" > $branch_dir_prefix/branch-2-bad &&
    ++	printf "ref: refs/heads/.branch\n" >$branch_dir_prefix/branch-bad-1 &&
     +	test_must_fail git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	error: refs/heads/branch-2-bad: badSymrefPointee: points to refname with invalid format
    ++	error: refs/heads/branch-bad-1: badSymrefTarget: points to refname with invalid format
     +	EOF
    -+	rm $branch_dir_prefix/branch-2-bad &&
    ++	rm $branch_dir_prefix/branch-bad-1 &&
    ++	test_cmp expect err &&
    ++
    ++	printf "ref: reflogs/heads/main\n" >$branch_dir_prefix/branch-bad-2 &&
    ++	test_must_fail git refs verify 2>err &&
    ++	cat >expect <<-EOF &&
    ++	error: refs/heads/branch-bad-2: badSymrefTarget: points to ref outside the refs directory
    ++	EOF
    ++	rm $branch_dir_prefix/branch-bad-2 &&
    ++	test_cmp expect err &&
    ++
    ++	printf "ref: refs/heads/a\n" >$branch_dir_prefix/branch-bad-3 &&
    ++	test_must_fail git refs verify 2>err &&
    ++	cat >expect <<-EOF &&
    ++	error: refs/heads/branch-bad-3: badSymrefTarget: points to the directory
    ++	EOF
    ++	rm $branch_dir_prefix/branch-bad-3 &&
     +	test_cmp expect err
     +'
    ++
    ++test_expect_success 'textual symref content should be checked (aggregate)' '
    ++	test_when_finished "rm -rf repo" &&
    ++	git init repo &&
    ++	branch_dir_prefix=.git/refs/heads &&
    ++	tag_dir_prefix=.git/refs/tags &&
    ++	cd repo &&
    ++	test_commit default &&
    ++	mkdir -p "$branch_dir_prefix/a/b" &&
    ++
    ++	printf "ref: refs/heads/branch\n" >$branch_dir_prefix/branch-good &&
    ++	printf "ref: refs/heads/branch" >$branch_dir_prefix/branch-no-newline-1 &&
    ++	printf "ref: refs/heads/branch     " >$branch_dir_prefix/a/b/branch-trailing-1 &&
    ++	printf "ref: refs/heads/branch\n\n" >$branch_dir_prefix/a/b/branch-trailing-2 &&
    ++	printf "ref: refs/heads/branch \n" >$branch_dir_prefix/a/b/branch-trailing-3 &&
    ++	printf "ref: refs/heads/branch \n  " >$branch_dir_prefix/a/b/branch-complicated &&
    ++	printf "ref: refs/heads/.branch\n" >$branch_dir_prefix/branch-bad-1 &&
    ++	printf "ref: reflogs/heads/main\n" >$branch_dir_prefix/branch-bad-2 &&
    ++	printf "ref: refs/heads/a\n" >$branch_dir_prefix/branch-bad-3 &&
    ++
    ++	test_must_fail git refs verify 2>err &&
    ++	cat >expect <<-EOF &&
    ++	error: refs/heads/branch-bad-1: badSymrefTarget: points to refname with invalid format
    ++	error: refs/heads/branch-bad-2: badSymrefTarget: points to ref outside the refs directory
    ++	error: refs/heads/branch-bad-3: badSymrefTarget: points to the directory
    ++	warning: refs/heads/a/b/branch-complicated: refMissingNewline: missing newline
    ++	warning: refs/heads/a/b/branch-complicated: trailingRefContent: trailing garbage in ref
    ++	warning: refs/heads/a/b/branch-trailing-1: refMissingNewline: missing newline
    ++	warning: refs/heads/a/b/branch-trailing-1: trailingRefContent: trailing garbage in ref
    ++	warning: refs/heads/a/b/branch-trailing-2: trailingRefContent: trailing garbage in ref
    ++	warning: refs/heads/a/b/branch-trailing-3: trailingRefContent: trailing garbage in ref
    ++	warning: refs/heads/branch-no-newline-1: refMissingNewline: missing newline
    ++	EOF
    ++	sort err >sorted_err &&
    ++	test_cmp expect sorted_err
    ++'
     +
      test_done
4:  2008f8635c ! 4:  4105bfa1e3 ref: add symlink ref check for files backend
    @@ Metadata
     Author: shejialuo <[email protected]>
     
      ## Commit message ##
    -    ref: add symlink ref check for files backend
    +    ref: add symlink ref content check for files backend
     
         We have already introduced "files_fsck_symref_target". We should reuse
    -    this function to handle the symrefs which are legacy symbolic links. We
    -    should not check the trailing garbage for symbolic links. Add a new
    +    this function to handle the symrefs which use legacy symbolic links. We
    +    should not check the trailing garbage for symbolic refs. Add a new
         parameter "symbolic_link" to disable some checks which should only be
    -    used for symbolic ref.
    +    executed for textual symrefs.
     
    -    We firstly use the "strbuf_add_real_path" to resolve the symlinks and
    -    get the absolute path "pointee_path" which the symlink ref points to.
    -    Then we can get the absolute path "abs_gitdir" of the "gitdir". By
    -    combining "pointee_path" and "abs_gitdir", we can extract the
    +    We firstly use the "strbuf_add_real_path" to resolve the symlink and
    +    get the absolute path "referent_path" which the symlink ref points
    +    to. Then we can get the absolute path "abs_gitdir" of the "gitdir".
    +    By combining "referent_path" and "abs_gitdir", we can extract the
         "referent". Thus, we can reuse "files_fsck_symref_target" function to
         seamlessly check the symlink refs.
     
    +    Because we are going to drop support for "core.prefersymlinkrefs", add a
    +    new fsck message "symlinkRef" to let the user be aware of this
    +    information.
    +
         Mentored-by: Patrick Steinhardt <[email protected]>
         Mentored-by: Karthik Nayak <[email protected]>
         Signed-off-by: shejialuo <[email protected]>
     
    + ## Documentation/fsck-msgids.txt ##
    +@@
    + 	(INFO) A ref does not end with newline. This kind of ref may
    + 	be considered ERROR in the future.
    + 
    ++`symlinkRef`::
    ++	(INFO) A symref uses the symbolic link. This kind of symref may
    ++	be considered ERROR in the future when totally dropping the
    ++	symlink support.
    ++
    + `trailingRefContent`::
    + 	(INFO) A ref has trailing contents. This kind of ref may be
    + 	considered ERROR in the future.
    +
    + ## fsck.h ##
    +@@ fsck.h: enum fsck_msg_type {
    + 	FUNC(BAD_TAG_NAME, INFO) \
    + 	FUNC(MISSING_TAGGER_ENTRY, INFO) \
    + 	FUNC(REF_MISSING_NEWLINE, INFO) \
    ++	FUNC(SYMLINK_REF, INFO) \
    + 	FUNC(TRAILING_REF_CONTENT, INFO) \
    + 	/* ignored (elevated when requested) */ \
    + 	FUNC(EXTRA_HEADER_ENTRY, IGNORE)
    +
      ## refs/files-backend.c ##
     @@
      #include "../git-compat-util.h"
    @@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *ref
      			goto out;
      		}
     @@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
    + 
      /*
    -  * Check the symref "pointee_name" and "pointee_path". The caller should
    -  * make sure that "pointee_path" is absolute. For symbolic ref, "pointee_name"
    -- * would be the content after "refs:".
    -+ * would be the content after "refs:". For symblic link, "pointee_name" would
    -+ * be the relative path agaignst "gitdir".
    +  * Check the symref "referent" and "referent_path". For textual symref,
    +- * "referent" would be the content after "refs:".
    ++ * "referent" would be the content after "refs:". For symlink ref,
    ++ * "referent" would be the relative path agaignst "gitdir" which should
    ++ * be the same as the textual symref literally.
       */
      static int files_fsck_symref_target(struct fsck_options *o,
      				    struct fsck_ref_report *report,
    --				    const char *refname,
    - 				    struct strbuf *pointee_name,
    --				    struct strbuf *pointee_path)
    -+				    struct strbuf *pointee_path,
    + 				    struct strbuf *referent,
    +-				    struct strbuf *referent_path)
    ++				    struct strbuf *referent_path,
     +				    unsigned int symbolic_link)
      {
    - 	const char *newline_pos = NULL;
    + 	size_t len = referent->len - 1;
      	const char *p = NULL;
     @@ refs/files-backend.c: static int files_fsck_symref_target(struct fsck_options *o,
      		goto out;
      	}
      
    --	newline_pos = strrchr(p, '\n');
    --	if (!newline_pos || *(newline_pos + 1)) {
    --		ret = fsck_report_ref(o, report,
    --				      FSCK_MSG_REF_MISSING_NEWLINE,
    --				      "missing newline");
    -+	if (!symbolic_link) {
    -+		newline_pos = strrchr(p, '\n');
    -+		if (!newline_pos || *(newline_pos + 1)) {
    -+			ret = fsck_report_ref(o, report,
    -+					      FSCK_MSG_REF_MISSING_NEWLINE,
    -+					      "missing newline");
    -+		}
    +-	if (referent->buf[referent->len - 1] != '\n') {
    ++	if (!symbolic_link && referent->buf[referent->len - 1] != '\n') {
    + 		ret = fsck_report_ref(o, report,
    + 				      FSCK_MSG_REF_MISSING_NEWLINE,
    + 				      "missing newline");
    + 		len++;
      	}
      
    - 	if (check_refname_format(pointee_name->buf, 0)) {
    --		/*
    --		 * When containing null-garbage, "check_refname_format" will
    --		 * fail, we should trim the "pointee" to check again.
    --		 */
    --		strbuf_rtrim(pointee_name);
    --		if (!check_refname_format(pointee_name->buf, 0)) {
    --			ret = fsck_report_ref(o, report,
    --					      FSCK_MSG_TRAILING_REF_CONTENT,
    --					      "trailing null-garbage");
    --			goto out;
    -+		if (!symbolic_link) {
    -+			/*
    -+			* When containing null-garbage, "check_refname_format" will
    -+			* fail, we should trim the "pointee" to check again.
    -+			*/
    -+			strbuf_rtrim(pointee_name);
    -+			if (!check_refname_format(pointee_name->buf, 0)) {
    -+				ret = fsck_report_ref(o, report,
    -+						      FSCK_MSG_TRAILING_REF_CONTENT,
    -+						      "trailing null-garbage");
    -+				goto out;
    -+			}
    - 		}
    +-	strbuf_rtrim(referent);
    ++	if (!symbolic_link)
    ++		strbuf_rtrim(referent);
    ++
    + 	if (check_refname_format(referent->buf, 0)) {
    + 		ret = fsck_report_ref(o, report,
    + 				      FSCK_MSG_BAD_SYMREF_TARGET,
    +@@ refs/files-backend.c: static int files_fsck_symref_target(struct fsck_options *o,
    + 		goto out;
    + 	}
      
    +-	if (len != referent->len) {
    ++	if (!symbolic_link && len != referent->len) {
      		ret = fsck_report_ref(o, report,
    + 				      FSCK_MSG_TRAILING_REF_CONTENT,
    + 				      "trailing garbage in ref");
     @@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_store,
      {
    - 	struct strbuf pointee_path = STRBUF_INIT;
    + 	struct strbuf referent_path = STRBUF_INIT;
      	struct strbuf ref_content = STRBUF_INIT;
     +	struct strbuf abs_gitdir = STRBUF_INIT;
      	struct strbuf referent = STRBUF_INIT;
      	struct strbuf refname = STRBUF_INIT;
      	struct fsck_ref_report report = {0};
    -+	const char *pointee_name = NULL;
     +	unsigned int symbolic_link = 0;
      	const char *trailing = NULL;
      	unsigned int type = 0;
      	int failure_errno = 0;
     @@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_store,
    - 		} else {
    - 			strbuf_addf(&pointee_path, "%s/%s",
    - 				    ref_store->gitdir, referent.buf);
    --			ret = files_fsck_symref_target(o, &report, refname.buf,
    -+			ret = files_fsck_symref_target(o, &report,
    - 						       &referent,
    --						       &pointee_path);
    -+						       &pointee_path,
    -+						       symbolic_link);
    - 		}
    - 		goto cleanup;
    - 	}
    + 	strbuf_addf(&refname, "%s/%s", refs_check_dir, iter->relative_path);
    + 	report.path = refname.buf;
      
    -+	symbolic_link = 1;
    -+
    -+	strbuf_add_real_path(&pointee_path, iter->path.buf);
    -+	strbuf_add_absolute_path(&abs_gitdir, ref_store->gitdir);
    -+	strbuf_normalize_path(&abs_gitdir);
    -+	if (!is_dir_sep(abs_gitdir.buf[abs_gitdir.len - 1]))
    -+		strbuf_addch(&abs_gitdir, '/');
    +-	if (S_ISLNK(iter->st.st_mode))
    ++	if (S_ISLNK(iter->st.st_mode)) {
    ++		const char* relative_referent_path;
     +
    -+	if (!skip_prefix(pointee_path.buf, abs_gitdir.buf, &pointee_name)) {
    ++		symbolic_link = 1;
     +		ret = fsck_report_ref(o, &report,
    -+				      FSCK_MSG_BAD_SYMREF_POINTEE,
    -+				      "point to target outside gitdir");
    -+		goto cleanup;
    -+	}
    ++				      FSCK_MSG_SYMLINK_REF,
    ++				      "use deprecated symbolic link for symref");
     +
    -+	strbuf_addstr(&referent, pointee_name);
    -+	ret = files_fsck_symref_target(o, &report,
    -+				       &referent, &pointee_path,
    -+				       symbolic_link);
    ++		strbuf_add_absolute_path(&abs_gitdir, ref_store->gitdir);
    ++		strbuf_normalize_path(&abs_gitdir);
    ++		if (!is_dir_sep(abs_gitdir.buf[abs_gitdir.len - 1]))
    ++			strbuf_addch(&abs_gitdir, '/');
     +
    ++		strbuf_add_real_path(&referent_path, iter->path.buf);
    ++
    ++		if (!skip_prefix(referent_path.buf,
    ++				 abs_gitdir.buf,
    ++				 &relative_referent_path)) {
    ++			ret = fsck_report_ref(o, &report,
    ++					      FSCK_MSG_BAD_SYMREF_TARGET,
    ++					      "point to target outside gitdir");
    ++			goto cleanup;
    ++		}
    ++
    ++		strbuf_addstr(&referent, relative_referent_path);
    ++		ret = files_fsck_symref_target(o, &report,
    ++					       &referent, &referent_path,
    ++					       symbolic_link);
    ++
    + 		goto cleanup;
    ++	}
    + 
    + 	if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) {
    + 		ret = error_errno(_("%s/%s: unable to read the ref"),
    +@@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_store,
    + 		strbuf_rtrim(&referent_path);
    + 		ret = files_fsck_symref_target(o, &report,
    + 					       &referent,
    +-					       &referent_path);
    ++					       &referent_path,
    ++					       symbolic_link);
    + 	}
    + 
      cleanup:
    - 	strbuf_release(&refname);
    +@@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_store,
      	strbuf_release(&ref_content);
      	strbuf_release(&referent);
    - 	strbuf_release(&pointee_path);
    + 	strbuf_release(&referent_path);
     +	strbuf_release(&abs_gitdir);
      	return ret;
      }
      
     
      ## t/t0602-reffiles-fsck.sh ##
    -@@ t/t0602-reffiles-fsck.sh: test_expect_success 'symbolic ref content should be checked' '
    - 	test_cmp expect err
    +@@ t/t0602-reffiles-fsck.sh: test_expect_success 'textual symref content should be checked (aggregate)' '
    + 	test_cmp expect sorted_err
      '
      
    -+test_expect_success SYMLINKS 'symbolic ref (symbolic link) content should be checked' '
    ++test_expect_success SYMLINKS 'symlink symref content should be checked (individual)' '
     +	test_when_finished "rm -rf repo" &&
     +	git init repo &&
     +	branch_dir_prefix=.git/refs/heads &&
     +	tag_dir_prefix=.git/refs/tags &&
     +	cd repo &&
    -+	git commit --allow-empty -m initial &&
    -+	git checkout -b branch-1 &&
    -+	git tag tag-1 &&
    -+	git checkout -b a/b/branch-2 &&
    ++	test_commit default &&
    ++	mkdir -p "$branch_dir_prefix/a/b" &&
    ++
    ++	ln -sf ./main $branch_dir_prefix/branch-symbolic-good &&
    ++	git refs verify 2>err &&
    ++	cat >expect <<-EOF &&
    ++	warning: refs/heads/branch-symbolic-good: symlinkRef: use deprecated symbolic link for symref
    ++	EOF
    ++	rm $branch_dir_prefix/branch-symbolic-good &&
    ++	test_cmp expect err &&
    ++
    ++	ln -sf ../../../../branch $branch_dir_prefix/branch-symbolic-1 &&
    ++	test_must_fail git refs verify 2>err &&
    ++	cat >expect <<-EOF &&
    ++	warning: refs/heads/branch-symbolic-1: symlinkRef: use deprecated symbolic link for symref
    ++	error: refs/heads/branch-symbolic-1: badSymrefTarget: point to target outside gitdir
    ++	EOF
    ++	rm $branch_dir_prefix/branch-symbolic-1 &&
    ++	test_cmp expect err &&
     +
    -+	ln -sf ../../../../branch $branch_dir_prefix/branch-symbolic &&
    ++	ln -sf ../../logs/branch-bad $branch_dir_prefix/branch-symbolic-2 &&
     +	test_must_fail git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	error: refs/heads/branch-symbolic: badSymrefPointee: point to target outside gitdir
    ++	warning: refs/heads/branch-symbolic-2: symlinkRef: use deprecated symbolic link for symref
    ++	error: refs/heads/branch-symbolic-2: badSymrefTarget: points to ref outside the refs directory
     +	EOF
    -+	rm $branch_dir_prefix/branch-symbolic &&
    ++	rm $branch_dir_prefix/branch-symbolic-2 &&
     +	test_cmp expect err &&
     +
    -+	ln -sf ../../logs/branch-bad $branch_dir_prefix/branch-symbolic &&
    ++	ln -sf ./"branch   space" $branch_dir_prefix/branch-symbolic-3 &&
     +	test_must_fail git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	error: refs/heads/branch-symbolic: badSymrefPointee: points to ref outside the refs directory
    ++	warning: refs/heads/branch-symbolic-3: symlinkRef: use deprecated symbolic link for symref
    ++	error: refs/heads/branch-symbolic-3: badSymrefTarget: points to refname with invalid format
     +	EOF
    -+	rm $branch_dir_prefix/branch-symbolic &&
    ++	rm $branch_dir_prefix/branch-symbolic-3 &&
     +	test_cmp expect err &&
     +
    -+	ln -sf ./"branch   space" $branch_dir_prefix/branch-symbolic &&
    ++	ln -sf ./".tag" $tag_dir_prefix/tag-symbolic-1 &&
     +	test_must_fail git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	error: refs/heads/branch-symbolic: badSymrefPointee: points to refname with invalid format
    ++	warning: refs/tags/tag-symbolic-1: symlinkRef: use deprecated symbolic link for symref
    ++	error: refs/tags/tag-symbolic-1: badSymrefTarget: points to refname with invalid format
     +	EOF
    -+	rm $branch_dir_prefix/branch-symbolic &&
    ++	rm $tag_dir_prefix/tag-symbolic-1 &&
     +	test_cmp expect err &&
     +
    -+	ln -sf ./".branch" $branch_dir_prefix/branch-symbolic &&
    ++	ln -sf ./ $tag_dir_prefix/tag-symbolic-2 &&
     +	test_must_fail git refs verify 2>err &&
     +	cat >expect <<-EOF &&
    -+	error: refs/heads/branch-symbolic: badSymrefPointee: points to refname with invalid format
    ++	warning: refs/tags/tag-symbolic-2: symlinkRef: use deprecated symbolic link for symref
    ++	error: refs/tags/tag-symbolic-2: badSymrefTarget: points to the directory
     +	EOF
    -+	rm $branch_dir_prefix/branch-symbolic &&
    ++	rm $tag_dir_prefix/tag-symbolic-2 &&
     +	test_cmp expect err
     +'
    ++
    ++test_expect_success SYMLINKS 'symlink symref content should be checked (aggregate)' '
    ++	test_when_finished "rm -rf repo" &&
    ++	git init repo &&
    ++	branch_dir_prefix=.git/refs/heads &&
    ++	tag_dir_prefix=.git/refs/tags &&
    ++	cd repo &&
    ++	test_commit default &&
    ++	mkdir -p "$branch_dir_prefix/a/b" &&
    ++
    ++	ln -sf ./main $branch_dir_prefix/branch-symbolic-good &&
    ++	ln -sf ../../../../branch $branch_dir_prefix/branch-symbolic-1 &&
    ++	ln -sf ../../logs/branch-bad $branch_dir_prefix/branch-symbolic-2 &&
    ++	ln -sf ./"branch   space" $branch_dir_prefix/branch-symbolic-3 &&
    ++	ln -sf ./".tag" $tag_dir_prefix/tag-symbolic-1 &&
    ++	ln -sf ./ $tag_dir_prefix/tag-symbolic-2 &&
    ++
    ++	test_must_fail git refs verify 2>err &&
    ++	cat >expect <<-EOF &&
    ++	error: refs/heads/branch-symbolic-1: badSymrefTarget: point to target outside gitdir
    ++	error: refs/heads/branch-symbolic-2: badSymrefTarget: points to ref outside the refs directory
    ++	error: refs/heads/branch-symbolic-3: badSymrefTarget: points to refname with invalid format
    ++	error: refs/tags/tag-symbolic-1: badSymrefTarget: points to refname with invalid format
    ++	error: refs/tags/tag-symbolic-2: badSymrefTarget: points to the directory
    ++	warning: refs/heads/branch-symbolic-1: symlinkRef: use deprecated symbolic link for symref
    ++	warning: refs/heads/branch-symbolic-2: symlinkRef: use deprecated symbolic link for symref
    ++	warning: refs/heads/branch-symbolic-3: symlinkRef: use deprecated symbolic link for symref
    ++	warning: refs/heads/branch-symbolic-good: symlinkRef: use deprecated symbolic link for symref
    ++	warning: refs/tags/tag-symbolic-1: symlinkRef: use deprecated symbolic link for symref
    ++	warning: refs/tags/tag-symbolic-2: symlinkRef: use deprecated symbolic link for symref
    ++	EOF
    ++	sort err >sorted_err &&
    ++	test_cmp expect sorted_err
    ++'
     +
      test_done

2.46.0

In "fsck.c::fsck_refs_error_function", we need to tell whether "oid" and "referent" is NULL. So, we need to always initialize these parameters to NULL instead of letting them point to anywhere when creating a new "fsck_ref_report" structure.

The original code explicitly initializes the "path" member in the "struct fsck_ref_report" to NULL (which implicitly 0-initializes other members in the struct). It is more customary to use " {0} " to express that we are 0-initializing everything. In order to be align with the the codebase, initialize "fsck_ref_report" with zero.

---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8d6ec9458d..890d0324e1 100644
@@ -3446,7 +3446,7 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
 		goto cleanup;
 
 	if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {
-		struct fsck_ref_report report = { .path = NULL };
+		struct fsck_ref_report report = { 0 };
 
 		strbuf_addf(&sb, "%s/%s", refs_check_dir, iter->relative_path);
 		report.path = sb.buf;

2.46.0

We implicitly rely on "git-fsck(1)" to check the consistency of regular refs. However, when parsing the regular refs for files backend by using "files-backend.c::parse_loose_ref_contents", we allow the ref content to end with no newline or to contain some garbages.

Even though we never create such loose refs ourselves, we have accepted such loose refs. So, it is entirely possible that some third-party tools may rely on such loose refs being valid. We should not report an error fsck message at current. But let's notice such a "curiously formatted" loose refs being valid and tell the user our findings, so we can access the possible extent of damage when we tighten the parsing rules in the future.

And it's not suitable to either report a warn fsck message to the user. This is because if the caller set the "strict" field in "fsck_options", fsck warns will be automatically upgraded to errors. We should not allow user to specify the "--strict" flag to upgrade the fsck warnings to errors at current. It might cause compatibility issue which may break the legacy repository. So we add the following two fsck infos to represent the situation where the ref content ends without newline or has garbages:

1. "refMissingNewline(INFO)": A ref does not end with newline. This kind of ref may be considered ERROR in the future.
2. "trailingRefContent(INFO)": A ref has trailing contents. This kind of ref may be considered ERROR in the future.

It may seem that we could not give the user any warnings by creating fsck infos. However, in "fsck.c::fsck_vreport", we will convert "FSCK_INFO" to "FSCK_WARN" and we can still warn the user about these situations when using "git-refs verify" without introducing compatibility issue.

In current "git-fsck(1)", it will report an error when the ref content is bad, so we should following this to report an error to the user when "parse_loose_ref_contents" fails. And we add a new fsck error message called "badRefContent(ERROR)" to represent that a ref has a bad content.

In order to tell whether the ref has trailing content, add a new parameter "trailing" to "parse_loose_ref_contents". Then introduce a new function "files_fsck_refs_content" to check the regular refs to enhance the "git-refs verify".

---
 Documentation/fsck-msgids.txt |  11 ++++
 fsck.h                        |   3 +
 refs.c                        |   2 +-
 refs/files-backend.c          |  68 ++++++++++++++++++-
 refs/refs-internal.h          |   2 +-
 t/t0602-reffiles-fsck.sh      | 120 ++++++++++++++++++++++++++++++++++
 6 files changed, 202 insertions(+), 4 deletions(-)

index 68a2801f15..06d045ac48 100644
@@ -19,6 +19,9 @@
 `badParentSha1`::
 	(ERROR) A commit object has a bad parent sha1.
 
+`badRefContent`::
+	(ERROR) A ref has a bad content.
+
 `badRefFiletype`::
 	(ERROR) A ref has a bad file type.
 

@@ -170,6 +173,14 @@
 `nullSha1`::
 	(WARN) Tree contains entries pointing to a null sha1.
 
+`refMissingNewline`::
+	(INFO) A ref does not end with newline. This kind of ref may
+	be considered ERROR in the future.
+
+`trailingRefContent`::
+	(INFO) A ref has trailing contents. This kind of ref may be
+	considered ERROR in the future.
+
 `treeNotSorted`::
 	(ERROR) A tree is not properly sorted.
 

index 500b4c04d2..b85072df57 100644
@@ -31,6 +31,7 @@ enum fsck_msg_type {
 	FUNC(BAD_NAME, ERROR) \
 	FUNC(BAD_OBJECT_SHA1, ERROR) \
 	FUNC(BAD_PARENT_SHA1, ERROR) \
+	FUNC(BAD_REF_CONTENT, ERROR) \
 	FUNC(BAD_REF_FILETYPE, ERROR) \
 	FUNC(BAD_REF_NAME, ERROR) \
 	FUNC(BAD_TIMEZONE, ERROR) \

@@ -84,6 +85,8 @@ enum fsck_msg_type {
 	FUNC(MAILMAP_SYMLINK, INFO) \
 	FUNC(BAD_TAG_NAME, INFO) \
 	FUNC(MISSING_TAGGER_ENTRY, INFO) \
+	FUNC(REF_MISSING_NEWLINE, INFO) \
+	FUNC(TRAILING_REF_CONTENT, INFO) \
 	/* ignored (elevated when requested) */ \
 	FUNC(EXTRA_HEADER_ENTRY, IGNORE)
 

index 74de3d3009..5e74881945 100644
@@ -1758,7 +1758,7 @@ static int refs_read_special_head(struct ref_store *ref_store,
 	}
 
 	result = parse_loose_ref_contents(ref_store->repo->hash_algo, content.buf,
-					  oid, referent, type, failure_errno);
+					  oid, referent, type, NULL, failure_errno);
 
 done:
 	strbuf_release(&full_path);

index 2313c830d8..73b05f971b 100644
@@ -715,7 +715,7 @@ struct ref_store {
 int parse_loose_ref_contents(const struct git_hash_algo *algop,
 			     const char *buf, struct object_id *oid,
 			     struct strbuf *referent, unsigned int *type,
-			     int *failure_errno);
+			     const char **trailing, int *failure_errno);
 
 /*
  * Fill in the generic part of refs and add it to our collection of

2.46.0

s/to either/either to

This is formulated a bit curiously: it reads as if we wanted to limit what the user can do, but what we really want to ensure is that the --strict flag doesn't convert it into an error. So maybe something like this instead of the second sentence:

We don't (yet) want the "--strict" flag that controls this bit to end up generating errors for such weirdly-formatted reference contents, as we first want to assess whether this retroactive tightening will cause issues for any tools out there.

s/issue/issues

I wouldn't call it "legacy" just yet, as we didn't yet decide whether we're going to make this formatting invalid in the first place. It's rather a test balloon.

s/garbages/trailing garbage

In both cases, "may be considered ERROR" -> "may be considered an error". Also in the actual messages.

s/"git-refs verify"/"git refs verify". We don't use dashed builtins nowadays anymore.

s/issue/issues

Okay, so this is basically porting over behaviour that git-fsck(1) already has to git refs verify and should thus not cause new issues anywhere. I think it would have made sense to do so in a first step and then introduce the tightened rules in a separate commit.

Will we eventually remove those checks from git-fsck(1) when we adapt it to call git refs verify? If so, we should likely note that in the commit message.

This paragraph only re-explains what the diff already tells us, so it can likely be removed.

Thanks, I will improve this in the next version.

I agree, we should drop "legacy" here.

By reading the whole comments, we'd better create a commit which ports the existing checks to "git refs verify" both for regular refs and symrefs.

So, I will add more commits in the next version with the following sequences:

1. Set up the infrastructure to check the contents for refs.
2. Port existing checks in "git-fsck(1)" to "git refs verify".
3. Introduce the tightened rules.

We should do this, as we have discussed before, "git-fsck(1)" implicitly checks some refs which makes the code hard to understand.

Yes, it will be much more clear. So, I should not simply classify the situations by the type of refs.

Thanks,
Jialuo

Nit: s/for/in the/

The 'no newline' reads a bit odd, perhaps, "we allow the ref's content to end with garbage or without a newline."

s/such a/such/ since the next line uses 'refs' in plural form.

s/access/assess

We could also rewrite the last sentence to make it a little more clearer as "We should notify the users about such 'curiously formatted' loose refs so that adequate care is taken before we decide to tighter the rules in the future."

I think Patrick touched base here and I agree with his comments.

s/contents/content

s/could/would

I think we can also rephrase this first sentence a little better, perhaps:

It might appear that we can't provide the user with any warnings by using FSCK_INFO.

s/issue/issues

I would rephrase this a bit, as:

The "git-fsck(1)" command reports an error when the ref content is invalid. Following this, add a similar check to "git refs verify". A a new fsck error message called "badRefContent(ERROR)" to represent that a ref has a invalid content.

[snip]

On Tue, Sep 10, 2024 at 09:07:15AM -0700, karthik nayak wrote:

[snip]

Today I am handling the reviews, there is a misunderstanding here. It's correct that "We don't need to check the target for a symbolic link". But we do need to check the symbolic links. It might be a symlink symref. In here, we just ignore the implementation and will be implemented in the later patch.

We have already introduced the checks for regular refs. There is no need to check the consistency of the target which the symref points to. Instead, we just need to check the content of teh symref itself.

In order to check the content of the symref, create a function "files_fsck_symref_target". It will first check whether the "referent" is under the "refs/" directory and then we will check the symref contents.

A regular file is accepted as a textual symref if it begins with "ref:", followed by zero or more whitespaces, followed by the full refname, followed only by whitespace characters. We always write a single SP after "ref:" and a single LF after the refname, but third-party reimplementations of Git may have taken advantage of the looser syntax. Put it more specific, we accept the following contents of the symref:

1. "ref: refs/heads/master "
2. "ref: refs/heads/master \n \n"
3. "ref: refs/heads/master\n\n"

But we do not allow any other trailing garbage. The followings are bad symref contents which will be reported as fsck error by "git-fsck(1)".

1. "ref: refs/heads/master garbage\n"
2. "ref: refs/heads/master \n\n\n garbage "

In order to provide above checks, we will first check whether the symref content misses the newline by peeking the last byte of the "referent" to see whether it is '\n'.

And we will remember the untrimmed length of the "referent" and call "strbuf_rtrim()" on "referent". Then, we will call "check_refname_format" to chceck whether the trimmed referent format is valid. If not, we will report to the user that the symref points to referent which has invalid format. If it is valid, we will compare the untrimmed length and trimmed length, if they are not the same, we need to warn the user there is some trailing garbage in the symref content.

At last, we need to check whether the referent is the directory. We cannot distinguish whether the "refs/heads/a" is a directory or not by using "check_refname_format". We have already checked bad file type when iterating the "refs/" directory but we ignore the directory. Thus, we need to explicitly add check here.

---
 Documentation/fsck-msgids.txt |   4 ++
 fsck.h                        |   1 +
 refs/files-backend.c          |  81 +++++++++++++++++++++++
 t/t0602-reffiles-fsck.sh      | 117 ++++++++++++++++++++++++++++++++++
 4 files changed, 203 insertions(+)