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

[shejialuo <shejialuo@gmail.com>]

On Tue, Aug 27, 2024 at 11:21:34AM -0700, Junio C Hamano wrote:
> shejialuo <shejialuo@gmail.com> writes:
> 
> > 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.
> 
> "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."
> 

Thanks, I will fix this in the next version.

> > 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 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 think I could organize the above to the commit message to better show
the motivation why we should not report an error fsck message.

> > We should not allow the user to upgrade the fsck warnings to errors. It
> > might cause compatibility issue which will break the legacy repository.
> 
> 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?
> 

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.

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

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

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

Thanks,
Jialuo