On Mon, Aug 26, 2019 at 10:20:20AM -0700, Junio C Hamano wrote: > Stefan Sperling <stsp@xxxxxxxxx> writes: > > > The root cause of this bug seems to be that the valid assumption > > that obj->parsed implies a successfully parsed object is broken by > > parse_tag_buffer() because this function sets the 'parsed' flag even > > if errors occur during parsing. > > I am mildly negative about that approach. obj->parsed is about > "we've done all we need to do to attempt parsing this object" (so > that next person who gets hold of the object knows that fact---one > of the reasons why may be that the caller who wants to ensure that > the fields are ready to be accessed does not have to spend extra > cycles, but that is not the only one). Those that want to look at > various fields in the object (e.g. the tagged object of a tag, the > tagger identity of a tag, etc.) should be prepared to see and react > to NULL in there so that they can gracefully handle "slightly" > corrupt objects. > I will respectfully agree to disagree :-) If an object is corrupt the repository is broken and should be fixed. Now, if this code was running in a tool which intends to fix up such problems, sure, let it handle corrupt objects. But I don't see the point of complicating code all over the place just to have the main tool's intended functionality partly working in face of corruption. That said, since you state that the 'parsed' flag already carries a different meaning than I was assuming it did, my patch is wrong and should be rewritten by someone else who can fully make sense of the existing internals.
