On Mon, Mar 29, 2021 at 03:34:03PM +0200, Ævar Arnfjörð Bjarmason wrote: > > 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
