From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 09 Oct 2025 05:22:58 GMT
Subject: Re: [PATCH v2 12/14] refs: don't store peeled object IDs for invalid tags
Message-ID: <aOdGsqgyy5K6Lynu@pks.im>
In-Reply-To: <aOaRDhwZ0YkPdjkd@ArchLinux>

On Thu, Oct 09, 2025 at 12:27:58AM +0800, shejialuo wrote:
> On Wed, Oct 08, 2025 at 05:50:27PM +0200, Patrick Steinhardt wrote:
> > Both the "files" and "reftable" backend store peeled object IDs for
> > references that point to tags:
> > 
> >   - The "files" backend stores the value when packing refs, where each
> >     peeled object ID is prefixed with "^".
> > 
> >   - The "reftable" backend stores the value whenever writing a new
> >     reference that points to a tag via a special ref record type.
> > 
> > Both of these backends use `peel_object()` to find the peeled object ID.
> > But as explained in the preceding commit, that function does not detect
> > the case where the tag's tagged object and its claimed type mismatch.
> > 
> > The consequence of storing these bogus peeled object IDs is that we're
> > less likely to detect such corruption in other parts of Git.
> > git-for-each-ref(1) for example does not notice anymore that the tag is
> > broken when using "--format=%(*objectname)" to dereference tags.
> > 
> > One could claim that this is good, because it still allows us to mostly
> > use the tag as intended. But the biggest problem here is that we now
> > have different behaviour for such a broken tag depending on whether or
> > not we have its peeled value in the refdb.
> > 
> > Fix the issue by verifying the object type when peeling the object. If
> > that verification fails we simply skip storing the peeled value in
> > either of the reference formats.
> > 
> 
> I have a design question here: should we just report an error to the
> user or just die instead of skipping storing the peeled value? If the
> annotated tag is corrupted in the first place, it means the refdb is
> also corrupted. And "git-fsck(1)" would definitely report an error to
> the user. But here we just ignore the problem and give an illusion that
> everything is fine.

The question is whether the user can do anything about it. The tag may
exist due to whatever reason, and it may not be prunable from the repo's
references.

Tools like git-fsck(1) should definitely complain about this, and they
in fact already do:

    $ git fsck
    Checking ref database: 100% (1/1), done.
    error: object d10476e1da82e779f64cfa12bd655b579c3fddbe is a commit, not a blob
    error: bad tag pointer to d10476e1da82e779f64cfa12bd655b579c3fddbe in ef5b01be3c1ad24fae2181040ced5776456a197a
    error: ef5b01be3c1ad24fae2181040ced5776456a197a: object could not be parsed: .git/objects/ef/5b01be3c1ad24fae2181040ced5776456a197a
    Checking object directories: 100% (256/256), done.
    error: object d10476e1da82e779f64cfa12bd655b579c3fddbe is a commit, not a blob
    error: bad tag pointer to d10476e1da82e779f64cfa12bd655b579c3fddbe in ef5b01be3c1ad24fae2181040ced5776456a197a
    error: refs/tags/tag-2: invalid sha1 pointer ef5b01be3c1ad24fae2181040ced5776456a197a

But for operations like optimizing references it is not as clean-cut
from my perspective. We definitely don't want to error out, as it would
mean that the user cannot have their reference optimized as long as such
a broken reference exist. And other operations should make sure that
they don't return invalid data in face of such a corrupted repository,
too.

We may want to add a warning in such cases though? I'd like to have some
more opinions on this.

Patrick