[RFC] 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/ZrSqMmD-quQ18a9F@ArchLinux.localdomain/

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

shejialuo <shejialuo@gmail.com> writes:

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.

On Thu, Aug 15, 2024 at 03:19:50AM -0700, karthik nayak wrote:

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

On Thu, Aug 15, 2024 at 09:37:44PM +0800, shejialuo wrote:

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

Patrick Steinhardt <ps@pks.im> writes:

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(-)

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".

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 fsck.h               | 6 ++++++
 refs/files-backend.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

shejialuo <shejialuo@gmail.com> writes:

The above is correct, but ...

... the code without this patch is already doing so.

When designated initializers are used to initialize a struct, all members that are not initialized explicitly are implicitly initialized the same as for objects that have static storage duration (meaning: pointers are initialized to NULL, arithmetics are initialized to zero).

So I do not quite see why this change is needed. By hiding the fact that the "report" structure is zero-initialized behind the macro, it makes it less obvious that we are clearing everything.

If the patch were to rewrite the above like so:

		struct fsck_ref_report report = { 0 }

it would make it even more clear that everything is zero initialized, and also makes it obvious that .path member is not any special.

Thanks.

On Tue, Aug 20, 2024 at 09:25:56AM -0700, Junio C Hamano wrote:

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.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 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(-)

shejialuo <shejialuo@gmail.com> writes:

"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.

On Tue, Aug 20, 2024 at 09:49:23AM -0700, Junio C Hamano wrote:

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

On Tue, Aug 20, 2024 at 09:49:23AM -0700, Junio C Hamano wrote:

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

Patrick Steinhardt <ps@pks.im> writes:

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.

Junio C Hamano <gitster@pobox.com> writes:

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.

On Thu, Aug 22, 2024 at 09:17:08AM -0700, Junio C Hamano wrote:

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

On Fri, Aug 23, 2024 at 09:21:14AM +0200, Patrick Steinhardt wrote:

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.

On Sun, Aug 18, 2024 at 11:01:44PM +0800, shejialuo wrote:

We can avoid having to indent the remainder of this function if we `goto cleanup` here.

Patrick

On Thu, Aug 22, 2024 at 10:48:30AM +0200, Patrick Steinhardt wrote:

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.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 Documentation/fsck-msgids.txt |  3 ++
 fsck.h                        |  1 +
 refs/files-backend.c          | 87 +++++++++++++++++++++++++++++++++++
 t/t0602-reffiles-fsck.sh      | 52 +++++++++++++++++++++
 4 files changed, 143 insertions(+)

On Sun, Aug 18, 2024 at 11:01:52PM +0800, shejialuo wrote:

We typically write this `while (*p)`.

Can't we replace this with a single `strchr('\n')` call to check for the newline and then verify that the next character is a `\0`? The check for spaces would then be handled by `check_refname_format()`.

What exactly are we guarding against here? Don't we already verify that files in `refs/` have the correct type? Or are we checking that it does not point to a directory?

Patrick

On Thu, Aug 22, 2024 at 10:53:57AM +0200, Patrick Steinhardt wrote:

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.

On Thu, Aug 22, 2024 at 08:42:02PM +0800, shejialuo wrote:

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

On Fri, Aug 23, 2024 at 07:36:10AM +0200, Patrick Steinhardt wrote:

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.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 refs/files-backend.c     | 82 ++++++++++++++++++++++++++++------------
 t/t0602-reffiles-fsck.sh | 44 +++++++++++++++++++++
 2 files changed, 101 insertions(+), 25 deletions(-)

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/cover.1418055173.git.johannes.schindelin@gmx.de/

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 <shejialuo@gmail.com>
     
      ## 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.
     
         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Mentored-by: Karthik Nayak <karthik.188@gmail.com>
         Signed-off-by: shejialuo <shejialuo@gmail.com>
     
    - ## 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".
     
         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Mentored-by: Karthik Nayak <karthik.188@gmail.com>
    @@ 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 <ps@pks.im>
         Mentored-by: Karthik Nayak <karthik.188@gmail.com>
    @@ 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 <shejialuo@gmail.com>
     
      ## 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);

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.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

shejialuo <shejialuo@gmail.com> writes:

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".

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 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(-)

On Wed, Aug 28, 2024 at 12:08:03AM +0800, shejialuo wrote:

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.

shejialuo <shejialuo@gmail.com> writes:

"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.

There is no mention of "you shouldn't promote these to error" here, which is good. But wouldn't we want to tell users to report such curiously formatted loose refs, after figuring out who created them, to help us to eventually make the check stricter in the future?

Git 3.0 boundary might be a good time to tighten interoperability rules such that we won't accept anything we wouldn't have written ourselves (not limited to loose ref format, but this applies to anything on-disk or on-wire), but we'd need enough preparation if we want to be able to do so in the future.

Thanks.

On Tue, Aug 27, 2024 at 11:21:34AM -0700, Junio C Hamano wrote:

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

Patrick Steinhardt <ps@pks.im> writes:

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.

On Wed, Aug 28, 2024 at 09:32:16AM -0700, Junio C Hamano wrote:

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

On Tue, Aug 27, 2024 at 11:21:34AM -0700, Junio C Hamano wrote:

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

shejialuo <shejialuo@gmail.com> writes:

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.

On Wed, Aug 28, 2024 at 12:07:58AM +0800, shejialuo wrote:

This reads a bit funny to me. If the ref is valid, why do we complain?

Maybe this would read better if you said "An otherwise valid ref does not end with a newline".

This is still indenting the whole body. You mentioned that you don't want to use `goto`, but in our codebase it's actually quite idiomatic. And you already use it anyway.

Wouldn't it be sufficient to only create a single commit, e.g. via `test_commit`? From all I can see all you need is some object ID, so creating the tags and second commit doesn't seem to be necessary.

We don't typically have spaces after the redirect. So you should remove them here and in all the subsequent instances.

I was wondering whether each of these cases should be a separate test, but that may be a bit wasteful. Alternatively, can we maybe set up a single repository with all the garbage that we want to verify and then double check that executing `git refs verify` surfaces them all in a single invocation?

Patrick

On Wed, Aug 28, 2024 at 02:50:01PM +0200, Patrick Steinhardt wrote:

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

Patrick Steinhardt <ps@pks.im> writes:

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.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 Documentation/fsck-msgids.txt |  3 ++
 fsck.h                        |  1 +
 refs/files-backend.c          | 77 +++++++++++++++++++++++++++++++++++
 t/t0602-reffiles-fsck.sh      | 54 ++++++++++++++++++++++++
 4 files changed, 135 insertions(+)

shejialuo <shejialuo@gmail.com> writes:

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.

If newline_pos is NULL, it is truly a "missing newline" situation. If I am reading the code correctly, the severity level is set to INFO, which is good.

If newline_pos is not NULL but newline_pos[1] is not NUL, however, that is not a "missing newline". "refs: refs/heads/master\n " would trigger this report, for example.

As far as I can tell, such a textual symbolic ref is taken as a valid symbolic ref pointing at "refs/heads/master" by refs/files-backend.c:read_ref_internal(), so we are trying to detect a valid but curiously formatted textual symbolic ref file with the above code?

And strrchr() to find the last LF is not sufficient for that purpose. We would never write "refs: refs/head/master \n", but the above code will find the LF, be satisified that the LF is followed by NUL, without realizing that SP there is not something we would have written!

I am not sure if that is worth detecting that if it is something we would have written, but if that were the case, then you would probably need to do

    (1) check the last byte of pointee_name.buf[] to make sure that
        it is LF; and
    (2) remember pointee_name.len, run strbuf_rtrim() on pointee_name,
        and that LF at the end was the only thing that was trimmed by
        checking the pointee_name.len after trimming.

or something like that. Then you do not have to have an ugly "oh we need to check again"---the production code would not do that, either.

IOW, the above "let's retry" feels totally wrong. You shouldn't have to do so, and that comes from running check_refname_format() before rtrimming the pointee_name.

Good. With this check, we know that the referent, if exists, is well-formed. The contents of the referent will then be checked just like all other refs that may not be pointed by any symbolic ref.

Good.

I do not think it is wrong per se, but I am not sure if this check is needed, either. When "git fsck" or "git refs verify" is told to check the loose refs, wouldn't it walk the refs directory and report such an unusual filesystem entity that is not a regular file, symbolic link, or a directory as "there is unusual cruft exist here"?

On Tue, Aug 27, 2024 at 12:19:11PM -0700, Junio C Hamano wrote:

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

On Wed, Aug 28, 2024 at 12:08:07AM +0800, shejialuo wrote:

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]

I think we'd want to clarify what "bad" is supposed to mean. Like, is a missing symref pointee bad? If this is about the format of the pointee's name, we might want to call this "badSymrefPointeeName".

Also, I think we don't typically call the value of a symbolic ref "pointee", but "target". Searching for "pointee" in our codebase only gives a single hit, and that one is not related to symbolic refs.

The second condition `*(newline_pos + 1)` checks whether there is any data after the newline, doesn't it? That indicates a different kind of error than "missing newline", namely that there is trailing garbage. I guess we'd want to report a separate info-level message for this.

Also, shouldn't we use `strchr` instead of `strrchr()`? Otherwise, we're only checking for trailing garbage after the _last_ newline, not after the first one.

Ah, I didn't get at first that we're doing the check a second time here. As mentioned above, I think we should check for trailing garbage further up already and more explicitly.

Same comments here as in the preceding patch for the tests.

Patrick

On Wed, Aug 28, 2024 at 02:50:09PM +0200, Patrick Steinhardt wrote:

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

Patrick Steinhardt <ps@pks.im> writes:

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.

On Wed, Aug 28, 2024 at 08:41:08AM -0700, Junio C Hamano wrote:

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.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 refs/files-backend.c     | 68 +++++++++++++++++++++++++++++-----------
 t/t0602-reffiles-fsck.sh | 44 ++++++++++++++++++++++++++
 2 files changed, 94 insertions(+), 18 deletions(-)

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(-)

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(-)

On Wed, Aug 28, 2024 at 02:28:47PM -0700, Junio C Hamano wrote:

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/20151229060055.GA17047@sigill.intra.peff.net/

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

Jeff King <peff@peff.net> writes:

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

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

On Wed, Aug 28, 2024 at 09:59:58PM -0700, Junio C Hamano wrote:

Yes, I'm very much in favor of that. As Peff said, I don't see a single reason why it would make sense to use symlinks nowadays. We have also supported the "new" syntax for ages now, and I'd be surprised if there were repos out there using it on purpose.

We should probably do the above together with a new check that starts to warn about symbolic links in "refs/" such that users become aware of this deprecation. We'd have to grow the infrastructure to also scan root refs though, which to the best of my knowledge we don't currently scan.

Patrick

Patrick Steinhardt <ps@pks.im> writes:

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.

On Thu, Aug 29, 2024 at 09:00:58AM +0200, Patrick Steinhardt wrote:

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

On Wed, Aug 28, 2024 at 09:59:58PM -0700, Junio C Hamano wrote:

From my current understanding, I think I need to rebase two patches provided by your here:

  https://lore.kernel.org/git/xmqqle0gzdyh.fsf_-_@gitster.g/
  https://lore.kernel.org/git/xmqqbk1cz69c.fsf@gitster.g/

I think in this patch, we just info the user that we will drop "core.prefersymlinkrefs" later, so I should not concern about this patch and also the [PATCH 8/6].

Thanks, Jialuo

shejialuo <shejialuo@gmail.com> writes:

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.

Jeff King <peff@peff.net> writes:

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/CodingGuidelines |  5 +++--
 git-compat-util.h              | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

On Thu, Aug 29, 2024 at 08:00:19AM -0700, Junio C Hamano wrote:

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

Here I was going to suggest explaining why you'd use one or the other (because I'm afraid of people using MAYBE_UNUSED when UNUSED would be more appropriate). But I think the extra comments you added later are even better, as it lets us explain without cluttering up the CodingGuidelines document.

Looks good.

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

Jeff King <peff@peff.net> writes:

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/CodingGuidelines |  5 +++--
 git-compat-util.h              | 24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * 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(+)

On Thu, Aug 29, 2024 at 11:27:39AM -0700, Junio C Hamano wrote:

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.

So yeah, I'm not necessarily opposed to this, but I don't think it's really doing anything in practice.

-Peff

Jeff King <peff@peff.net> writes:

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 ;-)

On Thu, Aug 29, 2024 at 11:18:06AM -0700, Junio C Hamano wrote:

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/xmqqle0gzdyh.fsf_-_@gitster.g/
      https://lore.kernel.org/git/xmqqbk1cz69c.fsf@gitster.g/

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.
     
         Mentored-by: Patrick Steinhardt <ps@pks.im>
         Mentored-by: Karthik Nayak <karthik.188@gmail.com>
    @@ 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 <shejialuo@gmail.com>
     
      ## 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 <ps@pks.im>
         Mentored-by: Karthik Nayak <karthik.188@gmail.com>
    @@ 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 <shejialuo@gmail.com>
     
      ## 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 <ps@pks.im>
         Mentored-by: Karthik Nayak <karthik.188@gmail.com>
         Signed-off-by: shejialuo <shejialuo@gmail.com>
     
    + ## 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

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.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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".

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 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(-)

On Tue, Sep 03, 2024 at 08:20:46PM +0800, shejialuo wrote:

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.

s/a bad content/bad content

I'd reformulate the second sentence to "This will be considered an error in the future". This indicates that we have the intent to tighten this check to any user and would urge them to speak up in case they disagree with such a tightening.

Same.

We typically have the name of things we read trailing and not leading in error messages. So this should rather be "unable do read ref '%s/%s'".

Coming back to my comment further up, I guess this whole block here could be introduced in a separate commit. So the first commit introduces the infra to check loose ref contents as an obvious step because we simply port over rules that already exist in git-fsck(1). And the second step could then do this retroactive tightening with the justification you have spelt out in the commit message.

`if (!*trailing)`

Patrick

On Mon, Sep 09, 2024 at 05:04:07PM +0200, Patrick Steinhardt wrote:

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

shejialuo <shejialuo@gmail.com> writes:

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]

Since we iterate over all refs, we don't need to check the target for a symbolic link. So we skip all symbolic links. Makes sense. Would be nice to have a comment here.

[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.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 Documentation/fsck-msgids.txt |   4 ++
 fsck.h                        |   1 +
 refs/files-backend.c          |  81 +++++++++++++++++++++++
 t/t0602-reffiles-fsck.sh      | 117 ++++++++++++++++++++++++++++++++++
 4 files changed, 203 insertions(+)

On Tue, Sep 03, 2024 at 08:20:54PM +0800, shejialuo wrote:

s/teh/the

I'd still argue that we should do the same retroactive tightening as we introduce for normal references, also with an INFO level at first. Otherwise we're being inconsistent across the ref types.

These are two separate error cases, and we even have different code paths raising them. Shouldn't we thus also have two different diagnostic codes for this?

There's a superfluous newline here.

Also, you never use the value of `p`, so you can instead use `starts_with()`.

We may also want to verify that `errno == ENOENT` here.

Patrick

On Mon, Sep 09, 2024 at 05:04:11PM +0200, Patrick Steinhardt wrote:

Actually, for above situations, we will use the same fsck error message ids introduce in [PATCH v3 2/4]. And I think we must refer to this in this commit message.

But it makes me wonder should we use a new commit to introduce these two fsck message ids?

I agree. I will improve in the next version.

Thanks, actually I have searched the code with "is_prefix". Well, I didn't think about "starts_<>".

I agree, if "errno != ENOENT", we should report to the user about this ref-unrelated failure.

Thanks, Jialuo

shejialuo <shejialuo@gmail.com> writes:

[snip]

s/chceck/check

s/is the/is a/

It would be a little clearer if we say

   We cannot distinguish whether a given reference like 'refs/heads/a'
   is a file or a directory.

[snip]

Would this work with a symref containing:

    ref: refs/heads/feature\ngarbage\n

Since we check last character and rtrim, wouldn't this bypass our checks? Isn't it better to find the first `\n` and check if the index < referent->len?

I think the common terminology for this is 'dangling symref'. Perhaps we could shorten this to simply say:

    Dangling symrefs are common and so we don't report them.

s/is/is a/ s/nots/not/

[snip]

On Tue, Sep 10, 2024 at 03:19:49PM -0700, karthik nayak wrote:

[snip]

We will check the above example by "check_refname_format". It will report the following message:

  error: ... : badSymrefTarget: points to refname with invalid format

From the context, I guess you suggest that we should report there is a trailing garbage in the ref. However, for the above situation, we should report an error which is align with the behavior of the "git-fsck(1)".

So there is no need to check whether there is a trailing garbage when we encounter an error.

And we cannot use this way, for example:

  ref: refs/heads/feature   \n

If we find the first '\n' index. In this example, index will be equal to "referent->len". And we totally ignore this case.

Thanks, I will improve this in the next version.

We have already introduced "files_fsck_symref_target". We should reuse 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 executed for textual symrefs.

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 <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 Documentation/fsck-msgids.txt |  5 ++
 fsck.h                        |  1 +
 refs/files-backend.c          | 68 +++++++++++++++++++-----
 t/t0602-reffiles-fsck.sh      | 97 +++++++++++++++++++++++++++++++++++
 4 files changed, 157 insertions(+), 14 deletions(-)

On Tue, Sep 03, 2024 at 08:21:03PM +0800, shejialuo wrote:

I don't we fully decided to drop support for symrefs via symbolic links yet, so this is a tad too strong of a statement. I'd rather say that we consider deprecating it in the future, but first need to asses whether they may still be used.

Also, didn't we say that we'd want to remove support for _writing_ symbolic links, but not for reading them? Not a 100% sure though.

Why does the writing side need to change?

This variable isn't used, as both code paths that end up using it could just statically set it to `1` or `0`.

Patrick

On Mon, Sep 09, 2024 at 05:04:17PM +0200, Patrick Steinhardt wrote:

Yes, that will be much better.

I have re-read the Junio's patch about the breaking change. We will drop the support for writing. But for reading we may or may not. I will improve this in the next version.

I squash two patches provided by Junio to sync with the "master" branch to make sure the build could be passed. This is because Peff has introduced the "UNUSED" check when building.

So we could just ignore this part.

Thanks, Jialuo

Hi All:

This version handles some minor problems mainly focus at the improving commit messages, comments and some minor problems.

1. Split [PATCH v3 2/4] into two commits [PATCH v4 2/5] and [PATCH v4
3/5]. [PATCH v4 2/5] integrates "git-fsck(1)"'s check and [PATCH v4 3/5]
tightens rules to check the refs with trailing garbage and refs without
newline.

2. Handle a lot of typo errors in original [PATCH v3 2/4]. And improve
the fsck-msgids documentation.

3. Improve [PATCH v4 4/5]'s commit message to first introduce the
tighten rules to be consistent with the [PATCH v4 3/5].

4. Remove "badSymrefTarget(ERROR)" fsck message. Add three new messages
to be more specific:

  1. badReferentFiletype(ERROR): The referent of a symref has a bad file
  type.

  2. badReferentName(ERROR): The referent name of a symref is invalid.

  3. escapeReferent(ERROR): The referent of a symref is outside the
  ref directory

5. Handle typos and some minor problems.

Because I add more commits, I provide the "--interdiff" here to make the reviewer's life easier.

However, because I have not merged the latest ci fixup, so I cannot verify some jobs in CIs. May need the help from Junio to verify.

Thanks, Jialuo

shejialuo (5):
  ref: initialize "fsck_ref_report" with zero
  ref: port git-fsck(1) regular refs check for files backend
  ref: add more strict checks for regular refs
  ref: add symref content check for files backend
  ref: add symlink ref content check for files backend

 Documentation/fsck-msgids.txt |  25 +++
 fsck.h                        |   7 +
 refs.c                        |   2 +-
 refs/files-backend.c          | 202 +++++++++++++++++++-
 refs/refs-internal.h          |   2 +-
 t/t0602-reffiles-fsck.sh      | 334 ++++++++++++++++++++++++++++++++++
 6 files changed, 560 insertions(+), 12 deletions(-)

Interdiff against v3:

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.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

shejialuo <shejialuo@gmail.com> writes:

" {0} " -> "{ 0 }" 

"be align with the the" -> "align with the"

Both I'll amend in-place so no need to reroll just for these.

Thanks.

We implicitly rely on "git-fsck(1)" to check the consistency of regular refs. However, we have already set up the infrastructure of the ref consistency checks. We need to port original checks from "git-fsck(1)". Thus, we could clean the "git-fsck(1)" code by removing these implicit checks.

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

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 Documentation/fsck-msgids.txt |  3 ++
 fsck.h                        |  1 +
 refs/files-backend.c          | 43 +++++++++++++++++++++++++
 t/t0602-reffiles-fsck.sh      | 60 +++++++++++++++++++++++++++++++++++
 4 files changed, 107 insertions(+)

shejialuo <shejialuo@gmail.com> writes:

The above reads as if you are, in preparation to "port" the checks we have in "fsck" to elsewhere (presumably to "refs verify"), you are removing the checks that _will_ become redundant from "fsck".

But that does not seem to be what is happening. Let me try to paraphrase, in order to check my understanding of what you wanted to say:

    "git-fsck(1) has some consistency checks for regular refs.  As
    we want to align the checks "git refs verify" performs with
    them (and eventually call the unified code that checks refs from
    both), port the logic "git fsck" has to "git refs verify".

If we fail to achieve the "a single unified code to check called by both fsck and refs-verify" at the end of this series, and instead end up with duplicated code that implements the checks in two separate code, risking them to be slightly different and drift away over time from each other, that is fine, as long as our intention is to continue the effort for unification in a follow up series.

But such a plan needs to be spelled out.

"symbolic links are OK" for now. We'll add sanity checks for them in later steps. OK.

Is there a reason why we cannot to use report.path aka refname.buf, and instead we have to recompute the same path again?

Should this error be propagated back to the caller, not just to the end-user, by a call to fsck_report_ref(), like you do for a ref file that has questionable contents? If ref iteration (like for-each-ref) claims there is this ref, and you cannot read its value when you try to use it, it is just as bad as having a loose ref file that has unusable contents, isn't it?

It is a separate matter if such a failure mode deserves its own error code (FSCK_MSG_UNREADABLE_REF) or can be rolled into the same FSCK_MSG_BAD_REF_CONTENT. I can see arguments for both sides and offhand have no strong preference either way.

Thanks.

On Wed, Sep 18, 2024 at 11:59:45AM -0700, Junio C Hamano wrote:

[snip]

Thanks, I have re-read my words, I did not explain this thing well.

Thanks for pointing out this, because this part I wrote a long time ago and I think it's unrelated to the fsck part. So, I forgot to change.

I agree. The initial motivation for this design is that I think this is OS-specific issue (It may be read successfully in the next time). So, I don't put it into the fsck part. But It make senses that we should report this.

We could just use "FSCK_MSG_BAD_REF_CONTENT" and add a message "cannot open this file". I guess this should be enough.

We have already used "parse_loose_ref_contents" function to check whether the ref content is valid in files backend. However, by using "parse_loose_ref_contents", we allow the ref's content to end with garbage or without a newline.

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. We should notify the users about such "curiously formatted" loose refs so that adequate care is taken before we decide to tighten the rules in the future.

And it's not suitable either to report a warn fsck message to the user. 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. It may cause compatibility issues which may break the repository. So we add the following two fsck infos to represent the situation where the ref content ends without newline or has trailing garbages:

1. refMissingNewline(INFO): A ref does not end with newline. This will
   be considered an error in the future.
2. trailingRefContent(INFO): A ref has trailing content. This will be
   considered an error in the future.

It might appear that we can't provide the user with any warnings by using FSCK_INFO. 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 issues.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 Documentation/fsck-msgids.txt |  8 +++++
 fsck.h                        |  2 ++
 refs.c                        |  2 +-
 refs/files-backend.c          | 27 ++++++++++++++--
 refs/refs-internal.h          |  2 +-
 t/t0602-reffiles-fsck.sh      | 60 +++++++++++++++++++++++++++++++++++
 6 files changed, 96 insertions(+), 5 deletions(-)

shejialuo <shejialuo@gmail.com> writes:

It is ONLY files backend's loose-ref representation to store the object name that is the value of the ref as hexadecimal text terminated with a newline. With packed backend, even if the file ends with an incomplete line, it would be confusing to say that such lack of terminating LF is associated with a particular ref. With reftable backend, the object name may not even be hexadecimal but binary without any terminating LF.

At least you should say "A loose ref file that does not end with...", because a ref NEVER ends or contains newline, and what you are expecting to be terminated with LF is not even a ref, but the value of it.

Also, isn't it too strong to say "will be" without giving any further information, like:

    As valid implementations of Git never created such a loose ref
    file, it may become an error in the future.  Report to the
    git@vger.kernel.org mailing list if you see this error, as we
    need to know what tools created such a file.

or something?

The same comment applies to the next entry.

In the pre-context of this hunk, if parse_oid_hex_algoph() failed to recognise the initial segment of buf as a valid hexadecimal object name, it would have already returned, so we know 'p' is always valid here. It is the byte that comes immediately after the hexadecimal object name.

OK.

Just like we punted for S_ISLNK() in an earlier step, we for now deal with regular refs in this step. OK.

Not limited to this patch, but isn't fsck_report_ref() misdesigned, or is it just they are used poorly in these patches? In these two callsites, the message string parameter does not give any more information than what the FSCK_MSG_* enum gives.

In fact, MSG_REF_MISSING_NEWLINE at least says that the complaint is about refs, but "missing newline" does not even say from what the newline is missing. For TRAILING_REF_CONTENT, people may expect to see what garbage follows the expected contents, but that information (i.e. contents of *trailing) is lost here.

On Wed, Sep 18, 2024 at 12:39:13PM -0700, Junio C Hamano wrote:

Thanks, I will improve this in the next version.

This is nice. I know the intention here.

I agree with you here, I use way too general words to describe what happens. I will improve this. Actually, I feel hard to find words for "MSG_REF_MISSING_NEWLINE". I think we should say:

	LF should be at the end of the file.

Thanks, Jialuo

shejialuo <shejialuo@gmail.com> writes:

Giving a human-readable message when we have an enum can be done at a lot higher layer with the current way the fsck_report_ref() function is used (i.e. in that function, not by its callers).

That is what I meant by "misdesigned"---if one message enum always corresponds to one human-readable message, there is not much point in forcing callers to supply both, is there?

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 the symref itself.

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"

Thus, we could reuse "refMissingNewline" and "trailingRefContent" FSCK_INFOs to do the same retroactive tightening as we introduce for regular references.

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  "

And we introduce a new "badReferentName(ERROR)" fsck message to report above errors to the user.

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, if not, we will report "escapeReferent" fsck error message to notify the user this situation.

Then, 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 check 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 a directory. We cannot distinguish whether a given reference like "refs/heads/a" is a file or a directory 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 <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 Documentation/fsck-msgids.txt |   9 +++
 fsck.h                        |   3 +
 refs/files-backend.c          |  81 +++++++++++++++++++++++
 t/t0602-reffiles-fsck.sh      | 117 ++++++++++++++++++++++++++++++++++
 4 files changed, 210 insertions(+)

shejialuo <shejialuo@gmail.com> writes:

Expect that people do not read the body of the message as completing a paragrpah the title started. I.e. ...

... this needs a bit of preamble, like

    We have code that check regular ref contents, but we do not yet
    check contents of symbolic refs.

This description needs to be updated, as it is unclear if you are talking about errors we already detect, or if you are planning to update fsck to notice and report these errors.

OK.

"Then, we will first" -> "Then it checks" or something.

You already consumed "first" for the check to limit the referent to those under "refs/" hierarchy.

That is an implementation detail of what you did. But if the implementation were buggy and did not exactly what you intended to do, the above description gives no information to help others to fix it up so that it works as you intended it to work, because you do not explain it.

So what did you want to achieve in the third step (the first being "limit to refs/ hiararchy", the second being "no incomplete lines allowed")?

    Third, we want to make sure that the contents of a textual
    symref MUST have a single LF after the target refname and
    NOTHING ELSE.

or something.

"a directory" -> "an existing directory"?

I am not comfortable to see the word "directory" used in this proposed log message, as some refs could be stored in the packed backend and are referenced by the symbolic ref you are inspecting (this comment also refers to the "refs/ directory" you mentioned earlier as "the first check").

    Lastly, a symbolic ref MUST either point to an existing ref,
    or if the referent does not exist, it MUST NOT be a leading
    subpath for another existing ref (e.g., when "refs/heads/main"
    exists, a symbolic ref that points at "refs/heads" is a no-no).

or something (but again, I am open to a phrasing better than "subpath").

Design question. What do we want to do when we have no loose refs under the "refs/heads/historical/" hiearchy, (i.e. all of them are in packed-refs file) hence ".git/refs/heads/historical" directory does not exist on the filesystem. And a symbolic ref points at "refs/heads/historical". Shouldn't we give the same error whether the .git/refs/heads/historical directory exist or not, as long as the refs/heads/historical/main branch exists (in the packed-refs backend)?

I am not sure starting this as ERROR is wise. Users and third-party tools make creative uses of the system and I cannot offhand think of an argument why it should be forbidden to create a symbolic link to our own HEAD or to some worktree-specific ref in another worktree.

As you initialized "len" to "referent->len-1" earlier, wouldn't it more natural to use it here? That would match the incrementing of len++ later in this block.

Having said that, the above should be simplified more like:

 * declare but not initialize "len".  better yet, declare "orig_len"
   and leave it uninitialized.

 * do not touch "len++" in the above block (actually, you can
   discard the above "if(it does not end with LF)" block, see
   below).

 * instead grab "referent->len" in "len" (or "orig_len") immediately
   before you first modify referent, i.e. before strbuf_rtrim() call.

	orig_len = referent->len;
	orig_last_byte = referent->buf[orig_len - 1];

Similar to an earlier step, the message does not give any more information than the enum. Wouldn't the user who got this error want to learn what referent->buf said and which part of it was bad in the same message, instead of having to look it up on their own after fsck finishes?

At this point we know check_refname_format() is happy with what is left after rtrimming the referent. There are four cases:

 - rtrim() did not trim anything (orig_len == referent->len); the file
   lacked the terminating LF.

 - rtrim() trimmed one byte (orig_len - 1 == referent->len) and
   the byte was not LF (orig_last_byte != '\n').  The file lacked
   the terminating LF.

 - rtrim() trimmed exactly one byte (orig_len - 1 == referent->len)
   and the byte was LF (orig_last_byte == '\n').  There is no error.

 - all other cases, i.e., rtrim() trimmed two or more bytes.  The
   file had trailing whitespaces after a valid referent that passed
   check_refname_format().

So in short,

	if (referent->len == orig_len ||
	    referent->len == orig_len - 1 && orig_last_byte != '\n') {
		FSCK_MSG_REF_MISSING_NEWLINE;
	} else if (referent->len < orig_len - 1) {
		FSCK_MSG_REF_TRAILING_WHITESPACE;
	}

can replace the next block you wrote, and we can also remove the earlier "it is an error if it does not end with '\n'", I think.

As check_refname_format() was happy, the difference between orig_len and referent->len are only coming from trailing whitespaces, i.e. it is not that it had arbitrary garbage. Shouldn't we be more explicit about that?

If referent_path->buf refers to "refs/heads/historical/", and all the branches under the hierarchy have been sent to packed-refs, then this check will not trigger.

I wonder if this check is the right thing to enforce in the first place, though.

As far as the end user is concerned, refs/heads/historical/master branch stil exists, and there is no refs/heads/historical branch, so such a symbolic ref, for all intents and purposes, is the same as any other dangling symbolic refs, no?

Of course, "git update-ref SUCH_A_SYMREF HEAD" will complain because there is refs/heads/historical, with something like

    "refs/heads/historical/master" exists, cannot create "refs/heads/historical"

but that is to be expected. If you remove the last branch in the refs/heads/historical hierarchy, you should be able to do such an update-ref to instanciate refs/heads/historical as a regular ref.

I doubt this is a good design. We have referent, and the symbolic ref checker knows that the true referent refname may be followed by whitespaces, so instead of inventing referent _path here, it would be a better design to let the files_fsck_symref_target() to decide what file to open and check based on referent, no? Give it the refstore or refstore's gitdir and have the concatenation with the rtrimmed contents in the referent->buf after it inspected it instead, perhaps?

On Wed, Sep 18, 2024 at 01:19:13PM -0700, Junio C Hamano wrote:

Thanks, I will improve this in the next version.

Yes, When I was writing this part, I felt a little painful to express my words. I have thought how could I express the connection between the current patch and the previous one.

From the above comments, I need to organize the commit message of this patch to make things clear here.

I guess I need to think carefully here. Actually, my intention is that I want to concentrate on the loose refs and then take consideration about the packed refs.

However, from what you have said above, it seems I could not do this. They are connected. But at current, I am not so familiar with packed refs behavior, I could not answer all the questions above.

I decide to understand what packed-ref done. So, this series may be stalled sometime until I have a good knowledge and re-think the design here.

Do we allow this cross-access (hack)? It might cause some trouble from my perspective.

Yes, exactly.

I agree.

Yes, I agree. I will improve this.

That's so clear. My implementation is not good compared with this.

Yes, I made a lot of mistakes when calling the "fsck_report_ref". I will report the exact garbage content to the user.

Yes, because "refs/heads/historical" will not appear in the filesystem.

I am a little shocked here. I do this in action and find the directory will be automatically converted to a regular file in the filesystem. So, I agree with you here. We should never check this, because we allow symref to point to a directory. As long as there is no loose refs and packed refs under this directory, we could use "git update-ref" for this symref.

Thanks,

Yes, I agree with you here. We should use "files_fsck_symref_target" to do this.

----

From this review, I think I need to understand more behaviors about files backend and packed backend. Thanks for your so dedicated reviews. I may spend more time to send the next version. And there may be some delay.

Thanks, Jialuo

shejialuo <shejialuo@gmail.com> writes:

If the current implementation allows users to set up and take advantage of, then it is not a hack. It would cause breakage if we make it an error. Does such a symref successfully refer to the referent right now? I think it does.

Thanks.

We have already introduced "files_fsck_symref_target". We should reuse 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 executed for textual symrefs.

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 consider deprecating writing the symbolic links and for reading, we may or may not deprecate. We first need to asses whether symbolic links may still be used. So, add a new fsck message "symlinkRef(INFO)" to let the user be aware of this information.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 Documentation/fsck-msgids.txt |  5 ++
 fsck.h                        |  1 +
 refs/files-backend.c          | 65 ++++++++++++++++++-----
 t/t0602-reffiles-fsck.sh      | 97 +++++++++++++++++++++++++++++++++++
 4 files changed, 154 insertions(+), 14 deletions(-)

shejialuo <shejialuo@gmail.com> writes:

If that is the intention, the the documentation entry is somewhat out of line.

    A symbolic link is used as a symref.  Report to the
    git@vger.kernel.org mailing list if you see this error, as we
    are assessing the feasibility of dropping the support to use
    symbolic links as a symref.

But quite honestly, I do not think it is necessary to deprecate (let alone remove) the support for reading side.

shejialuo <shejialuo@gmail.com> writes:

Yeah, for the changes from the previous iteration of this series, range-diff comparison is pretty much useless. Interdiff is indeed more usable, but essentially this iteration deserves reviews with fresh sets of eyes.

A good way to do so is to fork a temporary branch at the tip of these 5 commits, and then either merge or cherry-pick the CI fixup. Such a temporary branch should be usable for CI testing, right?

Thanks.

PS.

I am not feeling well today; please expect delayed and/or sparse responses.

Hi All:

This version handles a lot of review from Junio.

1. [PATCH v5 1/9] enhances the commit message compared with the previous
[PATCH v4 1/5].

2. [PATCH v5 2/9] is a new topic which has not never been introduced in
the previous. It supports multiple worktrees check for refs. During the
GSoC PATCH: <ZrSqMmD-quQ18a9F@ArchLinux.localdomain>, I do not implement
the code to support worktree check. However, we need to add this due to
the review from Junio:
  > > +`escapeReferent`::
  > > +	(ERROR) The referent of a symref is outside the "ref" directory.
  >
  > I am not sure starting this as ERROR is wise.  Users and third-party
  > tools make creative uses of the system and I cannot offhand think of
  > an argument why it should be forbidden to create a symbolic link to
  > our own HEAD or to some worktree-specific ref in another worktree.
When checking the escape situation of the referent, I didn't consider
the worktree. So, I decide to first add checks for multiple worktree.
And then add a new test for multiple worktrees.

3. The intention of the [PATCH v5 3/9] is the same as the [PATCH v4
2/5].

  + Enhance the commit message suggested by Junio.
  + Use "fsck_ref_report" to tell the user we cannot read the file
  instead of reporting general error.
  + For "FSCK_MSG_BAD_REF_CONTENT" message id, instead of just reporting
  the no-information message "invalid ref content", report the actual
  content of the ref, i.e., "ref_content.buf".

4. The intention of the [PATCH v5 4/9] is the same as the [PATCH v4
3/5].

  + Instead of using the concrete "refMissingNewline" and
  "trailingRefContent" fsck messages, create a fsck info message
  "unofficialFormattedRef"
  + Follow the advice from Junio, use "fsck_ref_report" to report more
  useful information. For example, what is the trailing garbage.

5. The PATCH[v4 4/5] is split into 4 commits from [PATCH v5 5/9] to
[PATCH v5 8/9]. The reason why I decide to do this is that I introduce
the check for worktree and the version 4 is a little messy for the
commit message. Although the C code is not changed too much, the commit
message is hard to write and make the reviewer confused.

6. [PATCH v5 5/9] aims to add checks for textual symref except escape
situation.

  + Because I split commit here, it's easy to write the clean commit
  message, which should be changed according to the review from Junio.
  + Followed the advice from Junio to gracefully check the symref. Thus,
  the commit message is more clean.
  + Drop the check for "referent" pointing to a directory. We allow
  this, it's a dangling symref. No need to check this. So we could drop
  the parameter "referent_path" in "files_fsck_symref_target()".
  + Enhance the "fsck_ref_report" to report more useful information.

7. [PATCH v5 6/9] enhances the check for escape situation. Introduce a
new fsck message "escapeReferent(INFO)".

8. [PATCH v5 7/9] enhances the situation where we use multiple
worktrees. In practice, we allow point to ref of one of the linked
worktrees from primary worktree or one of the linked worktrees. We
should not warn about this.

9. [PATCH v5 8/9] enhances the test script for worktrees.

10. The intention of [PATCH v5 9/9] is the same as the [PATCH v4 5/5].
Not so much change.

Because I do not sync the upstream for a long time. For this series, I sync the latest upstream and generate the patch, it is based on

  3857aae53f (Git 2.47-rc0, 2024-09-25)

And I don't think range-diff is useful, it is messy for the reviewers. Actually, there are not so many logic changes in this new version.

Thanks, Jialuo

shejialuo (9):
  ref: initialize "fsck_ref_report" with zero
  builtin/refs: support multiple worktrees check for refs.
  ref: port git-fsck(1) regular refs check for files backend
  ref: add more strict checks for regular refs
  ref: add basic symref content check for files backend
  ref: add escape check for the referent of symref
  ref: enhance escape situation for worktrees
  t0602: add ref content checks for worktrees
  ref: add symlink ref content check for files backend

 Documentation/fsck-msgids.txt |  28 +++
 builtin/refs.c                |  11 +-
 fsck.h                        |   5 +
 refs.c                        |   2 +-
 refs/files-backend.c          | 168 ++++++++++++-
 refs/refs-internal.h          |   2 +-
 t/t0602-reffiles-fsck.sh      | 442 ++++++++++++++++++++++++++++++++++
 7 files changed, 646 insertions(+), 12 deletions(-)

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 align with the the codebase, initialize "fsck_ref_report" with zero.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

shejialuo <shejialuo@gmail.com> writes:

s/the//

[snip]

We have already set up the infrastructure to check the consistency for refs, but we do not support multiple worktrees. As we decide to add more checks for ref content, we need to set up support for multiple worktrees. Use "get_worktrees" and "get_worktree_ref_store" to check refs under the worktrees.

Because we should only check once for "packed-refs", let's call the fsck function for packed-backend when in the main worktree. In order to know which directory we check, we should default print this information instead of specifying "--verbose".

It's not suitable to print these information to the stderr. So, change to stdout.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 builtin/refs.c           | 11 ++++++--
 refs/files-backend.c     | 18 ++++++++----
 t/t0602-reffiles-fsck.sh | 59 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 7 deletions(-)

On Sun, Sep 29, 2024 at 03:15:26PM +0800, shejialuo wrote:

Makes sense.

This change should likely be evicted into its own commit with a bit more explanation.

This one, too. Why exactly is in not suitable to print to stderr?

I think it is more customary to say `ret |=` instead of `ref +=`. Otherwise we could at least in theory wrap around and even land at `ret == 0`, even though this is quite unlikely.

[snip]

What is the current behaviour? Is it that we verify the packed-refs file multiple times, or rather that we call `packed_ref_store->be->fsck()` many times even though we know it won't do anything for anything except for the main worktree?

If it is the former I very much agree that we should make this conditional. If it's the latter I'm more in the camp of letting it be such that if worktrees were to ever gain support for "packed-refs" we wouldn't have to change anything.

In any case, as proposed I think it would make sense to evict this into a standalone commit such that these details can be explained in the commit message.

Patrick

On Mon, Oct 07, 2024 at 08:58:30AM +0200, Patrick Steinhardt wrote:

I am sorry for the confusion. We should not print which directory we check here into stderr. Because I think this will make test script contain many unrelated info when using "git refs verify 2>err".

The reason here is when checking the consistency of refs in multiple worktrees. The ref name could be repeat. For example, worktree A has its own ref called "test" under ".git/worktrees/A/refs/worktree/test" and worktree B has its own ref still called "test" under ".git/worktrees/B/refs/worktree/test".

However, the refname would be printed to "refs/worktree/test". It will make the user confused which "refs/worktree/test" is checked. So, we should print this information like:

    Checking references consistency in .git
    ...
    checking references consistency in .git/worktrees/A
    ...
    checking references consistency in .git/worktrees/B

However, when writing this, I feel a ".git" is a bad usage. It will make the user think it will check everything here. This should be improved in the next version.

I agree here. I will improve this in the next version.

[snip]

That's a good question. I think the second is the current behaviour. We will call `packed_ref_store->be->fsck()` many times. I understand what you mean here, we just put the check into `packed_ref_store->be->fsck()` function.

I agree.

Yes, the current commit message lacks of details.

Thanks, Jialuo

On Mon, Oct 07, 2024 at 04:42:21PM +0800, shejialuo wrote:

But wouldn't it be the better solution if we printed the fully-qualified reference name "worktrees/worktree/refs/worktree/test" instead? That would remove the need to say which directory we're currently verifying in the first place.

Patrick

On Mon, Oct 07, 2024 at 11:16:19AM +0200, Patrick Steinhardt wrote:

[snip]

Good idea. I will use this way in the next version.

Thanks

"git-fsck(1)" has some consistency checks for regular refs. As we want to align the checks "git refs verify" performs with them (and eventually call the unified code that checks refs from both), port the logic "git-fsck" has to "git refs verify".

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

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 Documentation/fsck-msgids.txt |  3 ++
 fsck.h                        |  1 +
 refs/files-backend.c          | 45 ++++++++++++++++++++++++
 t/t0602-reffiles-fsck.sh      | 66 +++++++++++++++++++++++++++++++++++
 4 files changed, 115 insertions(+)

On Sun, Sep 29, 2024 at 03:15:46PM +0800, shejialuo wrote:

What's missing here is the actual intent of this commit, namely why we want to align the checks. I assume that this prepares us for calling `git refs verify` as part of git-fsck(1), but readers not familiar with the larger picture may be left wondering.

It would help readers to know where the code is that you're porting over to `git refs verify` so that one can double check that the port is done faithfully to the original.

Patrick

On Mon, Oct 07, 2024 at 08:58:34AM +0200, Patrick Steinhardt wrote:

Indeed, I will improve this in the next version.

I am a little confused here. There are too many codes in "git-fsck(1)" to check the ref consistency. How could I accurately express this info in the commit message?

Thanks, Jialuo

On Mon, Oct 07, 2024 at 04:42:44PM +0800, shejialuo wrote:

Well, you say you ported over a specific consistency check from git-fsck(1) to `git refs verify` in the commit message. So I assume that it should match a specific check in git-fsck(1), shouldn't it?

Patrick

On Mon, Oct 07, 2024 at 11:18:24AM +0200, Patrick Steinhardt wrote:

I understand your meaning here. I will improve the commit message in the next version.

shejialuo <shejialuo@gmail.com> writes:

[snip]

Shouldn't we use `die_errno` here instead? I mean, this is not really a bad ref content issue. If we don't want to die here, it would still probably be nice to get the actual issue using `strerror` instead and use that instead of the generic message we have here.

[snip]

On Tue, Oct 08, 2024 at 12:43:20AM -0700, Karthik Nayak wrote:

Well, I think I need to dive into the "open" system call here. Actually, we have two opinions now. Junio thought that we should use "fsck_report_ref" to report. Karthik, Patrick and I thought that we should report using "*errno" because this is a general error.

Let me investigate what situations will make "open" system call fail. Thus, we could fully explain which choice we will choose.

Thanks, Jialuo

shejialuo <shejialuo@gmail.com> writes:

What do you mean by "a general error"? It is true that we failed to read a ref file, so even if it is an I/O error, I'd think it is OK to report it as an error while reading one particular ref.

Giving more information is a separate issue. If fsck_report_ref() can be extended to take something like

    "cannot read ref file '%s': (%s)", iter->path.buf, strerror(errno)

that would give the user necessary information.

And I agree with half-of what Karthik said, i.e., we do not want to die here if this is meant to run as a part of "git fsck".

I may have said this before, but quite frankly, the API into the fsck_report_ref() function is misdesigned. If the single constant string "cannot read ref file" cnanot give more information than FSCK_MSG_BAD_REF_CONTENT, the parameter has no value.

The fsck.c:report() function, which is the main function to report fsck's findings before fsck_report_ref() was introduced, did not have such a problem, as it allowed "const char *fmt, ..." at the end. Is it too late to fix the fsck_report_ref()?

Thanks.

On Tue, Oct 08, 2024 at 10:44:53AM -0700, Junio C Hamano wrote:

Yeah, this is also in line with what I proposed elsewhere, where we have been discussing the 1:1 mapping between error codes and error messages. If the error messages were dynamic they'd be a whole lot useful overall and could provide more context.

True in the current form, yeah. If `fsck_report_ref()` learned to take a vararg argument and treat its first argument as a string format it would be justified though, as the message is now dynamic and can contain more context around the specific failure that cannot be provided statically via the 1:1 mapping between error code and message.

I don't think so, I think we should be able to refactor the code rather easily to do so.

Patrick

On Wed, Oct 09, 2024 at 10:05:19AM +0200, Patrick Steinhardt wrote:

[snip]

It is not "learned". At current, `fsck_report_ref` can do this and is the same as "fsck.c::report". I have explained this when replying to Junio.

It's not hard to refactor the code. But this is not the problem. I am a little confused here. Because we already allowed "fsck_report_ref" having "const char *fmt, ..." at the end.

Thanks, Jialuo

On Wed, Oct 09, 2024 at 07:59:20PM +0800, shejialuo wrote:

Ah, I didn't double check, but was operating on what I understood from this thread. In that case I think that the current interface is okay.

Patrick

Patrick Steinhardt <ps@pks.im> writes:

I didn't, either. So there is an obvious way out for "why aren't we telling the errno to users" issue? That's good.

On Tue, Oct 08, 2024 at 10:44:53AM -0700, Junio C Hamano wrote:

Make sense.

At current, the `fsck_report_ref` can do this. I think I used `fsck_report_ref` function badly in this case.

Yes, we should not die the program. Instead, we need to continuously check other refs.

I agree that if the FSCK message id could explain the error well, there is no need for us to provide extra message. But, I want to say the `fsck_report_ref` is not misdesigned here. It is just the same as the "fsck.c::report" function which has "const char *fmt, ..." at the end like the following shows:

    int fsck_report_ref(struct fsck_options *options,
                        struct fsck_ref_report *report,
                        enum fsck_msg_id msg_id,
                        const char *fmt, ...)

And I do think "fsck.c::report" function also has the above problems. Let me give you some examples here in "fsck.c":

    report(options, tree_oid, OBJ_TREE,
           FSCK_MSG_BAD_FILEMODE,
           "contains bad file modes");

    report(options, tree_oid, OBJ_TREE,
           FSCK_MSG_DUPLICATE_ENTRIES,
           "contains duplicate file entries");

    ...

So, I want to say there is no difference between "fsck_ref_report" and "fsck.c::report". When I refactored the code in GSoC journey, the main problem is that we should reuse the original "fsck.c::report" code instead of writing redundant codes.

The final result is I extract a new function "fsck_vreport" here (I leverage the original "fsck.c::report" function) which will be called by "fsck_ref_report" and "fsck.c::report".

    static int fsck_vreport(struct fsck_options *options,
                            void *fsck_report,
                            enum fsck_msg_id msg_id,
                            const char *fmt, va_list ap)

From my perspective, if we decide to refactor, we should allow the user call the followings:

    fsck_ref_report(..., FSCK_MSG_BAD_REF_CONTENT, NULL);
    report(..., FSCK_MSG_DUPLICATE_ENTRIES, NULL);

So, we should check whether `fmt` is NULL in the `fsck_vreport` function to make sure that if FSCK message is good enough to explain what happens, we should not pass any message.

Thanks, Jialuo

We have already used "parse_loose_ref_contents" function to check whether the ref content is valid in files backend. However, by using "parse_loose_ref_contents", we allow the ref's content to end with garbage or without a newline.

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. We should notify the users about such "curiously formatted" loose refs so that adequate care is taken before we decide to tighten the rules in the future.

And it's not suitable either to report a warn fsck message to the user. 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. It may cause compatibility issues which may break the repository. So we add the "unofficialFormattedRef(INFO)" fsck message to represent the situation where the ref format is not officially created by us and notify the users it may become an error in the future.

It might appear that we can't provide the user with any warnings by using FSCK_INFO. 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 issues.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 Documentation/fsck-msgids.txt |  8 +++++
 fsck.h                        |  1 +
 refs.c                        |  2 +-
 refs/files-backend.c          | 26 +++++++++++++--
 refs/refs-internal.h          |  2 +-
 t/t0602-reffiles-fsck.sh      | 59 +++++++++++++++++++++++++++++++++++
 6 files changed, 93 insertions(+), 5 deletions(-)

On Sun, Sep 29, 2024 at 03:16:00PM +0800, shejialuo wrote:

I find "unofficial" to be a tad weird. Do we rather want to say something like "badRefTrailingGarbage"?

I think we should discern these two error cases and provide different message IDs.

Patrick

On Mon, Oct 07, 2024 at 08:58:37AM +0200, Patrick Steinhardt wrote:

Well, I will answer this question just in below question together.

Actually, in the previous versions, I have mapped one message id to one error case. But, in the v4, Junio asked a question

  Not limited to this patch, but isn't fsck_report_ref() misdesigned,
  or is it just they are used poorly in these patches?  In these two
  callsites, the message string parameter does not give any more
  information than what the FSCK_MSG_* enum gives.

  That is what I meant by "misdesigned"---if one message enum always
  corresponds to one human-readable message, there is not much point
  in forcing callers to supply both, is there?

In my opinion, we should have only one case here for trailing garbage and not end with a newline. When writing the code, I chose the name "unofficialFormattedRef" for the following reason:

  1. If we use two message ids here, for every message id, we need write
  to info the user "please report this to git mailing list".

  2. If we decide to make this as an error. We could just classify them
  into "badRefContent" message category.

  3. The semantic is correct here, they are truly curious formatted
  refs, and eventually we will give the info to the user what is
  curious.

So, I think we should not always map one message to one error case.

Thanks, Jialuo

On Mon, Oct 07, 2024 at 04:44:16PM +0800, shejialuo wrote:

From my point of view the error codes should be the single source of truth, as this is what a user can use to disable specific checks. So if one code maps to multiple messages they have the problem that they can only disable all of those messages.

I don't disagree with what Junio is saying. It is somewhat duplicate that the user has to pass both a code and a message in the current form-- it should be sufficient for them to pass the code, and the message can then e.g. be extracted from a central array that maps codes to messages.

But you can also make the reverse argument: messages can be dynamic, so that the caller may include additional details around why specfically the check failed. The code and message would still be 1:1, but we may include additional details like that to guide the user.

Patrick

On Mon, Oct 07, 2024 at 11:25:17AM +0200, Patrick Steinhardt wrote:

[snip]

Thanks for your remind here. I totally forgot this. I have changed my mind now, we should use one to one mapping here. As you said, if we do not, we will give the user the bad experience.

Yes, I will refactor the "fsck_report" to allow the user pass the "NULL" message if the fsck message id is clear enough to indicate the error case.

So, more things to do here.

Thanks, Jialuo

We have code that checks regular ref contents, but we do not yet check the contents of symbolic refs. By using "parse_loose_ref_content" for symbolic refs, we will get the information of the "referent".

We do not need to check the "referent" by opening the file. This is because if "referent" exists in the file system, we will eventually check its correctness by inspecting every file in the "refs" directory. If the "referent" does not exist in the filesystem, this is OK as it is seen as the dangling symref.

So we just need to check the "referent" string content. A regular could be 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. However, we always write a single SP after "ref:" and a single LF after the refname. It may seem that we should report a fsck error message when the "referent" does not apply above rules and we should not be so aggressive because third-party reimplementations of Git may have taken advantage of the looser syntax. Put it more specific, we accept the following "referent":

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

When introducing the regular ref content checks, we created a new fsck message "unofficialFormattedRef" which exactly represents above situation. So we will reuse this fsck message to write checks to info the user about these situations.

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  "

And we introduce a new "badReferent(ERROR)" fsck message to report above errors by using "ref.c::check_refname_format". But we cannot just pass the "referent" to this function because the "referent" might contain some whitespaces which will cause "check_refname_format" failing.

In order to add checks, we will do the following things:

1. Record the untrimmed length "orig_len" and untrimmed last byte
   "orig_last_byte".
2. Use "strbuf_rtrim" to trim the whitespaces or newlines to make sure
   "check_refname_format" won't be failed by them.
3. Use "orig_len" and "orig_last_byte" to check whether the "referent"
   misses '\n' at the end or it has trailing whitespaces or newlines.

Mentored-by: Patrick Steinhardt <ps@pks.im>
Mentored-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 Documentation/fsck-msgids.txt |  3 ++
 fsck.h                        |  1 +
 refs/files-backend.c          | 40 +++++++++++++++
 t/t0602-reffiles-fsck.sh      | 97 +++++++++++++++++++++++++++++++++++
 4 files changed, 141 insertions(+)