Re: [PATCH v4 15/19] object-file.c: release the "tag" in check_tag()

From: Jeff King <peff@peff.net>
Date: Jan 18, 2023, 18:39 UTC
Message-ID: <Y8g84mABtIiHmxTI@coredump.intra.peff.net>
In-Reply-To: <fd883d86-0c85-6c72-a331-2e8b2064befe@web.de>

On Wed, Jan 18, 2023 at 07:05:32PM +0100, René Scharfe wrote:

Show 11 quoted lines

> >   2. The point of this code is to find malformed input to hash-object.
> >      We're probably better off feeding the buffer to fsck_commit(), etc.
> >      It does more thorough checks, and these days it does not need an
> >      object struct at all.
> 
> I like the second one, as long as it won't check too much.  c879daa237
> (Make hash-object more robust against malformed objects, 2011-02-05) added
> the checks that are now in object-file.c and intended to only validate the
> internal structure of objects, not relations between.  It gave the example
> to allow adding a commit before its tree, which should be allowed.  And
> IIUC fsck_object() fits that bill.

Yes, I think it will do what the right thing here. And having just written up a quick series, the only tests which needed changes were ones with syntactic problems. :)

I'll send it out in a few minutes.

Show 5 quoted lines

> > Either of which would naturally fix the leak for tags. I'm not sure
> > there actually is a leak for commits, as commit structs don't store any
> > strings themselves.
> 
> parse_commit_buffer() allocates the list of parents.

Yes, but it does so with lookup_commit(), so the resulting commit objects are themselves reachable from the usual obj_hash, and thus not leaked.

> Hmm, and it looks them up.  Doesn't this violate the goal to allow
> dangling references?

No, because lookup_commit() is just about creating an in-process struct. It doesn't look at the object database at all (though it would complain if we had seen the same oid in-process as another type).

-Peff