Re: [PATCH 01/10] builtin/fsck: use `fsck_obj_buffer()` when checking loose objects

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 31, 2026, 06:00 UTC
Message-ID: <apUYeK7IvIfaxdtf@pks.im>
In-Reply-To: <CAOLa=ZQyGk65qhF2uRV6Q48c_F948XuTWOdHOTwenfGestLxWw@mail.gmail.com>

On Thu, Aug 27, 2026 at 06:00:37AM -0400, Karthik Nayak wrote:

Show 9 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > When checking loose objects we manually parse the object buffer we have
> > read from the on-disk file, mark the object and then call `fsck_obj()`.
> > Almost the exact same steps are also performed by `fsck_obj_buffer()`.
> >
> 
> I was wondering what the difference was, there seems to be none, nit:
> perhaps we can drop 'Almost'.

I actually didn't have the "almost" initially but added it later, but there isn't really much of a reason why specifically I did so. I'll drop it again.

Show 33 quoted lines

> > diff --git a/builtin/fsck.c b/builtin/fsck.c
> > index 892c5661d9..3c4127f4d8 100644
> > --- a/builtin/fsck.c
> > +++ b/builtin/fsck.c
> > @@ -751,21 +750,7 @@ static int fsck_loose(const struct object_id *oid, const char *path,
> >  	if (!contents && type != OBJ_BLOB)
> >  		BUG("read_loose_object streamed a non-blob");
> >
> > -	obj = parse_object_buffer(data->repo, oid, type, size,
> > -				  contents, &eaten);
> > -
> > -	if (!obj) {
> > -		errors_found |= ERROR_OBJECT;
> > -		error(_("%s: object could not be parsed: %s"),
> > -		      oid_to_hex(oid), path);
> > -		if (!eaten)
> > -			free(contents);
> 
> This is now moved to the bottom below fsck_obj_buffer() call. So that's
> okay.
> 
> > -		return 0; /* keep checking other objects */
> > -	}
> > -
> > -	obj->flags &= ~(REACHABLE | SEEN);
> > -	obj->flags |= HAS_OBJ;
> > -	if (fsck_obj(data->repo, obj, contents, size))
> > +	if (fsck_obj_buffer(oid, type, size, contents, &eaten, data->repo))
> >  		errors_found |= ERROR_OBJECT;
> >
> 
> I see `fsck_obj_buffer()` also sets adds the `ERROR_OBJECT` flag, but
> that's okay.

Yeah. We could just drop this, but then it'd feel a tiny bit weird as we call the function without checking its return value at all. So I decided to just keep this as-is.

Patrick