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

[shejialuo <shejialuo@gmail.com>]

On Mon, Sep 09, 2024 at 05:04:07PM +0200, Patrick Steinhardt wrote:
> > 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.
> 
> 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.
> 

Thanks, I will improve this in the next version.

> > the legacy repository. So we add the following two fsck infos to
> 
> 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.
> 

I agree, we should drop "legacy" here.

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

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.

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

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

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

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

Thanks,
Jialuo