Re: [PATCH v2 00/10] improve reporting of unexpected objects

On Mon, Mar 29, 2021 at 03:34:03PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

But I think it still has the "oops, somebody made a wrong reference much
earlier" problem. The actual bug is in some other object entirely, whose
identity is long forgotten. I think we would be much better off to say
something like "somebody expected X to be a commit, but now somebody
else expects it to be a blob", which is all that we can reliably say.
And the next step really ought to be running "git fsck" to figure out
what is going on (and we should perhaps even say so via advise()).

Yes I'm totally side-stepping the issue, but I don't see a way around
that that doesn't make the whole object lookup code either much slower,
or more complex.

I.e. the whole thing is an emergent effect of us seeing a tag object,
and noting in-memory that we saw a given OID of type X, but we don't
even know if we can look it up at that point, or that it's not type Y.

I don't think it's guaranteed that we're neatly in one single object
traversal at that point (e.g. if we're looking at N tags, and only later
dereferencing their "object" pointers). So passing a "object A which I
have now says B is a X, assert!" wouldn't work.

We could eagerly get the object from disk when parsing tags (slow?), or
have a void* in the object struct or whatever to say "this is the OID
that claimed you were XYZ" (ew!).

Or, which I think makes sense here, just don't worry about it and error
with the limited info we have at hand. Yes we can't report who the
ultimate culprit is without an fsck, but that's not different than a lot
of other error() and die() messages in the object code now.

Yes, that "don't worry too much about it" was where my line of thinking
is going. But then I do not see all that much point in your final patch
at all. I.e., I think just changing the message to more clearly say what
we do know in lookup_commit(), etc, would be sufficient.

So if we're going to emit an advise() that seems generally useful for
many of those error()/die() messages, and not something we should tack
onto this incremental improvement to one error.

Yeah, I think doing an advise() is probably overkill. My next step would
always be "run fsck", and I was thinking only that we might point the
user in that direction. But it's probably fine to just emit the error.

-Peff