Re: [PATCH v1 2/4] ref: add regular ref content check for files backend

From: shejialuo <shejialuo@gmail.com>
Date: Aug 23, 2024, 11:30 UTC
Message-ID: <ZshyuH55IP_a55z0@ArchLinux>
In-Reply-To: <Zsg4ZLHFD4nzISc6@tanuki>

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

Show 31 quoted lines

> On Thu, Aug 22, 2024 at 09:17:08AM -0700, Junio C Hamano wrote:
> > Junio C Hamano <gitster@pobox.com> writes:
> > 
> > > Patrick Steinhardt <ps@pks.im> writes:
> > >
> > >> 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.
> > >
> > > 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.
> > 
> > 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").
> 
> 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.
>

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.