On Thu, Sep 16, 2021 at 05:29:30PM -0400, Taylor Blau wrote: > On Tue, Sep 07, 2021 at 12:58:03PM +0200, Ævar Arnfjörð Bjarmason wrote: > > When the loose_object_info() function returns an error stop faking up > > the "oi->typep" to OBJ_BAD. Let the return value of the function > > itself suffice. This code cleanup simplifies subsequent changes. > > The obvious danger (which you mention) is that somebody is relying on > what typep points to, and is reading it even if we returned non-zero > from whatever called this function. > > Hopefully nobody is, but this change makes me a little uncomfortable > nonetheless, since there are so many potential callers (even though this > function has only one caller, it doesn't take long before the number of > indirect callers explodes). > > So it would be nice if we could do without it, but you claim that it > simplifies changes that happen later on. So let's continue to see if we > really do need it... I'm actually reasonable comfortable with this patch. If we return an error from the *_object_info() functions, then I think all bets are off on what is in the resulting object_info struct. E.g., we'd already leave sizep uninitialized in such a case. It feels like oi->typep may be a little bit special because we conflate "error" and "type" in the return from oid_object_info(). But oid_object_info_extended() does not do that, and the innards of oid_object_info() do the right thing. Of course we _have_ been setting typep in this way for a while, so it's worth making sure nobody is depending on. Notably packed_object_info() does not behave in this way; if it hits an error, typep may be left unset. So any oid_object_info_extended() callers depending on this were already potentially buggy. I'd be OK with a quick sweep of the hits of "git grep typep" here. I just did that, and all the sites look pretty reasonable (they call oid_object_info_extended() and bail as soon as they see that it fails). -Peff
